Flag and FlagEntity Model Draft #1269
Conversation
✅ Deploy Preview for activist-org ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Thank you for the pull request! ❤️The activist team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the |
Maintainer ChecklistThe following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)
|
|
Realizing that I didn't get back to you on the Figma question, @sh-ran! Sorry about that! Has been a busy week with work and a presentation I needed to give for this and the other community I work in 😊 Let's touch base on this and try to bring it in soon :) Thank you for your efforts! 🚀 |
|
No worries @andrewtavis I realized that when I saw the matrix post and tried to figure out whatever I can. Decided to send this PR so we can discuss and talk about the changes tonight in the dev sync. |
|
@to-sta, pinging you here for support for @sh-ran as we're not sure exactly where to go here :) Copying over the points from the dev sync summary: For the flag model...
Do you have any suggestions here? |
I think splitting it up is a good idea and then I would a add One-to-Many relationship with e.g. Org and User. Then we can retrieve the related information and check the permissions within the API (admin,...). |
|
So there wouldn't be a |
|
So we are using the same models as in figma but we are just flagging everything separate and then creating a table with one-to-many relationship? If that's the case, I'll get to working on it now! Thank you @to-sta and @andrewtavis |
|
No the Figma models need to change, @sh-ran :) I can get to that later. We'd have We're basically doing away with the idea of a unified flag dashboard and more separate dashboards based on entity types, which likely will be easier to work with anyway 😊 |
|
something like this? Sorry for bombarding with these many questions.
|
|
No need to apologize! Thank you for asking so many questions 😊 That looks good to me! If someone removes the flag of the entity, then we can just remove the entry as I don't think we need to collect data on whether someone has flagged something and decided against it. |
|
If you don't mind, I could write some code up for the organization part and maybe you and tobi can both review and suggest changes as needed. |
|
Sounds perfect! Thanks so much, @sh-ran! 😊 |
her: "I'm complicated" me: "I resolve git conflicts" Jokes aside current commit has code for organization_flags. Models, Viewsets, Serializers, Factories and tests are all included and working perfectly.
|
For me the current rendition looks great 😊 I think you can proceed to the other flaggable entities and then we can do the final review when the rest are in :) Thanks, @sh-ran! |
|
Thank you @andrewtavis Will be sending the remaining PR's ASAP 😄 |
| class OrganizationFlag(models.Model): | ||
| """ | ||
| Model for flagged organizations. | ||
| """ | ||
|
|
||
| id = models.UUIDField(primary_key=True, default=uuid4, editable=False) | ||
| flagged_org_id = models.ForeignKey( | ||
| "communities.Organization", on_delete=models.CASCADE | ||
| ) | ||
| flagger_id = models.ForeignKey("authentication.UserModel", on_delete=models.CASCADE) | ||
| flagged_on = models.DateTimeField(auto_now=True) |
There was a problem hiding this comment.
Django automatically appends _id to foreign key fields at the database level, so something like flagger_id would become flagger_id_id. To avoid this confusion and improve consistency, let's rename the field to flagged_by, which aligns better with our existing field created_by.
Additionally, let's update the flagged_on field to created_at for consistency with how we name timestamp fields throughout the project.
Since this is a many-to-many relationship with extra fields, we should use Django’s ManyToManyField along with the through keyword.
There was a problem hiding this comment.
I shall read through the docs and figure out how to fix it. Thank you @to-sta
|
Hello @to-sta So i made the changes as you had suggested although I am a bit skeptical about if what I have submitted is correct. I had to change the tests as well as i was getting an error with regards to For reference of
|
|
Quick note via our discussion: We need to update the schemas on Figma before this is merged :) Please let's remember this 😊 |
|
Nice that the tests are passing, @sh-ran! So from here we expand this to groups and events, right? Or is there something more that needs to be added in before it's propagated to other entities? |
|
Yup pretty much it for this PR. I am almost done with the groups and will be sending in the PR later this evening. I was wondering if there will be any merge conflicts between the two as we have made changes to a few of the same files? Just wanted to mention that there is a small bug in the delete test about which @to-sta and I discussed yesterday. We couldn't figure out what was causing it. I tried digging through the codebase last night but couldn't find anything helpful. For now I have excluded the 403 part of the test and will get back to it once I start writing the tests again. I'll be excluding that part in the next few PR's as well. Just a heads up. |
|
All sounds good, @sh-ran! We'll finalize the review and bring this in soon, and if there are any conflicts with groups we'll handle them with you 😊 |
|
Netlify is having some down time for deployment it seems given the error from main merging in. I'll check later and hopefully bring it in then :) |
Contributor checklist
Description
This is a just a draft for Flag and FlagEntity model, viewsets and serializers.
Related issue