Add JournalEntries when adding/removing roles

tests/unit/manage/test_views.py

@@ -19,7 +19,7 @@
 
 from warehouse.manage import views
 from warehouse.accounts.interfaces import IUserService
-from warehouse.packaging.models import Role
+from warehouse.packaging.models import JournalEntry, Role
 
 from ...common.db.packaging import ProjectFactory, RoleFactory, UserFactory
 
@@ -125,6 +125,8 @@ def test_post_new_role(self, db_request):
         )
         db_request.method = "POST"
         db_request.POST = pretend.stub()
+        db_request.remote_addr = "10.10.10.10"
+        db_request.user = UserFactory.create()
         form_obj = pretend.stub(
             validate=pretend.call_recorder(lambda: True),
             username=pretend.stub(data=user.username),
@@ -160,6 +162,13 @@ def test_post_new_role(self, db_request):
             "form": form_obj,
         }
 
+        entry = db_request.db.query(JournalEntry).one()
+
+        assert entry.name == project.name
+        assert entry.action == "add Owner testuser"
+        assert entry.submitted_by == db_request.user
+        assert entry.submitted_from == db_request.remote_addr
+
     def test_post_duplicate_role(self, db_request):
         project = ProjectFactory.create(name="foobar")
         user = UserFactory.create(username="testuser")
@@ -226,7 +235,8 @@ def test_change_role(self, db_request):
         new_role_name = "Maintainer"
 
         db_request.method = "POST"
