-
Notifications
You must be signed in to change notification settings - Fork 183
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
Conversation
1fd6d6e
to
68eec35
Compare
There was a problem hiding this 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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(praise) 🎊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user.profile
to Profile.objects.get(user=user)
user.profile
to Profile.objects.get(user=user)
The pattern
profile = Profile.objects.get(user=user)
was useful whenuser
had aForiegnKey
relationship. Since #2749, it has been unnecessary. Switching these touser.profile
has some benefits:user.profile
and the stand-aloneProfile
object, which could introduce bugsProfile
is no longer needed, which will reduce the code changes when movingProfile
toprivaterelay
.