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

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

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Mariatta
Contributor

Mariatta commented Mar 18, 2018

  • 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 #3264

Email notification when user's role is changed/removed.
- 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 #3264
@brainwane

This comment has been minimized.

Member

brainwane commented Mar 18, 2018

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

Mariatta added some commits Mar 18, 2018

@Mariatta

This comment has been minimized.

Contributor

Mariatta commented Mar 18, 2018

Thanks @brainwane. All checks passed now 🎉

@nlhkabu

nlhkabu requested changes Mar 19, 2018 edited

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

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

This comment has been minimized.

@nlhkabu

nlhkabu Mar 19, 2018

Member

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.

This comment has been minimized.

@Mariatta

Mariatta Mar 20, 2018

Contributor

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?

This comment has been minimized.

@nlhkabu

nlhkabu Mar 20, 2018

Member

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

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

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

This comment has been minimized.

@nlhkabu

nlhkabu Mar 19, 2018

Member

As above :)

@Mariatta

This comment has been minimized.

Contributor

Mariatta commented Mar 20, 2018

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

@nlhkabu

Perfect! thank you!

@nlhkabu nlhkabu requested review from di and ewdurbin Mar 20, 2018

@nlhkabu

This comment has been minimized.

Member

nlhkabu commented Mar 20, 2018

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

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)

This comment has been minimized.

@ewdurbin

ewdurbin Mar 20, 2018

Member

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.

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)

This comment has been minimized.

@ewdurbin

ewdurbin Mar 20, 2018

Member

Notify the user making this change as well.

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)

This comment has been minimized.

@ewdurbin

ewdurbin Mar 20, 2018

Member

Notify the user making this change as well.

@ewdurbin

This comment has been minimized.

Member

ewdurbin commented Mar 20, 2018

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

This comment has been minimized.

Contributor

Mariatta commented Mar 21, 2018

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

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

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.

This comment has been minimized.

@Mariatta

Mariatta Mar 30, 2018

Contributor

Makes sense I've changed this.

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

Same here as above w/ regards to the filter.

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

And here.

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

Can we move this condition into the send_collaborator_added_email function instead?

This comment has been minimized.

@Mariatta

Mariatta Mar 30, 2018

Contributor

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?

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

Can we move this condition into the send_role_changed_for_user_email function instead?

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

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

This comment has been minimized.

@Mariatta

Mariatta Mar 30, 2018

Contributor

Updated.

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

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

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

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

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

Maybe "Role was previously:"

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

This comment has been minimized.

@di

di Mar 21, 2018

Member

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

@brainwane

This comment has been minimized.

Member

brainwane commented Mar 30, 2018

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

Mariatta added some commits Mar 30, 2018

Update email subjects.
Update tests.
Since we're always sending email to all owners now when role changed,
email recipients shouldn't ever be empty.
@Mariatta

This comment has been minimized.

Contributor

Mariatta commented Mar 30, 2018

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

This comment has been minimized.

Member

brainwane commented Mar 31, 2018

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

@waseem18

This comment has been minimized.

Contributor

waseem18 commented Mar 31, 2018

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

@waseem18

This comment has been minimized.

Contributor

waseem18 commented Apr 1, 2018

@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

This comment has been minimized.

Member

dstufft commented Apr 1, 2018

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

@brainwane

This comment has been minimized.

@di di self-assigned this Apr 2, 2018

@brainwane

This comment has been minimized.

Member

brainwane commented Apr 24, 2018

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

This comment has been minimized.

Contributor

Mariatta commented Apr 25, 2018

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

This comment has been minimized.

Contributor

Mariatta commented Apr 26, 2018

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

This comment has been minimized.

Contributor

waseem18 commented Apr 26, 2018

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

@waseem18

This comment has been minimized.

Contributor

waseem18 commented Apr 28, 2018

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

This comment has been minimized.

Member

nlhkabu commented Apr 29, 2018

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

@nlhkabu nlhkabu closed this Apr 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment