Merge pull request #1220 from OpenTechFund/feature/1191-review-ordering-tables

GH-1191 Make reviewer ordering consistent and tweak styles on tables

Author: frjo

opentech/apply/funds/models/submissions.py

@@ -7,7 +7,18 @@
 from django.contrib.postgres.fields import JSONField
 from django.core.exceptions import PermissionDenied
 from django.db import models
-from django.db.models import Case, Count, IntegerField, OuterRef, Subquery, Sum, Q, Prefetch, When
+from django.db.models import (
+    Case,
+    Count,
+    IntegerField,
+    F,
+    OuterRef,
+    Prefetch,
+    Q,
+    Subquery,
+    Sum,
+    When,
+)
 from django.db.models.expressions import RawSQL, OrderBy
 from django.db.models.functions import Coalesce
 from django.dispatch import receiver
@@ -22,17 +33,19 @@
 
 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.models import ReviewOpinion
 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 (
+    COMMUNITY_REVIEWER_GROUP_NAME,
     LIMIT_TO_STAFF,
     LIMIT_TO_REVIEWER_GROUPS,
     LIMIT_TO_PARTNERS,
+    PARTNER_GROUP_NAME,
     REVIEW_GROUPS,
     REVIEWER_GROUP_NAME,
     STAFF_GROUP_NAME,
@@ -167,7 +180,9 @@ def for_table(self, user):
                 output_field=IntegerField(),
             ),
             review_submitted_count=Subquery(
-                reviewers.reviewed().values('submission').annotate(count=Count('pk')).values('count'),
+                reviewers.reviewed().values('submission').annotate(
+                    count=Count('pk', distinct=True)
+                ).values('count'),
                 output_field=IntegerField(),
             ),
             review_recommendation=Case(
@@ -182,10 +197,18 @@ def for_table(self, user):
             role_icon=Subquery(roles_for_review[:1].values('role__icon')),
         ).prefetch_related(
             Prefetch(
-                'reviews', queryset=Review.objects.select_related('author').prefetch_related(
-                    Prefetch('opinions', queryset=ReviewOpinion.objects.select_related('author'))
-                )
+                'assigned',
+                queryset=AssignedReviewers.objects.reviewed().review_order().prefetch_related(
+                    Prefetch('opinions', queryset=ReviewOpinion.objects.select_related('author__reviewer'))
+                ),
+                to_attr='has_reviewed'
+            ),
+            Prefetch(
+                'assigned',
+                queryset=AssignedReviewers.objects.not_reviewed().staff(),
+                to_attr='hasnt_reviewed'
             )
+
         ).select_related(
             'page',
             'round',
@@ -758,6 +781,43 @@ def get_absolute_url(self):
 
 
 class AssignedReviewersQuerySet(models.QuerySet):
+    def review_order(self):
+        review_order = [
+            STAFF_GROUP_NAME,
+            PARTNER_GROUP_NAME,
+            COMMUNITY_REVIEWER_GROUP_NAME,
+            REVIEWER_GROUP_NAME,
+        ]
+
+        ordering = [
+            models.When(type__name=review_type, then=models.Value(i))
+            for i, review_type in enumerate(review_order)
+        ]
+        return self.exclude(
+            # Remove people from the list who are opinionated but
+            # didn't review, they appear elsewhere
+            opinions__isnull=False,
+            review__isnull=True,
+        ).annotate(
+            type_order=models.Case(
+                *ordering,
+                output_field=models.IntegerField(),
+            ),
+            has_review=models.Case(
+                models.When(review__isnull=True, then=models.Value(1)),
+                models.When(review__is_draft=True, then=models.Value(1)),
+                default=models.Value(0),
+                output_field=models.IntegerField(),
+            )
+        ).order_by(
+            'type_order',
+            'has_review',
+            F('role__order').asc(nulls_last=True),
+        ).select_related(
+            'reviewer',
+            'role',
+        )
+
     def with_roles(self):
         return self.filter(role__isnull=False)
 
@@ -766,13 +826,14 @@ def without_roles(self):
 
     def reviewed(self):
         return self.filter(
-            Q(opinions__isnull=False) | Q(Q(review__isnull=False) & Q(review__is_draft=False))
+            Q(opinions__opinion=AGREE) |
+            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,
+            Q(opinions__isnull=True) | Q(opinions__opinion=DISAGREE),
         ).distinct()
 
     def never_tried_to_review(self):

opentech/apply/funds/templates/funds/includes/review_sidebar.html

@@ -31,15 +31,4 @@
             {% endif %}
         {% endfor %}
     {% endfor %}
-
-    {% 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.external_not_reviewed %}
-            {% include 'funds/includes/review_sidebar_item.html' with reviewer=review_data.reviewer missing=True class="is-hidden" %}
-        {% endfor %}
-
-    {% endif %}
 </ul>

opentech/apply/funds/templates/funds/includes/review_sidebar_item.html

@@ -1,7 +1,7 @@
 {% load wagtailimages_tags %}
 
 <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 %}
