From 60f514df11d252bb5e1bb32ee115d9b7c31784bf Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 26 Apr 2024 19:55:08 +0200 Subject: [PATCH 1/8] display latest meaningful activity status in approval queue view --- .../components/review_list_item.html | 20 +++++------ .../reviewers/extensions_review_list.html | 10 +++--- reviewers/views.py | 34 +++++++++++++++++-- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/reviewers/templates/reviewers/components/review_list_item.html b/reviewers/templates/reviewers/components/review_list_item.html index 8de1cfb5..2a74721a 100644 --- a/reviewers/templates/reviewers/components/review_list_item.html +++ b/reviewers/templates/reviewers/components/review_list_item.html @@ -6,27 +6,23 @@ {{ extension.name }} - - {% if extension.authors.count %} - {% include "extensions/components/authors.html" %} - {% endif %} - + {% include "extensions/components/authors.html" %} {{ extension.date_created|naturaltime_compact }} - {{ extension.review_activity.all|length }} + {{ stats.count }} - - {% if extension.review_activity.all %} - - {{ extension.review_activity.all.last.date_created|naturaltime_compact }} + + {{ stats.last_activity.date_created|naturaltime_compact }} - {% endif %} {% include "files/components/scan_details_flag.html" with file=extension.latest_version.file %} - {% include "common/components/status.html" with object=extension class="d-block" %} +
+ + {{ stats.last_type_display }} +
diff --git a/reviewers/templates/reviewers/extensions_review_list.html b/reviewers/templates/reviewers/extensions_review_list.html index 64f5e080..96d0ec01 100644 --- a/reviewers/templates/reviewers/extensions_review_list.html +++ b/reviewers/templates/reviewers/extensions_review_list.html @@ -34,12 +34,10 @@ - {% for extension in object_list %} - {% if user.is_moderator %} - {% include 'reviewers/components/review_list_item.html' %} - {% elif extension.status_slug == 'awaiting-review' %} - {% include 'reviewers/components/review_list_item.html' %} - {% endif %} + {% for stats in object_list %} + {% with extension=stats.extension%} + {% include 'reviewers/components/review_list_item.html' with extension=extension stats=stats %} + {% endwith %} {% endfor %} diff --git a/reviewers/views.py b/reviewers/views.py index 7d2c9be0..99af1be5 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -19,9 +19,39 @@ class ApprovalQueueView(ListView): paginate_by = 100 def get_queryset(self): - return Extension.objects.exclude(status=Extension.STATUSES.APPROVED).order_by( - '-date_created' + qs = ( + ApprovalActivity.objects.prefetch_related( + 'extension', + 'extension__authors', + 'extension__versions', + 'extension__versions__file', + 'extension__versions__file__validation', + ) + .order_by('-date_created') + .all() ) + by_extension = {} + result = [] + for item in qs: + extension = item.extension + stats = by_extension.get(extension, None) + if not stats: + # this check guarantees that we add a record only once per extension, + # and iterating over qs we get result also ordered by item.date_created + stats = { + 'count': 0, + 'extension': extension, + 'last_activity': None, + 'last_type_display': None, + } + by_extension[extension] = stats + result.append(stats) + stats['count'] += 1 + if not stats.get('last_activity', None): + stats['last_activity'] = item + if not stats.get('last_type_display', None): + stats['last_type_display'] = item.get_type_display + return result template_name = 'reviewers/extensions_review_list.html' -- 2.30.2 From 54cfab2e6697b3aec7e9d7df5e32822bc28ac7b0 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 29 Apr 2024 12:53:21 +0200 Subject: [PATCH 2/8] fix test --- reviewers/tests/test_views.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/reviewers/tests/test_views.py b/reviewers/tests/test_views.py index dd80c1ff..7edf2cd3 100644 --- a/reviewers/tests/test_views.py +++ b/reviewers/tests/test_views.py @@ -3,13 +3,21 @@ from django.shortcuts import reverse from common.tests.factories.extensions import create_version from files.models import File +from reviewers.models import ApprovalActivity class CommentsViewTest(TestCase): fixtures = ['licenses'] def setUp(self): - self.default_version = create_version(file__status=File.STATUSES.AWAITING_REVIEW) + version = create_version(file__status=File.STATUSES.AWAITING_REVIEW) + self.default_version = version + ApprovalActivity( + type=ApprovalActivity.ActivityType.COMMENT, + user=version.file.user, + extension=version.extension, + message='test comment', + ).save() # List of extensions under review does not require authentication def test_list_visibility(self): -- 2.30.2 From 101b2b4178c757105f6fa1fe0b48ca7234a2e47a Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 29 Apr 2024 16:04:04 +0200 Subject: [PATCH 3/8] create ApprovalActivity on a new version upload --- constants/activity.py | 1 + extensions/signals.py | 24 ++++++++++++++++++++++++ extensions/tests/test_submit.py | 16 ++++++++++++++++ reviewers/models.py | 1 + reviewers/signals.py | 1 + 5 files changed, 43 insertions(+) diff --git a/constants/activity.py b/constants/activity.py index e5127d69..1da320ca 100644 --- a/constants/activity.py +++ b/constants/activity.py @@ -10,6 +10,7 @@ class Verb: REPORTED_RATING = 'reported rating' REQUESTED_CHANGES = 'requested changes' REQUESTED_REVIEW = 'requested review' + UPLOADED_NEW_VERSION = 'uploaded new version' class Flag: diff --git a/extensions/signals.py b/extensions/signals.py index 85ae6dbd..d874867e 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -8,6 +8,7 @@ from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delet from django.dispatch import receiver from constants.activity import Flag +from reviewers.models import ApprovalActivity import extensions.models import files.models @@ -165,3 +166,26 @@ def _auto_approve_subsequent_uploads( args = {'f_id': file.pk, 'pk': instance.pk, 'sender': sender, 's': file.source.name} logger.info('Auto-approving file pk=%(f_id)s of %(sender)s pk=%(pk)s source=%(s)s', args) file.save(update_fields={'status', 'date_modified'}) + + +@receiver(post_save, sender=extensions.models.Version) +def _maybe_create_approval_activity_for_new_version( + sender: object, + instance: extensions.models.Version, + created: bool, + raw: bool, + **kwargs: object, +): + if raw: + return + if not created: + return + extension = instance.extension + if not extension.is_listed or not instance.file: + return + ApprovalActivity( + type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, + user=instance.file.user, + extension=instance.extension, + message=f'uploaded new version: {instance.version}', + ).save() diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index cda1712c..f54ab10a 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -10,6 +10,7 @@ from common.tests.factories.users import UserFactory from common.tests.utils import _get_all_form_errors from extensions.models import Extension, Version from files.models import File +from reviewers.models import ApprovalActivity import utils @@ -425,6 +426,14 @@ class NewVersionTest(TestCase): f'/add-ons/{self.extension.slug}/manage/versions/new/{file.pk}/', ) self.assertEqual(self.extension.versions.count(), 1) + self.extension.approve() + self.assertEqual( + ApprovalActivity.objects.filter( + extension=self.extension, + type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, + ).count(), + 0, + ) # Check step 2: finalise new version and send to review url = response['Location'] @@ -444,3 +453,10 @@ class NewVersionTest(TestCase): self.assertEqual(new_version.schema_version, '1.0.0') self.assertEqual(new_version.release_notes, 'new version') self.assertEqual(new_version.file.get_status_display(), 'Approved') + self.assertEqual( + ApprovalActivity.objects.filter( + extension=self.extension, + type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, + ).count(), + 1, + ) diff --git a/reviewers/models.py b/reviewers/models.py index 3daacd78..4a862e76 100644 --- a/reviewers/models.py +++ b/reviewers/models.py @@ -80,6 +80,7 @@ class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model): APPROVED = "APR", _("Approved") AWAITING_CHANGES = "AWC", _("Awaiting Changes") AWAITING_REVIEW = "AWR", _("Awaiting Review") + UPLOADED_NEW_VERSION = "UNV", _("Uploaded New Version") user = models.ForeignKey(User, on_delete=models.CASCADE, blank=True, null=True) extension = models.ForeignKey( diff --git a/reviewers/signals.py b/reviewers/signals.py index 1e1b6a06..2311e524 100644 --- a/reviewers/signals.py +++ b/reviewers/signals.py @@ -30,6 +30,7 @@ def _create_action_from_review_and_follow( ApprovalActivity.ActivityType.AWAITING_CHANGES: Verb.REQUESTED_CHANGES, ApprovalActivity.ActivityType.AWAITING_REVIEW: Verb.REQUESTED_REVIEW, ApprovalActivity.ActivityType.COMMENT: Verb.COMMENTED, + ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION: Verb.UPLOADED_NEW_VERSION, } action.send( instance.user, -- 2.30.2 From b4c126b547cb0039b588afaff75b7f666632d847 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 29 Apr 2024 16:21:55 +0200 Subject: [PATCH 4/8] handle activity types properly in form and AQ list --- .../components/review_list_item.html | 6 ++- .../reviewers/extensions_review_detail.html | 3 +- reviewers/views.py | 46 ++++++++++--------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/reviewers/templates/reviewers/components/review_list_item.html b/reviewers/templates/reviewers/components/review_list_item.html index 2a74721a..68d16785 100644 --- a/reviewers/templates/reviewers/components/review_list_item.html +++ b/reviewers/templates/reviewers/components/review_list_item.html @@ -19,10 +19,12 @@ -
+ {% with last_type=stats.last_type_display|default:"Awaiting Review" %} +
- {{ stats.last_type_display }} + {{ last_type }}
+ {% endwith %} diff --git a/reviewers/templates/reviewers/extensions_review_detail.html b/reviewers/templates/reviewers/extensions_review_detail.html index f3975e91..71d46216 100644 --- a/reviewers/templates/reviewers/extensions_review_detail.html +++ b/reviewers/templates/reviewers/extensions_review_detail.html @@ -101,8 +101,7 @@ {% for activity in extension.review_activity.all %}
  • - {# All activities except comments. #} - {% if activity.type != 'COM' %} + {% if activity.type in status_change_types %}
    diff --git a/reviewers/views.py b/reviewers/views.py index 99af1be5..cae0df7e 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -13,6 +13,12 @@ from reviewers.models import ApprovalActivity log = logging.getLogger(__name__) +STATUS_CHANGE_TYPES = [ + ApprovalActivity.ActivityType.APPROVED, + ApprovalActivity.ActivityType.AWAITING_CHANGES, + ApprovalActivity.ActivityType.AWAITING_REVIEW, +] + class ApprovalQueueView(ListView): model = Extension @@ -49,7 +55,7 @@ class ApprovalQueueView(ListView): stats['count'] += 1 if not stats.get('last_activity', None): stats['last_activity'] = item - if not stats.get('last_type_display', None): + if not stats.get('last_type_display', None) and item.type in STATUS_CHANGE_TYPES: stats['last_type_display'] = item.get_type_display return result @@ -66,33 +72,31 @@ class ExtensionsApprovalDetailView(DetailView): ctx['pending_previews'] = self.object.preview_set.exclude( file__status=File.STATUSES.APPROVED ) + ctx['status_change_types'] = STATUS_CHANGE_TYPES if self.request.user.is_authenticated: form = ctx['comment_form'] = CommentForm() - # Remove 'Approved' status from dropdown it not moderator - filtered_activity_types = ApprovalActivity.ActivityType.choices + # anyone can comment + filtered_activity_types = {ApprovalActivity.ActivityType.COMMENT} user = self.request.user - if not (user.is_moderator or user.is_superuser): - filtered_activity_types = [ - t - for t in ApprovalActivity.ActivityType.choices - if t[0] - not in [ + if self.object.has_maintainer(user): + filtered_activity_types.add(ApprovalActivity.ActivityType.AWAITING_REVIEW) + if user.is_moderator or user.is_superuser: + filtered_activity_types.update( + [ ApprovalActivity.ActivityType.APPROVED, ApprovalActivity.ActivityType.AWAITING_CHANGES, ] - ] - if not self.object.has_maintainer(user): - # Other accounts can only comment - filtered_activity_types = [ - t - for t in ApprovalActivity.ActivityType.choices - if t[0] == ApprovalActivity.ActivityType.COMMENT - ] - form.fields['type'].choices = filtered_activity_types - form.fields['type'].widget.choices = filtered_activity_types - if len(filtered_activity_types) == 1: - form.fields['type'].widget = django.forms.HiddenInput() + ) + choices = list( + filter( + lambda c: c[0] in filtered_activity_types, ApprovalActivity.ActivityType.choices + ) + ) + form.fields['type'].choices = choices + form.fields['type'].widget.choices = choices + if len(choices) == 1: + form.fields['type'].widget = django.forms.HiddenInput() return ctx -- 2.30.2 From acb43778aa870f0a81cc111e4a5d1f8d2634193d Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 29 Apr 2024 18:23:48 +0200 Subject: [PATCH 5/8] better naming --- extensions/signals.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/signals.py b/extensions/signals.py index d874867e..edd2a48b 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -169,7 +169,7 @@ def _auto_approve_subsequent_uploads( @receiver(post_save, sender=extensions.models.Version) -def _maybe_create_approval_activity_for_new_version( +def _create_approval_activity_for_new_version_if_listed( sender: object, instance: extensions.models.Version, created: bool, -- 2.30.2 From eb7745647a2a4becb960fc14edfa5f54fa6ad57e Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 29 Apr 2024 18:30:14 +0200 Subject: [PATCH 6/8] fix accidental tabs --- files/templates/files/components/scan_details.html | 2 +- reviewers/templates/reviewers/extensions_review_list.html | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/files/templates/files/components/scan_details.html b/files/templates/files/components/scan_details.html index eca7ebec..1f2ff38d 100644 --- a/files/templates/files/components/scan_details.html +++ b/files/templates/files/components/scan_details.html @@ -5,7 +5,7 @@

    ⚠ {% trans "Suspicious upload" %}

    - {% blocktrans asvar alert_text %}Scan of the {{ suspicious_files.0 }} indicates malicious content.{% endblocktrans %} + {% blocktrans asvar alert_text %}Scan of the {{ suspicious_files.0 }} indicates malicious content.{% endblocktrans %}

    {{ alert_text }} {% if perms.files.view_file %}{# Moderators don't necessarily have access to the admin #} diff --git a/reviewers/templates/reviewers/extensions_review_list.html b/reviewers/templates/reviewers/extensions_review_list.html index 96d0ec01..e548b326 100644 --- a/reviewers/templates/reviewers/extensions_review_list.html +++ b/reviewers/templates/reviewers/extensions_review_list.html @@ -35,9 +35,9 @@ {% for stats in object_list %} - {% with extension=stats.extension%} + {% with extension=stats.extension%} {% include 'reviewers/components/review_list_item.html' with extension=extension stats=stats %} - {% endwith %} + {% endwith %} {% endfor %} -- 2.30.2 From 2f2a363dd852a1b3d85c770870f0f943471adac1 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 29 Apr 2024 18:40:39 +0200 Subject: [PATCH 7/8] separate UPLOADED_NEW_VERSION into its own patch --- constants/activity.py | 1 - extensions/signals.py | 24 ------------------------ extensions/tests/test_submit.py | 16 ---------------- reviewers/models.py | 1 - reviewers/signals.py | 1 - 5 files changed, 43 deletions(-) diff --git a/constants/activity.py b/constants/activity.py index 1da320ca..e5127d69 100644 --- a/constants/activity.py +++ b/constants/activity.py @@ -10,7 +10,6 @@ class Verb: REPORTED_RATING = 'reported rating' REQUESTED_CHANGES = 'requested changes' REQUESTED_REVIEW = 'requested review' - UPLOADED_NEW_VERSION = 'uploaded new version' class Flag: diff --git a/extensions/signals.py b/extensions/signals.py index edd2a48b..85ae6dbd 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -8,7 +8,6 @@ from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delet from django.dispatch import receiver from constants.activity import Flag -from reviewers.models import ApprovalActivity import extensions.models import files.models @@ -166,26 +165,3 @@ def _auto_approve_subsequent_uploads( args = {'f_id': file.pk, 'pk': instance.pk, 'sender': sender, 's': file.source.name} logger.info('Auto-approving file pk=%(f_id)s of %(sender)s pk=%(pk)s source=%(s)s', args) file.save(update_fields={'status', 'date_modified'}) - - -@receiver(post_save, sender=extensions.models.Version) -def _create_approval_activity_for_new_version_if_listed( - sender: object, - instance: extensions.models.Version, - created: bool, - raw: bool, - **kwargs: object, -): - if raw: - return - if not created: - return - extension = instance.extension - if not extension.is_listed or not instance.file: - return - ApprovalActivity( - type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, - user=instance.file.user, - extension=instance.extension, - message=f'uploaded new version: {instance.version}', - ).save() diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index f54ab10a..cda1712c 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -10,7 +10,6 @@ from common.tests.factories.users import UserFactory from common.tests.utils import _get_all_form_errors from extensions.models import Extension, Version from files.models import File -from reviewers.models import ApprovalActivity import utils @@ -426,14 +425,6 @@ class NewVersionTest(TestCase): f'/add-ons/{self.extension.slug}/manage/versions/new/{file.pk}/', ) self.assertEqual(self.extension.versions.count(), 1) - self.extension.approve() - self.assertEqual( - ApprovalActivity.objects.filter( - extension=self.extension, - type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, - ).count(), - 0, - ) # Check step 2: finalise new version and send to review url = response['Location'] @@ -453,10 +444,3 @@ class NewVersionTest(TestCase): self.assertEqual(new_version.schema_version, '1.0.0') self.assertEqual(new_version.release_notes, 'new version') self.assertEqual(new_version.file.get_status_display(), 'Approved') - self.assertEqual( - ApprovalActivity.objects.filter( - extension=self.extension, - type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, - ).count(), - 1, - ) diff --git a/reviewers/models.py b/reviewers/models.py index 4a862e76..3daacd78 100644 --- a/reviewers/models.py +++ b/reviewers/models.py @@ -80,7 +80,6 @@ class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model): APPROVED = "APR", _("Approved") AWAITING_CHANGES = "AWC", _("Awaiting Changes") AWAITING_REVIEW = "AWR", _("Awaiting Review") - UPLOADED_NEW_VERSION = "UNV", _("Uploaded New Version") user = models.ForeignKey(User, on_delete=models.CASCADE, blank=True, null=True) extension = models.ForeignKey( diff --git a/reviewers/signals.py b/reviewers/signals.py index 2311e524..1e1b6a06 100644 --- a/reviewers/signals.py +++ b/reviewers/signals.py @@ -30,7 +30,6 @@ def _create_action_from_review_and_follow( ApprovalActivity.ActivityType.AWAITING_CHANGES: Verb.REQUESTED_CHANGES, ApprovalActivity.ActivityType.AWAITING_REVIEW: Verb.REQUESTED_REVIEW, ApprovalActivity.ActivityType.COMMENT: Verb.COMMENTED, - ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION: Verb.UPLOADED_NEW_VERSION, } action.send( instance.user, -- 2.30.2 From 9dd630c88e256637ddb85c9837c077b5a290cb05 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 29 Apr 2024 19:16:22 +0200 Subject: [PATCH 8/8] missing space --- reviewers/templates/reviewers/extensions_review_list.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reviewers/templates/reviewers/extensions_review_list.html b/reviewers/templates/reviewers/extensions_review_list.html index e548b326..3912d605 100644 --- a/reviewers/templates/reviewers/extensions_review_list.html +++ b/reviewers/templates/reviewers/extensions_review_list.html @@ -35,7 +35,7 @@ {% for stats in object_list %} - {% with extension=stats.extension%} + {% with extension=stats.extension %} {% include 'reviewers/components/review_list_item.html' with extension=extension stats=stats %} {% endwith %} {% endfor %} -- 2.30.2