diff --git a/opentech/apply/activity/messaging.py b/opentech/apply/activity/messaging.py
index d38841979716887ec80ee92c46a5ea6067299075..4ee98bb81eebc2fb1032f8276cac7e6f0340d78c 100644
--- a/opentech/apply/activity/messaging.py
+++ b/opentech/apply/activity/messaging.py
@@ -1,5 +1,6 @@
import json
import requests
+from collections import defaultdict
from django.db import models
from django.conf import settings
@@ -20,6 +21,24 @@ def link_to(target, request):
return request.scheme + '://' + request.get_host() + target.get_absolute_url()
+def group_reviewers(reviewers):
+ groups = defaultdict(list)
+ for reviewer in reviewers:
+ groups[reviewer.role].append(reviewer.reviewer)
+ return groups
+
+
+def reviewers_message(reviewers):
+ messages = []
+ for role, reviewers in group_reviewers(reviewers).items():
+ message = ', '.join(str(reviewer) for reviewer in reviewers)
+ if role:
+ message += ' as ' + str(role)
+ message += '.'
+ messages.append(message)
+ return messages
+
+
neat_related = {
MESSAGES.DETERMINATION_OUTCOME: 'determination',
MESSAGES.UPDATE_LEAD: 'old_lead',
@@ -192,11 +211,11 @@ class ActivityAdapter(AdapterBase):
message = ['Reviewers updated.']
if added:
message.append('Added:')
- message.append(', '.join([str(user) for user in added]) + '.')
+ message.extend(reviewers_message(added))
if removed:
message.append('Removed:')
- message.append(', '.join([str(user) for user in removed]) + '.')
+ message.extend(reviewers_message(removed))
return ' '.join(message)
@@ -265,7 +284,7 @@ class SlackAdapter(AdapterBase):
MESSAGES.COMMENT: 'A new {comment.visibility} comment has been posted on <{link}|{submission.title}> by {user}',
MESSAGES.EDIT: '{user} has edited <{link}|{submission.title}>',
MESSAGES.APPLICANT_EDIT: '{user} has edited <{link}|{submission.title}>',
- MESSAGES.REVIEWERS_UPDATED: '{user} has updated the reviewers on <{link}|{submission.title}>',
+ MESSAGES.REVIEWERS_UPDATED: 'reviewers_updated',
MESSAGES.BATCH_REVIEWERS_UPDATED: 'handle_batch_reviewers',
MESSAGES.TRANSITION: '{user} has updated the status of <{link}|{submission.title}>: {old_phase.display_name} → {submission.phase}',
MESSAGES.DETERMINATION_OUTCOME: 'A determination for <{link}|{submission.title}> was sent by email. Outcome: {determination.clean_outcome}',
@@ -308,6 +327,19 @@ class SlackAdapter(AdapterBase):
} for lead in leads
]
+ def reviewers_updated(self, submission, link, user, added=list(), removed=list(), **kwargs):
+ message = [f'{user} has updated the reviewers on <{link}|{submission.title}>.']
+
+ if added:
+ message.append('Added:')
+ message.extend(reviewers_message(added))
+
+ if removed:
+ message.append('Removed:')
+ message.extend(reviewers_message(removed))
+
+ return ' '.join(message)
+
def handle_batch_reviewers(self, submissions, links, user, added, **kwargs):
submissions_text = ', '.join(
f'<{links[submission.id]}|{submission.title}>'
diff --git a/opentech/apply/activity/tests/test_messaging.py b/opentech/apply/activity/tests/test_messaging.py
index 4b9838aaf062e2106768f668114736621d0c2d4a..e2c8b6318234ba997d35a0d04d96a0b259e23e23 100644
--- a/opentech/apply/activity/tests/test_messaging.py
+++ b/opentech/apply/activity/tests/test_messaging.py
@@ -10,7 +10,11 @@ from django.test import TestCase, override_settings
from django.contrib.messages import get_messages
from opentech.apply.utils.testing import make_request
-from opentech.apply.funds.tests.factories import ApplicationSubmissionFactory
+from opentech.apply.funds.tests.factories import (
+ ApplicationSubmissionFactory,
+ AssignedReviewersFactory,
+ AssignedWithRoleReviewersFactory,
+)
from opentech.apply.review.tests.factories import ReviewFactory
from opentech.apply.users.tests.factories import ReviewerFactory, UserFactory
@@ -202,26 +206,44 @@ class TestActivityAdapter(TestCase):
self.assertEqual(activity.submission, submission)
def test_reviewers_message_no_removed(self):
- message = self.adapter.reviewers_updated([1], [])
+ assigned_reviewer = AssignedReviewersFactory()
+
+ message = self.adapter.reviewers_updated([assigned_reviewer], [])
self.assertTrue('Added' in message)
self.assertFalse('Removed' in message)
- self.assertTrue('1' in message)
+ self.assertTrue(str(assigned_reviewer.reviewer) in message)
def test_reviewers_message_no_added(self):
- message = self.adapter.reviewers_updated([], [1])
+ assigned_reviewer = AssignedReviewersFactory()
+ message = self.adapter.reviewers_updated([], [assigned_reviewer])
self.assertFalse('Added' in message)
self.assertTrue('Removed' in message)
- self.assertTrue('1' in message)
+ self.assertTrue(str(assigned_reviewer.reviewer) in message)
def test_reviewers_message_both(self):
- message = self.adapter.reviewers_updated([1], [2])
+ added, removed = AssignedReviewersFactory.create_batch(2)
+ message = self.adapter.reviewers_updated([added], [removed])
self.assertTrue('Added' in message)
self.assertTrue('Removed' in message)
- self.assertTrue('1' in message)
- self.assertTrue('2' in message)
+ self.assertTrue(str(added.reviewer) in message)
+ self.assertTrue(str(removed.reviewer) in message)
+
+ def test_reviewers_with_role(self):
+ with_role = AssignedWithRoleReviewersFactory()
+ message = self.adapter.reviewers_updated([with_role], [])
+ self.assertTrue(str(with_role.reviewer) in message)
+ self.assertTrue(str(with_role.role) in message)
+
+ def test_reviewers_with_and_without_role(self):
+ with_role = AssignedWithRoleReviewersFactory()
+ without_role = AssignedReviewersFactory()
+ message = self.adapter.reviewers_updated([with_role, without_role], [])
+ self.assertTrue(str(with_role.reviewer) in message)
+ self.assertTrue(str(with_role.role) in message)
+ self.assertTrue(str(without_role.reviewer) in message)
def test_internal_transition_kwarg_for_invisible_transition(self):
submission = ApplicationSubmissionFactory(status='post_review_discussion')
@@ -275,11 +297,15 @@ class TestActivityAdapter(TestCase):
activity = Activity.objects.first()
self.assertEqual(activity.related_object, None)
- def test_review_saved_on_activtiy(self):
- submission = ApplicationSubmissionFactory()
- user = UserFactory()
- review = ReviewFactory(submission=submission)
- self.adapter.send_message('a message', user=user, submission=submission, submissions=[], related=review)
+ def test_review_saved_on_activity(self):
+ review = ReviewFactory()
+ self.adapter.send_message(
+ 'a message',
+ user=review.author,
+ submission=review.submission,
+ submissions=[],
+ related=review,
+ )
activity = Activity.objects.first()
self.assertEqual(activity.related_object, review)
diff --git a/opentech/apply/funds/admin.py b/opentech/apply/funds/admin.py
index 689f73027216344b55a4b82eed731de5b7b55dfb..9f1cbc8464a1bd34f8f87b42682fdbbd928da97c 100644
--- a/opentech/apply/funds/admin.py
+++ b/opentech/apply/funds/admin.py
@@ -1,7 +1,7 @@
from wagtail.contrib.modeladmin.helpers import PermissionHelper
from wagtail.contrib.modeladmin.options import ModelAdmin, ModelAdminGroup
-from opentech.apply.funds.models import ScreeningStatus
+from opentech.apply.funds.models import ReviewerRole, ScreeningStatus
from opentech.apply.review.admin import ReviewFormAdmin
from opentech.apply.utils.admin import ListRelatedMixin
from .admin_helpers import (
@@ -74,6 +74,12 @@ class LabAdmin(ModelAdmin):
menu_label = 'Labs'
+class ReviewerRoleAdmin(ModelAdmin):
+ model = ReviewerRole
+ menu_icon = 'group'
+ menu_label = 'Reviewer Roles'
+
+
class NoDeletePermission(PermissionHelper):
def user_can_delete_obj(self, user, obj):
return False
@@ -106,4 +112,5 @@ class ApplyAdminGroup(ModelAdminGroup):
ReviewFormAdmin,
CategoryAdmin,
ScreeningStatusAdmin,
+ ReviewerRoleAdmin,
)
diff --git a/opentech/apply/funds/forms.py b/opentech/apply/funds/forms.py
index a75cda24e32c1b113f30aa31bb7bc1643640b6c4..0a71b92e8892a8f0e27251e79c5abb814e7595d5 100644
--- a/opentech/apply/funds/forms.py
+++ b/opentech/apply/funds/forms.py
@@ -1,9 +1,11 @@
from django import forms
+from django.utils.text import slugify
+from django.utils.translation import ugettext_lazy as _
from opentech.apply.users.models import User
-from .models import ApplicationSubmission
-from .widgets import Select2MultiCheckboxesWidget
+from .models import ApplicationSubmission, AssignedReviewers, ReviewerRole
+from .widgets import Select2MultiCheckboxesWidget, Select2IconWidget
class ProgressSubmissionForm(forms.ModelForm):
@@ -50,13 +52,8 @@ class UpdateSubmissionLeadForm(forms.ModelForm):
class UpdateReviewersForm(forms.ModelForm):
- staff_reviewers = forms.ModelMultipleChoiceField(
- queryset=User.objects.staff(),
- widget=Select2MultiCheckboxesWidget(attrs={'data-placeholder': 'Staff'}),
- required=False,
- )
reviewer_reviewers = forms.ModelMultipleChoiceField(
- queryset=User.objects.reviewers().exclude(id__in=User.objects.staff()),
+ queryset=User.objects.reviewers().only('pk', 'full_name'),
widget=Select2MultiCheckboxesWidget(attrs={'data-placeholder': 'Reviewers'}),
label='Reviewers',
required=False,
@@ -69,12 +66,47 @@ class UpdateReviewersForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
self.user = kwargs.pop('user')
super().__init__(*args, **kwargs)
- reviewers = self.instance.reviewers.all()
- self.submitted_reviewers = User.objects.filter(id__in=self.instance.reviews.values('author'))
- self.prepare_field('staff_reviewers', reviewers, self.submitted_reviewers)
+ assigned_roles = {
+ assigned.role: assigned.reviewer
+ for assigned in self.instance.assigned.filter(
+ role__isnull=False
+ )
+ }
+
+ staff_reviewers = User.objects.staff().only('full_name', 'pk')
+
+ self.role_fields = {}
+ for role in ReviewerRole.objects.all().order_by('order'):
+ field_name = 'role_reviewer_' + slugify(str(role))
+ self.role_fields[field_name] = role
+
+ self.fields[field_name] = forms.ModelChoiceField(
+ queryset=staff_reviewers,
+ widget=Select2IconWidget(attrs={
+ 'data-placeholder': 'Select a reviewer', 'icon': role.icon
+ }),
+ required=False,
+ label=f'{role.name} Reviewer',
+ )
+ # Pre-populate form field
+ self.fields[field_name].initial = assigned_roles.get(role)
+
+ self.submitted_reviewers = User.objects.filter(
+ id__in=self.instance.reviews.values('author'),
+ )
+
if self.can_alter_external_reviewers(self.instance, self.user):
- self.prepare_field('reviewer_reviewers', reviewers, self.submitted_reviewers)
+
+ reviewers = self.instance.reviewers.all().only('pk')
+ self.prepare_field(
+ 'reviewer_reviewers',
+ initial=reviewers,
+ excluded=self.submitted_reviewers
+ )
+
+ # Move the non-role reviewers field to the end of the field list
+ self.fields.move_to_end('reviewer_reviewers')
else:
self.fields.pop('reviewer_reviewers')
@@ -86,18 +118,77 @@ class UpdateReviewersForm(forms.ModelForm):
def can_alter_external_reviewers(self, instance, user):
return instance.stage.has_external_review and (user == instance.lead or user.is_superuser)
+ def clean(self):
+ cleaned_data = super().clean()
+ role_reviewers = [
+ user
+ for field, user in self.cleaned_data.items()
+ if field != 'reviewer_reviewers'
+ ]
+
+ # If any of the users match and are set to multiple roles, throw an error
+ if len(role_reviewers) != len(set(role_reviewers)) and any(role_reviewers):
+ self.add_error(None, _('Users cannot be assigned to multiple roles.'))
+
+ return cleaned_data
+
def save(self, *args, **kwargs):
instance = super().save(*args, **kwargs)
+ """
+ 1. Update role reviewers
+ 2. Update non-role reviewers
+ 2a. Remove those not on form
+ 2b. Add in any new non-role reviewers selected
+ 3. Add in anyone who has already reviewed but who is not selected as a reviewer on the form
+ """
+
+ # 1. Update role reviewers
+ assigned_roles = {
+ role: self.cleaned_data[field]
+ for field, role in self.role_fields.items()
+ }
+ for role, reviewer in assigned_roles.items():
+ if reviewer:
+ AssignedReviewers.objects.update_or_create(
+ submission=instance,
+ role=role,
+ defaults={'reviewer': reviewer},
+ )
+
+ # 2. Update non-role reviewers
+ # 2a. Remove those not on form
if self.can_alter_external_reviewers(self.instance, self.user):
reviewers = self.cleaned_data.get('reviewer_reviewers')
- else:
- reviewers = instance.reviewers_not_reviewed
-
- instance.reviewers.set(
- self.cleaned_data['staff_reviewers'] |
- reviewers |
- self.submitted_reviewers
+ assigned_reviewers = instance.assigned.without_roles()
+ assigned_reviewers.exclude(
+ reviewer__in=reviewers | self.submitted_reviewers
+ ).delete()
+
+ remaining_reviewers = assigned_reviewers.values_list('reviewer_id', flat=True)
+
+ # 2b. Add in any new non-role reviewers selected
+ AssignedReviewers.objects.bulk_create(
+ AssignedReviewers(
+ submission=instance,
+ role=None,
+ reviewer=reviewer
+ ) for reviewer in reviewers
+ if reviewer.id not in remaining_reviewers
+ )
+
+ # 3. Add in anyone who has already reviewed but who is not selected as a reviewer on the form
+ orphaned_reviews = instance.reviews.exclude(
+ author__in=instance.assigned.values('reviewer')
+ ).select_related('author')
+
+ AssignedReviewers.objects.bulk_create(
+ AssignedReviewers(
+ submission=instance,
+ role=None,
+ reviewer=review.author
+ ) for review in orphaned_reviews
)
+
return instance
diff --git a/opentech/apply/funds/migrations/0051_reviewerrole.py b/opentech/apply/funds/migrations/0051_reviewerrole.py
new file mode 100644
index 0000000000000000000000000000000000000000..57049c521b645d74dc8264e879ab0890649e1692
--- /dev/null
+++ b/opentech/apply/funds/migrations/0051_reviewerrole.py
@@ -0,0 +1,24 @@
+# Generated by Django 2.0.10 on 2019-02-07 15:46
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('images', '0003_customimage_drupal_id'),
+ ('funds', '0050_roundsandlabs'),
+ ]
+
+ operations = [
+ migrations.CreateModel(
+ name='ReviewerRole',
+ fields=[
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+ ('name', models.CharField(max_length=128)),
+ ('order', models.IntegerField(blank=True, help_text='The order this role should appear in the Update Reviewers form.', null=True)),
+ ('icon', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to='images.CustomImage')),
+ ],
+ ),
+ ]
diff --git a/opentech/apply/funds/migrations/0052_assigned_reviewers_pre.py b/opentech/apply/funds/migrations/0052_assigned_reviewers_pre.py
new file mode 100644
index 0000000000000000000000000000000000000000..66f76fe41c3552f3885114bac4ed5c17a501fd42
--- /dev/null
+++ b/opentech/apply/funds/migrations/0052_assigned_reviewers_pre.py
@@ -0,0 +1,43 @@
+# Generated by Django 2.0.10 on 2019-02-07 15:52
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+ ('funds', '0051_reviewerrole'),
+ ]
+
+ operations = [
+ migrations.CreateModel(
+ name='AssignedReviewers',
+ fields=[
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+ ('reviewer', models.ForeignKey(limit_choices_to={'groups__name__in': ['Staff', 'Reviewer']}, on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
+ ('role', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to='funds.ReviewerRole')),
+ ],
+ ),
+ migrations.AlterField(
+ model_name='applicationsubmission',
+ name='reviewers',
+ field=models.ManyToManyField(blank=True, limit_choices_to={'groups__name__in': ['Staff', 'Reviewer']}, related_name='submissions_reviewer_OLD', to=settings.AUTH_USER_MODEL),
+ ),
+ migrations.AddField(
+ model_name='assignedreviewers',
+ name='submission',
+ field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='assigned', to='funds.ApplicationSubmission'),
+ ),
+ migrations.AddField(
+ model_name='applicationsubmission',
+ name='reviewers_new',
+ field=models.ManyToManyField(blank=True, related_name='submissions_reviewer', through='funds.AssignedReviewers', to=settings.AUTH_USER_MODEL),
+ ),
+ migrations.AlterUniqueTogether(
+ name='assignedreviewers',
+ unique_together={('submission', 'role')},
+ ),
+ ]
diff --git a/opentech/apply/funds/migrations/0053_move_reviewer_data.py b/opentech/apply/funds/migrations/0053_move_reviewer_data.py
new file mode 100644
index 0000000000000000000000000000000000000000..8e64748220abe5d0a8899c44dc76a071c2dc391a
--- /dev/null
+++ b/opentech/apply/funds/migrations/0053_move_reviewer_data.py
@@ -0,0 +1,29 @@
+# Generated by Django 2.0.10 on 2019-02-07 15:54
+
+from django.db import migrations
+
+
+def move_reviewer_data(apps, schema_editor):
+ # We need to move data to the new `reviewers_new` field which will be renamed to `reviewers` in the next migration
+ # This data migration is necessary because you cannot add a through model to an existing M2M field
+ ApplicationSubmission = apps.get_model('funds', 'ApplicationSubmission')
+ AssignedReviewers = apps.get_model('funds', 'AssignedReviewers')
+ for submission in ApplicationSubmission.objects.all():
+ AssignedReviewers.objects.bulk_create(
+ AssignedReviewers(
+ submission=submission,
+ reviewer=reviewer,
+ role=None,
+ ) for reviewer in submission.reviewers.all()
+ )
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('funds', '0052_assigned_reviewers_pre'),
+ ]
+
+ operations = [
+ migrations.RunPython(move_reviewer_data, reverse_code=migrations.RunPython.noop),
+ ]
diff --git a/opentech/apply/funds/migrations/0054_remove_applicationsubmission_reviewers.py b/opentech/apply/funds/migrations/0054_remove_applicationsubmission_reviewers.py
new file mode 100644
index 0000000000000000000000000000000000000000..c9806dcc8b8501c61fb7e8b4e298a33f31649522
--- /dev/null
+++ b/opentech/apply/funds/migrations/0054_remove_applicationsubmission_reviewers.py
@@ -0,0 +1,17 @@
+# Generated by Django 2.0.10 on 2019-02-07 16:17
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('funds', '0053_move_reviewer_data'),
+ ]
+
+ operations = [
+ migrations.RemoveField(
+ model_name='applicationsubmission',
+ name='reviewers',
+ ),
+ ]
diff --git a/opentech/apply/funds/migrations/0055_reviewers_rename.py b/opentech/apply/funds/migrations/0055_reviewers_rename.py
new file mode 100644
index 0000000000000000000000000000000000000000..66ed97192288a211849479fd2209282a654276c9
--- /dev/null
+++ b/opentech/apply/funds/migrations/0055_reviewers_rename.py
@@ -0,0 +1,18 @@
+# Generated by Django 2.0.10 on 2019-02-07 16:17
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('funds', '0054_remove_applicationsubmission_reviewers'),
+ ]
+
+ operations = [
+ migrations.RenameField(
+ model_name='applicationsubmission',
+ old_name='reviewers_new',
+ new_name='reviewers',
+ ),
+ ]
diff --git a/opentech/apply/funds/models/__init__.py b/opentech/apply/funds/models/__init__.py
index 101a4b48011e5d342fcf8164a75a073020d4de32..4730a3a44167279ff536202439cdaa88cd6c12c0 100644
--- a/opentech/apply/funds/models/__init__.py
+++ b/opentech/apply/funds/models/__init__.py
@@ -2,11 +2,12 @@ from django.utils.translation import ugettext_lazy as _
from .applications import ApplicationBase, RoundBase, LabBase, RoundsAndLabs # NOQA
from .forms import ApplicationForm
+from .reviewer_role import ReviewerRole
from .screening import ScreeningStatus
-from .submissions import ApplicationSubmission, ApplicationRevision
+from .submissions import ApplicationSubmission, AssignedReviewers, ApplicationRevision
-__all__ = ['ApplicationSubmission', 'ApplicationRevision', 'ApplicationForm', 'ScreeningStatus']
+__all__ = ['ApplicationSubmission', 'AssignedReviewers', 'ApplicationRevision', 'ApplicationForm', 'ScreeningStatus', 'ReviewerRole']
class FundType(ApplicationBase):
diff --git a/opentech/apply/funds/models/reviewer_role.py b/opentech/apply/funds/models/reviewer_role.py
new file mode 100644
index 0000000000000000000000000000000000000000..d06477a4e053bae5acf14c2bb3555181a1403f4d
--- /dev/null
+++ b/opentech/apply/funds/models/reviewer_role.py
@@ -0,0 +1,28 @@
+from django.db import models
+from wagtail.admin.edit_handlers import FieldPanel
+from wagtail.images.edit_handlers import ImageChooserPanel
+
+
+class ReviewerRole(models.Model):
+ name = models.CharField(max_length=128)
+ icon = models.ForeignKey(
+ 'images.CustomImage',
+ null=True,
+ blank=True,
+ related_name='+',
+ on_delete=models.SET_NULL
+ )
+ order = models.IntegerField(
+ help_text='The order this role should appear in the Update Reviewers form.',
+ null=True,
+ blank=True,
+ )
+
+ panels = [
+ FieldPanel('name'),
+ ImageChooserPanel('icon'),
+ FieldPanel('order'),
+ ]
+
+ def __str__(self):
+ return self.name
diff --git a/opentech/apply/funds/models/submissions.py b/opentech/apply/funds/models/submissions.py
index 397b2ec9f532e500785eeb5ea459277673369b45..0e1f7d50cb36be9533e8b4d7f37f82b8ce40f806 100644
--- a/opentech/apply/funds/models/submissions.py
+++ b/opentech/apply/funds/models/submissions.py
@@ -304,8 +304,8 @@ class ApplicationSubmission(
reviewers = models.ManyToManyField(
settings.AUTH_USER_MODEL,
related_name='submissions_reviewer',
- limit_choices_to=LIMIT_TO_STAFF_AND_REVIEWERS,
blank=True,
+ through='AssignedReviewers',
)
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True)
search_data = models.TextField()
@@ -525,7 +525,11 @@ class ApplicationSubmission(
if creating:
self.process_file_data(files)
- self.reviewers.set(self.get_from_parent('reviewers').all())
+ for reviewer in self.get_from_parent('reviewers').all():
+ AssignedReviewers.objects.create(
+ reviewer=reviewer,
+ submission=self
+ )
first_revision = ApplicationRevision.objects.create(
submission=self,
form_data=self.form_data,
@@ -674,3 +678,51 @@ class ApplicationRevision(AccessFormData, models.Model):
'to': self.id,
'from': previous_revision.id,
})
+
+
+class AssignedReviewersQuerySet(models.QuerySet):
+ def with_roles(self):
+ return self.filter(role__isnull=False)
+
+ def without_roles(self):
+ return self.filter(role__isnull=True)
+
+
+class AssignedReviewers(models.Model):
+ reviewer = models.ForeignKey(
+ settings.AUTH_USER_MODEL,
+ on_delete=models.CASCADE,
+ limit_choices_to=LIMIT_TO_STAFF_AND_REVIEWERS,
+ )
+ submission = models.ForeignKey(
+ ApplicationSubmission,
+ related_name='assigned',
+ on_delete=models.CASCADE
+ )
+ role = models.ForeignKey(
+ 'funds.ReviewerRole',
+ related_name='+',
+ on_delete=models.SET_NULL,
+ null=True,
+ )
+
+ objects = AssignedReviewersQuerySet.as_manager()
+
+ class Meta:
+ unique_together = ('submission', 'role')
+
+ def __str__(self):
+ return f'{self.reviewer} as {self.role}'
+
+ def __eq__(self, other):
+ if not isinstance(other, models.Model):
+ return False
+ if self._meta.concrete_model != other._meta.concrete_model:
+ return False
+ my_pk = self.pk
+ if my_pk is None:
+ return self is other
+ return all([
+ self.reviewer_id == other.reviewer_id,
+ self.role_id == other.role_id,
+ ])
diff --git a/opentech/apply/funds/templates/funds/includes/delegated_form_base.html b/opentech/apply/funds/templates/funds/includes/delegated_form_base.html
index 794d7fc93f08f054d0549c2f2ee6f484f1623b63..c6f0746dc01080d2b61ae9874c63155cb935f8c0 100644
--- a/opentech/apply/funds/templates/funds/includes/delegated_form_base.html
+++ b/opentech/apply/funds/templates/funds/includes/delegated_form_base.html
@@ -1,7 +1,10 @@
+{% load util_tags %}
<form class="form {{extra_classes}}" method="post" id="{{ form.name }}">
{% csrf_token %}
<div class="form__item">
{{ form }}
</div>
+
<input class="button button--primary button--top-space" id="{{ form.name }}-submit" name="{{ form_prefix }}{{ form.name }}" type="submit" form="{{ form.name }}" value="{{ value }}">
+
</form>
diff --git a/opentech/apply/funds/templates/funds/widgets/icon_select2.html b/opentech/apply/funds/templates/funds/widgets/icon_select2.html
new file mode 100644
index 0000000000000000000000000000000000000000..0449cc92a9ae92152ecc7b820adbc22209fffb9f
--- /dev/null
+++ b/opentech/apply/funds/templates/funds/widgets/icon_select2.html
@@ -0,0 +1,3 @@
+{% load wagtailimages_tags %}
+{% if widget.attrs.icon %}<div>{% image widget.attrs.icon max-20x20 %}</div>{% endif %}
+{% include 'django/forms/widgets/select.html' %}
diff --git a/opentech/apply/funds/tests/factories/models.py b/opentech/apply/funds/tests/factories/models.py
index 6cc1e8165379c40dc229242e941bcb90c123e21d..713b3dc1b046fa2e1f0d93c80f7d5e9244aee248 100644
--- a/opentech/apply/funds/tests/factories/models.py
+++ b/opentech/apply/funds/tests/factories/models.py
@@ -6,9 +6,11 @@ import wagtail_factories
from opentech.apply.funds.models import (
ApplicationSubmission,
ApplicationRevision,
+ AssignedReviewers,
FundType,
LabType,
RequestForPartners,
+ ReviewerRole,
Round,
ScreeningStatus,
SealedRound,
@@ -37,6 +39,8 @@ __all__ = [
'ApplicationFormFactory',
'ApplicationRevisionFactory',
'ApplicationSubmissionFactory',
+ 'AssignedReviewersFactory',
+ 'AssignedWithRoleReviewersFactory',
'InvitedToProposalFactory',
'RoundFactory',
'RoundBaseFormFactory',
@@ -47,6 +51,7 @@ __all__ = [
'ScreeningStatusFactory',
'SealedRoundFactory',
'SealedSubmissionFactory',
+ 'ReviewerRoleFactory',
'TodayRoundFactory',
'workflow_for_stages',
]
@@ -244,7 +249,34 @@ class ApplicationSubmissionFactory(factory.DjangoModelFactory):
@factory.post_generation
def reviewers(self, create, reviewers, **kwargs):
if create and reviewers:
- self.reviewers.set(reviewers)
+ AssignedReviewers.objects.bulk_create(
+ AssignedReviewers(
+ reviewer=reviewer,
+ submission=self,
+ role=None
+ ) for reviewer in reviewers
+ )
+
+
+class ReviewerRoleFactory(factory.DjangoModelFactory):
+ class Meta:
+ model = ReviewerRole
+
+ name = factory.Faker('word')
+ order = factory.Sequence(lambda n: n)
+
+
+class AssignedReviewersFactory(factory.DjangoModelFactory):
+ class Meta:
+ model = AssignedReviewers
+
+ submission = factory.SubFactory(ApplicationSubmissionFactory)
+ role = None
+ reviewer = factory.SubFactory(StaffFactory)
+
+
+class AssignedWithRoleReviewersFactory(AssignedReviewersFactory):
+ role = factory.SubFactory(ReviewerRoleFactory)
class InvitedToProposalFactory(ApplicationSubmissionFactory):
diff --git a/opentech/apply/funds/tests/test_forms.py b/opentech/apply/funds/tests/test_forms.py
new file mode 100644
index 0000000000000000000000000000000000000000..cf85c793df9bda67dae919d8d8a312f242936fbe
--- /dev/null
+++ b/opentech/apply/funds/tests/test_forms.py
@@ -0,0 +1,115 @@
+from django.test import TestCase
+from opentech.apply.funds.tests.factories import (
+ ApplicationSubmissionFactory,
+ AssignedWithRoleReviewersFactory,
+ AssignedReviewersFactory,
+ InvitedToProposalFactory,
+ ReviewerRoleFactory,
+)
+from opentech.apply.review.tests.factories import ReviewFactory
+from opentech.apply.users.tests.factories import (
+ ReviewerFactory,
+ StaffFactory,
+)
+
+
+from opentech.apply.funds.forms import UpdateReviewersForm
+
+
+class TestReviewerFormQueries(TestCase):
+ def test_queries_init_and_render(self):
+ user = StaffFactory()
+
+ ReviewerRoleFactory.create_batch(3)
+
+ StaffFactory.create_batch(3)
+ ReviewerFactory.create_batch(3)
+
+ submission = InvitedToProposalFactory(lead=user, workflow_stages=2)
+
+ # Reviewers
+ # Assigned Reviewers
+ # Roles
+ with self.assertNumQueries(3):
+ form = UpdateReviewersForm(user=user, instance=submission)
+
+ # 3 x Staff - 1 per Role
+ # 1 x reviewers queryset
+ # 1 x submission reviewers
+ with self.assertNumQueries(5):
+ form.as_p()
+
+ def test_queries_roles_swap(self):
+ user = StaffFactory()
+ submission = ApplicationSubmissionFactory()
+
+ staff = StaffFactory.create_batch(4)
+ roles = ReviewerRoleFactory.create_batch(2)
+
+ form = UpdateReviewersForm(user=user, instance=submission)
+
+ AssignedWithRoleReviewersFactory(role=roles[0], submission=submission, reviewer=staff[0])
+ AssignedWithRoleReviewersFactory(role=roles[1], submission=submission, reviewer=staff[1])
+
+ data = {}
+ for field, user in zip(form.fields, staff):
+ if field.startswith('role'):
+ data[field] = user.id
+ else:
+ data[field] = None
+
+ form = UpdateReviewersForm(data, user=user, instance=submission)
+
+ self.assertTrue(form.is_valid())
+
+ # 1 - Submission
+ # 8 - 1 per role (2 savepoint, 1 get, 1 update)
+ # 1 - check - orphaned
+ with self.assertNumQueries(10):
+ form.save()
+
+ def test_queries_reviewers_swap(self):
+ user = StaffFactory()
+ submission = InvitedToProposalFactory(lead=user)
+
+ reviewers = ReviewerFactory.create_batch(4)
+
+ AssignedReviewersFactory(submission=submission, reviewer=reviewers[0])
+ AssignedReviewersFactory(submission=submission, reviewer=reviewers[1])
+
+ data = {'reviewer_reviewers': [reviewer.id for reviewer in reviewers[2:]]}
+
+ form = UpdateReviewersForm(data, user=user, instance=submission)
+
+ self.assertTrue(form.is_valid())
+
+ # 1 - Submission
+ # 1 - Delete old
+ # 1 - Cache existing
+ # 1 - Add new
+ # 1 - check - orphaned
+ with self.assertNumQueries(5):
+ form.save()
+
+ def test_queries_existing_reviews(self):
+ user = StaffFactory()
+ submission = InvitedToProposalFactory(lead=user)
+
+ reviewers = ReviewerFactory.create_batch(4)
+
+ ReviewFactory(submission=submission, author=reviewers[0])
+ ReviewFactory(submission=submission, author=reviewers[1])
+
+ data = {'reviewer_reviewers': [reviewer.id for reviewer in reviewers[2:]]}
+
+ form = UpdateReviewersForm(data, user=user, instance=submission)
+
+ self.assertTrue(form.is_valid())
+
+ # 1 - Submission
+ # 1 - Delete old
+ # 1 - Cache existing
+ # 1 - Add new
+ # 1 - check - orphaned
+ with self.assertNumQueries(5):
+ form.save()
diff --git a/opentech/apply/funds/tests/test_views.py b/opentech/apply/funds/tests/test_views.py
index 44b1f6f3d6450f995316d3b1d082c6d09aa63177..2eee0123a4632af75ee2309c5faebb6c45e5f2d4 100644
--- a/opentech/apply/funds/tests/test_views.py
+++ b/opentech/apply/funds/tests/test_views.py
@@ -1,17 +1,23 @@
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 (
ApplicationSubmissionFactory,
ApplicationRevisionFactory,
+ AssignedWithRoleReviewersFactory,
+ AssignedReviewersFactory,
InvitedToProposalFactory,
LabSubmissionFactory,
+ ReviewerRoleFactory,
ScreeningStatusFactory,
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,
@@ -200,36 +206,37 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase):
super().setUpTestData()
cls.staff = StaffFactory.create_batch(4)
cls.reviewers = ReviewerFactory.create_batch(4)
+ cls.roles = ReviewerRoleFactory.create_batch(2)
- def post_form(self, submission, staff=list(), reviewers=list()):
- return self.post_page(submission, {
+ def post_form(self, submission, reviewer_roles=list(), reviewers=list()):
+ data = {
'form-submitted-reviewer_form': '',
- 'staff_reviewers': [s.id for s in staff],
'reviewer_reviewers': [r.id for r in reviewers]
- })
+ }
+ data.update(
+ **{
+ f'role_reviewer_{str(role)}': reviewer.id
+ for role, reviewer in zip(self.roles, reviewer_roles)
+ }
+ )
+ return self.post_page(submission, data)
def test_lead_can_add_staff_single(self):
submission = ApplicationSubmissionFactory(lead=self.user)
- self.post_form(submission, self.staff)
-
- self.assertCountEqual(submission.reviewers.all(), self.staff)
+ self.post_form(submission, reviewer_roles=[self.staff[0]])
- def test_lead_can_remove_staff_single(self):
- submission = ApplicationSubmissionFactory(lead=self.user, reviewers=self.staff)
- self.assertCountEqual(submission.reviewers.all(), self.staff)
-
- self.post_form(submission, [])
-
- self.assertCountEqual(submission.reviewers.all(), [])
+ self.assertCountEqual(submission.reviewers.all(), [self.staff[0]])
- def test_lead_can_remove_some_staff(self):
- submission = ApplicationSubmissionFactory(lead=self.user, reviewers=self.staff)
- self.assertCountEqual(submission.reviewers.all(), self.staff)
+ def test_lead_can_change_staff_single(self):
+ submission = ApplicationSubmissionFactory(lead=self.user)
+ AssignedWithRoleReviewersFactory(role=self.roles[0], submission=submission, reviewer=self.staff[0])
+ self.assertCountEqual(submission.reviewers.all(), [self.staff[0]])
- self.post_form(submission, self.staff[0:2])
+ self.post_form(submission, reviewer_roles=[self.staff[1]])
- self.assertCountEqual(submission.reviewers.all(), self.staff[0:2])
+ self.assertCountEqual(submission.reviewers.all(), [self.staff[1]])
+ self.assertEqual(submission.assigned.with_roles().first().reviewer, self.staff[1])
def test_lead_cant_add_reviewers_single(self):
submission = ApplicationSubmissionFactory(lead=self.user)
@@ -238,43 +245,28 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase):
self.assertCountEqual(submission.reviewers.all(), [])
- def test_lead_can_add_staff_and_reviewers_for_proposal(self):
+ def test_lead_can_add_reviewers_for_proposal(self):
submission = InvitedToProposalFactory(lead=self.user)
- self.post_form(submission, self.staff, self.reviewers)
+ self.post_form(submission, reviewers=self.reviewers)
- self.assertCountEqual(submission.reviewers.all(), self.staff + self.reviewers)
+ self.assertCountEqual(submission.reviewers.all(), self.reviewers)
- def test_lead_can_remove_staff_and_reviewers_for_proposal(self):
- submission = InvitedToProposalFactory(lead=self.user, reviewers=self.staff + self.reviewers)
- self.assertCountEqual(submission.reviewers.all(), self.staff + self.reviewers)
+ def test_lead_can_remove_reviewers_for_proposal(self):
+ submission = InvitedToProposalFactory(lead=self.user, reviewers=self.reviewers)
+ self.assertCountEqual(submission.reviewers.all(), self.reviewers)
self.post_form(submission)
self.assertCountEqual(submission.reviewers.all(), [])
- def test_lead_can_remove_some_staff_and_reviewers_for_proposal(self):
- submission = InvitedToProposalFactory(lead=self.user, reviewers=self.staff + self.reviewers)
- self.assertCountEqual(submission.reviewers.all(), self.staff + self.reviewers)
-
- self.post_form(submission, self.staff[0:2], self.reviewers[0:2])
-
- self.assertCountEqual(submission.reviewers.all(), self.staff[0:2] + self.reviewers[0:2])
-
- def test_staff_can_add_staff_single(self):
- submission = ApplicationSubmissionFactory()
-
- self.post_form(submission, self.staff)
-
- self.assertCountEqual(submission.reviewers.all(), self.staff)
-
- def test_staff_can_remove_staff_single(self):
- submission = ApplicationSubmissionFactory(reviewers=self.staff)
- self.assertCountEqual(submission.reviewers.all(), self.staff)
+ def test_lead_can_remove_some_reviewers_for_proposal(self):
+ submission = InvitedToProposalFactory(lead=self.user, reviewers=self.reviewers)
+ self.assertCountEqual(submission.reviewers.all(), self.reviewers)
- self.post_form(submission, [])
+ self.post_form(submission, reviewers=self.reviewers[0:2])
- self.assertCountEqual(submission.reviewers.all(), [])
+ self.assertCountEqual(submission.reviewers.all(), self.reviewers[0:2])
def test_staff_cant_add_reviewers_proposal(self):
submission = ApplicationSubmissionFactory()
@@ -291,6 +283,77 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase):
self.assertCountEqual(submission.reviewers.all(), self.reviewers)
+ def test_lead_can_change_role_reviewer_and_review_remains(self):
+ submission = ApplicationSubmissionFactory()
+ AssignedWithRoleReviewersFactory(role=self.roles[0], submission=submission, reviewer=self.staff[0])
+
+ # Add a review from that staff reviewer
+ ReviewFactory(submission=submission, author=self.staff[0])
+
+ # Assign a different reviewer to the same role
+ self.post_form(submission, reviewer_roles=[self.staff[1]])
+
+ # Make sure that the ex-role-reviewer is still assigned record
+ self.assertCountEqual(submission.reviewers.all(), self.staff[0:2])
+
+ 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=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)
+
+ self.assertCountEqual(submission.reviewers.all(), [staff])
+
class TestApplicantSubmissionView(BaseSubmissionViewTestCase):
user_factory = UserFactory
diff --git a/opentech/apply/funds/views.py b/opentech/apply/funds/views.py
index 44145ee6f97929c79886b7a4f27bdb12ee400ba3..214436f296f078add7c38b0c598bfef010394c5f 100644
--- a/opentech/apply/funds/views.py
+++ b/opentech/apply/funds/views.py
@@ -38,7 +38,13 @@ from .forms import (
UpdateReviewersForm,
UpdateSubmissionLeadForm,
)
-from .models import ApplicationSubmission, ApplicationRevision, RoundsAndLabs, RoundBase, LabBase
+from .models import (
+ ApplicationSubmission,
+ ApplicationRevision,
+ RoundsAndLabs,
+ RoundBase,
+ LabBase
+)
from .tables import (
AdminSubmissionsTable,
ReviewerSubmissionsTable,
@@ -291,10 +297,13 @@ class UpdateReviewersView(DelegatedViewMixin, UpdateView):
context_name = 'reviewer_form'
def form_valid(self, form):
- old_reviewers = set(self.get_object().reviewers.all())
+ old_reviewers = set(
+ copy(reviewer)
+ for reviewer in form.instance.assigned.all()
+ )
response = super().form_valid(form)
- new_reviewers = set(form.instance.reviewers.all())
+ new_reviewers = set(form.instance.assigned.all())
added = new_reviewers - old_reviewers
removed = old_reviewers - new_reviewers
diff --git a/opentech/apply/funds/widgets.py b/opentech/apply/funds/widgets.py
index d7ac9d879f2ea91801d78b718bd2e9874071f787..dbaf8ba228462006ed3bfa4080dfa8d9a69b03e9 100644
--- a/opentech/apply/funds/widgets.py
+++ b/opentech/apply/funds/widgets.py
@@ -1,6 +1,6 @@
from django.contrib.staticfiles.templatetags.staticfiles import static
-from django_select2.forms import Select2MultipleWidget
+from django_select2.forms import Select2Widget, Select2MultipleWidget
class Select2MultiCheckboxesWidget(Select2MultipleWidget):
@@ -20,3 +20,13 @@ class Select2MultiCheckboxesWidget(Select2MultipleWidget):
attrs = super().build_attrs(*args, **kwargs)
attrs['class'] = attrs['class'].replace('django-select2', 'django-select2-checkboxes')
return attrs
+
+
+class Select2IconWidget(Select2Widget):
+ template_name = 'funds/widgets/icon_select2.html'
+
+ def __init__(self, *args, **kwargs):
+ attrs = kwargs.get('attrs', {})
+ attrs.setdefault('icon', '')
+ kwargs['attrs'] = attrs
+ super().__init__(*args, **kwargs)
diff --git a/opentech/apply/review/models.py b/opentech/apply/review/models.py
index 75cfa7c1fad1fd8419eab33dbf4d45997db14aba..e4103ae65d9b6325e091ed26f4e9c9036ad34769 100644
--- a/opentech/apply/review/models.py
+++ b/opentech/apply/review/models.py
@@ -160,8 +160,12 @@ class Review(ReviewFormFieldsMixin, BaseStreamForm, AccessFormData, models.Model
@receiver(post_save, sender=Review)
def update_submission_reviewers_list(sender, **kwargs):
+ from opentech.apply.funds.models import AssignedReviewers
+
review = kwargs.get('instance')
# Make sure the reviewer is in the reviewers list on the submission
- if not review.submission.reviewers.filter(id=review.author.id).exists():
- review.submission.reviewers.add(review.author)
+ AssignedReviewers.objects.get_or_create(
+ submission=review.submission,
+ reviewer=review.author,
+ )
diff --git a/opentech/static_src/src/javascript/apply/fancybox-global.js b/opentech/static_src/src/javascript/apply/fancybox-global.js
index 616c78f49aa0bb3ea7e7990687c7bb680babe55e..2e61578bcfbe2a5cb28124a8c0405c98bbbfcdff 100644
--- a/opentech/static_src/src/javascript/apply/fancybox-global.js
+++ b/opentech/static_src/src/javascript/apply/fancybox-global.js
@@ -18,4 +18,16 @@
$('.django-select2-checkboxes').select2('close');
});
+ $(document).ready(
+ $('.modal').each((idx, element) => {
+ var modal = $(element);
+ var error = modal.has('.errorlist');
+ if (error.length) {
+ const modalID = modal.attr('id');
+ const buttonTrigger = $(`[data-src="#${modalID}"]`);
+ buttonTrigger[0].click();
+ }
+ })
+ );
+
})(jQuery);