-        db_request.user = pretend.stub()
+        db_request.user = UserFactory.create()
+        db_request.remote_addr = "10.10.10.10"
         db_request.POST = MultiDict({
             "role_id": role.id,
             "role_name": new_role_name,
@@ -250,6 +260,13 @@ def test_change_role(self, db_request):
         assert isinstance(result, HTTPSeeOther)
         assert result.headers["Location"] == "/the-redirect"
 
+        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_invalid_role_name(self, pyramid_request):
         project = pretend.stub(name="foobar")
 
@@ -282,7 +299,8 @@ def test_change_role_when_multiple(self, db_request):
         new_role_name = "Maintainer"
 
         db_request.method = "POST"
-        db_request.user = pretend.stub()
+        db_request.user = UserFactory.create()
+        db_request.remote_addr = "10.10.10.10"
         db_request.POST = MultiDict([
             ("role_id", owner_role.id),
             ("role_id", maintainer_role.id),
@@ -307,6 +325,13 @@ def test_change_role_when_multiple(self, db_request):
         assert isinstance(result, HTTPSeeOther)
         assert result.headers["Location"] == "/the-redirect"
 
+        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_change_missing_role(self, db_request):
         project = ProjectFactory.create(name="foobar")
         missing_role_id = str(uuid.uuid4())
@@ -360,6 +385,38 @@ def test_change_own_owner_role(self, db_request):
         assert isinstance(result, HTTPSeeOther)
         assert result.headers["Location"] == "/the-redirect"
 
+    def test_change_own_owner_role_when_multiple(self, db_request):
+        project = ProjectFactory.create(name="foobar")
+        user = UserFactory.create(username="testuser")
+        owner_role = RoleFactory.create(
+            user=user, project=project, role_name="Owner"
+        )
+        maintainer_role = RoleFactory.create(
+            user=user, project=project, role_name="Maintainer"
+        )
+
+        db_request.method = "POST"
+        db_request.user = user
+        db_request.POST = MultiDict([
+            ("role_id", owner_role.id),
+            ("role_id", maintainer_role.id),
+            ("role_name", "Maintainer"),
+        ])
+        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"
+        )
+
+        result = views.change_project_role(project, db_request)
+
+        assert db_request.session.flash.calls == [
+            pretend.call("Cannot remove yourself as Owner", queue="error"),
+        ]
+        assert isinstance(result, HTTPSeeOther)
+        assert result.headers["Location"] == "/the-redirect"
+
 
 class TestDeleteProjectRoles:
 
@@ -371,7 +428,8 @@ def test_delete_role(self, db_request):
         )
 
         db_request.method = "POST"
-        db_request.user = pretend.stub()
+        db_request.user = UserFactory.create()
+        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),
@@ -392,6 +450,13 @@ def test_delete_role(self, db_request):
         assert isinstance(result, HTTPSeeOther)
         assert result.headers["Location"] == "/the-redirect"
 
+        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_missing_role(self, db_request):
         project = ProjectFactory.create(name="foobar")
         missing_role_id = str(uuid.uuid4())

warehouse/manage/views.py

@@ -1,4 +1,5 @@
 # Licensed under the Apache License, Version 2.0 (the "License");
+
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
 #
@@ -20,7 +21,7 @@
 from warehouse.accounts.interfaces import IUserService
 from warehouse.accounts.models import User
 from warehouse.manage.forms import CreateRoleForm, ChangeRoleForm
-from warehouse.packaging.models import Role
+from warehouse.packaging.models import JournalEntry, Role
 
 
 @view_config(
@@ -86,6 +87,14 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm):
             request.db.add(
                 Role(user=user, project=project, role_name=form.role_name.data)
             )
+            request.db.add(
+                JournalEntry(
+                    name=project.name,
+                    action=f"add {role_name} {username}",
+                    submitted_by=request.user,
+                    submitted_from=request.remote_addr,
+                ),
+            )
             request.session.flash(
                 f"Added collaborator '{form.username.data}'",
                 queue="success"
@@ -131,19 +140,38 @@ def change_project_role(project, request, _form_class=ChangeRoleForm):
             # This user has more than one role, so just delete all the ones
             # that aren't what we want.
             #
-            # This should be removed when fixing GH-2745.
-            (
+            # This branch should be removed when fixing GH-2745.
+            roles = (
                 request.db.query(Role)
                 .filter(
                     Role.id.in_(role_ids),
                     Role.project == project,
                     Role.role_name != form.role_name.data
                 )
-                .delete(synchronize_session="fetch")
+                .all()
             )
-            request.session.flash(
-                'Successfully changed role', queue="success"
+            removing_self = any(
+                role.role_name == "Owner" and role.user == request.user
+                for role in roles
             )
+            if removing_self:
+                request.session.flash(
+                    "Cannot remove yourself as Owner", queue="error"
+                )
+            else:
+                for role in roles:
+                    request.db.delete(role)
+                    request.db.add(
+                        JournalEntry(
+                            name=project.name,
+                            action=f"remove {role.role_name} {role.user_name}",
+                            submitted_by=request.user,
+                            submitted_from=request.remote_addr,
+                        ),
+                    )
+                request.session.flash(
+                    'Successfully changed role', queue="success"
+                )
         else:
             # This user only has one role, so get it and change the type.
             try:
@@ -160,6 +188,18 @@ def change_project_role(project, request, _form_class=ChangeRoleForm):
                         "Cannot remove yourself as Owner", queue="error"
                     )
                 else:
+                    request.db.add(
+                        JournalEntry(
+                            name=project.name,
+                            action="change {} {} to {}".format(
+                                role.role_name,
+                                role.user_name,
+                                form.role_name.data,
+                            ),
+                            submitted_by=request.user,
+                            submitted_from=request.remote_addr,
+                        ),
+                    )
                     role.role_name = form.role_name.data
                     request.session.flash(
                         'Successfully changed role', queue="success"
@@ -202,6 +242,14 @@ def delete_project_role(project, request):
     else:
         for role in roles:
             request.db.delete(role)
+            request.db.add(
+                JournalEntry(
+                    name=project.name,
+                    action=f"remove {role.role_name} {role.user_name}",
+                    submitted_by=request.user,
+                    submitted_from=request.remote_addr,
+                ),
+            )
         request.session.flash("Successfully removed role", queue="success")
 
     return HTTPSeeOther(