+    {% if not reviewer.review or reviewer.review.is_draft %}
         <div class="reviews-sidebar__name">
             <span>{{ reviewer}}</span>
             {% if reviewer.role %}{% image reviewer.role.icon max-12x12 %}{% endif %}

opentech/apply/funds/templates/funds/tables/table.html

@@ -1,5 +1,5 @@
 {% extends 'django_tables2/table.html' %}
-{% load django_tables2 table_tags review_tags %}
+{% load django_tables2 table_tags review_tags wagtailimages_tags %}
 
 {% block table.tbody.row %}
     <tr {{ row.attrs.as_html }}>
@@ -36,20 +36,27 @@
                         <td>
                             <strong>{{ submission.screening_status|default:"Awaiting Screen status" }}</strong>
                         </td>
+
                         <td>
                             <ul class="list list--no-margin">
-                                {% for review in submission.reviews.submitted %}
+                                {% for reviewer in submission.has_reviewed %}
                                     <li class="list__item list__item--reviewer">
-                                        <span class="list__item--reviewer-name">{{ review.author }}</span>
-                                        <span class="list__item list__item--reviewer-outcome">{{ review.get_recommendation_display }}</span>
+                                        <span class="list__item--reviewer-name">
+                                            {{ reviewer }}
+                                            {% if reviewer.role %}{% image reviewer.role.icon max-12x12 %}{% endif %}
+                                        </span>
+                                        <span class="list__item list__item--reviewer-outcome">{{ reviewer.review.get_recommendation_display }}</span>
                                     </li>
-                                    {% for opinion in review.opinions.all %}
+                                    {% for opinion in reviewer.review.opinions.all %}
                                         {% if forloop.first %}
                                             <ul class="list list--opinion">
                                         {% endif %}
 
                                             <li class="list__item list__item--reviewer list__item--opinion">
-                                                <span class="list__item--reviewer-name">{{ opinion.author }}</span>
+                                                <span class="list__item--reviewer-name">
+                                                    {{ opinion.author }}
+                                                    {% if opinion.author.role %}{% image opinion.author.role.icon max-12x12 %}{% endif %}
+                                                </span>
                                                 <span class="list__item list__item--reviewer-outcome {{ opinion.get_opinion_display|lower }}">{{ opinion.get_opinion_display }}</span>
                                             </li>
 
@@ -58,9 +65,12 @@
                                         {% endif %}
                                     {% endfor %}
                                 {% endfor %}
-                                {% for reviewer in submission.staff_not_reviewed %}
+                                {% for reviewer in submission.hasnt_reviewed %}
                                     <li class="list__item list__item--reviewer">
-                                        <span class="list__item--reviewer-name">{{ reviewer }}</span>
+                                        <span class="list__item--reviewer-name">
+                                            {{ reviewer }}
+                                            {% if reviewer.role %}{% image reviewer.role.icon max-12x12 %}{% endif %}
+                                        </span>
                                         <span class="list__item list__item--reviewer-outcome">&mdash;</span>
                                     </li>
                                 {% endfor %}

opentech/apply/funds/tests/test_models.py

@@ -535,7 +535,23 @@ def test_disagree_review_is_maybe(self):
         submission = qs[0]
         self.assertEqual(submission.opinion_disagree, 1)
         self.assertEqual(submission.review_count, 2)
-        self.assertEqual(submission.review_submitted_count, 2)
+        # Reviewers that disagree are not counted
+        self.assertEqual(submission.review_submitted_count, 1)
+        self.assertEqual(submission.review_recommendation, MAYBE)
+
+    def test_opinionated_slash_confused_reviewer(self):
+        staff = StaffFactory()
+        submission = ApplicationSubmissionFactory()
+        review_one = ReviewFactory(submission=submission)
+        review_two = ReviewFactory(submission=submission)
+        ReviewOpinionFactory(opinion_disagree=True, review=review_one, author__reviewer=staff)
+        ReviewOpinionFactory(opinion_agree=True, review=review_two, author__reviewer=staff)
+        qs = ApplicationSubmission.objects.for_table(user=staff)
+        submission = qs[0]
+        self.assertEqual(submission.opinion_disagree, 1)
+        self.assertEqual(submission.review_count, 3)
+        # Reviewers that disagree are not counted
+        self.assertEqual(submission.review_submitted_count, 3)
         self.assertEqual(submission.review_recommendation, MAYBE)
 
     def test_dont_double_count_review_and_opinion(self):
