Added functionality to send email whenever primary email is changed #3158
Conversation
|
@di Can you please review this PR?! Thanks |
| @@ -0,0 +1,5 @@ | |||
| Someone, perhaps you, has changed the primary email for your PyPI account | |||
| '{{ username }}'. | |||
There was a problem hiding this comment.
I think it'd be nice here if this email included the emails it changed from/to. So something like
The primary email for your PyPI account:
'{{ username }}'
been changed from this address:
{{ old_email }}
to this address:
{{ new_email }}
If you did not make this change, you can reply to this email directly to
communicate with the PyPI administrators.
tests/unit/manage/test_views.py
Outdated
| @@ -379,8 +379,8 @@ def test_change_primary_email(self, monkeypatch, db_request): | |||
| user = UserFactory() | |||
| old_primary = EmailFactory(primary=True, user=user) | |||
| new_primary = EmailFactory(primary=False, verified=True, user=user) | |||
|
|
|||
There was a problem hiding this comment.
Also, this is a very small issue, but it seems like your PR removes some empty lines like this one here -- could you make sure they remain?
Addressed code reviews
waseem18
force-pushed
the
confirmation-email
branch
from
3de62fc
to
0b2baaa
Compare
|
Addressed reviews @di |
| assert view.change_primary_email() == view.default_response | ||
| assert send_email.calls == [ | ||
| pretend.call(db_request, db_request.user, email.email) |
There was a problem hiding this comment.
@di After we merged this PR, I created another PR with the latest master - but I see my Travis test failing at this line which is no way related to #3111 . I'm not getting why this particular test failed on Travis. I'm not able to replicate the test failure locally.
I've been looking into this since a couple of hours but in vain so thought of seeking you help.
Could you please help me with this? Thanks
There was a problem hiding this comment.
Strange! Now when I updated my branch in #3111 there wasn't any test failure.
| @@ -379,8 +379,10 @@ def test_change_primary_email(self, monkeypatch, db_request): | |||
| user = UserFactory() | |||
| old_primary = EmailFactory(primary=True, user=user) | |||
| new_primary = EmailFactory(primary=False, verified=True, user=user) | |||
| email = EmailFactory(user=user) | |||
There was a problem hiding this comment.
@waseem18 The issue is that we're adding an extra email here. Since we've already created the old_primary email above, and since EmailFactory defaults to making the created email primary as well, we had created two primary emails for the user in this test.
The way User.email works is that it assumes there might be multiple primary emails, and returns the first primary email it finds for the user (not totally sure why this is the case, but I'm guessing backwards compatibility with legacy).
In this test, most of the time, user.email would refer to this email, but sometimes (not sure why) they might be created out of order, so user.email would be old_primary instead, causing the test to fail.
The fix is in #3178: just create one primary email for the user, and test that email is the one that gets the message.
There was a problem hiding this comment.
Thank you so much for the detailed response and the PR @di
There was a problem hiding this comment.
No worries! Sorry that I missed it in review and you spent so much time trying to debug it.
…ypi#3158) Addressed code reviews
…ypi#3158) Addressed code reviews
…ypi#3158) Addressed code reviews
) * Send email when a new collaborator has been added to the project. - send the email to the newly added collaborator - send the email to other owners - email not sent to the person who added the collaborator Closes #1000 * Remove prints * Make linter happy. * Added functionality to send email whenever primary email is changed (#3158) Addressed code reviews * - Send a separate welcome email to the new collaborator - Add owners emails in bcc field * PEP 8 * Fix linters errors * - Deindent - Add footer to email * Rebased with master * Fix broken unit test. * Fix linter error * PEP 8 * Actually send the email to bcc recipients
Fixes #998
Ais the current primary email and emailBhas been made primary then an email will be sent to the emailA.