Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create table project_sponsor_partner_xref #270

Merged

Conversation

freaky4wrld
Copy link
Member

@freaky4wrld freaky4wrld commented Mar 18, 2024

Fixes #65 , #272

What changes did you make?

  • Created a model for the ProjectSponsorPartnerXref
  • Registered the model to adminSite
  • Created API endpoints
  • Renamed the model sponsor_partner to affiliate

Why did you make the changes (we will use this info to test)?

  • To add a table ProjectSponsorPartnerXref in the database
  • To make the model name shorter to affiliate

@freaky4wrld freaky4wrld changed the title Create table project sponsor partner xref 65 Create table project_sponsor_partner_xref Mar 18, 2024
Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look great! Just a few things I noticed in the code should be changed.

app/core/models.py Outdated Show resolved Hide resolved
app/core/tests/test_api.py Outdated Show resolved Hide resolved
app/core/tests/test_models.py Outdated Show resolved Hide resolved
app/core/models.py Show resolved Hide resolved
@fyliu fyliu mentioned this pull request Mar 18, 2024
3 tasks
@freaky4wrld freaky4wrld requested a review from fyliu March 19, 2024 04:57
@freaky4wrld
Copy link
Member Author

@fyliu the changes you suggested are done, except the one with the is_sponsor one.

Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes. I left another comment on the model and thought of something new. Can you take a look.

app/core/models.py Show resolved Hide resolved
@freaky4wrld freaky4wrld force-pushed the create-table-project-sponsor-partner-xref-65 branch from 150c395 to e862c55 Compare March 20, 2024 04:04
@freaky4wrld freaky4wrld requested a review from fyliu March 20, 2024 04:05
@freaky4wrld
Copy link
Member Author

@fyliu the changes requested are done please check

@fyliu
Copy link
Member

fyliu commented Apr 7, 2024

I'm realizing that this PR is essentially ready and is actually waiting for me.

We were discussing changes to it over slack chat. I'm pasting that here for reference

  • updating the table name to project_affiliate_xref
  • updating the model name to Affiliation, specifying that the db_table is the name above
  • converting the boolean to an enum choice of sponsor or partner
  • maybe also changing the sponsor_partner table name to affiliate and the FK field in Affiliation

I need to create issues for these and you can make any of the changes here you feel makes sense.

@freaky4wrld
Copy link
Member Author

@fyliu I've made the suggested change, have a look and do resolve the issue I've mentioned in slack!!

Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The migrations applied fine for me.

There a few extra commits here that I think you brought over from main. They're making the github's PR changes view look more complicated. We should figure out how to make it cleaner for future PRs.

You made good assumptions with little information, but some of the things I just need to specify better. I think I need to create those other issues to better communicate what's needed.

app/core/models.py Outdated Show resolved Hide resolved
app/core/models.py Outdated Show resolved Hide resolved
@freaky4wrld freaky4wrld requested a review from fyliu April 15, 2024 08:39
@freaky4wrld
Copy link
Member Author

@fyliu renamed the model and implemented Python Enums as per requirement, please check.

Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes! I just found a couple small things that need changing.

app/core/admin.py Outdated Show resolved Hide resolved
app/core/models.py Outdated Show resolved Hide resolved
@fyliu
Copy link
Member

fyliu commented Apr 15, 2024

@freaky4wrld I just realized that the boolean to enum change hasn't been agreed on with @Neecolaa yet. Will have to bring that up for discussion in the next meeting.

@freaky4wrld
Copy link
Member Author

@freaky4wrld I just realized that the boolean to enum change hasn't been agreed on with @Neecolaa yet. Will have to bring that up for discussion in the next meeting.

@fyliu did you had the discussion with @Neecolaa. I'm still waiting for the confirmation and further info on changes to be made!!

@Neecolaa
Copy link
Member

Neecolaa commented May 3, 2024

We're adding a row:
is_partner is a boolean that indicated whether or not the given affiliate is a partner for the given project. Projects can now have an affiliate be both a partner and sponsor. We now have definitions for the affiliate types as well:
A Partner is an entity that's doing ongoing work with us on the project.
A sponsor is someone who has provided funds or an in-kind donation.

We won't be using an enum, just the two booleans.

@Neecolaa
Copy link
Member

Neecolaa commented May 3, 2024

@freaky4wrld see update comment above

@freaky4wrld freaky4wrld requested a review from fyliu May 6, 2024 04:59
@freaky4wrld
Copy link
Member Author

@fyliu and @Neecolaa the changes have been made as suggested, please check and provide feedback
Thanks

Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the update and sorry for the change in requirement.

I found a couple things that could be improved. I'm short on time today so I'll continue tomorrow.

app/core/api/views.py Outdated Show resolved Hide resolved
app/core/models.py Outdated Show resolved Hide resolved
app/core/api/serializers.py Show resolved Hide resolved
@freaky4wrld freaky4wrld requested a review from fyliu May 7, 2024 05:46
Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates.

I think I wasn't clear enough about the created_at field. It's already provided by the AbstractBaseModel and you just have to include it in the AffiliationSerializer. Can you remove it from the Affiliation model class. That should make it use the inherited one. Thank you!

app/core/api/views.py Outdated Show resolved Hide resolved
@freaky4wrld freaky4wrld requested a review from fyliu May 8, 2024 03:38
@fyliu fyliu force-pushed the create-table-project-sponsor-partner-xref-65 branch from 342cf57 to 550e45c Compare May 13, 2024 16:37
- renamed sponsor_partner table and model to affiliate
- added model: affiliation
- registered admin: affiliation
- added endpoints: affiliation
@fyliu fyliu force-pushed the create-table-project-sponsor-partner-xref-65 branch from 550e45c to 5ab31d1 Compare May 16, 2024 21:19
Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great, thanks!

I tried to simplify the commit history but it looks like I have to just squash them down to a single commit because of 2 models being changed. I also added a few other fixes that you can see at the end of the branch history in my forked branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Table: project_sponsor_partner_xref
3 participants