@@ -570,7 +586,8 @@ def test_submissions_dont_conflict(self):
         self.assertEqual(submission, submission_one)
         self.assertEqual(submission.opinion_disagree, 1)
         self.assertEqual(submission.review_count, 2)
-        self.assertEqual(submission.review_submitted_count, 2)
+        # Reviewers that disagree are not counted
+        self.assertEqual(submission.review_submitted_count, 1)
         self.assertEqual(submission.review_recommendation, MAYBE)
 
         submission = qs[1]

opentech/apply/review/templates/review/review_list.html

@@ -23,7 +23,9 @@ <h5>For <a href="{% url "funds:submissions:detail" submission.id %}">{{ submissi
         <tr class="reviews-list__tr">
             <th class="reviews-list__th">{{ answers.question }}</th>
             {% for answer in answers.answers %}
-                {% if answers.question == "Opinions" %}
+                {% if forloop.parentloop.first %}
+                    <td class="reviews-list__td reviews-list__td--author">{{ answer|safe }}</td>
+                {% elif answers.question == "Opinions"%}
                     <td class="reviews-list__td">{{ answer }}</td>
                 {% else %}
                     <td class="reviews-list__td">{{ answer|bleach }}</td>

opentech/apply/review/views.py

@@ -1,7 +1,6 @@
 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
@@ -17,36 +16,18 @@
 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.groups import REVIEWER_GROUP_NAME, STAFF_GROUP_NAME, PARTNER_GROUP_NAME, COMMUNITY_REVIEWER_GROUP_NAME
+from opentech.apply.users.groups import REVIEWER_GROUP_NAME
 from opentech.apply.utils.views import CreateOrUpdateView
+from opentech.apply.utils.image import generate_image_tag
 
 from .models import Review
 from .options import DISAGREE
 
 
 class ReviewContextMixin:
     def get_context_data(self, **kwargs):
-        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',
-        )
+        assigned_reviewers = self.object.assigned.review_order()
+
         if not self.object.stage.has_external_review:
             assigned_reviewers = assigned_reviewers.staff()
 
@@ -286,9 +267,17 @@ def get_context_data(self, **kwargs):
         review_data['comments'] = {'question': 'Comments', 'answers': list()}
 
         responses = self.object_list.count()
+        ordered_reviewers = AssignedReviewers.objects.filter(submission=self.submission).reviewed().review_order()
+
+        reviews = {review.author: review for review in self.object_list}
+        for i, reviewer in enumerate(ordered_reviewers):
+            review = reviews[reviewer]
+            author = '<a href="{}"><span>{}</span></a>'.format(review.get_absolute_url(), review.author)
+            if review.author.role:
+                author += generate_image_tag(review.author.role.icon, '12x12')
+            author = f'<div>{author}</div>'
 
-        for i, review in enumerate(self.object_list):
-            review_data['title']['answers'].append('<a href="{}">{}</a>'.format(review.get_absolute_url(), review.author))
+            review_data['title']['answers'].append(author)
             opinions_template = get_template('review/includes/review_opinions_list.html')
             opinions_html = opinions_template.render({'opinions': review.opinions.select_related('author').all()})
             review_data['opinions']['answers'].append(opinions_html)

opentech/static_src/src/sass/apply/components/_list.scss

@@ -1,5 +1,6 @@
 .list {
     $root: &;
+    max-width: 290px;
 
     &--no-margin {
         margin: 0;
@@ -12,7 +13,6 @@
         border-bottom: 1px solid $color--mid-grey;
         margin: 10px 0;
         padding: 5px 0;
-        max-width: 150px;
 
         #{$root}__item--opinion:first-child {
             span:last-child {
@@ -39,14 +39,19 @@
         &--reviewer {
             display: flex;
             justify-content: space-between;
-            max-width: 150px;
         }
 
         &--reviewer-name {
-            max-width: 100px;
+            max-width: 190px;
             overflow: hidden;
             font-weight: $weight--bold;
             text-overflow: ellipsis;
+            display: flex;
+            align-items: center;
+
+            > img {
+                margin-left: 10px;
+            }
 
             // show truncated emails on hover
             &:hover {

opentech/static_src/src/sass/apply/components/_reviews-list.scss

@@ -21,6 +21,17 @@
         max-width: 340px;
         min-width: 240px;
         padding: 20px;
+
+        &--author {
+            > div {
+                display: flex;
+                align-items: center;
+
+                img {
+                    margin-left: 7px;
+                }
+            }
+        }
     }
 
     &__th,