From 3244d0e3f54c95869cf8f99822bc03f6910a9ae2 Mon Sep 17 00:00:00 2001 From: sgfost Date: Fri, 29 Sep 2023 16:14:47 -0700 Subject: [PATCH] feat: allow peer reviews to be closed analogous to closing on github serves two purposes: 1. de-cluttering the dashboard, particularly in the case of legacy reviews on public releases that stall when changes are requested and a new release has to be made (resolves comses/planning#32) 2. allowing an author/requester to close inactive reviews since we now only allow one active review per codebase open at one time also includes some small refactors/bugfixes (resolves comses/planning#172) --- .../core/member_profiles/retrieve.jinja | 2 + django/library/forms.py | 1 + .../library/codebases/releases/retrieve.jinja | 9 +- .../jinja2/library/review/dashboard.jinja | 8 +- .../jinja2/library/review/editor_update.jinja | 15 ++- .../library/review/includes/macros.jinja | 44 +++++++ .../migrations/0024_add_release_status.py | 2 +- .../migrations/0025_add_peerreview_closed.py | 45 +++++++ django/library/models.py | 122 +++++++++++++++--- django/library/tests/test_views.py | 20 +++ django/library/urls.py | 5 + django/library/views.py | 90 ++++++------- frontend/src/apps/review_editor.ts | 2 +- frontend/src/components/ReviewEditor.vue | 6 +- frontend/src/components/ReviewFeedback.vue | 6 +- frontend/src/components/ReviewInvitations.vue | 4 +- frontend/src/components/UserSearch.vue | 2 + 17 files changed, 308 insertions(+), 75 deletions(-) create mode 100644 django/library/migrations/0025_add_peerreview_closed.py diff --git a/django/core/jinja2/core/member_profiles/retrieve.jinja b/django/core/jinja2/core/member_profiles/retrieve.jinja index e462e89f8..bf8288da0 100644 --- a/django/core/jinja2/core/member_profiles/retrieve.jinja +++ b/django/core/jinja2/core/member_profiles/retrieve.jinja @@ -140,6 +140,8 @@ Review complete {% elif not invitation.accepted %} Declined review request + {% elif invitation.review.closed %} + Review closed {% else %} {% set badge_class="bg-warning" if invitation.review.is_awaiting_reviewer_feedback else "bg-info" %} {{ invitation.review.get_status_display() }} diff --git a/django/library/forms.py b/django/library/forms.py index 52c5cbc20..abd6ff69a 100644 --- a/django/library/forms.py +++ b/django/library/forms.py @@ -194,6 +194,7 @@ class Meta: class PeerReviewFilterForm(forms.Form): + include_closed = forms.BooleanField(required=False) requires_editor_input = forms.BooleanField(required=False) author_changes_requested = forms.BooleanField(required=False) reviewer_feedback_requested = forms.BooleanField(required=False) diff --git a/django/library/jinja2/library/codebases/releases/retrieve.jinja b/django/library/jinja2/library/codebases/releases/retrieve.jinja index 3e91e43e7..e6785a2c2 100644 --- a/django/library/jinja2/library/codebases/releases/retrieve.jinja +++ b/django/library/jinja2/library/codebases/releases/retrieve.jinja @@ -1,6 +1,6 @@ {% extends "sidebar_layout.jinja" %} {% from "common.jinja" import breadcrumb, embed_discourse_comments, share_card, search_tag_href, member_profile_href %} -{% from "library/review/includes/macros.jinja" import include_review_reminders %} +{% from "library/review/includes/macros.jinja" import include_review_reminders, confirm_change_closed_modal %} {% set open_code_badge_url = request.build_absolute_uri(static("images/icons/open-code-badge.png")) %} {% set codebase = release.codebase %} @@ -442,7 +442,14 @@ {% elif review is not none %} +

Review Status:

+ {% if not review.is_complete %} +
+ {{ csrf_input }} + {{ confirm_change_closed_modal(review, show_reopen=False) }} +
+ {% endif %} {% endif %} {% endwith %} {# end has_change_perm #} diff --git a/django/library/jinja2/library/review/dashboard.jinja b/django/library/jinja2/library/review/dashboard.jinja index 83a3891a3..6ec03bbf0 100644 --- a/django/library/jinja2/library/review/dashboard.jinja +++ b/django/library/jinja2/library/review/dashboard.jinja @@ -1,4 +1,5 @@ {% extends 'sidebar_layout.jinja' %} +{% from "library/review/includes/macros.jinja" import display_closed_status %} {% block introduction %}

Peer Review Editor Dashboard

@@ -13,7 +14,12 @@ {% for release in codebase.releases.all() %} {% set review = release.review %} -
Review: {{ release.version_number }}
+
+ Review: + {{ display_closed_status(review) }} + {{ release.version_number }} + +

{% set badge_class="bg-success" if review.is_complete else "bg-warning" %}

diff --git a/django/library/jinja2/library/review/editor_update.jinja b/django/library/jinja2/library/review/editor_update.jinja index c5d22fc0b..3fafb5e9e 100644 --- a/django/library/jinja2/library/review/editor_update.jinja +++ b/django/library/jinja2/library/review/editor_update.jinja @@ -1,12 +1,24 @@ {% extends 'base.jinja' %} {% from 'common.jinja' import format_date %} +{% from "library/review/includes/macros.jinja" import display_closed_status, confirm_change_closed_modal %} {% block introduction %}

CoMSES Net Peer Review Editor

{% endblock %} {% block content %} -

Peer review for {{ review.title }}

+
+

+ Peer review for {{ review.title }} + {{ display_closed_status(review) }} +

+ {% if not review.is_complete %} +
+ {{ csrf_input }} + {{ confirm_change_closed_modal(review, show_reopen=True) }} +
+ {% endif %} +

Requested by {{ review.submitter.name }} {% if review.is_complete %} @@ -19,6 +31,7 @@

{% endblock %} diff --git a/django/library/jinja2/library/review/includes/macros.jinja b/django/library/jinja2/library/review/includes/macros.jinja index 072e208a2..27310442b 100644 --- a/django/library/jinja2/library/review/includes/macros.jinja +++ b/django/library/jinja2/library/review/includes/macros.jinja @@ -14,6 +14,50 @@ fas {% if q %}fa-check text-success{% else %}fa-times text-danger{% endif %} {% endmacro %} +{% macro display_closed_status(review) %} + {% if review.is_complete %} + + {% elif review.closed %} + + {% else %} + + {% endif %} +{% endmacro %} + +{% macro confirm_change_closed_modal(review, show_reopen=False) %} + {% set action = "reopen" if review.closed else "close" %} + {% if not review.closed %} + + {% elif show_reopen %} + + {% endif %} + +{% endmacro %} + {% macro display_reviewer_feedback(feedback, include_private=false, include_editor=false) %}

Reviewer Comments

diff --git a/django/library/migrations/0024_add_release_status.py b/django/library/migrations/0024_add_release_status.py index 9bd8e5417..4430181d6 100644 --- a/django/library/migrations/0024_add_release_status.py +++ b/django/library/migrations/0024_add_release_status.py @@ -37,9 +37,9 @@ class Migration(migrations.Migration): choices=[ ("draft", "Draft"), ("under_review", "Under review"), + ("review_complete", "Review complete"), ("published", "Published"), ("unpublished", "Unpublished"), - ("review_complete", "Review complete"), ], default="draft", help_text="The current status of this codebase release.", diff --git a/django/library/migrations/0025_add_peerreview_closed.py b/django/library/migrations/0025_add_peerreview_closed.py new file mode 100644 index 000000000..733519c1b --- /dev/null +++ b/django/library/migrations/0025_add_peerreview_closed.py @@ -0,0 +1,45 @@ +# Generated by Django 3.2.20 on 2023-09-29 17:18 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("library", "0024_add_release_status"), + ] + + operations = [ + migrations.AddField( + model_name="peerreview", + name="closed", + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name="peerrevieweventlog", + name="action", + field=models.CharField( + choices=[ + ("invitation_sent", "Reviewer has been invited"), + ("invitation_resent", "Reviewer invitation has been resent"), + ("invitation_accepted", "Reviewer has accepted invitation"), + ("invitation_declined", "Reviewer has declined invitation"), + ("reviewer_feedback_submitted", "Reviewer has given feedback"), + ("author_resubmitted", "Author has resubmitted release for review"), + ("review_status_updated", "Editor manually changed review status"), + ( + "revisions_requested", + "Editor has requested revisions to this release", + ), + ( + "release_certified", + "Editor has taken reviewer feedback into account and certified this release as peer reviewed", + ), + ("review_closed", "Peer review was closed"), + ("review_reopened", "Peer review was reopened"), + ], + help_text="status action requested.", + max_length=32, + ), + ), + ] diff --git a/django/library/models.py b/django/library/models.py index 6911c94b0..75a49dde7 100644 --- a/django/library/models.py +++ b/django/library/models.py @@ -13,7 +13,7 @@ from django.core.files.images import ImageFile from django.core.files.storage import FileSystemStorage from django.db import models, transaction -from django.db.models import Prefetch, Q +from django.db.models import Prefetch, Q, Count, Max from django.utils.functional import cached_property from django.urls import reverse from django.utils import timezone @@ -383,6 +383,31 @@ def with_contributors(self, release_contributor_qs=None, user=None, **kwargs): ) ) + def with_reviews(self, reviews): + return ( + self.filter(releases__review__in=reviews) + .prefetch_related( + Prefetch( + "releases", + queryset=CodebaseRelease.objects.filter(review__in=reviews) + .select_related("review") + .annotate( + n_accepted_invites=Count( + "review__invitation_set", + filter=Q(review__invitation_set__accepted=True), + ) + ), + ) + ) + .annotate( + min_n_accepted_invites=Count( + "releases__review__invitation_set", + filter=Q(releases__review__invitation_set__accepted=True), + ) + ) + .annotate(max_last_modified=Max("releases__review__last_modified")) + ) + @staticmethod def cache_contributors(codebases): """Add all_contributors property to all codebases in queryset. @@ -1247,7 +1272,8 @@ def get_notify_reviewers_of_changes_url(self): ) def get_review(self): - return getattr(self, "review", None) + review = getattr(self, "review", None) + return review if review and not review.closed else None def get_review_status_display(self): review = self.get_review() @@ -1738,6 +1764,8 @@ class PeerReviewEvent(models.TextChoices): RELEASE_CERTIFIED = "release_certified", _( "Editor has taken reviewer feedback into account and certified this release as peer reviewed" ) + REVIEW_CLOSED = "review_closed", _("Peer review was closed") + REVIEW_REOPENED = "review_reopened", _("Peer review was reopened") class PeerReviewQuerySet(models.QuerySet): @@ -1760,6 +1788,36 @@ def find_candidate_reviewers(self, query=None): return get_search_queryset(query, queryset) return queryset + def review_open(self, **kwargs): + return self.filter(closed=False, **kwargs) + + def requires_editor_input(self): + return self.annotate( + n_accepted_invites=Count( + "invitation_set", filter=Q(invitation_set__accepted=True) + ) + ).filter( + Q(n_accepted_invites=0) | Q(status=ReviewStatus.AWAITING_EDITOR_FEEDBACK) + ) + + def author_changes_requested(self): + return self.filter(status=ReviewStatus.AWAITING_AUTHOR_CHANGES) + + def reviewer_feedback_requested(self): + return self.filter(status=ReviewStatus.AWAITING_REVIEWER_FEEDBACK) + + def with_filters(self, query_params): + queryset = self + if not query_params.get("include_closed"): + queryset = queryset.review_open() + if query_params.get("requires_editor_input"): + queryset = queryset.requires_editor_input() + if query_params.get("author_changes_requested"): + queryset = queryset.author_changes_requested() + if query_params.get("reviewer_feedback_requested"): + queryset = queryset.reviewer_feedback_requested() + return queryset + def completed(self, **kwargs): return self.filter(status=ReviewStatus.COMPLETE, **kwargs) @@ -1779,6 +1837,7 @@ class PeerReview(models.Model): help_text=_("The current status of this review."), max_length=32, ) + closed = models.BooleanField(default=False) codebase_release = models.OneToOneField( CodebaseRelease, related_name="review", on_delete=models.PROTECT ) @@ -1793,6 +1852,10 @@ class PeerReview(models.Model): FieldPanel("status"), ] + @property + def is_open(self): + return not self.closed + @property def is_complete(self): return self.status == ReviewStatus.COMPLETE @@ -1811,6 +1874,9 @@ def get_simplified_status_display(self): def get_edit_url(self): return reverse("library:profile-edit", kwargs={"user_pk": self.user.pk}) + def get_change_closed_url(self): + return reverse("library:peer-review-change-closed", kwargs={"slug": self.slug}) + def get_invite(self, member_profile): return self.invitation_set.filter(candidate_reviewer=member_profile).first() @@ -1840,15 +1906,7 @@ def set_complete_status(self, editor: MemberProfile): self.codebase_release.save() self.codebase_release.codebase.peer_reviewed = True self.codebase_release.codebase.save() - - # FIXME: consider moving this into explicit send_model_certified_email() - send_markdown_email( - subject="Peer review completed", - template_name="library/review/email/model_certified.jinja", - context={"review": self}, - to=[self.submitter.email], - cc=[settings.REVIEW_EDITOR_EMAIL], - ) + self.send_model_certified_email() def log(self, message: str, action: PeerReviewEvent, author: MemberProfile): return self.event_set.create(message=message, action=action.name, author=author) @@ -1865,12 +1923,37 @@ def editor_change_review_status(self, editor: MemberProfile, status: ReviewStatu def author_resubmitted_changes(self, changes_made=None): author = self.submitter self.log( - message="Release has been resubmitted for review: {}".format(changes_made), + message=f"Release has been resubmitted for review: {changes_made}", action=PeerReviewEvent.AUTHOR_RESUBMITTED, author=author, ) self.send_author_updated_content_email() + def close(self, submitter_or_editor: MemberProfile): + if self.closed: + return + + self.closed = True + self.log( + message=f"Peer review closed by {'submitter' if submitter_or_editor == self.submitter else 'editor'}", + action=PeerReviewEvent.REVIEW_CLOSED, + author=submitter_or_editor, + ) + self.save() + # do we want to send an email when a review is closed? + + def reopen(self, submitter_or_editor: MemberProfile): + if not self.closed: + return + + self.closed = False + self.log( + message=f"Peer review re-opened by {'submitter' if submitter_or_editor == self.submitter else 'editor'}", + action=PeerReviewEvent.REVIEW_REOPENED, + author=submitter_or_editor, + ) + self.save() + def send_author_updated_content_email(self): qs = self.invitation_set.filter(accepted=True) # if there are no currently accepted invitations, status should be set to awaiting editor feedback @@ -1891,6 +1974,15 @@ def send_author_requested_peer_review_email(self): to=[settings.REVIEW_EDITOR_EMAIL], ) + def send_model_certified_email(self): + send_markdown_email( + subject="Peer review completed", + template_name="library/review/email/model_certified.jinja", + context={"review": self}, + to=[self.submitter.email], + cc=[settings.REVIEW_EDITOR_EMAIL], + ) + @cached_property def review_status_json(self): return json.dumps( @@ -1910,8 +2002,9 @@ def contact_author_name(self): @classmethod def get_codebase_latest_active_review(cls, codebase): + """returns the latest review for a codebase that is not complete and not closed""" qs = cls.objects.filter(codebase_release__codebase=codebase).exclude( - status=ReviewStatus.COMPLETE + Q(status=ReviewStatus.COMPLETE) | Q(closed=True) ) return qs.latest("date_created") if qs.exists() else None @@ -2195,7 +2288,7 @@ def reviewer_completed(self): ) @transaction.atomic - def editor_called_for_revisions(self): + def editor_called_for_revisions(self, editor: MemberProfile): """Add an editor called for revisions event to the log Preconditions: @@ -2203,7 +2296,6 @@ def editor_called_for_revisions(self): editor updated feedback """ review = self.invitation.review - editor = self.invitation.editor review.log( action=PeerReviewEvent.REVISIONS_REQUESTED, author=editor, diff --git a/django/library/tests/test_views.py b/django/library/tests/test_views.py index b87f95fb3..100f92601 100644 --- a/django/library/tests/test_views.py +++ b/django/library/tests/test_views.py @@ -278,6 +278,26 @@ def test_request_peer_review_existing_review(self): PeerReview.objects.filter(codebase_release=second_release).count(), 0 ) + def test_request_peer_review_existing_closed_review(self): + self.client.login( + username=self.submitter.username, password=self.user_factory.password + ) + first_release = ReleaseSetup.setUpPublishableDraftRelease(self.codebase) + pr = PeerReview.objects.create( + codebase_release=first_release, submitter=self.submitter.member_profile + ) + pr.close(self.submitter.member_profile) + second_release = ReleaseSetup.setUpPublishableDraftRelease(self.codebase) + + response = self.client.post( + second_release.get_request_peer_review_url(), + HTTP_ACCEPT="application/json", + ) + + self.assertTrue( + PeerReview.objects.filter(codebase_release=second_release).exists() + ) + class CodebaseReleaseUnpublishedFilesTestCase( ApiAccountMixin, ResponseStatusCodesMixin, TestCase diff --git a/django/library/urls.py b/django/library/urls.py index f020c9ab7..1cb0a60c5 100644 --- a/django/library/urls.py +++ b/django/library/urls.py @@ -61,6 +61,11 @@ views.PeerReviewEditorView.as_view(), name="peer-review-detail", ), + path( + "reviews//change-closed/", + views.PeerReviewChangeClosedView.as_view(), + name="peer-review-change-closed", + ), path( "reviews//events/", views.list_review_event_log, diff --git a/django/library/views.py b/django/library/views.py index 6bbd48939..23db7a1a6 100644 --- a/django/library/views.py +++ b/django/library/views.py @@ -113,53 +113,9 @@ def get_query_params(self): def get_queryset(self): query_params = self.get_query_params() - requires_editor_input = query_params.get("requires_editor_input") - author_changes_requested = query_params.get("author_changes_requested") - reviewer_feedback_requested = query_params.get("reviewer_feedback_requested") order_by = query_params.get("order_by") - reviews = PeerReview.objects.annotate( - n_accepted_invites=Count( - "invitation_set", filter=Q(invitation_set__accepted=True) - ) - ) - - filters = Q() - if requires_editor_input: - filters |= Q(n_accepted_invites=0) | Q( - status=ReviewStatus.AWAITING_EDITOR_FEEDBACK - ) - if author_changes_requested: - filters |= Q(status=ReviewStatus.AWAITING_AUTHOR_CHANGES) - if reviewer_feedback_requested: - filters |= Q(status=ReviewStatus.AWAITING_REVIEWER_FEEDBACK) - if filters: - reviews = reviews.filter(filters) - - codebases = ( - Codebase.objects.filter(releases__review__in=reviews) - .prefetch_related( - Prefetch( - "releases", - queryset=CodebaseRelease.objects.filter(review__in=reviews) - .select_related("review") - .annotate( - n_accepted_invites=Count( - "review__invitation_set", - filter=Q(review__invitation_set__accepted=True), - ) - ), - ) - ) - .annotate( - min_n_accepted_invites=Count( - "releases__review__invitation_set", - filter=Q(releases__review__invitation_set__accepted=True), - ) - ) - .annotate(max_last_modified=Max("releases__review__last_modified")) - .order_by(order_by) - ) - return codebases + reviews = PeerReview.objects.with_filters(query_params) + return Codebase.objects.with_reviews(reviews).order_by(order_by) def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) @@ -187,6 +143,35 @@ def put(self, request, *args, **kwargs): return _change_peer_review_status(request) +class PeerReviewChangeClosedView(PermissionRequiredMixin, DetailView): + context_object_name = "review" + model = PeerReview + permission_required = "library.change_peerreview" + slug_field = "slug" + + def has_permission(self): + has_base_permission = super().has_permission() + if has_base_permission: + return True + obj = self.get_object() + return obj.submitter.user == self.request.user + + def post(self, request, *args, **kwargs): + review = self.get_object() + action = request.POST.get("action") + if action == "close": + review.close(request.user.member_profile) + elif action == "reopen": + review.reopen(request.user.member_profile) + else: + raise ValidationError("Invalid action") + messages.success( + request, + (f"Review successfully {action}{'d' if action == 'close' else 'ed'}"), + ) + return redirect(request.META.get("HTTP_REFERER", "")) + + class PeerReviewReviewerListView(mixins.ListModelMixin, viewsets.GenericViewSet): queryset = MemberProfile.objects.all() serializer_class = RelatedMemberProfileSerializer @@ -342,6 +327,11 @@ class PeerReviewEditorFeedbackUpdateView(UpdateView): pk_url_kwarg = "feedback_id" queryset = PeerReviewerFeedback.objects.all() + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + kwargs["editor"] = self.request.user.member_profile + return kwargs + def get_success_url(self): return self.object.invitation.review.get_absolute_url() @@ -798,10 +788,7 @@ def notify_reviewers_of_changes(self, request, **kwargs): def request_peer_review(self, request, identifier, version_number): """ If a peer review is requestable (publishable, not reviewed, no related review exists): - - Create a new "under review" draft release if: - - the release from which the request was made is published - - the requester chose to convert a draft release to a draft under review with the - knowledge that it will not be publishable until the review is complete + - Create a new "under review" draft release if the release from which the request was made is published - Otherwise, update the existing draft release to be "under review" - Create a new peer review object and send an email to the author """ @@ -840,7 +827,8 @@ def request_peer_review(self, request, identifier, version_number): messages.success(request, "Peer review request submitted.") else: messages.info( - request, "An active peer review already exists for this codebase." + request, + "An active peer review already exists for this codebase. Close it below if you wish to open a new one", ) return self.build_review_request_response(request, review_release, review) diff --git a/frontend/src/apps/review_editor.ts b/frontend/src/apps/review_editor.ts index b659f8ed9..a44f67111 100644 --- a/frontend/src/apps/review_editor.ts +++ b/frontend/src/apps/review_editor.ts @@ -4,5 +4,5 @@ import { createApp } from "vue"; import ReviewEditor from "@/components/ReviewEditor.vue"; import { extractDataParams } from "@/util"; -const props = extractDataParams("review-editor", ["reviewId", "statusLevels", "status"]); +const props = extractDataParams("review-editor", ["reviewId", "statusLevels", "status", "closed"]); createApp(ReviewEditor, props).mount("#review-editor"); diff --git a/frontend/src/components/ReviewEditor.vue b/frontend/src/components/ReviewEditor.vue index 9d3a259d6..dab449a9c 100644 --- a/frontend/src/components/ReviewEditor.vue +++ b/frontend/src/components/ReviewEditor.vue @@ -1,9 +1,10 @@