From 0eb3e174171583b11c4a2eb069df553cd2daa486 Mon Sep 17 00:00:00 2001
From: Erin Mullaney <erin.mullaney@torchbox.com>
Date: Tue, 12 Feb 2019 12:46:53 -0500
Subject: [PATCH] #957 change to the way assigned reviewers save for role
reviewers add/remove when they have reviews and a test
---
opentech/apply/funds/forms.py | 31 ++++++++++++++++++------
opentech/apply/funds/tests/test_views.py | 5 ++--
2 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/opentech/apply/funds/forms.py b/opentech/apply/funds/forms.py
index 5cf6293f9..28a3ba59d 100644
--- a/opentech/apply/funds/forms.py
+++ b/opentech/apply/funds/forms.py
@@ -153,11 +153,9 @@ 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
+ # Clear out old reviewers, as long as they aren't assigned to a role
instance.assigned.filter(role=None).exclude(reviewer__in=current_reviewers).delete()
# Add new reviewers
@@ -170,13 +168,32 @@ class UpdateReviewersForm(forms.ModelForm):
if reviewer not in instance.reviewers.filter(assignedreviewers__role=None)
)
+ # Update or create role reviewers
for role, reviewer in assigned_roles.items():
if reviewer:
- AssignedReviewers.objects.update_or_create(
+ # Edge case: handle any role reviewers that reviewed and now have been removed from role assignment,
+ # We want to have a AssignedReviewer record for that orphaned reviewer
+ # because they have left a review but no longer have a role
+ existing = AssignedReviewers.objects.filter(
submission=instance,
- role=role,
- defaults={'reviewer': reviewer},
- )
+ role=role)
+ if existing:
+ existing_reviewer = existing.first().reviewer
+ if existing_reviewer != reviewer:
+ if instance.reviewed_by(existing_reviewer):
+ AssignedReviewers.objects.create(
+ submission=instance,
+ role=None,
+ reviewer=existing_reviewer,
+ )
+ existing.first().reviewer = reviewer
+ existing.first().save()
+ else:
+ AssignedReviewers.objects.create(
+ submission=instance,
+ role=role,
+ reviewer=reviewer,
+ )
return instance
diff --git a/opentech/apply/funds/tests/test_views.py b/opentech/apply/funds/tests/test_views.py
index 56fec895d..cd63b1536 100644
--- a/opentech/apply/funds/tests/test_views.py
+++ b/opentech/apply/funds/tests/test_views.py
@@ -291,8 +291,9 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase):
# 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())
+ # Make sure that the ex-role-reviewer (self.staff[0]) still has a assignedreviewer record
+ self.assertEqual(submission.reviewers.all().count(), 2)
+ self.assertIn(self.staff[0], submission.reviewers.all())
class TestApplicantSubmissionView(BaseSubmissionViewTestCase):
--
GitLab