Merge pull request #562 from opensafely-core/issue-380

Rename enum and method names for requesting changes to file

airlock/business_logic.py

@@ -150,7 +150,12 @@ class ReviewTurnPhase(Enum):
 
 
 class AuditEventType(Enum):
-    """Audit log events."""
+    """Audit log events.
+
+    Note that the string values are stored in the local_db database via
+    AuditEvent.type.  Any changes to values will require a database migration.
+    See eg #562.
+    """
 
     # file access
     WORKSPACE_FILE_VIEW = "WORKSPACE_FILE_VIEW"
@@ -177,7 +182,7 @@ class AuditEventType(Enum):
     REQUEST_FILE_UPDATE = "REQUEST_FILE_UPDATE"
     REQUEST_FILE_WITHDRAW = "REQUEST_FILE_WITHDRAW"
     REQUEST_FILE_APPROVE = "REQUEST_FILE_APPROVE"
-    REQUEST_FILE_REJECT = "REQUEST_FILE_REJECT"
+    REQUEST_FILE_REQUEST_CHANGES = "REQUEST_FILE_REQUEST_CHANGES"
     REQUEST_FILE_RESET_REVIEW = "REQUEST_FILE_RESET_REVIEW"
     REQUEST_FILE_UNDECIDED = "REQUEST_FILE_UNDECIDED"
     REQUEST_FILE_RELEASE = "REQUEST_FILE_RELEASE"
@@ -221,7 +226,7 @@ class NotificationEventType(Enum):
     AuditEventType.REQUEST_FILE_UPDATE: "Updated file",
     AuditEventType.REQUEST_FILE_WITHDRAW: "Withdrew file from group",
     AuditEventType.REQUEST_FILE_APPROVE: "Approved file",
-    AuditEventType.REQUEST_FILE_REJECT: "Changes requested to file",
+    AuditEventType.REQUEST_FILE_REQUEST_CHANGES: "Changes requested to file",
     AuditEventType.REQUEST_FILE_RESET_REVIEW: "Reset review of file",
     AuditEventType.REQUEST_FILE_UNDECIDED: "Rejected file moved to undecided",
     AuditEventType.REQUEST_FILE_RELEASE: "File released",
@@ -1220,7 +1225,7 @@ def approve_file(
     ):
         raise NotImplementedError()
 
