diff --git a/opentech/apply/funds/forms.py b/opentech/apply/funds/forms.py index 0a85d28652c38b71640d272fec41de1311ee79d4..9e302e9e30ab89b750c955f3becdcf5534140630 100644 --- a/opentech/apply/funds/forms.py +++ b/opentech/apply/funds/forms.py @@ -65,12 +65,11 @@ class UpdateReviewersForm(forms.ModelForm): def __init__(self, *args, **kwargs): self.user = kwargs.pop('user') - self.request = kwargs.pop('request', None) super().__init__(*args, **kwargs) - self.submitted_reviewers = User.objects.filter(id__in=self.instance.reviews.values('author')) - self.submitted_reviewers = self.submitted_reviewers.exclude( - id__in=self.instance.assigned.with_roles().values('reviewer')) + self.submitted_reviewers = User.objects.filter( + id__in=self.instance.reviews.values('author'), + ) if self.can_alter_external_reviewers(self.instance, self.user): reviewers = self.instance.reviewers.all() diff --git a/opentech/apply/funds/tests/test_views.py b/opentech/apply/funds/tests/test_views.py index 761f73d46831e424cb3d8b004d51817e5949db69..ebbd9c0292c8c016868c75701099782c7de18a70 100644 --- a/opentech/apply/funds/tests/test_views.py +++ b/opentech/apply/funds/tests/test_views.py @@ -1,6 +1,8 @@ from datetime import datetime, timedelta import json +from django.test import TestCase + from opentech.apply.activity.models import Activity, INTERNAL from opentech.apply.determinations.tests.factories import DeterminationFactory from opentech.apply.funds.tests.factories import ( @@ -282,7 +284,7 @@ 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) + submission = ApplicationSubmissionFactory() AssignedWithRoleReviewersFactory(role=self.roles[0], submission=submission, reviewer=self.staff[0]) # Add a review from that staff reviewer @@ -291,23 +293,66 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase): # Assign a different reviewer to the same role self.post_form(submission, reviewer_roles=[self.staff[1]]) - # 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()) + # Make sure that the ex-role-reviewer is still assigned record + self.assertCountEqual(submission.reviewers.all(), self.staff[0:2]) - def test_reviewer_exists_after_submitting_review(self): - submission = ApplicationSubmissionFactory(lead=self.user, status='external_review', workflow_stages=2) - AssignedWithRoleReviewersFactory(role=self.roles[0], submission=submission, reviewer=self.staff[0]) - AssignedWithRoleReviewersFactory(role=self.roles[1], submission=submission, reviewer=self.staff[1]) - AssignedReviewersFactory(submission=submission, reviewer=self.reviewers[0]) - # Now, let's check that we have 3 reviewers to begin with - self.assertEqual(submission.reviewers.all().count(), 3) + def test_can_remove_external_reviewer_and_review_remains(self): + submission = ApplicationSubmissionFactory(lead=self.user) + reviewer = self.reviewers[0] + AssignedReviewersFactory(submission=submission, reviewer=reviewer) + ReviewFactory(submission=submission, author=reviewer) + + # Assign a different reviewer to the same role + self.post_form(submission, reviewers=[]) + + self.assertCountEqual(submission.reviewers.all(), [reviewer]) + + +class TestPostSaveOnReview(TestCase): + def test_external_reviewer_exists_after_review(self): + reviewer = ReviewerFactory() + submission = ApplicationSubmissionFactory() # Add a review from a new reviewer who isn't assigned - ReviewFactory(submission=submission, author=self.reviewers[1]) + ReviewFactory(submission=submission, author=reviewer) + + self.assertCountEqual(submission.reviewers.all(), [reviewer]) + + def test_assigned_external_reviewer_exists_after_review(self): + reviewer = ReviewerFactory() + submission = ApplicationSubmissionFactory() + + # Add a review from a new reviewer who is assigned + AssignedReviewersFactory(submission=submission, reviewer=reviewer) + ReviewFactory(submission=submission, author=reviewer) + + self.assertCountEqual(submission.reviewers.all(), [reviewer]) + + def test_staff_reviewer_exists_after_submitting_review(self): + staff = StaffFactory() + submission = ApplicationSubmissionFactory() + + ReviewFactory(submission=submission, author=staff) + + self.assertCountEqual(submission.reviewers.all(), [staff]) + + def test_assigned_staff_reviewer_exists_after_review(self): + staff = StaffFactory() + submission = ApplicationSubmissionFactory() + + AssignedReviewersFactory(submission=submission, reviewer=staff) + ReviewFactory(submission=submission, author=staff) + + self.assertCountEqual(submission.reviewers.all(), [staff]) + + def test_role_reviewer_exists_after_review(self): + staff = StaffFactory() + submission = ApplicationSubmissionFactory() + + AssignedWithRoleReviewersFactory(submission=submission, reviewer=staff) + ReviewFactory(submission=submission, author=staff) - # Now there should be 4 reviewers assigned, because an outsider reviewed - self.assertEqual(submission.reviewers.all().count(), 4) + self.assertCountEqual(submission.reviewers.all(), [staff]) class TestApplicantSubmissionView(BaseSubmissionViewTestCase): diff --git a/opentech/apply/funds/views.py b/opentech/apply/funds/views.py index ffb43a23b4564e57e4cb18b724dc63cc3063556c..4e5167f33a93c055143964ceb40f03599085edbf 100644 --- a/opentech/apply/funds/views.py +++ b/opentech/apply/funds/views.py @@ -296,11 +296,6 @@ class UpdateReviewersView(DelegatedViewMixin, UpdateView): form_class = UpdateReviewersForm context_name = 'reviewer_form' - def get_form_kwargs(self): - kwargs = super().get_form_kwargs() - kwargs['request'] = self.request - return kwargs - def form_invalid(self, form): messages.error( self.request,