diff --git a/opentech/apply/activity/tests/test_messaging.py b/opentech/apply/activity/tests/test_messaging.py index 818982ecbece97c0160cb17a74f6b517fdad6f73..78c1d19b6ba54a7c730f1b64b1b0a6f1e9ada01a 100644 --- a/opentech/apply/activity/tests/test_messaging.py +++ b/opentech/apply/activity/tests/test_messaging.py @@ -301,7 +301,7 @@ class TestActivityAdapter(TestCase): review = ReviewFactory() self.adapter.send_message( 'a message', - user=review.author, + user=review.author.reviewer, submission=review.submission, submissions=[], related=review, diff --git a/opentech/apply/activity/views.py b/opentech/apply/activity/views.py index cba63a229760a14bc68c2d3907b140cd876ecf89..25b216107e5528f0eaa36aaaad3f95c950b97b93 100644 --- a/opentech/apply/activity/views.py +++ b/opentech/apply/activity/views.py @@ -33,20 +33,19 @@ class AllActivityContextMixin: class ActivityContextMixin: def get_context_data(self, **kwargs): extra = { + # Do not prefetch on the related_object__author as the models + # are not homogeneous and this will fail 'actions': Activity.actions.filter(submission=self.object).select_related( 'user', ).prefetch_related( - 'related_object__author', 'related_object__submission', ).visible_to(self.request.user), 'comments': Activity.comments.filter(submission=self.object).select_related( 'user', ).prefetch_related( - 'related_object__author', 'related_object__submission', ).visible_to(self.request.user), } - return super().get_context_data(**extra, **kwargs) diff --git a/opentech/apply/dashboard/tests/test_views.py b/opentech/apply/dashboard/tests/test_views.py index 722410de90938ed98aa0a059b1f1f1e3a954223b..54aba6a63a4a97204163699093cb308ddfde4440 100644 --- a/opentech/apply/dashboard/tests/test_views.py +++ b/opentech/apply/dashboard/tests/test_views.py @@ -73,8 +73,8 @@ class TestStaffDashboard(BaseViewTestCase): def test_waiting_for_review_after_agreement_is_empty(self): staff = StaffFactory() submission = ApplicationSubmissionFactory(status='external_review', workflow_stages=2, reviewers=[staff, self.user]) - review = ReviewFactory(submission=submission, author=staff, recommendation_yes=True) - ReviewOpinionFactory(review=review, author=self.user, opinion_agree=True) + review = ReviewFactory(submission=submission, author__reviewer=staff, author__staff=True, recommendation_yes=True) + ReviewOpinionFactory(review=review, author__reviewer=self.user, opinion_agree=True) response = self.get_page() self.assertContains(response, 'Waiting for your review') self.assertContains(response, "Nice! You're all caught up.") diff --git a/opentech/apply/funds/api_views.py b/opentech/apply/funds/api_views.py index f2442300cddb21ef4f454fd33953ad6d03931ba6..55ad0c604a22f7f1ba47435dc14b2d8843c10038 100644 --- a/opentech/apply/funds/api_views.py +++ b/opentech/apply/funds/api_views.py @@ -1,6 +1,6 @@ from django.core.exceptions import PermissionDenied as DjangoPermissionDenied from django.db import transaction -from django.db.models import Q +from django.db.models import Q, Prefetch from django.utils import timezone from rest_framework import generics, mixins, permissions from rest_framework.response import Response @@ -12,6 +12,7 @@ from opentech.api.pagination import StandardResultsSetPagination from opentech.apply.activity.models import Activity, COMMENT from opentech.apply.activity.messaging import messenger, MESSAGES from opentech.apply.determinations.views import DeterminationCreateOrUpdateView +from opentech.apply.review.models import Review from .models import ApplicationSubmission, RoundsAndLabs from .serializers import ( @@ -67,7 +68,9 @@ class SubmissionList(generics.ListAPIView): class SubmissionDetail(generics.RetrieveAPIView): - queryset = ApplicationSubmission.objects.all() + queryset = ApplicationSubmission.objects.all().prefetch_related( + Prefetch('reviews', Review.objects.submitted()), + ) serializer_class = SubmissionDetailSerializer permission_classes = ( permissions.IsAuthenticated, IsApplyStaffUser, diff --git a/opentech/apply/funds/forms.py b/opentech/apply/funds/forms.py index fe93ed094a6c98fe72672a3196c204de2c539429..4057a2906143566e572b6e1acb8497137eb6d51b 100644 --- a/opentech/apply/funds/forms.py +++ b/opentech/apply/funds/forms.py @@ -109,8 +109,8 @@ class UpdateReviewersForm(forms.ModelForm): self.role_fields[field_name] = data['role'] self.fields[field_name].initial = assigned_roles.get(data['role']) - self.submitted_reviewers = User.objects.filter( - id__in=self.instance.reviews.values('author'), + submitted_reviewers = User.objects.filter( + id__in=self.instance.assigned.reviewed().values('reviewer'), ) if self.can_alter_external_reviewers(self.instance, self.user): @@ -119,7 +119,7 @@ class UpdateReviewersForm(forms.ModelForm): self.prepare_field( 'reviewer_reviewers', initial=reviewers, - excluded=self.submitted_reviewers + excluded=submitted_reviewers ) # Move the non-role reviewers field to the end of the field list @@ -156,7 +156,6 @@ class UpdateReviewersForm(forms.ModelForm): 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 @@ -166,43 +165,25 @@ class UpdateReviewersForm(forms.ModelForm): } for role, reviewer in assigned_roles.items(): if reviewer: - AssignedReviewers.objects.filter(submission=instance, role=role).delete() - AssignedReviewers.objects.update_or_create(submission=instance, reviewer=reviewer, defaults={'role': role}) + AssignedReviewers.objects.update_role(role, reviewer, instance) # 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') assigned_reviewers = instance.assigned.without_roles() - assigned_reviewers.exclude( - reviewer__in=reviewers | self.submitted_reviewers + assigned_reviewers.never_tried_to_review().exclude( + reviewer__in=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 + AssignedReviewers.objects.bulk_create_reviewers( + [reviewer for reviewer in reviewers if reviewer.id not in remaining_reviewers], + instance, ) - # 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 @@ -247,43 +228,7 @@ class BatchUpdateReviewersForm(forms.Form): } for role, reviewer in assigned_roles.items(): if reviewer: - existing_assignments = AssignedReviewers.objects.filter( - submission__in=submissions, - role=role, - ) - - about_to_be_orphaned = [ - [assigned.reviewer, assigned.submission] - for assigned in existing_assignments - if submissions.get( - pk=assigned.submission.pk - ).reviews.filter(author=assigned.reviewer).exists() - ] - - # Being reassigned - AssignedReviewers.objects.filter( - submission__in=submissions, - role__isnull=False, - reviewer=reviewer, - ).delete() - - existing_assignments.update(reviewer=reviewer) - - AssignedReviewers.objects.bulk_create( - AssignedReviewers( - role=role, - reviewer=reviewer, - submission=submission, - ) for submission in submissions.exclude(pk__in=existing_assignments.values('submission')) - ) - - AssignedReviewers.objects.bulk_create( - AssignedReviewers( - role=None, - reviewer=reviewer, - submission=submission, - ) for reviewer, submission in about_to_be_orphaned - ) + AssignedReviewers.objects.update_role(role, reviewer, *submissions) return None diff --git a/opentech/apply/funds/migrations/0061_prepare_assigned_reviewers_for_data_migration.py b/opentech/apply/funds/migrations/0061_prepare_assigned_reviewers_for_data_migration.py new file mode 100644 index 0000000000000000000000000000000000000000..9b6685ae6b5100d44e65cb591827aa0cb1e051e2 --- /dev/null +++ b/opentech/apply/funds/migrations/0061_prepare_assigned_reviewers_for_data_migration.py @@ -0,0 +1,31 @@ +# Generated by Django 2.0.9 on 2019-02-24 20:08 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('auth', '0009_alter_user_last_name_max_length'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('funds', '0060_add_duration_type_in_duration_block'), + ] + + operations = [ + migrations.AddField( + model_name='assignedreviewers', + name='type', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.PROTECT, to='auth.Group'), + ), + migrations.AlterField( + model_name='assignedreviewers', + name='reviewer', + field=models.ForeignKey(limit_choices_to={'groups__name__in': ['Staff', 'Reviewer', 'Community reviewer', 'Partner']}, on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL), + ), + migrations.AlterUniqueTogether( + name='assignedreviewers', + unique_together={('submission', 'role'), ('submission', 'reviewer')}, + ), + ] diff --git a/opentech/apply/funds/migrations/0062_data_migrate_type_for_assigned_reviewers.py b/opentech/apply/funds/migrations/0062_data_migrate_type_for_assigned_reviewers.py new file mode 100644 index 0000000000000000000000000000000000000000..ed7f3003a08821a0cf4e45858ea1839434b6c1f5 --- /dev/null +++ b/opentech/apply/funds/migrations/0062_data_migrate_type_for_assigned_reviewers.py @@ -0,0 +1,55 @@ +# Generated by Django 2.0.9 on 2019-02-24 20:10 + +from django.db import migrations + +# Copied from opentech.apply.users.groups at time of migration to avoid +# importing and creating a future dependency. Changes to Group names should +# be handled in another migration + +STAFF_GROUP_NAME = 'Staff' +REVIEWER_GROUP_NAME = 'Reviewer' +PARTNER_GROUP_NAME = 'Partner' +COMMUNITY_REVIEWER_GROUP_NAME = 'Community reviewer' + +REVIEWER_GROUPS = set([ + STAFF_GROUP_NAME, + REVIEWER_GROUP_NAME, + COMMUNITY_REVIEWER_GROUP_NAME, + PARTNER_GROUP_NAME, +]) + + +def add_reviewer_type(apps, schema_editor): + AssignedReviewer = apps.get_model('funds', 'AssignedReviewers') + Group = apps.get_model('auth', 'Group') + for assigned in AssignedReviewer.objects.prefetch_related('reviewer__groups'): + groups = set(assigned.reviewer.groups.values_list('name', flat=True)) & REVIEWER_GROUPS + if len(groups) > 1: + if PARTNER_GROUP_NAME in groups and assigned.reviewer in assigned.submission.partners.all(): + groups = {PARTNER_GROUP_NAME} + elif COMMUNITY_REVIEWER_GROUP_NAME in groups: + groups = {COMMUNITY_REVIEWER_GROUP_NAME} + elif assigned.reviewer.is_staff or assigned.reviewer.is_superuser: + groups = {STAFF_GROUP_NAME} + else: + groups = {REVIEWER_GROUP_NAME} + elif not groups: + if assigned.reviewer.is_staff or assigned.reviewer.is_superuser: + groups = {STAFF_GROUP_NAME} + else: + groups = {REVIEWER_GROUP_NAME} + + group = Group.objects.get(name=groups.pop()) + assigned.type = group + assigned.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('funds', '0061_prepare_assigned_reviewers_for_data_migration'), + ] + + operations = [ + migrations.RunPython(add_reviewer_type, migrations.RunPython.noop), + ] diff --git a/opentech/apply/funds/migrations/0063_make_reviewer_type_required.py b/opentech/apply/funds/migrations/0063_make_reviewer_type_required.py new file mode 100644 index 0000000000000000000000000000000000000000..b094dfd8c40d8c1ac9e7cbbc57f171ba59d39931 --- /dev/null +++ b/opentech/apply/funds/migrations/0063_make_reviewer_type_required.py @@ -0,0 +1,19 @@ +# Generated by Django 2.0.9 on 2019-02-24 20:50 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('funds', '0062_data_migrate_type_for_assigned_reviewers'), + ] + + operations = [ + migrations.AlterField( + model_name='assignedreviewers', + name='type', + field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='auth.Group'), + ), + ] diff --git a/opentech/apply/funds/models/submissions.py b/opentech/apply/funds/models/submissions.py index 93442fa1e2ab35518ff80bed336238899962f96e..87d634b371a58f4c774f8f18ec57d755db2b6970 100644 --- a/opentech/apply/funds/models/submissions.py +++ b/opentech/apply/funds/models/submissions.py @@ -3,10 +3,11 @@ from functools import partialmethod from django.conf import settings from django.contrib.auth import get_user_model +from django.contrib.auth.models import Group from django.contrib.postgres.fields import JSONField from django.core.exceptions import PermissionDenied from django.db import models -from django.db.models import Count, IntegerField, OuterRef, Subquery, Sum, Q, Prefetch +from django.db.models import Case, Count, IntegerField, OuterRef, Subquery, Sum, Q, Prefetch, When from django.db.models.expressions import RawSQL, OrderBy from django.db.models.functions import Coalesce from django.dispatch import receiver @@ -22,13 +23,21 @@ from wagtail.contrib.forms.models import AbstractFormSubmission from opentech.apply.activity.messaging import messenger, MESSAGES from opentech.apply.determinations.models import Determination from opentech.apply.review.models import Review, ReviewOpinion -from opentech.apply.review.options import AGREE +from opentech.apply.review.options import MAYBE, AGREE, DISAGREE from opentech.apply.stream_forms.blocks import UploadableMediaBlock from opentech.apply.stream_forms.files import StreamFieldDataEncoder from opentech.apply.stream_forms.models import BaseStreamForm from .mixins import AccessFormData -from .utils import LIMIT_TO_STAFF, LIMIT_TO_STAFF_AND_REVIEWERS, LIMIT_TO_PARTNERS, WorkflowHelpers +from .utils import ( + LIMIT_TO_STAFF, + LIMIT_TO_REVIEWER_GROUPS, + LIMIT_TO_PARTNERS, + REVIEW_GROUPS, + REVIEWER_GROUP_NAME, + STAFF_GROUP_NAME, + WorkflowHelpers, +) from ..blocks import ApplicationCustomFormFieldsBlock, NAMED_BLOCKS from ..workflow import ( active_statuses, @@ -91,15 +100,16 @@ class ApplicationSubmissionQueryset(JSONOrderable): def in_review_for(self, user, assigned=True): user_review_statuses = get_review_active_statuses(user) - qs = self.filter(Q(status__in=user_review_statuses), ~Q(reviews__author=user) | Q(reviews__is_draft=True)) + qs = self.prefetch_related('reviews__author__reviewer') + qs = qs.filter(Q(status__in=user_review_statuses), ~Q(reviews__author__reviewer=user) | Q(reviews__is_draft=True)) if assigned: qs = qs.filter(reviewers=user) # If this user has agreed with a review, then they have reviewed this submission already - qs = qs.exclude(reviews__opinions__opinion=AGREE, reviews__opinions__author=user) + qs = qs.exclude(reviews__opinions__opinion=AGREE, reviews__opinions__author__reviewer=user) return qs.distinct() def reviewed_by(self, user): - return self.filter(reviews__author=user) + return self.filter(reviews__author__reviewer=user) def partner_for(self, user): return self.filter(partners=user) @@ -129,7 +139,10 @@ class ApplicationSubmissionQueryset(JSONOrderable): roles_for_review = self.model.assigned.field.model.objects.with_roles().filter( submission=OuterRef('id'), reviewer=user) - reviews = self.model.reviews.field.model.objects.filter(submission=OuterRef('id')) + review_model = self.model.reviews.field.model + reviews = review_model.objects.filter(submission=OuterRef('id')) + opinions = review_model.opinions.field.model.objects.filter(review__submission=OuterRef('id')) + reviewers = self.model.assigned.field.model.objects.filter(submission=OuterRef('id')) return self.with_latest_update().annotate( comment_count=Coalesce( @@ -139,22 +152,33 @@ class ApplicationSubmissionQueryset(JSONOrderable): ), 0, ), - review_count=Subquery( - reviews.values('submission').annotate(count=Count('pk')).values('count'), + opinion_disagree=Subquery( + opinions.filter(opinion=DISAGREE).values( + 'review__submission' + ).annotate(count=Count('*')).values('count')[:1], output_field=IntegerField(), ), review_staff_count=Subquery( - reviews.by_staff().values('submission').annotate(count=Count('pk')).values('count'), + reviewers.staff().values('submission').annotate(count=Count('pk')).values('count'), output_field=IntegerField(), ), - review_submitted_count=Subquery( - reviews.submitted().values('submission').annotate(count=Count('pk')).values('count'), + review_count=Subquery( + reviewers.values('submission').annotate(count=Count('pk')).values('count'), output_field=IntegerField(), ), - review_recommendation=Subquery( - reviews.submitted().values('submission').annotate(calc_recommendation=Sum('recommendation') / Count('recommendation')).values('calc_recommendation'), + review_submitted_count=Subquery( + reviewers.reviewed().values('submission').annotate(count=Count('pk')).values('count'), output_field=IntegerField(), ), + review_recommendation=Case( + When(opinion_disagree__gt=0, then=MAYBE), + default=Subquery( + reviews.submitted().values('submission').annotate( + calc_recommendation=Sum('recommendation') / Count('recommendation'), + ).values('calc_recommendation'), + output_field=IntegerField(), + ) + ), role_icon=Subquery(roles_for_review[:1].values('role__icon')), ).prefetch_related( Prefetch( @@ -581,9 +605,9 @@ class ApplicationSubmission( @property def missing_reviewers(self): - reviews_submitted = self.reviews.submitted().values('author') - reviewers = self.reviewers.exclude(id__in=reviews_submitted) - partners = self.partners.exclude(id__in=reviews_submitted) + reviewers_submitted = self.assigned.reviewed().values('reviewer') + reviewers = self.reviewers.exclude(id__in=reviewers_submitted) + partners = self.partners.exclude(id__in=reviewers_submitted) return reviewers | partners @property @@ -599,7 +623,7 @@ class ApplicationSubmission( return self.missing_reviewers.partners().exclude(id__in=self.staff_not_reviewed) def reviewed_by(self, user): - return self.reviews.submitted().filter(author=user).exists() + return self.assigned.reviewed().filter(reviewer=user).exists() def has_permission_to_review(self, user): if user.is_apply_staff: @@ -740,16 +764,94 @@ class AssignedReviewersQuerySet(models.QuerySet): def without_roles(self): return self.filter(role__isnull=True) + def reviewed(self): + return self.filter( + Q(opinions__isnull=False) | Q(Q(review__isnull=False) & Q(review__is_draft=False)) + ).distinct() + + def not_reviewed(self): + return self.filter( + Q(review__isnull=True) | Q(review__is_draft=True), + opinions__isnull=True, + ).distinct() + + def never_tried_to_review(self): + # Different from not reviewed as draft reviews allowed + return self.filter( + review__isnull=True, + opinions__isnull=True, + ).distinct() + def staff(self): - User = get_user_model() - return self.filter(reviewer__in=User.objects.staff()) + return self.filter(type__name=STAFF_GROUP_NAME) + + def get_or_create_for_user(self, submission, reviewer): + groups = set(reviewer.groups.values_list('name', flat=True)) & set(REVIEW_GROUPS) + if len(groups) > 1: + if PARTNER_GROUP_NAME in groups and reviewer in submission.partners.all(): + groups = {PARTNER_GROUP_NAME} + elif COMMUNITY_REVIEWER_GROUP_NAME in groups: + groups = {COMMUNITY_REVIEWER_GROUP_NAME} + elif review.author.is_apply_staff: + groups = {STAFF_GROUP_NAME} + else: + groups = {REVIEWER_GROUP_NAME} + elif not groups: + if assigned.reviewer.is_staff or assigned.reviewer.is_superuser: + groups = {STAFF_GROUP_NAME} + else: + groups = {REVIEWER_GROUP_NAME} + + group = Group.objects.get(name=groups.pop()) + + return self.get_or_create( + submission=submission, + reviewer=reviewer, + type=group, + ) + + def get_or_create_staff(self, submission, reviewer): + return self.get_or_create( + submission=submission, + reviewer=reviewer, + type=Group.objects.get(name=STAFF_GROUP_NAME), + ) + + def bulk_create_reviewers(self, reviewers, submission): + group = Group.objects.get(name=REVIEWER_GROUP_NAME) + self.bulk_create( + self.model( + submission=submission, + role=None, + reviewer=reviewer, + type=group, + ) for reviewer in reviewers + ) + + def update_role(self, role, reviewer, *submissions): + # Remove role who didn't review + self.filter(submission__in=submissions, role=role).never_tried_to_review().delete() + # Anyone else we remove their role + self.filter(submission__in=submissions, role=role).update(role=None) + # Create/update the new role reviewers + group = Group.objects.get(name=STAFF_GROUP_NAME) + for submission in submissions: + self.update_or_create( + submission=submission, + reviewer=reviewer, + defaults={'role': role, 'type': group}, + ) class AssignedReviewers(models.Model): reviewer = models.ForeignKey( settings.AUTH_USER_MODEL, on_delete=models.CASCADE, - limit_choices_to=LIMIT_TO_STAFF_AND_REVIEWERS, + limit_choices_to=LIMIT_TO_REVIEWER_GROUPS, + ) + type = models.ForeignKey( + 'auth.Group', + on_delete=models.PROTECT, ) submission = models.ForeignKey( ApplicationSubmission, @@ -766,10 +868,10 @@ class AssignedReviewers(models.Model): objects = AssignedReviewersQuerySet.as_manager() class Meta: - unique_together = ('submission', 'role') + unique_together = (('submission', 'role'), ('submission', 'reviewer')) def __str__(self): - return f'{self.reviewer} as {self.role}' + return f'{self.reviewer}' def __eq__(self, other): if not isinstance(other, models.Model): diff --git a/opentech/apply/funds/models/utils.py b/opentech/apply/funds/models/utils.py index c6c8ecb6153718edac9468dd0848c472888d23d1..f6661614fc08fdb6d955ab9bb6a1121f62c714ef 100644 --- a/opentech/apply/funds/models/utils.py +++ b/opentech/apply/funds/models/utils.py @@ -17,11 +17,17 @@ from opentech.apply.users.groups import REVIEWER_GROUP_NAME, STAFF_GROUP_NAME, P from ..workflow import WORKFLOWS +REVIEW_GROUPS = [ + STAFF_GROUP_NAME, + REVIEWER_GROUP_NAME, + COMMUNITY_REVIEWER_GROUP_NAME, + PARTNER_GROUP_NAME, +] LIMIT_TO_STAFF = {'groups__name': STAFF_GROUP_NAME} LIMIT_TO_REVIEWERS = {'groups__name': REVIEWER_GROUP_NAME} -LIMIT_TO_STAFF_AND_REVIEWERS = {'groups__name__in': [STAFF_GROUP_NAME, REVIEWER_GROUP_NAME]} LIMIT_TO_PARTNERS = {'groups__name': PARTNER_GROUP_NAME} LIMIT_TO_COMMUNITY_REVIEWERS = {'groups__name': COMMUNITY_REVIEWER_GROUP_NAME} +LIMIT_TO_REVIEWER_GROUPS = {'groups__name__in': REVIEW_GROUPS} def admin_url(page): diff --git a/opentech/apply/funds/serializers.py b/opentech/apply/funds/serializers.py index d2a620f84c4d748d8439f617f8408647ab1b1dc5..54478823b417cab8b1a3c054b7fac2f6a3d0a0a7 100644 --- a/opentech/apply/funds/serializers.py +++ b/opentech/apply/funds/serializers.py @@ -9,6 +9,7 @@ from opentech.apply.activity.models import Activity from opentech.apply.determinations.views import DeterminationCreateOrUpdateView from opentech.apply.review.models import Review, ReviewOpinion from opentech.apply.review.options import RECOMMENDATION_CHOICES +from opentech.apply.users.groups import PARTNER_GROUP_NAME, STAFF_GROUP_NAME from .models import ApplicationSubmission, RoundsAndLabs User = get_user_model() @@ -60,6 +61,7 @@ class ReviewSerializer(serializers.ModelSerializer): url = serializers.ReadOnlyField(source='get_absolute_url') opinions = OpinionSerializer(read_only=True, many=True) recommendation = serializers.SerializerMethodField() + score = serializers.ReadOnlyField(source='get_score_display') class Meta: model = Review @@ -76,7 +78,6 @@ class ReviewSummarySerializer(serializers.Serializer): reviews = ReviewSerializer(many=True, read_only=True) count = serializers.ReadOnlyField(source='reviews.count') score = serializers.ReadOnlyField(source='reviews.score') - assigned = ReviewSerializer(many=True, read_only=True) recommendation = serializers.SerializerMethodField() assigned = serializers.SerializerMethodField() @@ -88,37 +89,20 @@ class ReviewSummarySerializer(serializers.Serializer): } def get_assigned(self, obj): - assigned_reviewers = obj.assigned.select_related('reviewer', 'role') + assigned_reviewers = obj.assigned.select_related('reviewer', 'role', 'type') response = [ { - 'id': assigned.reviewer.id, + 'id': assigned.id, 'name': str(assigned.reviewer), 'role': { 'icon': assigned.role and assigned.role.icon_url('fill-12x12'), 'name': assigned.role and assigned.role.name, 'order': assigned.role and assigned.role.order, }, - 'is_staff': assigned.reviewer.is_apply_staff, - 'is_partner': assigned.reviewer.is_partner, + 'is_staff': assigned.type.name == STAFF_GROUP_NAME, + 'is_partner': assigned.type.name == PARTNER_GROUP_NAME, } for assigned in assigned_reviewers ] - - opinionated_reviewers = ReviewOpinion.objects.filter(review__submission=obj).values('author').distinct() - extra_reviewers = opinionated_reviewers.exclude(author__in=assigned_reviewers.values('reviewer')) - response.extend([ - { - 'id': user.id, - 'name': str(user), - 'role': { - 'icon': None, - 'name': None, - 'order': None, - }, - 'is_staff': user.is_apply_staff, - 'is_partner': user.is_partner, - } for user in User.objects.filter(id__in=extra_reviewers) - ]) - return response diff --git a/opentech/apply/funds/tables.py b/opentech/apply/funds/tables.py index a08dc0512c1c99a539bc3cc1a9e205ee380b644e..0918ac0d33af3aee816d41c7dc0126683af4175a 100644 --- a/opentech/apply/funds/tables.py +++ b/opentech/apply/funds/tables.py @@ -113,7 +113,7 @@ class LabeledCheckboxColumn(tables.CheckBoxColumn): class BaseAdminSubmissionsTable(SubmissionsTable): lead = tables.Column(order_by=('lead.full_name',)) - reviews_stats = tables.TemplateColumn(template_name='funds/tables/column_reviews.html', verbose_name=mark_safe("Reviews\n<span>Assgn.\tComp.</span>"), orderable=False) + reviews_stats = tables.TemplateColumn(template_name='funds/tables/column_reviews.html', verbose_name=mark_safe("Reviews<div>Assgn.\tComp.</div>"), orderable=False) screening_status = tables.Column(verbose_name="Screening") class Meta(SubmissionsTable.Meta): diff --git a/opentech/apply/funds/templates/funds/includes/review_sidebar.html b/opentech/apply/funds/templates/funds/includes/review_sidebar.html index 5db34c90f216c6bce5cc3bf853461420a2d06407..c812b7d9b54a62b7f319daab4f1bdbd20fcf1918 100644 --- a/opentech/apply/funds/templates/funds/includes/review_sidebar.html +++ b/opentech/apply/funds/templates/funds/includes/review_sidebar.html @@ -1,57 +1,45 @@ {% load review_tags %} <ul class="reviews-sidebar js-reviews-sidebar"> - {% if reviews_exist %} - <li class="reviews-sidebar__item reviews-sidebar__item--header"> - <div></div> - <div>{{ recommendation|traffic_light }}</div> - <div></div> - </li> + <li class="reviews-sidebar__item reviews-sidebar__item--header"> + <div></div> + <div>{{ recommendation|traffic_light }}</div> + <div></div> + </li> + + {% if not staff_reviewers_exist %} + <li class="reviews-sidebar__no-reviews">No staff reviewers yet</li> + <hr class="reviews-sidebar__split"> {% endif %} - {% if not assigned_staff %} - <li class="reviews-sidebar__no-reviews">No reviews yet</li> - {% endif %} + {% regroup assigned_reviewers by type as reviewers_list %} - {% for review_data in reviews_block.role_reviewed %} - {% include 'funds/includes/review_sidebar_item.html' with review=review_data.review reviewer=review_data.reviewer role=review_data.role opinions=review_data.opinions %} - {% endfor %} - - {% for review_data in reviews_block.staff_reviewed %} - {% include 'funds/includes/review_sidebar_item.html' with review=review_data.review reviewer=review_data.reviewer opinions=review_data.opinions %} - {% endfor %} - - {% for review_data in reviews_block.role_not_reviewed %} - {% include 'funds/includes/review_sidebar_item.html' with reviewer=review_data.reviewer role=review_data.role missing=True %} - {% endfor %} + {% for reviewer_type, reviewers in reviewers_list %} + {% if not forloop.first %} + {% ifchanged reviewer_type %} + <hr class="reviews-sidebar__split"> + {% endifchanged %} + {% endif %} - {% for review_data in reviews_block.staff_not_reviewed %} - {% include 'funds/includes/review_sidebar_item.html' with reviewer=review_data.reviewer missing=True %} + {% for reviewer in reviewers %} + {% if reviewer_type.name in hidden_types %} + {% include 'funds/includes/review_sidebar_item.html' with reviewer=reviewer hidden=True %} + {% if forloop.last %} + <li><a class="link link--bold link--underlined js-toggle-reviewers" href="#">All Assigned Advisors</a></li> + {% endif %} + {% else %} + {% include 'funds/includes/review_sidebar_item.html' with reviewer=reviewer %} + {% endif %} + {% endfor %} {% endfor %} - {% if reviews_block.partner_reviewed or reviews_block.partner_not_reviewed %} - <hr class="reviews-sidebar__split"> - - {% for review_data in reviews_block.partner_reviewed %} + {% if reviews_block.external_reviewed or reviews_block.external_not_reviewed %} + {% for review_data in reviews_block.external_reviewed %} {% include 'funds/includes/review_sidebar_item.html' with review=review_data.review reviewer=review_data.reviewer opinions=review_data.opinions %} {% endfor %} - {% for review_data in reviews_block.partner_not_reviewed %} - {% include 'funds/includes/review_sidebar_item.html' with reviewer=review_data.reviewer missing=True %} - {% endfor %} - - {% endif %} - - {% if object.stage.has_external_review %} - {% if reviews_block.external_reviewed or reviews_block.external_not_reviewed %} - <hr class="reviews-sidebar__split"> - {% for review_data in reviews_block.external_reviewed %} - {% include 'funds/includes/review_sidebar_item.html' with review=review_data.review reviewer=review_data.reviewer opinions=review_data.opinions %} - {% endfor %} - {% for review_data in reviews_block.external_not_reviewed %} + {% for review_data in reviews_block.external_not_reviewed %} {% include 'funds/includes/review_sidebar_item.html' with reviewer=review_data.reviewer missing=True class="is-hidden" %} - {% endfor %} + {% endfor %} - <li><a class="link link--bold link--underlined js-toggle-reviewers" href="#">All Assigned Advisors</a></li> - {% endif %} {% endif %} </ul> diff --git a/opentech/apply/funds/templates/funds/includes/review_sidebar_item.html b/opentech/apply/funds/templates/funds/includes/review_sidebar_item.html index 30ea362f223c7ff98db76f156cf30649948c96ec..3a6b8db9ed20a185ca19f18e19c9d549e295b5c5 100644 --- a/opentech/apply/funds/templates/funds/includes/review_sidebar_item.html +++ b/opentech/apply/funds/templates/funds/includes/review_sidebar_item.html @@ -1,43 +1,43 @@ {% load wagtailimages_tags %} -<li class="reviews-sidebar__item {{ class }} {% if missing %}no-response{% endif %}"> - {% if missing %} +<li class="reviews-sidebar__item {% if hidden and not reviewer.review %}is-hidden {% endif %}{% if not reviewer.review %}no-response {% endif %}"> + {% if not reviewer.review %} <div class="reviews-sidebar__name"> - <span>{{ reviewer }}</span> - {% if role %}{% image role.icon max-12x12 %}{% endif %} + <span>{{ reviewer}}</span> + {% if reviewer.role %}{% image reviewer.role.icon max-12x12 %}{% endif %} </div> <div>-</div> <div>-</div> {% else %} {% if request.user == reviewer or request.user.is_reviewer and review.reviewer_visibility or request.user.is_partner and review.reviewer_visibility or request.user.is_community_reviewer and review.reviewer_visibility or request.user.is_apply_staff %} <div> - <a href="{% url 'apply:submissions:reviews:review' submission_pk=review.submission.id pk=review.id %}"> + <a href="{% url 'apply:submissions:reviews:review' submission_pk=reviewer.submission.id pk=reviewer.review.id %}"> <div class="reviews-sidebar__name"> <span>{{ reviewer }}</span> - {% if role %}{% image role.icon max-12x12 %}{% endif %} + {% if reviewer.role %}{% image reviewer.role.icon max-12x12 %}{% endif %} </div> </a> - <div class="reviews-sidebar__date">{{ review.updated_at|date:"Y-m-d" }}</div> + <div class="reviews-sidebar__date">{{ reviewer.review.updated_at|date:"Y-m-d" }}</div> </div> {% else %} <div class="reviews-sidebar__name"> <span>{{ reviewer }}</span> - {% if role %}{% image role.icon max-12x12 %}{% endif %} + {% if reviewer.role %}{% image reviewer.role.icon max-12x12 %}{% endif %} </div> {% endif %} - <div>{{ review.get_recommendation_display }}</div> - <div>{{ review.get_score_display }}</div> + <div>{{ reviewer.review.get_recommendation_display }}</div> + <div>{{ reviewer.review.get_score_display }}</div> {% endif %} </li> -{% for opinion in review.opinions.all %} +{% for opinion in reviewer.review.opinions.all %} {% if forloop.first %} <ul class="reviews-sidebar__decision"> {% endif %} <li class="reviews-sidebar__item reviews-sidebar__item--decision"> <div class="reviews-sidebar__name"> <span>{{ opinion.author }}</span> - {% with role=opinion.get_author_role %} + {% with role=opinion.author.role %} {% if role %}{% image role.icon max-12x12 %}{% endif %} {% endwith %} </div> diff --git a/opentech/apply/funds/tests/factories/models.py b/opentech/apply/funds/tests/factories/models.py index 713b3dc1b046fa2e1f0d93c80f7d5e9244aee248..5aca57fa69bc98e232e65ac850f3368579c60f1e 100644 --- a/opentech/apply/funds/tests/factories/models.py +++ b/opentech/apply/funds/tests/factories/models.py @@ -25,10 +25,10 @@ from opentech.apply.funds.models.forms import ( RoundBaseReviewForm, ) from opentech.apply.funds.workflow import ConceptProposal, Request - -from opentech.apply.users.tests.factories import StaffFactory, UserFactory -from opentech.apply.stream_forms.testing.factories import FormDataFactory from opentech.apply.home.factories import ApplyHomePageFactory +from opentech.apply.stream_forms.testing.factories import FormDataFactory +from opentech.apply.users.groups import STAFF_GROUP_NAME, REVIEWER_GROUP_NAME +from opentech.apply.users.tests.factories import StaffFactory, UserFactory, GroupFactory from . import blocks @@ -249,13 +249,11 @@ class ApplicationSubmissionFactory(factory.DjangoModelFactory): @factory.post_generation def reviewers(self, create, reviewers, **kwargs): if create and reviewers: - AssignedReviewers.objects.bulk_create( - AssignedReviewers( + for reviewer in reviewers: + AssignedReviewers.objects.get_or_create_for_user( reviewer=reviewer, submission=self, - role=None - ) for reviewer in reviewers - ) + ) class ReviewerRoleFactory(factory.DjangoModelFactory): @@ -269,10 +267,15 @@ class ReviewerRoleFactory(factory.DjangoModelFactory): class AssignedReviewersFactory(factory.DjangoModelFactory): class Meta: model = AssignedReviewers + django_get_or_create = ('submission', 'reviewer') + + class Params: + staff = factory.Trait(type=factory.SubFactory(GroupFactory, name=STAFF_GROUP_NAME)) submission = factory.SubFactory(ApplicationSubmissionFactory) role = None reviewer = factory.SubFactory(StaffFactory) + type = factory.SubFactory(GroupFactory, name=REVIEWER_GROUP_NAME) class AssignedWithRoleReviewersFactory(AssignedReviewersFactory): diff --git a/opentech/apply/funds/tests/test_forms.py b/opentech/apply/funds/tests/test_forms.py index 443b84ef6bfa706d68d86e93dcb79ad76f4d0064..3525d19eb32abbf235846eb4e0033f4b2f2775cf 100644 --- a/opentech/apply/funds/tests/test_forms.py +++ b/opentech/apply/funds/tests/test_forms.py @@ -49,8 +49,8 @@ class TestReviewerFormQueries(TestCase): form = UpdateReviewersForm(user=user, instance=submission) - AssignedWithRoleReviewersFactory(role=roles[0], submission=submission, reviewer=staff[0]) - AssignedWithRoleReviewersFactory(role=roles[1], submission=submission, reviewer=staff[1]) + AssignedWithRoleReviewersFactory(role=roles[0], submission=submission, reviewer=staff[0], staff=True) + AssignedWithRoleReviewersFactory(role=roles[1], submission=submission, reviewer=staff[1], staff=True) data = {} for field, user in zip(form.fields, staff): @@ -64,9 +64,17 @@ class TestReviewerFormQueries(TestCase): self.assertTrue(form.is_valid()) # 1 - Submission - # 14 - 7 per role = 1 - delete 2 - savepoint 1 - get 1 - update 2 - release savepoint - # 1 - check - orphaned - with self.assertNumQueries(16): + # 24 - 12 per role = + # 1 - delete role no review + # 1 - select review + # 2 - cascades + # 1 - update role with review + # 1 - auth group + # 2 - savepoint + # 1 - get + # 1 - update + # 2 - release savepoint + with self.assertNumQueries(25): form.save() def test_queries_reviewers_swap(self): @@ -85,11 +93,13 @@ class TestReviewerFormQueries(TestCase): self.assertTrue(form.is_valid()) # 1 - Submission - # 1 - Delete old + # 1 - Select Review + # 2 - Cascase + # 1 - Fetch data # 1 - Cache existing + # 1 - auth group # 1 - Add new - # 1 - check - orphaned - with self.assertNumQueries(5): + with self.assertNumQueries(8): form.save() def test_queries_existing_reviews(self): @@ -98,8 +108,8 @@ class TestReviewerFormQueries(TestCase): reviewers = ReviewerFactory.create_batch(4) - ReviewFactory(submission=submission, author=reviewers[0]) - ReviewFactory(submission=submission, author=reviewers[1]) + ReviewFactory(submission=submission, author__reviewer=reviewers[0]) + ReviewFactory(submission=submission, author__reviewer=reviewers[1]) data = {'reviewer_reviewers': [reviewer.id for reviewer in reviewers[2:]]} @@ -111,6 +121,5 @@ class TestReviewerFormQueries(TestCase): # 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_models.py b/opentech/apply/funds/tests/test_models.py index b606227e4783889a66639830798ac5bcbc248c78..545ce1c57ad7faf5fe48e3dfc66055f23254e591 100644 --- a/opentech/apply/funds/tests/test_models.py +++ b/opentech/apply/funds/tests/test_models.py @@ -12,10 +12,14 @@ from django.test import TestCase, override_settings from opentech.apply.funds.models import ApplicationSubmission from opentech.apply.funds.blocks import EmailBlock, FullNameBlock from opentech.apply.funds.workflow import Request +from opentech.apply.review.tests.factories import ReviewFactory, ReviewOpinionFactory +from opentech.apply.review.options import NO, MAYBE from opentech.apply.utils.testing import make_request +from opentech.apply.users.tests.factories import StaffFactory from .factories import ( ApplicationSubmissionFactory, + AssignedReviewersFactory, CustomFormFieldsFactory, FundTypeFactory, LabFactory, @@ -496,3 +500,82 @@ class TestRequestForPartners(TestCase): response = rfp.serve(request) self.assertNotContains(response, 'not accepting') self.assertContains(response, 'Submit') + + +class TestForTableQueryset(TestCase): + def test_assigned_but_not_reviewed(self): + staff = StaffFactory() + submission = ApplicationSubmissionFactory() + AssignedReviewersFactory(submission=submission, reviewer=staff) + + qs = ApplicationSubmission.objects.for_table(user=staff) + submission = qs[0] + self.assertEqual(submission.opinion_disagree, None) + self.assertEqual(submission.review_count, 1) + self.assertEqual(submission.review_submitted_count, None) + self.assertEqual(submission.review_recommendation, None) + + def test_review_outcome(self): + staff = StaffFactory() + submission = ApplicationSubmissionFactory() + ReviewFactory(submission=submission) + qs = ApplicationSubmission.objects.for_table(user=staff) + submission = qs[0] + self.assertEqual(submission.opinion_disagree, None) + self.assertEqual(submission.review_count, 1) + self.assertEqual(submission.review_submitted_count, 1) + self.assertEqual(submission.review_recommendation, NO) + + def test_disagree_review_is_maybe(self): + staff = StaffFactory() + submission = ApplicationSubmissionFactory() + review = ReviewFactory(submission=submission) + ReviewOpinionFactory(opinion_disagree=True, review=review) + qs = ApplicationSubmission.objects.for_table(user=staff) + submission = qs[0] + self.assertEqual(submission.opinion_disagree, 1) + self.assertEqual(submission.review_count, 2) + self.assertEqual(submission.review_submitted_count, 2) + self.assertEqual(submission.review_recommendation, MAYBE) + + def test_dont_double_count_review_and_opinion(self): + staff = StaffFactory() + submission = ApplicationSubmissionFactory() + + review = ReviewFactory(submission=submission, author__reviewer=staff, author__staff=True) + opinion = ReviewOpinionFactory(opinion_disagree=True, review=review) + + # Another pair of review/opinion + review_two = ReviewFactory(author=opinion.author, submission=submission) + ReviewOpinionFactory(opinion_disagree=True, author__reviewer=staff, author__staff=True, review=review_two) + + qs = ApplicationSubmission.objects.for_table(user=staff) + submission = qs[0] + self.assertEqual(submission.opinion_disagree, 2) + self.assertEqual(submission.review_count, 2) + self.assertEqual(submission.review_submitted_count, 2) + self.assertEqual(submission.review_recommendation, MAYBE) + + def test_submissions_dont_conflict(self): + staff = StaffFactory() + submission_one = ApplicationSubmissionFactory() + submission_two = ApplicationSubmissionFactory() + review_one = ReviewFactory(submission=submission_one) + ReviewOpinionFactory(opinion_disagree=True, review=review_one) + + ReviewFactory(submission=submission_two) + + qs = ApplicationSubmission.objects.for_table(user=staff) + submission = qs[0] + self.assertEqual(submission, submission_one) + self.assertEqual(submission.opinion_disagree, 1) + self.assertEqual(submission.review_count, 2) + self.assertEqual(submission.review_submitted_count, 2) + self.assertEqual(submission.review_recommendation, MAYBE) + + submission = qs[1] + self.assertEqual(submission, submission_two) + self.assertEqual(submission.opinion_disagree, None) + self.assertEqual(submission.review_count, 1) + self.assertEqual(submission.review_submitted_count, 1) + self.assertEqual(submission.review_recommendation, NO) diff --git a/opentech/apply/funds/tests/test_views.py b/opentech/apply/funds/tests/test_views.py index 29bb2dfc0a7cdb423612b2d353d96d750195885b..41c17d983419f59cf29ac387d5680ac7e079c1a6 100644 --- a/opentech/apply/funds/tests/test_views.py +++ b/opentech/apply/funds/tests/test_views.py @@ -1,7 +1,6 @@ from datetime import datetime, timedelta import json -from django.test import TestCase from django.utils.text import slugify from opentech.apply.activity.models import Activity, INTERNAL @@ -289,7 +288,7 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase): 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]) + ReviewFactory(submission=submission, author__reviewer=self.staff[0], author__staff=True) # Assign a different reviewer to the same role self.post_form(submission, reviewer_roles=[self.staff[1]]) @@ -300,68 +299,30 @@ class TestReviewersUpdateView(BaseSubmissionViewTestCase): def test_can_be_made_role_and_not_duplciated(self): submission = ApplicationSubmissionFactory() - ReviewFactory(submission=submission, author=self.staff[0]) + ReviewFactory(submission=submission, author__reviewer=self.staff[0], author__staff=True) self.post_form(submission, reviewer_roles=[self.staff[0]]) self.assertCountEqual(submission.reviewers.all(), [self.staff[0]]) def test_can_remove_external_reviewer_and_review_remains(self): - submission = ApplicationSubmissionFactory(lead=self.user) + submission = InvitedToProposalFactory(lead=self.user) reviewer = self.reviewers[0] AssignedReviewersFactory(submission=submission, reviewer=reviewer) - ReviewFactory(submission=submission, author=reviewer) + ReviewFactory(submission=submission, author__reviewer=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 + def test_can_add_external_reviewer_and_review_remains(self): + submission = InvitedToProposalFactory(lead=self.user) + reviewer = self.reviewers[0] 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() + ReviewFactory(submission=submission, author__reviewer=reviewer) - AssignedWithRoleReviewersFactory(submission=submission, reviewer=staff) - ReviewFactory(submission=submission, author=staff) + self.post_form(submission, reviewers=[self.reviewers[1]]) - self.assertCountEqual(submission.reviewers.all(), [staff]) + self.assertCountEqual(submission.reviewers.all(), [reviewer, self.reviewers[1]]) class TestApplicantSubmissionView(BaseSubmissionViewTestCase): diff --git a/opentech/apply/funds/tests/views/test_batch_reviewers.py b/opentech/apply/funds/tests/views/test_batch_reviewers.py index 5df52e496b2a7893e445e0765c2eb2f996590269..f3d35b6a92cc0704dc9564a7c15e7504fe2338c9 100644 --- a/opentech/apply/funds/tests/views/test_batch_reviewers.py +++ b/opentech/apply/funds/tests/views/test_batch_reviewers.py @@ -76,8 +76,8 @@ class StaffTestCase(BaseBatchReviewerTestCase): def test_doesnt_remove_if_already_reviewed(self): AssignedWithRoleReviewersFactory(reviewer=self.staff[1], submission=self.submissions[0], role=self.roles[0]) - ReviewFactory(author=self.staff[1], submission=self.submissions[0], draft=False) - ReviewFactory(author=self.staff[1], submission=self.submissions[1], draft=False) + ReviewFactory(author__reviewer=self.staff[1], author__staff=True, submission=self.submissions[0], draft=False) + ReviewFactory(author__reviewer=self.staff[1], author__staff=True, submission=self.submissions[1], draft=False) submissions = self.submissions[0:2] reviewer_roles = [self.staff[0]] self.post_page(data=self.data(reviewer_roles, submissions)) diff --git a/opentech/apply/review/forms.py b/opentech/apply/review/forms.py index fba3b158d5543fadf37a76b6394d4bdcfe53fb2b..ba6e382d6030f1df4d5308d4977794c679eb94f8 100644 --- a/opentech/apply/review/forms.py +++ b/opentech/apply/review/forms.py @@ -18,13 +18,12 @@ class ReviewModelForm(StreamBaseForm, forms.ModelForm, metaclass=MixedMetaClass) class Meta: model = Review - fields = ['recommendation', 'visibility', 'score', 'submission', 'author'] + fields = ['recommendation', 'visibility', 'score', 'submission'] widgets = { 'recommendation': forms.HiddenInput(), 'score': forms.HiddenInput(), 'submission': forms.HiddenInput(), - 'author': forms.HiddenInput(), 'visibility': forms.HiddenInput(), } @@ -34,9 +33,8 @@ class ReviewModelForm(StreamBaseForm, forms.ModelForm, metaclass=MixedMetaClass) } } - def __init__(self, *args, user, submission, initial={}, instance=None, **kwargs): + def __init__(self, *args, submission, user=None, initial={}, instance=None, **kwargs): initial.update(submission=submission.id) - initial.update(author=user.id) if instance: for key, value in instance.form_data.items(): diff --git a/opentech/apply/review/migrations/0017_add_temp_author_field.py b/opentech/apply/review/migrations/0017_add_temp_author_field.py new file mode 100644 index 0000000000000000000000000000000000000000..7a5e44a4a21586842ed31bf1b084817298aca6d5 --- /dev/null +++ b/opentech/apply/review/migrations/0017_add_temp_author_field.py @@ -0,0 +1,25 @@ +# Generated by Django 2.0.9 on 2019-02-24 03:14 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('review', '0016_review_visibility'), + ] + + operations = [ + migrations.AddField( + model_name='review', + name='author_temp', + field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='review', to='funds.AssignedReviewers', null=True), + ), + migrations.AddField( + model_name='reviewopinion', + name='author_temp', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='opinions', to='funds.AssignedReviewers', null=True), + ), + + ] diff --git a/opentech/apply/review/migrations/0018_migrate_author_data.py b/opentech/apply/review/migrations/0018_migrate_author_data.py new file mode 100644 index 0000000000000000000000000000000000000000..16ccf1cb060441b3b5478308de93bc2b8324b43d --- /dev/null +++ b/opentech/apply/review/migrations/0018_migrate_author_data.py @@ -0,0 +1,87 @@ +# Generated by Django 2.0.9 on 2019-02-24 03:16 + +from django.db import migrations +from django.core.exceptions import ObjectDoesNotExist + +# Copied from opentech.apply.users.groups at time of migration to avoid +# importing and creating a future dependency. Changes to Group names should +# be handled in another migration + +STAFF_GROUP_NAME = 'Staff' +REVIEWER_GROUP_NAME = 'Reviewer' +PARTNER_GROUP_NAME = 'Partner' +COMMUNITY_REVIEWER_GROUP_NAME = 'Community reviewer' + +REVIEWER_GROUPS = set([ + STAFF_GROUP_NAME, + REVIEWER_GROUP_NAME, + COMMUNITY_REVIEWER_GROUP_NAME, + PARTNER_GROUP_NAME, +]) + + +def add_to_assigned_reviewers(apps, schema_editor): + Review = apps.get_model('review', 'Review') + AssignedReviewer = apps.get_model('funds', 'AssignedReviewers') + Group = apps.get_model('auth', 'Group') + for review in Review.objects.select_related('author'): + groups = set(review.author.groups.values_list('name', flat=True)) & REVIEWER_GROUPS + if len(groups) > 1: + if PARTNER_GROUP_NAME in groups and review.author in review.submission.partners.all(): + groups = {PARTNER_GROUP_NAME} + elif COMMUNITY_REVIEWER_GROUP_NAME in groups: + groups = {COMMUNITY_REVIEWER_GROUP_NAME} + elif review.author.is_staff or review.author.is_superuser: + groups = {STAFF_GROUP_NAME} + else: + groups = {REVIEWER_GROUP_NAME} + elif not groups: + if review.author.is_staff or review.author.is_superuser: + groups = {STAFF_GROUP_NAME} + else: + groups = {REVIEWER_GROUP_NAME} + + group = Group.objects.get(name=groups.pop()) + + assignment, _ = AssignedReviewer.objects.update_or_create( + submission=review.submission, + reviewer=review.author, + defaults={'type': group}, + ) + review.author_temp = assignment + review.save() + for opinion in review.opinions.select_related('author'): + opinion_assignment, _ = AssignedReviewer.objects.update_or_create( + submission=review.submission, + reviewer=opinion.author, + defaults={'type': Group.objects.get(name=STAFF_GROUP_NAME)}, + ) + opinion.author_temp = opinion_assignment + opinion.save() + + +def add_to_review_and_opinion(apps, schema_editor): + AssignedReviewer = apps.get_model('funds', 'AssignedReviewers') + for assigned in AssignedReviewer.objects.all(): + try: + assigned.review.author = assigned.reviewer + assigned.review.save() + except ObjectDoesNotExist: + pass + if assigned.opinions.exists(): + for opinion in assigned.opinions.all(): + opinion.author = assigned.reviewer + opinion.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('review', '0017_add_temp_author_field'), + ('funds', '0063_make_reviewer_type_required'), + ('users', '0010_add_community_reviewer_group'), + ] + + operations = [ + migrations.RunPython(add_to_assigned_reviewers, add_to_review_and_opinion), + ] diff --git a/opentech/apply/review/migrations/0019_replace_existing_author_field.py b/opentech/apply/review/migrations/0019_replace_existing_author_field.py new file mode 100644 index 0000000000000000000000000000000000000000..a5de51876c95f40b2c9854838c8603c315166307 --- /dev/null +++ b/opentech/apply/review/migrations/0019_replace_existing_author_field.py @@ -0,0 +1,42 @@ +# Generated by Django 2.0.9 on 2019-02-24 03:38 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('review', '0018_migrate_author_data'), + ] + + operations = [ + migrations.RemoveField( + model_name='review', + name='author', + ), + migrations.RenameField( + model_name='review', + old_name='author_temp', + new_name='author', + ), + migrations.AlterField( + model_name='review', + name='author', + field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='review', to='funds.AssignedReviewers'), + ), + migrations.RemoveField( + model_name='reviewopinion', + name='author', + ), + migrations.RenameField( + model_name='reviewopinion', + old_name='author_temp', + new_name='author', + ), + migrations.AlterField( + model_name='reviewopinion', + name='author', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='opinions', to='funds.AssignedReviewers'), + ), + ] diff --git a/opentech/apply/review/models.py b/opentech/apply/review/models.py index b604831a529c97d20a1fed04ddaa25e6f26b2b8d..740f9d847f5e36709d68d12f60966a41e9457376 100644 --- a/opentech/apply/review/models.py +++ b/opentech/apply/review/models.py @@ -1,9 +1,6 @@ -from django.conf import settings from django.contrib.postgres.fields import JSONField from django.core.serializers.json import DjangoJSONEncoder from django.db import models -from django.db.models.signals import post_save -from django.dispatch import receiver from django.urls import reverse from django.utils.functional import cached_property from django.utils.translation import ugettext_lazy as _ @@ -12,7 +9,7 @@ from wagtail.core.fields import StreamField from opentech.apply.funds.models.mixins import AccessFormData from opentech.apply.stream_forms.models import BaseStreamForm -from opentech.apply.users.models import User +from opentech.apply.users.groups import STAFF_GROUP_NAME, REVIEWER_GROUP_NAME, PARTNER_GROUP_NAME from .blocks import ( ReviewCustomFormFieldsBlock, @@ -21,7 +18,7 @@ from .blocks import ( ScoreFieldBlock, VisibilityBlock, ) -from .options import NA, YES, NO, MAYBE, RECOMMENDATION_CHOICES, OPINION_CHOICES, VISIBILITY, PRIVATE, REVIEWER +from .options import NA, YES, NO, MAYBE, RECOMMENDATION_CHOICES, DISAGREE, OPINION_CHOICES, VISIBILITY, PRIVATE, REVIEWER class ReviewFormFieldsMixin(models.Model): @@ -77,14 +74,17 @@ class ReviewQuerySet(models.QuerySet): def submitted(self): return self.filter(is_draft=False) + def _by_group(self, group): + return self.select_related('author__type').filter(author__type__name=group) + def by_staff(self): - return self.submitted().filter(author__in=User.objects.staff()) + return self.submitted()._by_group(STAFF_GROUP_NAME) def by_reviewers(self): - return self.submitted().filter(author__in=User.objects.reviewers()) + return self.submitted()._by_group(REVIEWER_GROUP_NAME) def by_partners(self): - return self.submitted().filter(author__in=User.objects.partners()) + return self.submitted()._by_group(PARTNER_GROUP_NAME) def staff_score(self): return self.by_staff().score() @@ -102,6 +102,11 @@ class ReviewQuerySet(models.QuerySet): return self.exclude(score=NA).aggregate(models.Avg('score'))['score__avg'] def recommendation(self): + opinions = self.values_list('opinions__opinion', flat=True) + + if any(opinion == DISAGREE for opinion in opinions): + return MAYBE + recommendations = self.values_list('recommendation', flat=True) try: recommendation = sum(recommendations) / len(recommendations) @@ -121,9 +126,10 @@ class ReviewQuerySet(models.QuerySet): class Review(ReviewFormFieldsMixin, BaseStreamForm, AccessFormData, models.Model): submission = models.ForeignKey('funds.ApplicationSubmission', on_delete=models.CASCADE, related_name='reviews') revision = models.ForeignKey('funds.ApplicationRevision', on_delete=models.SET_NULL, related_name='reviews', null=True) - author = models.ForeignKey( - settings.AUTH_USER_MODEL, - on_delete=models.PROTECT, + author = models.OneToOneField( + 'funds.AssignedReviewers', + related_name='review', + on_delete=models.CASCADE, ) form_data = JSONField(default=dict, encoder=DjangoJSONEncoder) @@ -175,23 +181,12 @@ class Review(ReviewFormFieldsMixin, BaseStreamForm, AccessFormData, models.Model return self.visibility == REVIEWER -@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 - AssignedReviewers.objects.get_or_create( - submission=review.submission, - reviewer=review.author, - ) - - class ReviewOpinion(models.Model): review = models.ForeignKey(Review, on_delete=models.CASCADE, related_name='opinions') author = models.ForeignKey( - settings.AUTH_USER_MODEL, - on_delete=models.PROTECT, + 'funds.AssignedReviewers', + related_name='opinions', + on_delete=models.CASCADE, ) opinion = models.IntegerField(choices=OPINION_CHOICES) @@ -201,7 +196,3 @@ class ReviewOpinion(models.Model): @property def opinion_display(self): return self.get_opinion_display() - - def get_author_role(self): - assignment = self.review.submission.assigned.with_roles().filter(reviewer=self.author).first() - return assignment.role if assignment else None diff --git a/opentech/apply/review/tests/factories/models.py b/opentech/apply/review/tests/factories/models.py index 59dd321eef691091c99220f0dd02aaad495af969..ee7077e60528ea187680fe3c8abcb475a64d9232 100644 --- a/opentech/apply/review/tests/factories/models.py +++ b/opentech/apply/review/tests/factories/models.py @@ -1,8 +1,7 @@ import factory -from opentech.apply.funds.tests.factories import ApplicationSubmissionFactory +from opentech.apply.funds.tests.factories import ApplicationSubmissionFactory, AssignedReviewersFactory from opentech.apply.stream_forms.testing.factories import FormDataFactory -from opentech.apply.users.tests.factories import StaffFactory from ...options import YES, NO, MAYBE, AGREE, DISAGREE, PRIVATE, REVIEWER from ...models import Review, ReviewForm, ReviewOpinion @@ -29,7 +28,7 @@ class ReviewFactory(factory.DjangoModelFactory): submission = factory.SubFactory(ApplicationSubmissionFactory) revision = factory.SelfAttribute('submission.live_revision') - author = factory.SubFactory(StaffFactory) + author = factory.SubFactory(AssignedReviewersFactory, submission=factory.SelfAttribute('..submission')) form_fields = factory.LazyAttribute(lambda o: o.submission.round.review_forms.first().fields) form_data = factory.SubFactory( ReviewFormDataFactory, @@ -49,7 +48,7 @@ class ReviewOpinionFactory(factory.DjangoModelFactory): opinion_disagree = factory.Trait(opinion=DISAGREE) review = factory.SubFactory(ReviewFactory) - author = factory.SubFactory(StaffFactory) + author = factory.SubFactory(AssignedReviewersFactory, staff=True, submission=factory.SelfAttribute('..review.submission')) opinion = DISAGREE diff --git a/opentech/apply/review/tests/test_models.py b/opentech/apply/review/tests/test_models.py new file mode 100644 index 0000000000000000000000000000000000000000..9f986ec10f3e78183b662ab2929af55d8a60b9a2 --- /dev/null +++ b/opentech/apply/review/tests/test_models.py @@ -0,0 +1,79 @@ +from django.test import TestCase + +from opentech.apply.funds.tests.factories import ApplicationSubmissionFactory +from .factories import ReviewFactory, ReviewOpinionFactory +from ..options import MAYBE, NO, YES + + +class TestReviewQueryset(TestCase): + def test_reviews_yes(self): + submission = ApplicationSubmissionFactory() + ReviewFactory(recommendation_yes=True, submission=submission) + ReviewFactory(recommendation_yes=True, submission=submission) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, YES) + + def test_reviews_no(self): + submission = ApplicationSubmissionFactory() + ReviewFactory(submission=submission) + ReviewFactory(submission=submission) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, NO) + + def test_reviews_maybe(self): + submission = ApplicationSubmissionFactory() + ReviewFactory(recommendation_maybe=True, submission=submission) + ReviewFactory(recommendation_maybe=True, submission=submission) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, MAYBE) + + def test_reviews_mixed(self): + submission = ApplicationSubmissionFactory() + ReviewFactory(recommendation_yes=True, submission=submission) + ReviewFactory(submission=submission) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, MAYBE) + + def test_review_yes_opinion_agree(self): + submission = ApplicationSubmissionFactory() + review = ReviewFactory(recommendation_yes=True, submission=submission) + ReviewOpinionFactory(review=review, opinion_agree=True) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, YES) + + def test_review_yes_opinion_disagree(self): + submission = ApplicationSubmissionFactory() + review = ReviewFactory(recommendation_yes=True, submission=submission) + ReviewOpinionFactory(review=review, opinion_disagree=True) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, MAYBE) + + def test_review_no_opinion_agree(self): + submission = ApplicationSubmissionFactory() + review = ReviewFactory(submission=submission) + ReviewOpinionFactory(review=review, opinion_agree=True) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, NO) + + def test_review_no_opinion_disagree(self): + submission = ApplicationSubmissionFactory() + review = ReviewFactory(submission=submission) + ReviewOpinionFactory(review=review, opinion_disagree=True) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, MAYBE) + + def test_review_not_all_opinion(self): + submission = ApplicationSubmissionFactory() + ReviewFactory(recommendation_yes=True, submission=submission) + review = ReviewFactory(recommendation_yes=True, submission=submission) + ReviewOpinionFactory(review=review, opinion_agree=True) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, YES) + + def test_review_yes_mixed_opinion(self): + submission = ApplicationSubmissionFactory() + review = ReviewFactory(submission=submission) + ReviewOpinionFactory(review=review, opinion_agree=True) + ReviewOpinionFactory(review=review, opinion_disagree=True) + recommendation = submission.reviews.recommendation() + self.assertEqual(recommendation, MAYBE) diff --git a/opentech/apply/review/tests/test_views.py b/opentech/apply/review/tests/test_views.py index 1a9bfbe7a23df848adff5f77ba40dd05aeb70ae9..7f4614e629916420299604df7885813e6a302073 100644 --- a/opentech/apply/review/tests/test_views.py +++ b/opentech/apply/review/tests/test_views.py @@ -19,7 +19,7 @@ class StaffReviewsTestCase(BaseViewTestCase): return {'pk': instance.id, 'submission_pk': instance.submission.id} def test_can_access_review(self): - review = ReviewFactory(author=self.user) + review = ReviewFactory(author__reviewer=self.user, author__staff=True) response = self.get_page(review) self.assertContains(response, review.submission.title) self.assertContains(response, self.user.full_name) @@ -47,7 +47,7 @@ class StaffReviewListingTestCase(BaseViewTestCase): self.assertContains(response, submission.title) self.assertContains(response, reverse('funds:submissions:detail', kwargs={'pk': submission.id})) for review in reviews: - self.assertContains(response, review.author.full_name) + self.assertContains(response, review.author.reviewer.full_name) def test_draft_reviews_dont_appear(self): submission = ApplicationSubmissionFactory() @@ -55,7 +55,7 @@ class StaffReviewListingTestCase(BaseViewTestCase): response = self.get_page(submission, 'list') self.assertContains(response, submission.title) self.assertContains(response, reverse('funds:submissions:detail', kwargs={'pk': submission.id})) - self.assertNotContains(response, review.author.full_name) + self.assertNotContains(response, review.author.reviewer.full_name) class StaffReviewFormTestCase(BaseViewTestCase): @@ -82,13 +82,13 @@ class StaffReviewFormTestCase(BaseViewTestCase): self.assertEqual(response.status_code, 403) def test_cant_resubmit_review(self): - ReviewFactory(submission=self.submission, author=self.user) + ReviewFactory(submission=self.submission, author__reviewer=self.user, author__staff=True) response = self.post_page(self.submission, {'data': 'value'}, 'form') self.assertEqual(response.context['has_submitted_review'], True) self.assertEqual(response.context['title'], 'Update Review draft') def test_can_edit_draft_review(self): - ReviewFactory(submission=self.submission, author=self.user, is_draft=True) + ReviewFactory(submission=self.submission, author__reviewer=self.user, author__staff=True, is_draft=True) response = self.get_page(self.submission, 'form') self.assertEqual(response.context['has_submitted_review'], False) self.assertEqual(response.context['title'], 'Update Review draft') @@ -203,7 +203,7 @@ class ReviewDetailTestCase(BaseViewTestCase): def test_review_detail_recommendation(self): submission = ApplicationSubmissionFactory(status='draft_proposal', workflow_stages=2) - review = ReviewFactory(submission=submission, author=self.user, recommendation_yes=True) + review = ReviewFactory(submission=submission, author__reviewer=self.user, recommendation_yes=True) response = self.get_page(review) self.assertContains(response, submission.title) self.assertContains(response, "<p>Yes</p>") @@ -211,8 +211,8 @@ class ReviewDetailTestCase(BaseViewTestCase): def test_review_detail_opinion(self): staff = StaffFactory() submission = ApplicationSubmissionFactory(status='draft_proposal', workflow_stages=2) - review = ReviewFactory(submission=submission, author=self.user, recommendation_yes=True) - ReviewOpinionFactory(review=review, author=staff, opinion_disagree=True) + review = ReviewFactory(submission=submission, author__reviewer=self.user, recommendation_yes=True) + ReviewOpinionFactory(review=review, author__reviewer=staff, opinion_disagree=True) response = self.get_page(review) self.assertContains(response, "Disagrees") @@ -228,8 +228,8 @@ class ReviewListTestCase(BaseViewTestCase): def test_review_list_opinion(self): staff = StaffFactory() submission = ApplicationSubmissionFactory(status='draft_proposal', workflow_stages=2) - review = ReviewFactory(submission=submission, author=self.user, recommendation_yes=True) - ReviewOpinionFactory(review=review, author=staff, opinion_disagree=True) + review = ReviewFactory(submission=submission, author__reviewer=self.user, recommendation_yes=True) + ReviewOpinionFactory(review=review, author__reviewer=staff, opinion_disagree=True) response = self.get_page(review) response_opinion = response.context['review_data']['opinions']['answers'][0] self.assertIn("Disagrees", response_opinion) @@ -251,18 +251,28 @@ class StaffReviewOpinionCase(BaseViewTestCase): def test_can_see_opinion_buttons_on_others_review(self): staff = StaffFactory() - review = ReviewFactory(submission=self.submission, author=staff, recommendation_yes=True) + review = ReviewFactory( + submission=self.submission, + author__reviewer=staff, + author__staff=True, + recommendation_yes=True, + ) response = self.get_page(review) self.assertContains(response, 'name="agree"') def test_cant_see_opinion_buttons_on_self_review(self): - review = ReviewFactory(submission=self.submission, author=self.user, recommendation_yes=True) + review = ReviewFactory(submission=self.submission, author__reviewer=self.user, recommendation_yes=True) response = self.get_page(review) self.assertNotContains(response, 'name="agree"') def test_can_add_opinion_to_others_review(self): staff = StaffFactory() - review = ReviewFactory(submission=self.submission, author=staff, recommendation_yes=True) + review = ReviewFactory( + submission=self.submission, + author__reviewer=staff, + author__staff=True, + recommendation_yes=True, + ) response = self.post_page(review, {'agree': AGREE}) self.assertTrue(review.opinions.first().opinion_display in Activity.objects.first().message) self.assertEqual(ReviewOpinion.objects.all().count(), 1) @@ -272,7 +282,12 @@ class StaffReviewOpinionCase(BaseViewTestCase): def test_disagree_opinion_redirects_to_review_form(self): staff = StaffFactory() - review = ReviewFactory(submission=self.submission, author=staff, recommendation_yes=True) + review = ReviewFactory( + submission=self.submission, + author__reviewer=staff, + author__staff=True, + recommendation_yes=True, + ) response = self.post_page(review, {'disagree': DISAGREE}) url = self.url_from_pattern('funds:submissions:reviews:form', kwargs={'submission_pk': self.submission.id}) self.assertRedirects(response, url) @@ -293,7 +308,7 @@ class NonStaffReviewOpinionCase(BaseViewTestCase): def test_nonstaff_cant_post_opinion_to_review(self): staff = StaffFactory() - review = ReviewFactory(submission=self.submission, author=staff, recommendation_yes=True) + review = ReviewFactory(submission=self.submission, author__reviewer=staff, author__staff=True, recommendation_yes=True) response = self.post_page(review, {'agree': AGREE}) self.assertEqual(response.status_code, 403) @@ -308,14 +323,14 @@ class ReviewDetailVisibilityTestCase(BaseViewTestCase): def test_review_detail_visibility_private(self): submission = ApplicationSubmissionFactory(status='external_review', workflow_stages=2) - review = ReviewFactory(submission=submission, author=self.user, visibility_private=True) + review = ReviewFactory(submission=submission, author__reviewer=self.user, visibility_private=True) self.client.force_login(self.user_factory()) response = self.get_page(review) self.assertEqual(response.status_code, 403) def test_review_detail_visibility_reviewer(self): submission = ApplicationSubmissionFactory(status='external_review', workflow_stages=2) - review = ReviewFactory(submission=submission, author=self.user, visibility_reviewer=True) + review = ReviewFactory(submission=submission, author__reviewer=self.user, visibility_reviewer=True) self.client.force_login(self.user_factory()) response = self.get_page(review) self.assertEqual(response.status_code, 200) diff --git a/opentech/apply/review/views.py b/opentech/apply/review/views.py index 5852d3a81bba3265e64194d66c263bcc948423ca..c31a7b7001664ef8c2256f28ea184ff6c12972c1 100644 --- a/opentech/apply/review/views.py +++ b/opentech/apply/review/views.py @@ -1,8 +1,7 @@ -from collections import defaultdict - from django.contrib.auth.decorators import login_required from django.contrib.auth.mixins import UserPassesTestMixin from django.core.exceptions import PermissionDenied +from django.db import models from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404 from django.template.loader import get_template @@ -13,12 +12,12 @@ from django.views.generic import CreateView, ListView, DetailView, DeleteView from wagtail.core.blocks import RichTextBlock from opentech.apply.activity.messaging import messenger, MESSAGES -from opentech.apply.funds.models import ApplicationSubmission +from opentech.apply.funds.models import ApplicationSubmission, AssignedReviewers from opentech.apply.review.blocks import RecommendationBlock, RecommendationCommentsBlock from opentech.apply.review.forms import ReviewModelForm, ReviewOpinionForm from opentech.apply.stream_forms.models import BaseStreamForm from opentech.apply.users.decorators import staff_required -from opentech.apply.users.models import User +from opentech.apply.users.groups import REVIEWER_GROUP_NAME, STAFF_GROUP_NAME, PARTNER_GROUP_NAME, COMMUNITY_REVIEWER_GROUP_NAME from opentech.apply.utils.views import CreateOrUpdateView from .models import Review @@ -27,58 +26,38 @@ from .options import DISAGREE class ReviewContextMixin: def get_context_data(self, **kwargs): - assigned = self.object.assigned.order_by('role__order').select_related('reviewer') - reviews = self.object.reviews.submitted().select_related('author') - - reviews_dict = {} - for review in reviews: - reviews_dict[review.author.pk] = review - - # Get all the authors of opinions, these authors should not show up in the 'xxx_not_reviewed' lists - opinion_authors = User.objects.filter(pk__in=self.object.reviews.opinions().values('author')).distinct() - - reviews_block = defaultdict(list) - for assigned_reviewer in assigned: - reviewer = assigned_reviewer.reviewer - role = assigned_reviewer.role - review = reviews_dict.get(reviewer.pk, None) - key = None - if role: - if review: - key = 'role_reviewed' - elif reviewer not in opinion_authors: - key = 'role_not_reviewed' - elif reviewer.is_apply_staff: - if review: - key = 'staff_reviewed' - elif review not in opinion_authors: - key = 'staff_not_reviewed' - elif reviewer.is_partner: - if review: - key = 'partner_reviewed' - elif review not in opinion_authors: - key = 'partner_not_reviewed' - else: - if review: - key = 'external_reviewed' - else: - key = 'external_not_reviewed' - if key: # Do not add this reviewer to any list if they haven't reviewed but have left an opinion - review_info_dict = { - 'reviewer': reviewer, - 'review': review, - 'role': role, - } - reviews_block[key].append(review_info_dict) + review_order = [ + STAFF_GROUP_NAME, + COMMUNITY_REVIEWER_GROUP_NAME, + PARTNER_GROUP_NAME, + REVIEWER_GROUP_NAME, + ] + + ordering = [models.When(type__name=review_type, then=models.Value(i)) for i, review_type in enumerate(review_order)] + assigned_reviewers = self.object.assigned.annotate( + type_order=models.Case( + *ordering, + output_field=models.IntegerField(), + ) + ).order_by( + 'role__order', + 'review', + 'type_order' + ).select_related( + 'reviewer', + 'role', + ) + if not self.object.stage.has_external_review: + assigned_reviewers = assigned_reviewers.staff() # Calculate the recommendation based on role and staff reviews recommendation = self.object.reviews.by_staff().recommendation() return super().get_context_data( - reviews_block=reviews_block, + hidden_types=[REVIEWER_GROUP_NAME], + staff_reviewers_exist=assigned_reviewers.staff().exists(), + assigned_reviewers=assigned_reviewers, recommendation=recommendation, - reviews_exist=reviews.count(), - assigned_staff=assigned.staff().exists(), **kwargs, ) @@ -99,7 +78,7 @@ class ReviewCreateOrUpdateView(BaseStreamForm, CreateOrUpdateView): template_name = 'review/review_form.html' def get_object(self, queryset=None): - return self.model.objects.get(submission=self.submission, author=self.request.user) + return self.model.objects.get(submission=self.submission, author__reviewer=self.request.user) def dispatch(self, request, *args, **kwargs): self.submission = get_object_or_404(ApplicationSubmission, id=self.kwargs['submission_pk']) @@ -136,13 +115,18 @@ class ReviewCreateOrUpdateView(BaseStreamForm, CreateOrUpdateView): def form_valid(self, form): form.instance.form_fields = self.get_defined_fields() + form.instance.author, _ = AssignedReviewers.objects.get_or_create_for_user( + submission=self.submission, + reviewer=self.request.user, + ) + response = super().form_valid(form) if not self.object.is_draft: messenger( MESSAGES.NEW_REVIEW, request=self.request, - user=self.object.author, + user=self.request.user, submission=self.submission, related=self.object, ) @@ -158,9 +142,9 @@ class ReviewDisplay(UserPassesTestMixin, DetailView): def get_context_data(self, **kwargs): review = self.get_object() - if review.author != self.request.user: + if review.author.reviewer != self.request.user: consensus_form = ReviewOpinionForm( - instance=review.opinions.filter(author=self.request.user).first(), + instance=review.opinions.filter(author__reviewer=self.request.user).first(), ) else: consensus_form = None @@ -212,7 +196,7 @@ class ReviewOpinionFormView(UserPassesTestMixin, CreateView): self.object = self.get_object() kwargs = super().get_form_kwargs() instance = kwargs['instance'] - kwargs['instance'] = instance.opinions.filter(author=self.request.user).first() + kwargs['instance'] = instance.opinions.filter(author__reviewer=self.request.user).first() return kwargs def test_func(self): @@ -241,7 +225,11 @@ class ReviewOpinionFormView(UserPassesTestMixin, CreateView): def form_valid(self, form): self.review = self.get_object() - form.instance.author = self.request.user + author, _ = AssignedReviewers.objects.get_or_create_for_user( + submission=self.review.submission, + reviewer=self.request.user, + ) + form.instance.author = author form.instance.review = self.review response = super().form_valid(form) opinion = form.instance diff --git a/opentech/settings/dev.py b/opentech/settings/dev.py index c41bae6bedf56004c4e9439c0484e1332642fd6e..b6fd212bc1fa13d4ba601106914e106ea8906468 100644 --- a/opentech/settings/dev.py +++ b/opentech/settings/dev.py @@ -22,6 +22,7 @@ INSTALLED_APPS = INSTALLED_APPS + [ 'wagtail.contrib.styleguide', ] + SECURE_SSL_REDIRECT = False # Change these in local.py. diff --git a/opentech/static_src/src/app/src/redux/reducers/rounds.js b/opentech/static_src/src/app/src/redux/reducers/rounds.js index b57da6cfb11196c9b390f269e167a7cb66da61ce..5ce622d9b500f2349091fbbcff875fbc340451c4 100644 --- a/opentech/static_src/src/app/src/redux/reducers/rounds.js +++ b/opentech/static_src/src/app/src/redux/reducers/rounds.js @@ -101,16 +101,16 @@ function roundsByID(state = {}, action) { } -function errorMessage(state = null, action) { +function errorMessage(state = '', action) { switch(action.type) { case FAIL_LOADING_SUBMISSIONS_BY_ROUND: case FAIL_LOADING_ROUND: - return action.message; + return action.message || ''; case UPDATE_SUBMISSIONS_BY_ROUND: case START_LOADING_SUBMISSIONS_BY_ROUND: case UPDATE_ROUND: case START_LOADING_ROUND: - return null; + return ''; default: return state; } diff --git a/opentech/static_src/src/sass/apply/components/_all-submissions-table.scss b/opentech/static_src/src/sass/apply/components/_all-submissions-table.scss index a241d73f00016bb4445753a9eab09a6a752676ba..2e12efb8da5cfd2a96acf318a922a9f07ed25e87 100644 --- a/opentech/static_src/src/sass/apply/components/_all-submissions-table.scss +++ b/opentech/static_src/src/sass/apply/components/_all-submissions-table.scss @@ -15,7 +15,7 @@ &.reviews_stats { // sass-lint:disable-line class-name-format color: $color--black-60; - span { + div { font-size: 13px; } }