Update emails on every login #137
Conversation
Codecov ReportBase: 97.60% // Head: 97.61% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #137 +/- ##
==========================================
+ Coverage 97.60% 97.61% +0.01%
==========================================
Files 15 15
Lines 709 714 +5
==========================================
+ Hits 692 697 +5
Misses 17 17
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@joyliu-q bumping this |
|
Isn't this what the account management system exists for? This PR also clobbers any changes people make to their primary email in that system. |
Do you have a link to what you're talking about? From what I could see emails were set only on first login and never after. After this is merged, emails are updated on every login. The reason we are doing this is because the RE: primary emails, I think it's ok to enforce primary email to be what Penn gives us. If anyone has a problem with their email, they can take it up with Penn. |
|
Can you explain how what I am doing is redundant or not needed? I know that platform exists. From what I can see the previous code would only set emails on first login. |
|
Right, the code you modified is the part of platform that generates an initial django user object based on the data we get from shibboleth for a user. The entire point of the account management system was to give a way for students to modify that data. They can set a preferred first name to show up on our products rather than the one Penn gives us as well as add and verify email addresses and phone numbers so that way our other products can use that list from platform and not worry about doing their own validation. |
|
I see what you mean here, but I still think we should stick with this email setting mechanism. If we don't have this logic, one the one hand, for those students that do not have emails on the ISC API at the time of creation, we'll never (eventually) have their correct emails. (there are actually a lot of these students -- see #clubs and #platform) On the other hand, we'll always override someone's custom primary email -- this is a niche use case anyways. I think we can just go with the former and have people take it up with the Penn directory if they want their public facing email changed. |
|
It's certainly your decision to make, but "we'll never (eventually) have their correct emails" doesn't have to be the case. You can always point students to the account management system and tell them to add whatever email they want. There was also a plan to modify DLA so that it would be easier for products (like clubs) to have their own edit contact pages which send the correct requests to platform so that platform is always updated. |
|
That's definitely something we can do -- and we'll update this script if that ends up happening. I'll add it in a backlog somewhere. For now, just to be future proof and avoid having to backfill again, we'll stick with this. |
Previously, emails would only be set on first login. Update this on every login to prevent the default case of @upenn.edu emails persisting.