From 746fd8458fc7e3973acbabf25cab7a876e2dbe55 Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Sat, 17 Mar 2018 21:00:24 -0700 Subject: [PATCH 1/8] 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 https://github.com/pypa/warehouse/issues/3264 --- tests/unit/manage/test_views.py | 68 ++++- tests/unit/test_email.py | 272 ++++++++++++++++++ warehouse/email.py | 82 ++++++ warehouse/manage/views.py | 35 ++- .../email/removed-as-collaborator.body.txt | 6 + .../email/removed-as-collaborator.subject.txt | 1 + .../email/role-changed-for-user.body.txt | 9 + .../email/role-changed-for-user.subject.txt | 1 + .../email/role-removed-from-user.body.txt | 9 + .../email/role-removed-from-user.subject.txt | 1 + .../email/user-role-changed.body.txt | 6 + .../email/user-role-changed.subject.txt | 1 + 12 files changed, 486 insertions(+), 5 deletions(-) create mode 100644 warehouse/templates/email/removed-as-collaborator.body.txt create mode 100644 warehouse/templates/email/removed-as-collaborator.subject.txt create mode 100644 warehouse/templates/email/role-changed-for-user.body.txt create mode 100644 warehouse/templates/email/role-changed-for-user.subject.txt create mode 100644 warehouse/templates/email/role-removed-from-user.body.txt create mode 100644 warehouse/templates/email/role-removed-from-user.subject.txt create mode 100644 warehouse/templates/email/user-role-changed.body.txt create mode 100644 warehouse/templates/email/user-role-changed.subject.txt diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 2c862f5c2132..b863f27791f7 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -1385,7 +1385,7 @@ def test_post_duplicate_role(self, db_request): class TestChangeProjectRoles: - def test_change_role(self, db_request): + def test_change_role(self, db_request, monkeypatch): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") role = RoleFactory.create( @@ -1407,6 +1407,22 @@ def test_change_role(self, db_request): lambda *a, **kw: "/the-redirect" ) + send_user_role_changed_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_user_role_changed_email', + send_user_role_changed_email + ) + + send_role_changed_for_user_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_role_changed_for_user_email', + send_role_changed_for_user_email + ) + result = views.change_project_role(project, db_request) assert role.role_name == new_role_name @@ -1419,6 +1435,21 @@ def test_change_role(self, db_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" + assert send_user_role_changed_email.calls == [ + pretend.call( + db_request, + role, + ) + ] + + assert send_role_changed_for_user_email.calls == [ + pretend.call( + db_request, + role, + [], + ) + ] + entry = db_request.db.query(JournalEntry).one() assert entry.name == project.name @@ -1579,7 +1610,7 @@ def test_change_own_owner_role_when_multiple(self, db_request): class TestDeleteProjectRoles: - def test_delete_role(self, db_request): + def test_delete_role(self, db_request, monkeypatch): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") role = RoleFactory.create( @@ -1597,8 +1628,23 @@ def test_delete_role(self, db_request): lambda *a, **kw: "/the-redirect" ) - result = views.delete_project_role(project, db_request) + send_removed_from_role_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_removed_from_role_email', + send_removed_from_role_email + ) + send_role_removed_from_user_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_role_removed_from_user_email', + send_role_removed_from_user_email + ) + + result = views.delete_project_role(project, db_request) assert db_request.route_path.calls == [ pretend.call('manage.project.roles', project_name=project.name), ] @@ -1609,6 +1655,21 @@ def test_delete_role(self, db_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" + assert send_removed_from_role_email.calls == [ + pretend.call( + db_request, + role, + ) + ] + + assert send_role_removed_from_user_email.calls == [ + pretend.call( + db_request, + role, + [], + ) + ] + entry = db_request.db.query(JournalEntry).one() assert entry.name == project.name @@ -1664,6 +1725,7 @@ def test_delete_own_owner_role(self, db_request): assert result.headers["Location"] == "/the-redirect" + class TestManageProjectHistory: def test_get(self, db_request): diff --git a/tests/unit/test_email.py b/tests/unit/test_email.py index bf0cb502ef24..3a1477e6d270 100644 --- a/tests/unit/test_email.py +++ b/tests/unit/test_email.py @@ -20,6 +20,10 @@ from warehouse import email from warehouse.accounts.interfaces import ITokenService +from ..common.db.packaging import ( + JournalEntryFactory, ProjectFactory, ReleaseFactory, RoleFactory, + UserFactory, +) class TestSendEmail: @@ -507,3 +511,271 @@ def test_added_as_collaborator_email( recipients=[stub_user.email], ), ] + + +class TestRemovedAsCollaboratorEmail: + + def test_removed_as_collaborator_email( + self, pyramid_request, pyramid_config, monkeypatch): + + stub_user = pretend.stub( + email='email', + username='username', + ) + stub_submitter_user = pretend.stub( + email='submiteremail', + username='submitterusername' + ) + stub_role = pretend.stub( + role_name="Owner", + user=stub_user, + project=pretend.stub( + name="test_project" + ), + ) + pyramid_request.user = stub_submitter_user + + subject_renderer = pyramid_config.testing_add_renderer( + 'email/removed-as-collaborator.subject.txt' + ) + subject_renderer.string_response = 'Email Subject' + body_renderer = pyramid_config.testing_add_renderer( + 'email/removed-as-collaborator.body.txt' + ) + body_renderer.string_response = 'Email Body' + + send_email = pretend.stub( + delay=pretend.call_recorder(lambda *args, **kwargs: None) + ) + pyramid_request.task = pretend.call_recorder( + lambda *args, **kwargs: send_email + ) + monkeypatch.setattr(email, 'send_email', send_email) + + result = email.send_removed_from_role_email( + pyramid_request, + role=stub_role, + ) + + assert result == { + 'project': 'test_project', + 'role': 'Owner', + 'submitter': stub_submitter_user.username + } + subject_renderer.assert_() + body_renderer.assert_(submitter=stub_submitter_user.username) + body_renderer.assert_(project='test_project') + body_renderer.assert_(role='Owner') + + assert pyramid_request.task.calls == [ + pretend.call(send_email), + ] + assert send_email.delay.calls == [ + pretend.call( + 'Email Body', + 'Email Subject', + recipients=[stub_user.email], + ), + ] + + +class TestUserRoleChangedEmail: + + def test_user_role_changed_email( + self, pyramid_request, pyramid_config, monkeypatch): + + stub_user = pretend.stub( + email='email', + username='username', + ) + stub_submitter_user = pretend.stub( + email='submiteremail', + username='submitterusername' + ) + stub_role = pretend.stub( + role_name="Owner", + user=stub_user, + project=pretend.stub( + name="test_project" + ), + ) + pyramid_request.user = stub_submitter_user + + subject_renderer = pyramid_config.testing_add_renderer( + 'email/role-removed-from-user.subject.txt' + ) + subject_renderer.string_response = 'Email Subject' + body_renderer = pyramid_config.testing_add_renderer( + 'email/role-removed-from-user.body.txt' + ) + body_renderer.string_response = 'Email Body' + + send_email = pretend.stub( + delay=pretend.call_recorder(lambda *args, **kwargs: None) + ) + pyramid_request.task = pretend.call_recorder( + lambda *args, **kwargs: send_email + ) + monkeypatch.setattr(email, 'send_email', send_email) + + result = email.send_role_removed_from_user_email( + pyramid_request, + role=stub_role, + email_recipients=[stub_user.email, stub_submitter_user.email] + ) + + assert result == { + 'project': 'test_project', + 'role': 'Owner', + 'submitter': stub_submitter_user.username, + 'username': stub_user.username, + } + subject_renderer.assert_() + body_renderer.assert_(submitter=stub_submitter_user.username) + body_renderer.assert_(project='test_project') + body_renderer.assert_(role='Owner') + + assert pyramid_request.task.calls == [ + pretend.call(send_email), + ] + assert send_email.delay.calls == [ + pretend.call( + 'Email Body', + 'Email Subject', + bcc=[stub_user.email, stub_submitter_user.email], + ), + ] + + +class TestUserRoleChangedEmailEmail: + + def test_user_role_changed_email( + self, pyramid_request, pyramid_config, monkeypatch): + + stub_user = pretend.stub( + email='email', + username='username', + ) + stub_submitter_user = pretend.stub( + email='submiteremail', + username='submitterusername' + ) + stub_role = pretend.stub( + role_name="Owner", + user=stub_user, + project=pretend.stub( + name="test_project" + ), + ) + pyramid_request.user = stub_submitter_user + + subject_renderer = pyramid_config.testing_add_renderer( + 'email/user-role-changed.subject.txt' + ) + subject_renderer.string_response = 'Email Subject' + body_renderer = pyramid_config.testing_add_renderer( + 'email/user-role-changed.body.txt' + ) + body_renderer.string_response = 'Email Body' + + send_email = pretend.stub( + delay=pretend.call_recorder(lambda *args, **kwargs: None) + ) + pyramid_request.task = pretend.call_recorder( + lambda *args, **kwargs: send_email + ) + monkeypatch.setattr(email, 'send_email', send_email) + + result = email.send_user_role_changed_email( + pyramid_request, + role=stub_role, + ) + + assert result == { + 'project': 'test_project', + 'role': 'Owner', + 'submitter': stub_submitter_user.username + } + subject_renderer.assert_() + body_renderer.assert_(submitter=stub_submitter_user.username) + body_renderer.assert_(project='test_project') + body_renderer.assert_(role='Owner') + + assert pyramid_request.task.calls == [ + pretend.call(send_email), + ] + assert send_email.delay.calls == [ + pretend.call( + 'Email Body', + 'Email Subject', + recipients=[stub_user.email], + ), + ] + + +class TestRoleChangedForUserEmail: + + def test_role_changed_for_user_email( + self, pyramid_request, pyramid_config, monkeypatch): + + stub_user = pretend.stub( + email='email', + username='username', + ) + stub_submitter_user = pretend.stub( + email='submiteremail', + username='submitterusername' + ) + stub_role = pretend.stub( + role_name="Owner", + user=stub_user, + project=pretend.stub( + name="test_project" + ), + ) + pyramid_request.user = stub_submitter_user + + subject_renderer = pyramid_config.testing_add_renderer( + 'email/role-changed-for-user.subject.txt' + ) + subject_renderer.string_response = 'Email Subject' + body_renderer = pyramid_config.testing_add_renderer( + 'email/role-changed-for-user.body.txt' + ) + body_renderer.string_response = 'Email Body' + + send_email = pretend.stub( + delay=pretend.call_recorder(lambda *args, **kwargs: None) + ) + pyramid_request.task = pretend.call_recorder( + lambda *args, **kwargs: send_email + ) + monkeypatch.setattr(email, 'send_email', send_email) + + result = email.send_role_changed_for_user_email( + pyramid_request, + role=stub_role, + email_recipients=[stub_user.email, stub_submitter_user.email] + ) + + assert result == { + 'project': 'test_project', + 'role': 'Owner', + 'submitter': stub_submitter_user.username, + 'username': stub_user.username, + } + subject_renderer.assert_() + body_renderer.assert_(submitter=stub_submitter_user.username) + body_renderer.assert_(project='test_project') + body_renderer.assert_(role='Owner') + + assert pyramid_request.task.calls == [ + pretend.call(send_email), + ] + assert send_email.delay.calls == [ + pretend.call( + 'Email Body', + 'Email Subject', + bcc=[stub_user.email, stub_submitter_user.email], + ), + ] diff --git a/warehouse/email.py b/warehouse/email.py index 9a45c47d3cb7..6706f9d92900 100644 --- a/warehouse/email.py +++ b/warehouse/email.py @@ -189,3 +189,85 @@ def send_added_as_collaborator_email(request, submitter, project_name, role, request.task(send_email).delay(body, subject, recipients=[user_email]) return fields + + +def send_removed_from_role_email(request, role): + fields = { + 'project': role.project.name, + 'submitter': request.user.username, + 'role': role.role_name + } + + subject = render( + 'email/removed-as-collaborator.subject.txt', fields, request=request + ) + + body = render( + 'email/removed-as-collaborator.body.txt', fields, request=request + ) + + request.task(send_email).delay(body, subject, recipients=[role.user.email]) + + return fields + + +def send_role_removed_from_user_email(request, role, email_recipients): + fields = { + 'project': role.project.name, + 'submitter': request.user.username, + 'role': role.role_name, + 'username': role.user.username, + } + + subject = render( + 'email/role-removed-from-user.subject.txt', fields, request=request + ) + + body = render( + 'email/role-removed-from-user.body.txt', fields, request=request + ) + + request.task(send_email).delay(body, subject, bcc=email_recipients) + + return fields + + +def send_user_role_changed_email(request, role): + fields = { + 'project': role.project.name, + 'submitter': request.user.username, + 'role': role.role_name + } + + subject = render( + 'email/user-role-changed.subject.txt', fields, request=request + ) + + body = render( + 'email/user-role-changed.body.txt', fields, request=request + ) + + request.task(send_email).delay(body, subject, recipients=[role.user.email]) + + return fields + + +def send_role_changed_for_user_email(request, role, email_recipients): + fields = { + 'project': role.project.name, + 'submitter': request.user.username, + 'role': role.role_name, + 'username': role.user.username, + } + + subject = render( + 'email/role-changed-for-user.subject.txt', fields, request=request + ) + + body = render( + 'email/role-changed-for-user.body.txt', fields, request=request + ) + + request.task(send_email).delay(body, subject, bcc=email_recipients) + + return fields \ No newline at end of file diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 4306d31a7fe0..0641adaa2d71 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -24,7 +24,9 @@ from warehouse.email import ( send_account_deletion_email, send_added_as_collaborator_email, send_collaborator_added_email, send_email_verification_email, - send_password_change_email, send_primary_email_change_email + send_password_change_email, send_primary_email_change_email, + send_removed_from_role_email, send_role_changed_for_user_email, + send_role_removed_from_user_email, send_user_role_changed_email, ) from warehouse.manage.forms import ( AddEmailForm, ChangePasswordForm, CreateRoleForm, ChangeRoleForm, @@ -559,7 +561,8 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): .filter(Role.role_name == 'Owner', Role.project == project) ) owner_emails = [owner.user.email for owner in owners] - owner_emails.remove(request.user.email) + if request.user.email in owner_emails: + owner_emails.remove(request.user.email) send_collaborator_added_email( request, @@ -683,7 +686,23 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): submitted_from=request.remote_addr, ), ) + owners = ( + request.db.query(Role) + .join(Role.user) + .filter(Role.role_name == 'Owner', + Role.project == role.project) + ) + owner_emails = [owner.user.email for owner in owners] + if request.user.email in owner_emails: + owner_emails.remove(request.user.email) + role.role_name = form.role_name.data + send_user_role_changed_email(request, role) + send_role_changed_for_user_email( + request, + role, + owner_emails, + ) request.session.flash( 'Successfully changed role', queue="success" ) @@ -733,6 +752,18 @@ def delete_project_role(project, request): submitted_from=request.remote_addr, ), ) + owners = ( + request.db.query(Role) + .join(Role.user) + .filter(Role.role_name == 'Owner', + Role.project == role.project) + ) + owner_emails = [owner.user.email for owner in owners] + if request.user.email in owner_emails: + owner_emails.remove(request.user.email) + + send_removed_from_role_email(request, role) + send_role_removed_from_user_email(request, role, owner_emails) request.session.flash("Successfully removed role", queue="success") return HTTPSeeOther( diff --git a/warehouse/templates/email/removed-as-collaborator.body.txt b/warehouse/templates/email/removed-as-collaborator.body.txt new file mode 100644 index 000000000000..da6bb995d215 --- /dev/null +++ b/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 }}. + +If this was a mistake, you can reply to this email directly to communicate with +the PyPI administrators. diff --git a/warehouse/templates/email/removed-as-collaborator.subject.txt b/warehouse/templates/email/removed-as-collaborator.subject.txt new file mode 100644 index 000000000000..c22f6ec163c4 --- /dev/null +++ b/warehouse/templates/email/removed-as-collaborator.subject.txt @@ -0,0 +1 @@ +Role removed from PyPI diff --git a/warehouse/templates/email/role-changed-for-user.body.txt b/warehouse/templates/email/role-changed-for-user.body.txt new file mode 100644 index 000000000000..031ec2e32819 --- /dev/null +++ b/warehouse/templates/email/role-changed-for-user.body.txt @@ -0,0 +1,9 @@ +A collaborator's role has been changed in a project you own on PyPI: + + Username: {{ username }} + New role: {{ role }} + Collaborator for: {{ project }} + Changed by: {{ submitter }} + +If this was a mistake, you can reply to this email directly to communicate with +the PyPI administrators. diff --git a/warehouse/templates/email/role-changed-for-user.subject.txt b/warehouse/templates/email/role-changed-for-user.subject.txt new file mode 100644 index 000000000000..ebb44767c7ab --- /dev/null +++ b/warehouse/templates/email/role-changed-for-user.subject.txt @@ -0,0 +1 @@ +Role changed in PyPI \ No newline at end of file diff --git a/warehouse/templates/email/role-removed-from-user.body.txt b/warehouse/templates/email/role-removed-from-user.body.txt new file mode 100644 index 000000000000..11fd6955978c --- /dev/null +++ b/warehouse/templates/email/role-removed-from-user.body.txt @@ -0,0 +1,9 @@ +A collaborator's role has been removed from a project you own on PyPI: + + Username: {{ username }} + Role: {{ role }} + Collaborator for: {{ project }} + Changed by: {{ submitter }} + +If this was a mistake, you can reply to this email directly to communicate with +the PyPI administrators. diff --git a/warehouse/templates/email/role-removed-from-user.subject.txt b/warehouse/templates/email/role-removed-from-user.subject.txt new file mode 100644 index 000000000000..c22f6ec163c4 --- /dev/null +++ b/warehouse/templates/email/role-removed-from-user.subject.txt @@ -0,0 +1 @@ +Role removed from PyPI diff --git a/warehouse/templates/email/user-role-changed.body.txt b/warehouse/templates/email/user-role-changed.body.txt new file mode 100644 index 000000000000..c8d1784fd0d2 --- /dev/null +++ b/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 }}. + +If this was a mistake, you can reply to this email directly to communicate with +the PyPI administrators. diff --git a/warehouse/templates/email/user-role-changed.subject.txt b/warehouse/templates/email/user-role-changed.subject.txt new file mode 100644 index 000000000000..ebb44767c7ab --- /dev/null +++ b/warehouse/templates/email/user-role-changed.subject.txt @@ -0,0 +1 @@ +Role changed in PyPI \ No newline at end of file From 3c8556f566e20bd3e520596416c28a3a752f3fc3 Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Sun, 18 Mar 2018 08:43:31 -0700 Subject: [PATCH 2/8] Make linters happy --- tests/unit/manage/test_views.py | 1 - tests/unit/test_email.py | 4 ---- warehouse/email.py | 2 +- warehouse/manage/views.py | 16 ++++++++++------ 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index b863f27791f7..a2a4bd141c17 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -1725,7 +1725,6 @@ def test_delete_own_owner_role(self, db_request): assert result.headers["Location"] == "/the-redirect" - class TestManageProjectHistory: def test_get(self, db_request): diff --git a/tests/unit/test_email.py b/tests/unit/test_email.py index 3a1477e6d270..e133610fec36 100644 --- a/tests/unit/test_email.py +++ b/tests/unit/test_email.py @@ -20,10 +20,6 @@ from warehouse import email from warehouse.accounts.interfaces import ITokenService -from ..common.db.packaging import ( - JournalEntryFactory, ProjectFactory, ReleaseFactory, RoleFactory, - UserFactory, -) class TestSendEmail: diff --git a/warehouse/email.py b/warehouse/email.py index 6706f9d92900..5089ca2ee59f 100644 --- a/warehouse/email.py +++ b/warehouse/email.py @@ -270,4 +270,4 @@ def send_role_changed_for_user_email(request, role, email_recipients): request.task(send_email).delay(body, subject, bcc=email_recipients) - return fields \ No newline at end of file + return fields diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 0641adaa2d71..45eb4b0a8ea3 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -688,9 +688,11 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): ) owners = ( request.db.query(Role) - .join(Role.user) - .filter(Role.role_name == 'Owner', - Role.project == role.project) + .join(Role.user) + .filter( + Role.role_name == 'Owner', + Role.project == role.project, + ) ) owner_emails = [owner.user.email for owner in owners] if request.user.email in owner_emails: @@ -754,9 +756,11 @@ def delete_project_role(project, request): ) owners = ( request.db.query(Role) - .join(Role.user) - .filter(Role.role_name == 'Owner', - Role.project == role.project) + .join(Role.user) + .filter( + Role.role_name == 'Owner', + Role.project == role.project, + ) ) owner_emails = [owner.user.email for owner in owners] if request.user.email in owner_emails: From 1726c857859eb1c1dae1daa49a6947a4c50acdb5 Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Sun, 18 Mar 2018 10:32:47 -0700 Subject: [PATCH 3/8] Improve the tests and coverage --- tests/unit/manage/test_views.py | 411 ++++++++++++++++++++++++++++++-- warehouse/manage/views.py | 47 ++-- 2 files changed, 417 insertions(+), 41 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index a2a4bd141c17..059499cc0369 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -1292,16 +1292,7 @@ def test_post_new_role(self, monkeypatch, db_request): pretend.call("Added collaborator 'testuser'", queue="success"), ] - assert send_collaborator_added_email.calls == [ - pretend.call( - db_request, - user, - db_request.user, - project.name, - form_obj.role_name.data, - [] - ) - ] + assert send_collaborator_added_email.calls == [] assert send_added_as_collaborator_email.calls == [ pretend.call( @@ -1328,6 +1319,154 @@ def test_post_new_role(self, monkeypatch, db_request): assert entry.submitted_by == db_request.user assert entry.submitted_from == db_request.remote_addr + def test_post_new_role_notify_other_owners(self, monkeypatch, db_request): + project = ProjectFactory.create(name="foobar") + user = UserFactory.create(username="testuser") + + owner = UserFactory.create(username="owneruser") + EmailFactory.create(user=owner, email="owneremail") + other_owner = UserFactory.create(username="otherowner") + EmailFactory.create( + user=other_owner, + email="otherowneremail", + ) + + RoleFactory.create( + user=owner, project=project, role_name="Owner" + ) + RoleFactory.create( + user=other_owner, project=project, role_name="Owner" + ) + + user_service = pretend.stub( + find_userid=lambda username: user.id, + get_user=lambda userid: user, + ) + db_request.find_service = pretend.call_recorder( + lambda iface, context: user_service + ) + db_request.method = "POST" + db_request.POST = pretend.stub() + db_request.remote_addr = "10.10.10.10" + db_request.user = owner + form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: True), + username=pretend.stub(data=user.username), + role_name=pretend.stub(data="Owner"), + ) + form_class = pretend.call_recorder(lambda *a, **kw: form_obj) + db_request.session = pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + + send_collaborator_added_email = pretend.call_recorder(lambda *a: None) + monkeypatch.setattr( + views, + 'send_collaborator_added_email', + send_collaborator_added_email, + ) + + send_added_as_collaborator_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_added_as_collaborator_email', + send_added_as_collaborator_email, + ) + + views.manage_project_roles( + project, db_request, _form_class=form_class + ) + + assert db_request.session.flash.calls == [ + pretend.call("Added collaborator 'testuser'", queue="success"), + ] + + assert send_collaborator_added_email.calls == [ + pretend.call( + db_request, + user, + db_request.user, + project.name, + form_obj.role_name.data, + [other_owner.email] + ) + ] + + assert send_added_as_collaborator_email.calls == [ + pretend.call( + db_request, + db_request.user, + project.name, + form_obj.role_name.data, + user.email) + ] + + def test_post_new_role_no_other_owners(self, monkeypatch, db_request): + project = ProjectFactory.create(name="foobar") + user = UserFactory.create(username="testuser") + owner = UserFactory.create(username="owneruser") + EmailFactory.create(user=owner, email="owneremail") + + RoleFactory.create( + user=owner, project=project, role_name="Owner" + ) + + user_service = pretend.stub( + find_userid=lambda username: user.id, + get_user=lambda userid: user, + ) + db_request.find_service = pretend.call_recorder( + lambda iface, context: user_service + ) + db_request.method = "POST" + db_request.POST = pretend.stub() + db_request.remote_addr = "10.10.10.10" + db_request.user = owner + form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: True), + username=pretend.stub(data=user.username), + role_name=pretend.stub(data="Owner"), + ) + form_class = pretend.call_recorder(lambda *a, **kw: form_obj) + db_request.session = pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + + send_collaborator_added_email = pretend.call_recorder(lambda *a: None) + monkeypatch.setattr( + views, + 'send_collaborator_added_email', + send_collaborator_added_email, + ) + + send_added_as_collaborator_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_added_as_collaborator_email', + send_added_as_collaborator_email, + ) + + views.manage_project_roles( + project, db_request, _form_class=form_class + ) + + assert db_request.session.flash.calls == [ + pretend.call("Added collaborator 'testuser'", queue="success"), + ] + + assert send_collaborator_added_email.calls == [] + + assert send_added_as_collaborator_email.calls == [ + pretend.call( + db_request, + db_request.user, + project.name, + form_obj.role_name.data, + user.email) + ] + def test_post_duplicate_role(self, db_request): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") @@ -1442,20 +1581,138 @@ def test_change_role(self, db_request, monkeypatch): ) ] + assert send_role_changed_for_user_email.calls == [] + + entry = db_request.db.query(JournalEntry).one() + + assert entry.name == project.name + assert entry.action == "change Owner testuser to Maintainer" + assert entry.submitted_by == db_request.user + assert entry.submitted_from == db_request.remote_addr + + def test_change_role_email_sent_to_other_owners(self, db_request, + monkeypatch): + project = ProjectFactory.create(name="foobar") + user = UserFactory.create(username="testuser") + role = RoleFactory.create( + user=user, project=project, role_name="Owner" + ) + owner = UserFactory.create(username="owneruser") + EmailFactory.create(user=owner, email="owneremail") + other_owner = UserFactory.create(username="otherowner") + EmailFactory.create( + user=other_owner, + email="otherowneremail", + ) + + RoleFactory.create( + user=owner, project=project, role_name="Owner" + ) + RoleFactory.create( + user=other_owner, project=project, role_name="Owner" + ) + new_role_name = "Maintainer" + + db_request.method = "POST" + db_request.user = owner + db_request.remote_addr = "10.10.10.10" + db_request.POST = MultiDict({ + "role_id": role.id, + "role_name": new_role_name, + }) + db_request.session = pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + db_request.route_path = pretend.call_recorder( + lambda *a, **kw: "/the-redirect" + ) + + send_user_role_changed_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_user_role_changed_email', + send_user_role_changed_email + ) + + send_role_changed_for_user_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_role_changed_for_user_email', + send_role_changed_for_user_email + ) + + views.change_project_role(project, db_request) + + assert send_user_role_changed_email.calls == [ + pretend.call( + db_request, + role, + ) + ] + assert send_role_changed_for_user_email.calls == [ pretend.call( db_request, role, - [], + [other_owner.email], ) ] - entry = db_request.db.query(JournalEntry).one() + def test_change_role_email_no_other_owners(self, db_request, monkeypatch): + project = ProjectFactory.create(name="foobar") + user = UserFactory.create(username="testuser") + role = RoleFactory.create( + user=user, project=project, role_name="Owner" + ) + owner = UserFactory.create(username="owneruser") - assert entry.name == project.name - assert entry.action == "change Owner testuser to Maintainer" - assert entry.submitted_by == db_request.user - assert entry.submitted_from == db_request.remote_addr + RoleFactory.create( + user=owner, project=project, role_name="Owner" + ) + new_role_name = "Maintainer" + + db_request.method = "POST" + db_request.user = owner + db_request.remote_addr = "10.10.10.10" + db_request.POST = MultiDict({ + "role_id": role.id, + "role_name": new_role_name, + }) + db_request.session = pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + db_request.route_path = pretend.call_recorder( + lambda *a, **kw: "/the-redirect" + ) + + send_user_role_changed_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_user_role_changed_email', + send_user_role_changed_email + ) + + send_role_changed_for_user_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_role_changed_for_user_email', + send_role_changed_for_user_email + ) + + views.change_project_role(project, db_request) + + assert send_user_role_changed_email.calls == [ + pretend.call( + db_request, + role, + ) + ] + + assert send_role_changed_for_user_email.calls == [] def test_change_role_invalid_role_name(self, pyramid_request): project = pretend.stub(name="foobar") @@ -1662,20 +1919,130 @@ def test_delete_role(self, db_request, monkeypatch): ) ] + assert send_role_removed_from_user_email.calls == [] + + entry = db_request.db.query(JournalEntry).one() + + assert entry.name == project.name + assert entry.action == "remove Owner testuser" + assert entry.submitted_by == db_request.user + assert entry.submitted_from == db_request.remote_addr + + def test_delete_role_notify_other_owners(self, db_request, monkeypatch): + project = ProjectFactory.create(name="foobar") + user = UserFactory.create(username="testuser") + role = RoleFactory.create( + user=user, project=project, role_name="Owner" + ) + owner = UserFactory.create(username="owneruser") + EmailFactory.create(user=owner, email="owneremail") + other_owner = UserFactory.create(username="otherowner") + EmailFactory.create( + user=other_owner, + email="otherowneremail", + ) + + RoleFactory.create( + user=owner, project=project, role_name="Owner" + ) + RoleFactory.create( + user=other_owner, project=project, role_name="Owner" + ) + + db_request.method = "POST" + db_request.user = owner + db_request.remote_addr = "10.10.10.10" + db_request.POST = MultiDict({"role_id": role.id}) + db_request.session = pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + db_request.route_path = pretend.call_recorder( + lambda *a, **kw: "/the-redirect" + ) + + send_removed_from_role_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_removed_from_role_email', + send_removed_from_role_email + ) + + send_role_removed_from_user_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_role_removed_from_user_email', + send_role_removed_from_user_email + ) + + views.delete_project_role(project, db_request) + + assert send_removed_from_role_email.calls == [ + pretend.call( + db_request, + role, + ) + ] + assert send_role_removed_from_user_email.calls == [ pretend.call( db_request, role, - [], + [other_owner.email], ) ] - entry = db_request.db.query(JournalEntry).one() + def test_delete_role_no_other_owner(self, db_request, monkeypatch): + project = ProjectFactory.create(name="foobar") + user = UserFactory.create(username="testuser") + role = RoleFactory.create( + user=user, project=project, role_name="Owner" + ) + owner = UserFactory.create(username="owneruser") + EmailFactory.create(user=owner, email="owneremail") - assert entry.name == project.name - assert entry.action == "remove Owner testuser" - assert entry.submitted_by == db_request.user - assert entry.submitted_from == db_request.remote_addr + RoleFactory.create( + user=owner, project=project, role_name="Owner" + ) + + db_request.method = "POST" + db_request.user = owner + db_request.remote_addr = "10.10.10.10" + db_request.POST = MultiDict({"role_id": role.id}) + db_request.session = pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + db_request.route_path = pretend.call_recorder( + lambda *a, **kw: "/the-redirect" + ) + + send_removed_from_role_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_removed_from_role_email', + send_removed_from_role_email + ) + + send_role_removed_from_user_email = pretend.call_recorder( + lambda *a: None) + monkeypatch.setattr( + views, + 'send_role_removed_from_user_email', + send_role_removed_from_user_email + ) + + views.delete_project_role(project, db_request) + + assert send_removed_from_role_email.calls == [ + pretend.call( + db_request, + role, + ) + ] + + assert send_role_removed_from_user_email.calls == [] def test_delete_missing_role(self, db_request): project = ProjectFactory.create(name="foobar") diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 45eb4b0a8ea3..3a705e278576 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -560,19 +560,24 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): .join(Role.user) .filter(Role.role_name == 'Owner', Role.project == project) ) - owner_emails = [owner.user.email for owner in owners] + 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) - - send_collaborator_added_email( - request, - user, - request.user, - project.name, - form.role_name.data, - owner_emails - ) - + print("owneremails") + print(owner_emails) + print("owners") + print(owners) + print("---") + if owner_emails: + send_collaborator_added_email( + request, + user, + request.user, + project.name, + form.role_name.data, + owner_emails + ) send_added_as_collaborator_email( request, request.user, @@ -694,17 +699,19 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): Role.project == role.project, ) ) - owner_emails = [owner.user.email for owner in owners] + 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) role.role_name = form.role_name.data send_user_role_changed_email(request, role) - send_role_changed_for_user_email( - request, - role, - owner_emails, - ) + if owner_emails: + send_role_changed_for_user_email( + request, + role, + owner_emails, + ) request.session.flash( 'Successfully changed role', queue="success" ) @@ -762,12 +769,14 @@ def delete_project_role(project, request): Role.project == role.project, ) ) - owner_emails = [owner.user.email for owner in owners] + 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) send_removed_from_role_email(request, role) - send_role_removed_from_user_email(request, role, owner_emails) + if owner_emails: + send_role_removed_from_user_email(request, role, owner_emails) request.session.flash("Successfully removed role", queue="success") return HTTPSeeOther( From 8f5ae4bf50d3d3fb885463fc0d3b670adbb8baa9 Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Mon, 19 Mar 2018 21:54:48 -0700 Subject: [PATCH 4/8] Update email text. --- warehouse/manage/views.py | 6 +----- .../email/added-as-collaborator.body.txt | 15 ++++++++++++++- .../email/removed-as-collaborator.body.txt | 12 +++++++++++- .../templates/email/user-role-changed.body.txt | 12 +++++++++++- 4 files changed, 37 insertions(+), 8 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 3a705e278576..f554be91a4a1 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -564,11 +564,7 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): if owner.user.email] if request.user.email in owner_emails: owner_emails.remove(request.user.email) - print("owneremails") - print(owner_emails) - print("owners") - print(owners) - print("---") + if owner_emails: send_collaborator_added_email( request, diff --git a/warehouse/templates/email/added-as-collaborator.body.txt b/warehouse/templates/email/added-as-collaborator.body.txt index 2f18248512f6..edac65a29ff1 100644 --- a/warehouse/templates/email/added-as-collaborator.body.txt +++ b/warehouse/templates/email/added-as-collaborator.body.txt @@ -1,4 +1,17 @@ -You have been added as {{ role }} to the PyPI project {{ project }} by {{ submitter }}. +You have been added as a collaborator to the PyPI project {{ project }} +by {{ submitter }}. + +{% if role == 'Owner' %} +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. +{% endif %} + +{% if role == 'Maintainer' %} +You are now a project maintainer. This means that you may upload releases for +the project. You cannot add collaborators, delete files, delete releases, or +delete the project. +{% endif %} Congratulations! diff --git a/warehouse/templates/email/removed-as-collaborator.body.txt b/warehouse/templates/email/removed-as-collaborator.body.txt index da6bb995d215..5cd0326ab23f 100644 --- a/warehouse/templates/email/removed-as-collaborator.body.txt +++ b/warehouse/templates/email/removed-as-collaborator.body.txt @@ -1,6 +1,16 @@ Your role in the {{ project }} project has been removed by {{ submitter }}. -You are no longer an {{ role }}. +{% if role == 'Owner' %} +You are no longer a project owner. This means that you can no longer add other +collaborators, upload releases for the project, delete files and releases, or +delete the entire project. +{% endif %} + +{% if role == 'Maintainer' %} +You are no longer a project maintainer. This means that you can no longer add +other collaborators, upload releases for the project, delete files and releases, +or delete the entire project. +{% endif %} If this was a mistake, you can reply to this email directly to communicate with the PyPI administrators. diff --git a/warehouse/templates/email/user-role-changed.body.txt b/warehouse/templates/email/user-role-changed.body.txt index c8d1784fd0d2..5cf017551f84 100644 --- a/warehouse/templates/email/user-role-changed.body.txt +++ b/warehouse/templates/email/user-role-changed.body.txt @@ -1,6 +1,16 @@ Your role in the {{ project }} project has been changed by {{ submitter }}. -You are now a {{ role }}. +{% if role == 'Owner' %} +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. +{% endif %} + +{% if role == 'Maintainer' %} +You are now a project maintainer. This means that you may upload releases for +the project. You cannot add collaborators, delete files, delete releases, or +delete the project. +{% endif %} If this was a mistake, you can reply to this email directly to communicate with the PyPI administrators. From aebf92d2255a4229376579c9b7cbe5add87cad96 Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Tue, 20 Mar 2018 22:33:41 -0500 Subject: [PATCH 5/8] Notify all owners for any role changes. --- tests/unit/manage/test_views.py | 184 ++------------------------------ warehouse/manage/views.py | 6 -- 2 files changed, 7 insertions(+), 183 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 059499cc0369..fa5eb6548ed8 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -1319,7 +1319,7 @@ def test_post_new_role(self, monkeypatch, db_request): assert entry.submitted_by == db_request.user assert entry.submitted_from == db_request.remote_addr - def test_post_new_role_notify_other_owners(self, monkeypatch, db_request): + def test_post_new_role_notify_all_owners(self, monkeypatch, db_request): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") @@ -1389,7 +1389,7 @@ def test_post_new_role_notify_other_owners(self, monkeypatch, db_request): db_request.user, project.name, form_obj.role_name.data, - [other_owner.email] + [owner.email, other_owner.email] ) ] @@ -1402,71 +1402,6 @@ def test_post_new_role_notify_other_owners(self, monkeypatch, db_request): user.email) ] - def test_post_new_role_no_other_owners(self, monkeypatch, db_request): - project = ProjectFactory.create(name="foobar") - user = UserFactory.create(username="testuser") - owner = UserFactory.create(username="owneruser") - EmailFactory.create(user=owner, email="owneremail") - - RoleFactory.create( - user=owner, project=project, role_name="Owner" - ) - - user_service = pretend.stub( - find_userid=lambda username: user.id, - get_user=lambda userid: user, - ) - db_request.find_service = pretend.call_recorder( - lambda iface, context: user_service - ) - db_request.method = "POST" - db_request.POST = pretend.stub() - db_request.remote_addr = "10.10.10.10" - db_request.user = owner - form_obj = pretend.stub( - validate=pretend.call_recorder(lambda: True), - username=pretend.stub(data=user.username), - role_name=pretend.stub(data="Owner"), - ) - form_class = pretend.call_recorder(lambda *a, **kw: form_obj) - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None), - ) - - send_collaborator_added_email = pretend.call_recorder(lambda *a: None) - monkeypatch.setattr( - views, - 'send_collaborator_added_email', - send_collaborator_added_email, - ) - - send_added_as_collaborator_email = pretend.call_recorder( - lambda *a: None) - monkeypatch.setattr( - views, - 'send_added_as_collaborator_email', - send_added_as_collaborator_email, - ) - - views.manage_project_roles( - project, db_request, _form_class=form_class - ) - - assert db_request.session.flash.calls == [ - pretend.call("Added collaborator 'testuser'", queue="success"), - ] - - assert send_collaborator_added_email.calls == [] - - assert send_added_as_collaborator_email.calls == [ - pretend.call( - db_request, - db_request.user, - project.name, - form_obj.role_name.data, - user.email) - ] - def test_post_duplicate_role(self, db_request): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") @@ -1590,8 +1525,8 @@ def test_change_role(self, db_request, monkeypatch): assert entry.submitted_by == db_request.user assert entry.submitted_from == db_request.remote_addr - def test_change_role_email_sent_to_other_owners(self, db_request, - monkeypatch): + def test_change_role_email_notify_all_owners(self, db_request, + monkeypatch): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") role = RoleFactory.create( @@ -1656,64 +1591,10 @@ def test_change_role_email_sent_to_other_owners(self, db_request, pretend.call( db_request, role, - [other_owner.email], + [owner.email, other_owner.email], ) ] - def test_change_role_email_no_other_owners(self, db_request, monkeypatch): - project = ProjectFactory.create(name="foobar") - user = UserFactory.create(username="testuser") - role = RoleFactory.create( - user=user, project=project, role_name="Owner" - ) - owner = UserFactory.create(username="owneruser") - - RoleFactory.create( - user=owner, project=project, role_name="Owner" - ) - new_role_name = "Maintainer" - - db_request.method = "POST" - db_request.user = owner - db_request.remote_addr = "10.10.10.10" - db_request.POST = MultiDict({ - "role_id": role.id, - "role_name": new_role_name, - }) - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None), - ) - db_request.route_path = pretend.call_recorder( - lambda *a, **kw: "/the-redirect" - ) - - send_user_role_changed_email = pretend.call_recorder( - lambda *a: None) - monkeypatch.setattr( - views, - 'send_user_role_changed_email', - send_user_role_changed_email - ) - - send_role_changed_for_user_email = pretend.call_recorder( - lambda *a: None) - monkeypatch.setattr( - views, - 'send_role_changed_for_user_email', - send_role_changed_for_user_email - ) - - views.change_project_role(project, db_request) - - assert send_user_role_changed_email.calls == [ - pretend.call( - db_request, - role, - ) - ] - - assert send_role_changed_for_user_email.calls == [] - def test_change_role_invalid_role_name(self, pyramid_request): project = pretend.stub(name="foobar") @@ -1928,7 +1809,7 @@ def test_delete_role(self, db_request, monkeypatch): assert entry.submitted_by == db_request.user assert entry.submitted_from == db_request.remote_addr - def test_delete_role_notify_other_owners(self, db_request, monkeypatch): + def test_delete_role_notify_all_owners(self, db_request, monkeypatch): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") role = RoleFactory.create( @@ -1989,61 +1870,10 @@ def test_delete_role_notify_other_owners(self, db_request, monkeypatch): pretend.call( db_request, role, - [other_owner.email], + [owner.email, other_owner.email], ) ] - def test_delete_role_no_other_owner(self, db_request, monkeypatch): - project = ProjectFactory.create(name="foobar") - user = UserFactory.create(username="testuser") - role = RoleFactory.create( - user=user, project=project, role_name="Owner" - ) - owner = UserFactory.create(username="owneruser") - EmailFactory.create(user=owner, email="owneremail") - - RoleFactory.create( - user=owner, project=project, role_name="Owner" - ) - - db_request.method = "POST" - db_request.user = owner - db_request.remote_addr = "10.10.10.10" - db_request.POST = MultiDict({"role_id": role.id}) - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None), - ) - db_request.route_path = pretend.call_recorder( - lambda *a, **kw: "/the-redirect" - ) - - send_removed_from_role_email = pretend.call_recorder( - lambda *a: None) - monkeypatch.setattr( - views, - 'send_removed_from_role_email', - send_removed_from_role_email - ) - - send_role_removed_from_user_email = pretend.call_recorder( - lambda *a: None) - monkeypatch.setattr( - views, - 'send_role_removed_from_user_email', - send_role_removed_from_user_email - ) - - views.delete_project_role(project, db_request) - - assert send_removed_from_role_email.calls == [ - pretend.call( - db_request, - role, - ) - ] - - assert send_role_removed_from_user_email.calls == [] - def test_delete_missing_role(self, db_request): project = ProjectFactory.create(name="foobar") missing_role_id = str(uuid.uuid4()) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index f554be91a4a1..55da46643af5 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -562,8 +562,6 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): ) 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) if owner_emails: send_collaborator_added_email( @@ -697,8 +695,6 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): ) 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) role.role_name = form.role_name.data send_user_role_changed_email(request, role) @@ -767,8 +763,6 @@ def delete_project_role(project, request): ) 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) send_removed_from_role_email(request, role) if owner_emails: From 18c4cc13cee7c072036595fb51afe153949b03aa Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Thu, 29 Mar 2018 21:43:45 -0700 Subject: [PATCH 6/8] Update email subjects. Update tests. --- tests/unit/manage/test_views.py | 70 ++++++++++++++++--- warehouse/email.py | 10 +-- warehouse/manage/views.py | 40 +++++------ .../email/removed-as-collaborator.subject.txt | 2 +- .../email/role-changed-for-user.subject.txt | 2 +- .../email/role-removed-from-user.body.txt | 2 +- .../email/role-removed-from-user.subject.txt | 2 +- .../email/user-role-changed.subject.txt | 2 +- 8 files changed, 88 insertions(+), 42 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index fa5eb6548ed8..6caeb2460f5a 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -1239,7 +1239,15 @@ def test_post_new_role_validation_fails(self, db_request): def test_post_new_role(self, monkeypatch, db_request): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") + EmailFactory.create(user=user, email="useremail") + otherowner = UserFactory.create(username="otherowner") + otherowner_role = RoleFactory.create( + user=otherowner, + project=project, + role_name="Owner", + ) + EmailFactory.create(user=otherowner, email="otherowneremail") user_service = pretend.stub( find_userid=lambda username: user.id, get_user=lambda userid: user, @@ -1250,7 +1258,7 @@ def test_post_new_role(self, monkeypatch, db_request): db_request.method = "POST" db_request.POST = pretend.stub() db_request.remote_addr = "10.10.10.10" - db_request.user = UserFactory.create() + db_request.user = otherowner form_obj = pretend.stub( validate=pretend.call_recorder(lambda: True), username=pretend.stub(data=user.username), @@ -1292,7 +1300,16 @@ def test_post_new_role(self, monkeypatch, db_request): pretend.call("Added collaborator 'testuser'", queue="success"), ] - assert send_collaborator_added_email.calls == [] + assert send_collaborator_added_email.calls == [ + pretend.call( + db_request, + user, + db_request.user, + project.name, + form_obj.role_name.data, + [otherowner.email], + ), + ] assert send_added_as_collaborator_email.calls == [ pretend.call( @@ -1300,15 +1317,19 @@ def test_post_new_role(self, monkeypatch, db_request): db_request.user, project.name, form_obj.role_name.data, - user.email) + user.email, + ), ] # Only one role is created - role = db_request.db.query(Role).one() + role = db_request.db.query(Role).filter(Role.user == user).one() assert result == { "project": project, - "roles_by_user": {user.username: [role]}, + "roles_by_user": { + user.username: [role], + otherowner.username: [otherowner_role], + }, "form": form_obj, } @@ -1322,6 +1343,7 @@ def test_post_new_role(self, monkeypatch, db_request): def test_post_new_role_notify_all_owners(self, monkeypatch, db_request): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") + EmailFactory.create(user=user, email="useremail") owner = UserFactory.create(username="owneruser") EmailFactory.create(user=owner, email="owneremail") @@ -1465,10 +1487,17 @@ def test_change_role(self, db_request, monkeypatch): role = RoleFactory.create( user=user, project=project, role_name="Owner" ) + EmailFactory.create(user=user, email="useremail") + + otherowner = UserFactory.create(username="otherowner") + RoleFactory.create( + user=otherowner, project=project, role_name="Owner" + ) + EmailFactory.create(user=otherowner, email="otherowneremail") new_role_name = "Maintainer" db_request.method = "POST" - db_request.user = UserFactory.create() + db_request.user = otherowner db_request.remote_addr = "10.10.10.10" db_request.POST = MultiDict({ "role_id": role.id, @@ -1513,10 +1542,16 @@ def test_change_role(self, db_request, monkeypatch): pretend.call( db_request, role, - ) + ), ] - assert send_role_changed_for_user_email.calls == [] + assert send_role_changed_for_user_email.calls == [ + pretend.call( + db_request, + role, + [otherowner.email], + ), + ] entry = db_request.db.query(JournalEntry).one() @@ -1532,6 +1567,7 @@ def test_change_role_email_notify_all_owners(self, db_request, role = RoleFactory.create( user=user, project=project, role_name="Owner" ) + EmailFactory.create(user=user, email="useremail") owner = UserFactory.create(username="owneruser") EmailFactory.create(user=owner, email="owneremail") other_owner = UserFactory.create(username="otherowner") @@ -1754,9 +1790,15 @@ def test_delete_role(self, db_request, monkeypatch): role = RoleFactory.create( user=user, project=project, role_name="Owner" ) + EmailFactory.create(user=user, email="useremail") + otherowner = UserFactory.create(username="otherowner") + otherowner_role = RoleFactory.create( + user=otherowner, project=project, role_name="Owner" + ) + EmailFactory.create(user=otherowner, email="otherowneremail") db_request.method = "POST" - db_request.user = UserFactory.create() + db_request.user = otherowner db_request.remote_addr = "10.10.10.10" db_request.POST = MultiDict({"role_id": role.id}) db_request.session = pretend.stub( @@ -1786,7 +1828,7 @@ def test_delete_role(self, db_request, monkeypatch): assert db_request.route_path.calls == [ pretend.call('manage.project.roles', project_name=project.name), ] - assert db_request.db.query(Role).all() == [] + assert db_request.db.query(Role).all() == [otherowner_role] assert db_request.session.flash.calls == [ pretend.call("Successfully removed role", queue="success"), ] @@ -1800,7 +1842,13 @@ def test_delete_role(self, db_request, monkeypatch): ) ] - assert send_role_removed_from_user_email.calls == [] + assert send_role_removed_from_user_email.calls == [ + pretend.call( + db_request, + role, + [otherowner.email], + ), + ] entry = db_request.db.query(JournalEntry).one() diff --git a/warehouse/email.py b/warehouse/email.py index 5089ca2ee59f..036155010ebd 100644 --- a/warehouse/email.py +++ b/warehouse/email.py @@ -165,7 +165,8 @@ def send_collaborator_added_email(request, user, submitter, project_name, role, 'email/collaborator-added.body.txt', fields, request=request ) - request.task(send_email).delay(body, subject, bcc=email_recipients) + if email_recipients: + request.task(send_email).delay(body, subject, bcc=email_recipients) return fields @@ -227,8 +228,8 @@ def send_role_removed_from_user_email(request, role, email_recipients): 'email/role-removed-from-user.body.txt', fields, request=request ) - request.task(send_email).delay(body, subject, bcc=email_recipients) - + if email_recipients: + request.task(send_email).delay(body, subject, bcc=email_recipients) return fields @@ -268,6 +269,7 @@ def send_role_changed_for_user_email(request, role, email_recipients): 'email/role-changed-for-user.body.txt', fields, request=request ) - request.task(send_email).delay(body, subject, bcc=email_recipients) + if email_recipients: + request.task(send_email).delay(body, subject, bcc=email_recipients) return fields diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 55da46643af5..112012e77e36 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -561,17 +561,16 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): .filter(Role.role_name == 'Owner', Role.project == project) ) owner_emails = [owner.user.email for owner in owners - if owner.user.email] - - if owner_emails: - send_collaborator_added_email( - request, - user, - request.user, - project.name, - form.role_name.data, - owner_emails - ) + if owner.user.email != user.email] + + send_collaborator_added_email( + request, + user, + request.user, + project.name, + form.role_name.data, + owner_emails, + ) send_added_as_collaborator_email( request, request.user, @@ -694,16 +693,15 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): ) ) owner_emails = [owner.user.email for owner in owners - if owner.user.email] + if owner.user.email != role.user.email] role.role_name = form.role_name.data send_user_role_changed_email(request, role) - if owner_emails: - send_role_changed_for_user_email( - request, - role, - owner_emails, - ) + send_role_changed_for_user_email( + request, + role, + owner_emails, + ) request.session.flash( 'Successfully changed role', queue="success" ) @@ -761,12 +759,10 @@ def delete_project_role(project, request): Role.project == role.project, ) ) - owner_emails = [owner.user.email for owner in owners - if owner.user.email] + owner_emails = [owner.user.email for owner in owners] send_removed_from_role_email(request, role) - if owner_emails: - send_role_removed_from_user_email(request, role, owner_emails) + send_role_removed_from_user_email(request, role, owner_emails) request.session.flash("Successfully removed role", queue="success") return HTTPSeeOther( diff --git a/warehouse/templates/email/removed-as-collaborator.subject.txt b/warehouse/templates/email/removed-as-collaborator.subject.txt index c22f6ec163c4..5f78440df7a1 100644 --- a/warehouse/templates/email/removed-as-collaborator.subject.txt +++ b/warehouse/templates/email/removed-as-collaborator.subject.txt @@ -1 +1 @@ -Role removed from PyPI +Role removed on PyPI for project {{ project }} diff --git a/warehouse/templates/email/role-changed-for-user.subject.txt b/warehouse/templates/email/role-changed-for-user.subject.txt index ebb44767c7ab..2201101c0d24 100644 --- a/warehouse/templates/email/role-changed-for-user.subject.txt +++ b/warehouse/templates/email/role-changed-for-user.subject.txt @@ -1 +1 @@ -Role changed in PyPI \ No newline at end of file +Role changed on PyPI for project {{ project }} \ No newline at end of file diff --git a/warehouse/templates/email/role-removed-from-user.body.txt b/warehouse/templates/email/role-removed-from-user.body.txt index 11fd6955978c..aa1cea8330e5 100644 --- a/warehouse/templates/email/role-removed-from-user.body.txt +++ b/warehouse/templates/email/role-removed-from-user.body.txt @@ -1,7 +1,7 @@ A collaborator's role has been removed from a project you own on PyPI: Username: {{ username }} - Role: {{ role }} + Role was previously: {{ role }} Collaborator for: {{ project }} Changed by: {{ submitter }} diff --git a/warehouse/templates/email/role-removed-from-user.subject.txt b/warehouse/templates/email/role-removed-from-user.subject.txt index c22f6ec163c4..6d1c406878d4 100644 --- a/warehouse/templates/email/role-removed-from-user.subject.txt +++ b/warehouse/templates/email/role-removed-from-user.subject.txt @@ -1 +1 @@ -Role removed from PyPI +Role removed on PyPI for project {{ project }} \ No newline at end of file diff --git a/warehouse/templates/email/user-role-changed.subject.txt b/warehouse/templates/email/user-role-changed.subject.txt index ebb44767c7ab..2201101c0d24 100644 --- a/warehouse/templates/email/user-role-changed.subject.txt +++ b/warehouse/templates/email/user-role-changed.subject.txt @@ -1 +1 @@ -Role changed in PyPI \ No newline at end of file +Role changed on PyPI for project {{ project }} \ No newline at end of file From 6ece563025e3ef73f1b456f2e8d34a6c5b133d2d Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Thu, 29 Mar 2018 22:03:45 -0700 Subject: [PATCH 7/8] Since we're always sending email to all owners now when role changed, email recipients shouldn't ever be empty. --- warehouse/email.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/warehouse/email.py b/warehouse/email.py index 036155010ebd..f52d404102fc 100644 --- a/warehouse/email.py +++ b/warehouse/email.py @@ -165,8 +165,7 @@ def send_collaborator_added_email(request, user, submitter, project_name, role, 'email/collaborator-added.body.txt', fields, request=request ) - if email_recipients: - request.task(send_email).delay(body, subject, bcc=email_recipients) + request.task(send_email).delay(body, subject, bcc=email_recipients) return fields @@ -228,8 +227,7 @@ def send_role_removed_from_user_email(request, role, email_recipients): 'email/role-removed-from-user.body.txt', fields, request=request ) - if email_recipients: - request.task(send_email).delay(body, subject, bcc=email_recipients) + request.task(send_email).delay(body, subject, bcc=email_recipients) return fields @@ -269,7 +267,6 @@ def send_role_changed_for_user_email(request, role, email_recipients): 'email/role-changed-for-user.body.txt', fields, request=request ) - if email_recipients: - request.task(send_email).delay(body, subject, bcc=email_recipients) + request.task(send_email).delay(body, subject, bcc=email_recipients) return fields From aa144b64500f3b78b3c566b2d5f52e9946cf28b8 Mon Sep 17 00:00:00 2001 From: Mariatta Wijaya Date: Fri, 30 Mar 2018 10:09:53 -0700 Subject: [PATCH 8/8] The order matters? --- tests/unit/manage/test_views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 6caeb2460f5a..384520cdd0be 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -1411,7 +1411,7 @@ def test_post_new_role_notify_all_owners(self, monkeypatch, db_request): db_request.user, project.name, form_obj.role_name.data, - [owner.email, other_owner.email] + [other_owner.email, owner.email] ) ] @@ -1627,7 +1627,7 @@ def test_change_role_email_notify_all_owners(self, db_request, pretend.call( db_request, role, - [owner.email, other_owner.email], + [other_owner.email, owner.email], ) ] @@ -1918,7 +1918,7 @@ def test_delete_role_notify_all_owners(self, db_request, monkeypatch): pretend.call( db_request, role, - [owner.email, other_owner.email], + [other_owner.email, owner.email], ) ]