Email notification when user's role is changed/removed. #3306

Mariatta · 2018-03-18T04:00:39Z

Send email to the user whose role has been changed/removed.
Send email to other owners of the project.
The person who made the change will not receive the email.

- Send email to the user whose role has been changed/removed. - Send email to other owners of the project. - The person who made the change will not receive the email. Closes pypi#3264

brainwane · 2018-03-18T04:48:56Z

@Mariatta You see the build failing, right? Thanks for the PR - please comment again when it's fixed up and passing tests. :)

Mariatta · 2018-03-18T17:43:16Z

Thanks @brainwane. All checks passed now 🎉 ✅

nlhkabu

Hi @Mariatta thanks for the PR :) Just a small copy request from me:

nlhkabu · 2018-03-19T07:02:47Z

warehouse/templates/email/removed-as-collaborator.body.txt

@@ -0,0 +1,6 @@
+Your role in the {{ project }} project has been removed by {{ submitter }}.
+
+You are no longer an {{ role }}.


I think we might need to reword this, because it could either be "a maintainer" or "an owner".

It would also be nice to add some kind of text explaining what the role is.

e.g.

"You are now a project owner. This means that you may add other collaborators, upload releases for the project, delete files and releases, or delete the entire project."

Or

"You are now a project maintainer. This means you may upload releases for the project. You cannot add collaborators, delete files, delete releases, or delete the project.

see #3157 for reference.

Since this particular email is for when they are removed from the role, I will phrase it like "You are no longer a(n) owner/maintainer. You can no longer ...".
Does that sounds ok?

Ah, so it is. Sorry, I misread :)

Maybe "you are no longer a project owner/maintainer" to avoid the "a(n)" issue? wdyt?

nlhkabu · 2018-03-19T07:03:20Z

warehouse/templates/email/user-role-changed.body.txt

@@ -0,0 +1,6 @@
+Your role in the {{ project }} project has been changed by {{ submitter }}.
+
+You are now a {{ role }}.


As above :)

Mariatta · 2018-03-20T04:56:32Z

@nlhkabu I've updated the email text. Let me know what you think. Thanks.

nlhkabu

Perfect! thank you!

nlhkabu · 2018-03-20T06:24:07Z

This is ready on my side! @di or @ewdurbin could you please review the Python code and tests? Thx!

ewdurbin · 2018-03-20T08:27:09Z

warehouse/manage/views.py

+            owner_emails = [owner.user.email for owner in owners
+                            if owner.user.email]
+            if request.user.email in owner_emails:
+                owner_emails.remove(request.user.email)


We should probably still email the user making the change. This is a relatively major "operation" and if someone has intercepted a password and starts making changes as a user, we'd want them to notify.

Though I it's possible for an attacker to simply remove or replace all active email addresses before making these changes, the primary email address changing would initiate a notification.

ewdurbin · 2018-03-20T08:35:19Z

warehouse/manage/views.py

+            owner_emails = [owner.user.email for owner in owners
+                            if owner.user.email]
+            if request.user.email in owner_emails:
+                owner_emails.remove(request.user.email)


Notify the user making this change as well.

ewdurbin · 2018-03-20T08:35:37Z

warehouse/manage/views.py

+                    owner_emails = [owner.user.email for owner in owners
+                                    if owner.user.email]
+                    if request.user.email in owner_emails:
+                        owner_emails.remove(request.user.email)


Notify the user making this change as well.

ewdurbin · 2018-03-20T08:40:48Z

Thanks @Mariatta! I see that the logic for removing the submitting user from notifications was carried over from existing code, but I think this is a good opportunity to bump the visibility of these kinds of changes.

Mariatta · 2018-03-21T03:43:07Z

No prob. Emails are now being sent to all owners.

di · 2018-03-21T17:20:32Z

warehouse/manage/views.py

-            )
-
+            owner_emails = [owner.user.email for owner in owners
+                            if owner.user.email]


I think all Users should have an email? So if this is causing errors, an email address probably needs added to the fixture in the test where it breaks, rather than adding this filter here.

