#957 WIP fixing role reviewer saves, with a TODO and a new test

opentech/apply/funds/forms.py

@@ -68,10 +68,9 @@ class UpdateReviewersForm(forms.ModelForm):
         self.request = kwargs.pop('request', None)
         super().__init__(*args, **kwargs)
 
-        # All submitted reviews by non-role reviewers
         self.submitted_reviewers = User.objects.filter(id__in=self.instance.reviews.values('author'))
         self.submitted_reviewers = self.submitted_reviewers.exclude(
-            id__in=AssignedReviewers.objects.role_reviewers_by_submission(self.instance).values('reviewer'))
+            id__in=self.instance.assigned.with_roles().values('reviewer'))
 
         if self.can_alter_external_reviewers(self.instance, self.user):
             reviewers = self.instance.reviewers.all()
@@ -154,6 +153,8 @@ class UpdateReviewersForm(forms.ModelForm):
         else:
             reviewers = instance.reviewers_not_reviewed
 
+        # TODO, if a reviewer is moved from a role assignment, but they have reviewed the submission
+        # they should keep a record in AssignedReviewers with role=None
         current_reviewers = set(reviewers | self.submitted_reviewers | assigned_staff)
 
         # Clear out old reviewers

opentech/apply/funds/models/submissions.py

@@ -678,8 +678,8 @@ class ApplicationRevision(AccessFormData, models.Model):
 
 
 class AssignedReviewersQuerySet(models.QuerySet):
-    def role_reviewers_by_submission(self, submission):
-        return self.filter(role__isnull=False, submission=submission)
+    def with_roles(self):
+        return self.filter(role__isnull=False)
 
 
 class AssignedReviewers(models.Model):

opentech/apply/funds/tests/test_views.py

@@ -14,6 +14,7 @@ from opentech.apply.funds.tests.factories import (
     SealedRoundFactory,
     SealedSubmissionFactory,
 )
+from opentech.apply.review.tests.factories import ReviewFactory
 from opentech.apply.stream_forms.testing.factories import flatten_for_form
 from opentech.apply.users.tests.factories import (
     ReviewerFactory,
@@ -278,6 +279,21 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase):
 
         self.assertCountEqual(submission.reviewers.all(), self.reviewers)
 
+    def test_lead_can_change_role_reviewer_and_review_remains(self):
+        submission = ApplicationSubmissionFactory(lead=self.user)
+
+        self.post_form(submission, reviewer_roles=[self.staff[0]])
+        self.assertCountEqual(submission.reviewers.all(), [self.staff[0]])
+
+        # Add a review from that staff reviewer
+        review = ReviewFactory(submission=submission, author=self.staff[0])
+
+        # Assign a different reviewer to the same role
+        self.post_form(submission, reviewer_roles=[self.staff[1]])
+
+        # TODO: need to test that submission.assigned still contains self.staff[0]
+        # self.assertIn(self.staff[0], submission.reviewers.all())
+
 
 class TestApplicantSubmissionView(BaseSubmissionViewTestCase):
     user_factory = UserFactory