From 024fd8b1fa36358722388f3dc9ae1976dade8076 Mon Sep 17 00:00:00 2001
From: Erin Mullaney <erin.mullaney@torchbox.com>
Date: Tue, 12 Feb 2019 10:24:29 -0500
Subject: [PATCH] #957 WIP fixing role reviewer saves, with a TODO and a new
test
---
opentech/apply/funds/forms.py | 5 +++--
opentech/apply/funds/models/submissions.py | 4 ++--
opentech/apply/funds/tests/test_views.py | 16 ++++++++++++++++
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/opentech/apply/funds/forms.py b/opentech/apply/funds/forms.py
index f4f0e2f78..5cf6293f9 100644
--- a/opentech/apply/funds/forms.py
+++ b/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
diff --git a/opentech/apply/funds/models/submissions.py b/opentech/apply/funds/models/submissions.py
index 55c7a0030..345c9fba4 100644
--- a/opentech/apply/funds/models/submissions.py
+++ b/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):
diff --git a/opentech/apply/funds/tests/test_views.py b/opentech/apply/funds/tests/test_views.py
index dedcd1e4a..56fec895d 100644
--- a/opentech/apply/funds/tests/test_views.py
+++ b/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
--
GitLab