Makes sense I've changed this.

di · 2018-03-21T17:21:51Z

warehouse/manage/views.py

+                        )
+                    )
+                    owner_emails = [owner.user.email for owner in owners
+                                    if owner.user.email]


Same here as above w/ regards to the filter.

di · 2018-03-21T17:22:57Z

warehouse/manage/views.py

+                )
+            )
+            owner_emails = [owner.user.email for owner in owners
+                            if owner.user.email]


di · 2018-03-21T17:23:28Z

warehouse/manage/views.py

+            owner_emails = [owner.user.email for owner in owners
+                            if owner.user.email]
+
+            if owner_emails:


Can we move this condition into the send_collaborator_added_email function instead?

So since we'll always send the email to all owners anyway, and at any time there will always be one owner, I'm assuming that we'll always send the email. So I removed the condition. Is this ok?

di · 2018-03-21T17:23:48Z

warehouse/manage/views.py

                    role.role_name = form.role_name.data
+                    send_user_role_changed_email(request, role)
+                    if owner_emails:


Can we move this condition into the send_role_changed_for_user_email function instead?

di · 2018-03-21T17:25:38Z

warehouse/templates/email/role-removed-from-user.subject.txt

@@ -0,0 +1 @@
+Role removed from PyPI


Maybe Role removed on PyPI for project {{ project.name }}?

di · 2018-03-21T17:25:52Z

warehouse/templates/email/role-changed-for-user.subject.txt

@@ -0,0 +1 @@
+Role changed in PyPI


Maybe Role changed on PyPI for project {{ project.name }}?

di · 2018-03-21T17:26:19Z

warehouse/templates/email/removed-as-collaborator.subject.txt

@@ -0,0 +1 @@
+Role removed from PyPI


Maybe Role removed on PyPI for project {{ project.name }}?

di · 2018-03-21T17:26:50Z

warehouse/templates/email/role-removed-from-user.body.txt

+A collaborator's role has been removed from a project you own on PyPI:
+
+  Username: {{ username }}
+  Role: {{ role }}


Maybe "Role was previously:"

di · 2018-03-21T17:28:17Z

tests/unit/manage/test_views.py

-                []
-            )
-        ]
+        assert send_collaborator_added_email.calls == []


We probably still want this assertion to stay or a similar assertion to replace it.

brainwane · 2018-03-30T02:52:25Z

@Mariatta hey there -- will you be revising this PR, maybe next week, per the reviews on it? Thanks!

Update tests.

email recipients shouldn't ever be empty.

Mariatta · 2018-03-30T17:20:50Z

The test failed in 3.6.1, apparently because the order of the list items matters. I don't know how to properly test that...

brainwane · 2018-03-31T18:43:10Z

@waseem18 @yeraydiazdiaz do you have any thoughts on how to address the test issue?

waseem18 · 2018-03-31T18:44:48Z

@brainwane I'll check now and will comment back.

waseem18 · 2018-04-01T18:12:59Z

@Mariatta You can use sorted() on the list so that the order of elements in the list is preserved both in the main code as well as test cases.

In test cases you can change [other_owner.email, owner.email] to sorted([other_owner.email, owner.email]) whereas in warehouse/manage/views.py add sorted() respectively so that owner_emails changes to sorted(owner_emails)

Example:
send_role_changed_for_user_email( request, role, sorted(owner_emails), )

dstufft · 2018-04-01T19:03:20Z

Note that this PR conflicts with #3493 and whichever one lands second will have to be updated to handle the other.

brainwane · 2018-04-02T21:43:56Z

(Noting here that @Mariatta is on a break from FLOSS contribution right now but will still respond to revision requests on open PRs like this one.)

brainwane · 2018-04-24T17:47:38Z