-    def reject_file(
+    def request_changes_to_file(
         self, request_id: str, relpath: UrlPath, username: str, audit: AuditEvent
     ):
         raise NotImplementedError()
@@ -2028,7 +2033,7 @@ def approve_file(
             release_request.id, request_file.relpath, user.username, audit
         )
 
-    def reject_file(
+    def request_changes_to_file(
         self,
         release_request: ReleaseRequest,
         request_file: RequestFile,
@@ -2042,13 +2047,13 @@ def reject_file(
 
         audit = AuditEvent.from_request(
             request=release_request,
-            type=AuditEventType.REQUEST_FILE_REJECT,
+            type=AuditEventType.REQUEST_FILE_REQUEST_CHANGES,
             user=user,
             path=request_file.relpath,
             group=request_file.group,
         )
 
-        self._dal.reject_file(
+        self._dal.request_changes_to_file(
             release_request.id, request_file.relpath, user.username, audit
         )
 

airlock/views/request.py

@@ -507,7 +507,7 @@ def file_reject(request, request_id, path: str):
         raise Http404()
 
     try:
-        bll.reject_file(release_request, request_file, request.user)
+        bll.request_changes_to_file(release_request, request_file, request.user)
     except bll.ApprovalPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 

local_db/data_access.py

@@ -286,7 +286,7 @@ def approve_file(
 
             self._create_audit_log(audit)
 
-    def reject_file(
+    def request_changes_to_file(
         self, request_id: str, relpath: UrlPath, username: str, audit: AuditEvent
     ):
         with transaction.atomic():

local_db/migrations/0021_auto_20240719_1043.py

@@ -0,0 +1,37 @@
+# Generated by Django 5.0.6 on 2024-07-19 10:43
+
+from django.db import migrations
+
+
+# We've changed AuditEventType.REQUEST_FILE_REJECT to
+# AuditEventType.REQUEST_FILE_REQUEST_CHANGES, which requires a change to the
+# database.
+#
+# We'd like to have a RunPython migration like the following, but that fails
+# because the type parameter is expected to be an instance of AuditEventType
+#
+# def update_column_values(apps, schema_editor):
+#     AuditLog = apps.get_model("local_db", "AuditLog")
+#     AuditLog.objects.filter(type="REQUEST_FILE_REJECT").update(
+#         type="REQUEST_FILE_REQUEST_CHANGES"
+#     )
+#
+# We could work around that by sequencing the migration across two deploys, but
+# it's easier to write some SQL.
+
+sql = "UPDATE local_db_auditlog SET type = %s WHERE type = %s"
+old = "REQUEST_FILE_REJECT"
+new = "REQUEST_FILE_REQUEST_CHANGES"
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("local_db", "0020_requestmetadata_turn_reviewers"),
+    ]
+
+    operations = [
+        migrations.RunSQL(
+            sql=[(sql, [new, old])],
+            reverse_sql=[(sql, [old, new])],
+        ),
+    ]

tests/factories.py

@@ -481,7 +481,7 @@ def review_file(request, relpath, status, *users):
                 user=user,
             )
         elif status == RequestFileVote.REJECTED:
-            bll.reject_file(
+            bll.request_changes_to_file(
                 request,
                 request.get_request_file_from_output_path(relpath),
                 user=user,

tests/unit/test_business_logic.py

@@ -2626,7 +2626,7 @@ def test_approve_file_requires_two_plus_submitted_reviews(bll):
     request_file = release_request.get_request_file_from_output_path(path)
 
     bll.approve_file(release_request, request_file, checker1)
-    bll.reject_file(release_request, request_file, checker2)
+    bll.request_changes_to_file(release_request, request_file, checker2)
 
     # Reviewers must submit their independent review before we can assess
     # a file's decision
@@ -2654,7 +2654,7 @@ def test_approve_file_requires_two_plus_submitted_reviews(bll):
     )
 
 
-def test_reject_file(bll):
+def test_request_changes_to_file(bll):
     path = Path("path/file1.txt")
     release_request = factories.create_request_at_status(
         "workspace",
@@ -2673,7 +2673,7 @@ def test_reject_file(bll):
         == RequestFileDecision.INCOMPLETE
     )
 
-    bll.reject_file(release_request, request_file, checker)
+    bll.request_changes_to_file(release_request, request_file, checker)
 
     rfile = _get_request_file(release_request, path)
     assert rfile.get_file_vote_for_user(checker) == RequestFileVote.REJECTED
@@ -2685,14 +2685,14 @@ def test_reject_file(bll):
     audit_log = bll.get_audit_log(request=release_request.id)
     assert audit_log[0] == AuditEvent.from_request(
         release_request,
-        AuditEventType.REQUEST_FILE_REJECT,
+        AuditEventType.REQUEST_FILE_REQUEST_CHANGES,
         user=checker,
         path=path,
         group="group",
     )
 
 
-def test_approve_then_reject_file(bll):
+def test_approve_then_request_changes_to_file(bll):
     path = Path("path/file1.txt")
     release_request = factories.create_request_at_status(
         "workspace",
@@ -2718,7 +2718,7 @@ def test_approve_then_reject_file(bll):
         == RequestFileDecision.INCOMPLETE
     )
 
-    bll.reject_file(release_request, request_file, checker)
+    bll.request_changes_to_file(release_request, request_file, checker)
 
     rfile = _get_request_file(release_request, path)
     assert rfile.get_file_vote_for_user(checker) == RequestFileVote.REJECTED
@@ -2749,7 +2749,7 @@ def test_reviewreset_then_reset_review_file(bll, review):
     if review == RequestFileVote.APPROVED:
         bll.approve_file(release_request, request_file, checker)
     elif review == RequestFileVote.REJECTED:
-        bll.reject_file(release_request, request_file, checker)
+        bll.request_changes_to_file(release_request, request_file, checker)
     else:
         assert False
 
@@ -2820,7 +2820,7 @@ def test_request_file_status_decision(bll, votes, decision):
         if vote == "APPROVED":
             bll.approve_file(release_request, request_file, checker)
         else:
-            bll.reject_file(release_request, request_file, checker)
+            bll.request_changes_to_file(release_request, request_file, checker)
 
         rfile = _get_request_file(release_request, path)
         assert rfile.get_file_vote_for_user(checker) == RequestFileVote[vote]