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

Clear cache and refresh user profile on email verification #1970

Merged
merged 5 commits into from
Dec 17, 2020

Conversation

DavisRayM
Copy link
Contributor

Changes / Features implemented

  • Refresh user profile from database before updating metadata
  • Clear user-profiles cache on email verification

Steps taken to verify this change does what is intended

  • N/A

Side effects of implementing this change

  • N/A

Possible fix for #1948

FrankApiyo
FrankApiyo previously approved these changes Dec 14, 2020
ivermac
ivermac previously approved these changes Dec 14, 2020
@@ -100,7 +100,8 @@ def serializer_from_settings():


def set_is_email_verified(profile, is_email_verified):
profile.metadata.update({"is_email_verified": is_email_verified})
profile.refresh_from_db()
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this call before we call set_is_email_verified()? I see a reason why the refresh_from_db may need to happen on the GET request to verify, but perhaps not on the POST request. I would hope all requests on a POST use the master DB.

Is this issue due to a read replica update delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to be a read replica update delay; the change never shows up. Noticed accounts that had been verified a while back(2-3 days) but the metadata wasn't updated

Copy link
Member

@ukanga ukanga Dec 15, 2020

Choose a reason for hiding this comment

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

What then guarantees this issue is resolved considering the statement has always been there? The only new statements is the cache deletion and the refresh_from_db.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this in the latest commits. The main issue seems to have been the RegistrationProfile retrieval in the user profile viewset.

Caching the userprofile on retrieval seems like premature optimization
we've not necessarily encountered any issues with reading directly from
a read replica or master that would warrant caching the profile.
ivermac
ivermac previously approved these changes Dec 17, 2020
@DavisRayM DavisRayM merged commit 853883b into master Dec 17, 2020
@DavisRayM DavisRayM deleted the email-verification-issue branch December 17, 2020 15:49
@DavisRayM DavisRayM mentioned this pull request Jan 20, 2021
1 task
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.

4 participants