Hey @Mariatta -- we'd like to move forward on this fix, so please let us know whether you'll be revising the PR (to use Wasim's advice to get the tests to pass) within the next few days. If not, I'll find someone else to work on fixing #3264, and if and when you have time to do some Warehouse work in the future, let us know and we'll help you get going. Thanks!

Mariatta · 2018-04-25T18:47:08Z

Thanks for the ping @brainwane. I have some time later this evening (and this evening only) to look into this. My next availability will be at PyCon sprints.
If I couldn't this done, I'll let you know tomorrow. So someone else can continue. Thanks.

Mariatta · 2018-04-26T16:18:21Z

Sorry everyone, I didn't get around working on this last night. If this can't wait until PyCon US sprints, then perhaps it's best for someone else to continue working on this. I'll leave it to you all to close this PR.
Thanks all for the patience, and sorry I couldn't finish this up in time.

waseem18 · 2018-04-26T16:29:52Z

Hey @Mariatta, No worries! I'll add the test case changes today that would fix this PR.

waseem18 · 2018-04-28T18:08:12Z

Quick update on this - I'm resolving merge conflicts now and as soon as I'm done with testing I'll push the code.

Edit: I've added a new PR #3853 which fixes the conflicts of this PR. So this PR can be closed now.

nlhkabu · 2018-04-29T07:47:17Z

Thanks for your work on this @Mariatta, closing in favour of #3853 :)

Email notification when user's role is changed/removed.

746fd84

- Send email to the user whose role has been changed/removed. - Send email to other owners of the project. - The person who made the change will not receive the email. Closes pypi#3264

Mariatta added 2 commits March 18, 2018 08:43

Make linters happy

3c8556f

Improve the tests and coverage

1726c85

nlhkabu requested changes Mar 19, 2018

View reviewed changes

Update email text.

8f5ae4b

nlhkabu approved these changes Mar 20, 2018

View reviewed changes

Merge branch 'master' into issue-3264

7187263

nlhkabu requested review from di and ewdurbin March 20, 2018 06:23

ewdurbin mentioned this pull request Mar 20, 2018

Email notification when adding/removing email address. #3317

Open

ewdurbin requested changes Mar 20, 2018

View reviewed changes

Notify all owners for any role changes.

aebf92d

di requested changes Mar 21, 2018

View reviewed changes

Mariatta added 3 commits March 29, 2018 21:43

Update email subjects.

18c4cc1

Update tests.

Since we're always sending email to all owners now when role changed,

6ece563

email recipients shouldn't ever be empty.

The order matters?

aa144b6

dstufft mentioned this pull request Apr 1, 2018

Move email handling into a service #3493

Merged

di self-assigned this Apr 2, 2018

waseem18 mentioned this pull request Apr 29, 2018

Email notification when user's role is changed/removed. #3853

Closed

nlhkabu closed this Apr 29, 2018

		@@ -0,0 +1,6 @@
		Your role in the {{ project }} project has been removed by {{ submitter }}.

		You are no longer an {{ role }}.

		@@ -0,0 +1,6 @@
		Your role in the {{ project }} project has been changed by {{ submitter }}.

		You are now a {{ role }}.

Email notification when user's role is changed/removed. #3306

Email notification when user's role is changed/removed. #3306

Uh oh!

Conversation

Mariatta commented Mar 18, 2018

Uh oh!

brainwane commented Mar 18, 2018

Uh oh!

Mariatta commented Mar 18, 2018

Uh oh!

nlhkabu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlhkabu Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Mar 20, 2018

Uh oh!

nlhkabu left a comment

Choose a reason for hiding this comment

Uh oh!

nlhkabu commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewdurbin commented Mar 20, 2018

Uh oh!

Mariatta commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brainwane commented Mar 30, 2018

Uh oh!

Mariatta commented Mar 30, 2018

Uh oh!

brainwane commented Mar 31, 2018

Uh oh!

waseem18 commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waseem18 commented Apr 1, 2018

Uh oh!

dstufft commented Apr 1, 2018

Uh oh!

nlhkabu left a comment •

edited

Loading

nlhkabu Mar 19, 2018 •

edited

Loading

waseem18 commented Mar 31, 2018 •

edited

Loading

waseem18 commented Apr 28, 2018 •

edited

Loading