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

MPP-3827 - Prefer user.profile to Profile.objects.get(user=user) #4798

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

jwhitlock
Copy link
Member

@jwhitlock jwhitlock commented Jun 20, 2024

The pattern profile = Profile.objects.get(user=user) was useful when user had a ForiegnKey relationship. Since #2749, it has been unnecessary. Switching these to user.profile has some benefits:

  • In most cases, it reduces the lines of code
  • It removes a possible mismatch between user.profile and the stand-alone Profile object, which could introduce bugs
  • In some cases, importing Profile is no longer needed, which will reduce the code changes when moving Profile to privaterelay.

@jwhitlock jwhitlock force-pushed the user-profile-mpp-4763 branch from 1fd6d6e to 68eec35 Compare June 21, 2024 16:04
Copy link
Contributor

@say-yawn say-yawn left a comment

Choose a reason for hiding this comment

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

Most of the changes look good and relieved to see we can directly use profile instead doing the profile_set.get() thanks to the changes from #2749 that makes Profile and User one-to-one relationship.

emails/models.py Outdated
@@ -655,7 +655,8 @@ def delete(self, *args, **kwargs):
num_spam=self.num_spam,
)
deleted_address.save()
profile = Profile.objects.get(user=self.user)
profile = self.user.profile
profile.refresh_from_db()
Copy link
Contributor

Choose a reason for hiding this comment

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

(question, non-blocking) Why was the profile.refresh_from_db() needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it, tests were OK, so let's remove it.

A refresh_from_db() would be a good idea if we use F expressions to avoid race conditions here:

https://docs.djangoproject.com/en/4.2/ref/models/expressions/#avoiding-race-conditions-using-f

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa! I didn't know about F expressions. Good to know going forward we can use this especially for incrementing!

Comment on lines -862 to +859
# self.user_profile is a property and should not be used to
# update values on the user's profile
profile = Profile.objects.get(user=self.user)
profile = self.user.profile
Copy link
Contributor

Choose a reason for hiding this comment

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

(praise) 🎊

@jwhitlock jwhitlock requested a review from say-yawn June 26, 2024 00:17
Copy link
Contributor

@say-yawn say-yawn left a comment

Choose a reason for hiding this comment

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

@jwhitlock jwhitlock added this pull request to the merge queue Jun 26, 2024
Merged via the queue into main with commit eac5ed6 Jun 26, 2024
29 checks passed
@jwhitlock jwhitlock deleted the user-profile-mpp-4763 branch June 26, 2024 20:32
@jwhitlock jwhitlock changed the title MPP-4763 - Prefer user.profile to Profile.objects.get(user=user) MPP-3827 - Prefer user.profile to Profile.objects.get(user=user) Jun 28, 2024
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.

2 participants