From c6026f6d89bc9f1aace42c7d254daa3457a6d21b Mon Sep 17 00:00:00 2001 From: Christina Lin <44586776+chrstinalin@users.noreply.github.com> Date: Fri, 13 Sep 2024 13:58:33 -0400 Subject: [PATCH] Reviewer Tools Pastebin Re-implementation (#22640) * Added pastebin reimplementation on reviewer tools side * make lint * Moved attachment upload box to own section (and available for all actions) * Fixed css for textareas, returned datetime-wrapper * Missing assignment * Updated naming for files, limited to .txt and .zip, temporarily updated query number assertion * added waffle enable-activity-log-attachments for user facing menus; * testing * corrected test * Removed zip allowance * migration conflict * test (temp) * import * remove git_extraction.py from pr * ??? * numQueries * review changes * lint * prettier * review changes * lol lint * review comment --- src/olympia/activity/api_urls.py | 8 ++ ..._alter_activitylog_action_attachmentlog.py | 37 +++++++++ src/olympia/activity/models.py | 35 ++++++++ src/olympia/activity/tests/test_models.py | 11 +++ src/olympia/activity/tests/test_views.py | 39 ++++++++- src/olympia/activity/urls.py | 6 +- src/olympia/activity/views.py | 28 +++++++ src/olympia/api/urls.py | 6 +- src/olympia/lib/settings_base.py | 2 + src/olympia/reviewers/forms.py | 25 ++++++ .../migrations/0038_auto_20240909_1647.py | 16 ++++ .../templates/reviewers/includes/history.html | 11 +++ .../reviewers/templates/reviewers/review.html | 19 ++++- src/olympia/reviewers/tests/test_forms.py | 26 +++++- src/olympia/reviewers/tests/test_utils.py | 26 ++++++ src/olympia/reviewers/tests/test_views.py | 82 +++++++++++++++++-- src/olympia/reviewers/utils.py | 14 +++- src/olympia/reviewers/views.py | 4 +- src/olympia/urls.py | 2 + static/css/zamboni/reviewers.less | 14 ++++ static/css/zamboni/zamboni.css | 5 ++ static/js/zamboni/reviewers.js | 14 ++++ 22 files changed, 416 insertions(+), 14 deletions(-) create mode 100644 src/olympia/activity/api_urls.py create mode 100644 src/olympia/activity/migrations/0026_alter_activitylog_action_attachmentlog.py create mode 100644 src/olympia/reviewers/migrations/0038_auto_20240909_1647.py diff --git a/src/olympia/activity/api_urls.py b/src/olympia/activity/api_urls.py new file mode 100644 index 0000000000..50f55af592 --- /dev/null +++ b/src/olympia/activity/api_urls.py @@ -0,0 +1,8 @@ +from django.urls import re_path + +from . import views + + +urlpatterns = [ + re_path(r'^mail/', views.inbound_email, name='inbound-email-api'), +] diff --git a/src/olympia/activity/migrations/0026_alter_activitylog_action_attachmentlog.py b/src/olympia/activity/migrations/0026_alter_activitylog_action_attachmentlog.py new file mode 100644 index 0000000000..6de8cf4076 --- /dev/null +++ b/src/olympia/activity/migrations/0026_alter_activitylog_action_attachmentlog.py @@ -0,0 +1,37 @@ +# Generated by Django 4.2.15 on 2024-09-04 15:46 + +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import olympia.activity.models +import olympia.amo.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('activity', '0025_alter_activitylog_action_cinderpolicylog'), + ] + + operations = [ + migrations.AlterField( + model_name='activitylog', + name='action', + field=models.SmallIntegerField(choices=[(1, 1), (2, 2), (3, 3), (4, 4), (5, 5), (6, 6), (7, 7), (8, 8), (9, 9), (12, 12), (16, 16), (17, 17), (18, 18), (19, 19), (20, 20), (21, 21), (22, 22), (23, 23), (24, 24), (25, 25), (26, 26), (27, 27), (28, 28), (29, 29), (31, 31), (32, 32), (33, 33), (34, 34), (35, 35), (36, 36), (37, 37), (38, 38), (39, 39), (40, 40), (41, 41), (42, 42), (43, 43), (44, 44), (45, 45), (46, 46), (47, 47), (48, 48), (49, 49), (53, 53), (60, 60), (61, 61), (62, 62), (98, 98), (99, 99), (100, 100), (101, 101), (102, 102), (103, 103), (104, 104), (105, 105), (106, 106), (107, 107), (108, 108), (109, 109), (110, 110), (120, 120), (121, 121), (128, 128), (130, 130), (131, 131), (132, 132), (133, 133), (134, 134), (135, 135), (136, 136), (137, 137), (138, 138), (139, 139), (140, 140), (141, 141), (142, 142), (143, 143), (144, 144), (145, 145), (146, 146), (147, 147), (148, 148), (149, 149), (150, 150), (151, 151), (152, 152), (153, 153), (154, 154), (155, 155), (156, 156), (157, 157), (158, 158), (159, 159), (160, 160), (161, 161), (162, 162), (163, 163), (164, 164), (165, 165), (166, 166), (167, 167), (168, 168), (169, 169), (170, 170), (171, 171), (172, 172), (173, 173), (174, 174), (175, 175), (176, 176), (177, 177), (178, 178), (179, 179), (180, 180), (181, 181), (182, 182), (183, 183), (184, 184), (185, 185), (186, 186), (187, 187), (188, 188), (189, 189), (190, 190), (191, 191), (192, 192)]), + ), + migrations.CreateModel( + name='AttachmentLog', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', models.DateTimeField(blank=True, default=django.utils.timezone.now, editable=False)), + ('modified', models.DateTimeField(auto_now=True)), + ('file', models.FileField(blank=True, null=True, storage=olympia.activity.models.activity_attachment_storage, upload_to=olympia.activity.models.attachment_upload_path)), + ('activity_log', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='activity.activitylog')), + ], + options={ + 'db_table': 'log_activity_attachment', + 'ordering': ('-created',), + }, + bases=(olympia.amo.models.SaveUpdateMixin, models.Model), + ), + ] diff --git a/src/olympia/activity/models.py b/src/olympia/activity/models.py index 2ecdf6f02f..af14b4b82c 100644 --- a/src/olympia/activity/models.py +++ b/src/olympia/activity/models.py @@ -1,7 +1,9 @@ import json +import os import uuid from collections import defaultdict from copy import copy +from datetime import datetime from inspect import isclass from django.apps import apps @@ -20,6 +22,7 @@ from olympia.access.models import Group from olympia.addons.models import Addon from olympia.amo.fields import IPAddressBinaryField, PositiveAutoField from olympia.amo.models import BaseQuerySet, LongNameIndex, ManagerBase, ModelBase +from olympia.amo.utils import SafeStorage, id_to_path from olympia.bandwagon.models import Collection from olympia.blocklist.models import Block from olympia.constants.activity import _LOG @@ -39,6 +42,20 @@ MAX_TOKEN_USE_COUNT = 100 GENERIC_USER_NAME = gettext('Add-ons Review Team') +def attachment_upload_path(instance, filename): + ext = os.path.splitext(filename)[1] + timestamp = datetime.now().replace(microsecond=0) + return os.path.join( + 'activity_attachment', + id_to_path(instance.activity_log.pk, breadth=1), + f'{timestamp}{ext}', + ) + + +def activity_attachment_storage(): + return SafeStorage(root_setting='MEDIA_ROOT') + + class GenericMozillaUser(UserProfile): class Meta: proxy = True @@ -262,6 +279,24 @@ class RatingLog(ModelBase): ordering = ('-created',) +class AttachmentLog(ModelBase): + """ + This table is for indexing the activity log by attachment. + """ + + activity_log = models.OneToOneField('ActivityLog', on_delete=models.CASCADE) + file = models.FileField( + upload_to=attachment_upload_path, + storage=activity_attachment_storage, + blank=True, + null=True, + ) + + class Meta: + db_table = 'log_activity_attachment' + ordering = ('-created',) + + class DraftComment(ModelBase): """A model that allows us to draft comments for reviews before we have an ActivityLog instance ready. diff --git a/src/olympia/activity/tests/test_models.py b/src/olympia/activity/tests/test_models.py index eb5d97aaf1..8e32b2f052 100644 --- a/src/olympia/activity/tests/test_models.py +++ b/src/olympia/activity/tests/test_models.py @@ -2,6 +2,8 @@ from ipaddress import IPv4Address from unittest.mock import Mock from uuid import UUID +from django.core.files.base import ContentFile + from pyquery import PyQuery as pq from olympia import amo, core @@ -11,10 +13,12 @@ from olympia.activity.models import ( ActivityLog, ActivityLogToken, AddonLog, + AttachmentLog, DraftComment, GenericMozillaUser, IPLog, ReviewActionReasonLog, + attachment_upload_path, ) from olympia.addons.models import Addon, AddonUser from olympia.amo.tests import ( @@ -507,6 +511,13 @@ class TestActivityLog(TestCase): ActivityLog.objects.create(action=amo.LOG.ADD_TAG.id).log == amo.LOG.ADD_TAG ) + def test_attachment_upload_path(self): + log = ActivityLog.objects.create(amo.LOG.CUSTOM_TEXT, 'Test Attachment Log') + attachment = ContentFile('Pseudo File', name='attachment.txt') + attachment_log = AttachmentLog.objects.create(activity_log=log, file=attachment) + uploaded_name = attachment_upload_path(attachment_log, attachment.name) + assert uploaded_name.endswith('.txt') + class TestDraftComment(TestCase): def test_default_requirements(self): diff --git a/src/olympia/activity/tests/test_views.py b/src/olympia/activity/tests/test_views.py index 6be5047040..5d338184d9 100644 --- a/src/olympia/activity/tests/test_views.py +++ b/src/olympia/activity/tests/test_views.py @@ -4,13 +4,19 @@ from datetime import datetime, timedelta from unittest import mock from django.conf import settings +from django.core.files.base import ContentFile from django.test.utils import override_settings from rest_framework.exceptions import ErrorDetail from rest_framework.test import APIRequestFactory from olympia import amo -from olympia.activity.models import GENERIC_USER_NAME, ActivityLog, ActivityLogToken +from olympia.activity.models import ( + GENERIC_USER_NAME, + ActivityLog, + ActivityLogToken, + AttachmentLog, +) from olympia.activity.tests.test_serializers import LogMixin from olympia.activity.tests.test_utils import sample_message_content from olympia.activity.views import InboundEmailIPPermission, inbound_email @@ -20,6 +26,7 @@ from olympia.addons.models import ( AddonUser, ) from olympia.addons.utils import generate_addon_guid +from olympia.amo.reverse import reverse from olympia.amo.tests import ( APITestClientSessionID, TestCase, @@ -660,3 +667,33 @@ class TestEmailApi(TestCase): res = inbound_email(req) assert not _mock.called assert res.status_code == 403 + + +class TestDownloadAttachment(TestCase): + def setUp(self): + super().setUp() + self.addon = addon_factory( + guid=generate_addon_guid(), name='My Addôn', slug='my-addon' + ) + self.user = user_factory(email='admin@mozilla.com') + self.log = ActivityLog.objects.create( + user=self.user, action=amo.LOG.REVIEWER_REPLY_VERSION + ) + self.attachment = AttachmentLog.objects.create( + activity_log=self.log, + file=ContentFile('Pseudo File', name='attachment.txt'), + ) + + def test_download_attachment_success(self): + self.client.force_login(self.user) + self.grant_permission(self.user, 'Addons:Review', 'Addon Reviewers') + url = reverse('activity.attachment', args=[self.log.pk]) + response = self.client.get(url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertIn('.txt', response['Content-Disposition']) + + def test_download_attachment_failure(self): + self.client.force_login(self.user) + url = reverse('activity.attachment', args=[self.log.pk]) + response = self.client.get(url, follow=True) + self.assertEqual(response.status_code, 404) diff --git a/src/olympia/activity/urls.py b/src/olympia/activity/urls.py index 50f55af592..8191bd22ab 100644 --- a/src/olympia/activity/urls.py +++ b/src/olympia/activity/urls.py @@ -4,5 +4,9 @@ from . import views urlpatterns = [ - re_path(r'^mail/', views.inbound_email, name='inbound-email-api'), + re_path( + r'^attachment/(?P\d+)', + views.download_attachment, + name='activity.attachment', + ) ] diff --git a/src/olympia/activity/views.py b/src/olympia/activity/views.py index d61c4737ab..de2751afd3 100644 --- a/src/olympia/activity/views.py +++ b/src/olympia/activity/views.py @@ -1,4 +1,8 @@ +import os + +from django import http from django.conf import settings +from django.db.transaction import non_atomic_requests from django.shortcuts import get_object_or_404 from django.utils.translation import gettext, gettext_lazy as _ @@ -27,6 +31,7 @@ from olympia.activity.utils import ( log_and_notify, ) from olympia.addons.views import AddonChildMixin +from olympia.amo.utils import HttpResponseXSendFile from olympia.api.permissions import ( AllowAddonAuthor, AllowListedViewerOrReviewer, @@ -166,3 +171,26 @@ def inbound_email(request): spam_rating = request.data.get('SpamScore', 0.0) process_email.apply_async((message, spam_rating)) return Response(data=validation_response, status=status.HTTP_201_CREATED) + + +@non_atomic_requests +def download_attachment(request, log_id): + """ + Download attachment for a given activity log. + """ + log = get_object_or_404(ActivityLog, pk=log_id) + attachmentlog = log.attachmentlog + + is_reviewer = acl.action_allowed_for(request.user, amo.permissions.ADDONS_REVIEW) + if not is_reviewer: + raise http.Http404() + + response = HttpResponseXSendFile(request, attachmentlog.file.path) + path = attachmentlog.file.path + if not isinstance(path, str): + path = path.decode('utf8') + name = os.path.basename(path.replace('"', '')) + disposition = f'attachment; filename="{name}"'.encode() + response['Content-Disposition'] = disposition + response['Access-Control-Allow-Origin'] = '*' + return response diff --git a/src/olympia/api/urls.py b/src/olympia/api/urls.py index 9bde639a05..d0bbe8b47f 100644 --- a/src/olympia/api/urls.py +++ b/src/olympia/api/urls.py @@ -59,13 +59,13 @@ v3_api_urls = [ re_path(r'^reviews/', include(ratings_v3.urls)), re_path(r'^reviewers/', include('olympia.reviewers.api_urls')), re_path(r'^', include('olympia.signing.urls')), - re_path(r'^activity/', include('olympia.activity.urls')), + re_path(r'^activity/', include('olympia.activity.api_urls')), ] v4_api_urls = [ re_path(r'^abuse/', include('olympia.abuse.api_urls')), re_path(r'^accounts/', include(accounts_v4)), - re_path(r'^activity/', include('olympia.activity.urls')), + re_path(r'^activity/', include('olympia.activity.api_urls')), re_path(r'^addons/', include(addons_v4)), re_path(r'^applications/', include('olympia.applications.api_urls')), re_path(r'^blocklist/', include('olympia.blocklist.urls')), @@ -81,7 +81,7 @@ v4_api_urls = [ v5_api_urls = [ re_path(r'^abuse/', include('olympia.abuse.api_urls')), re_path(r'^accounts/', include(accounts_v4)), - re_path(r'^activity/', include('olympia.activity.urls')), + re_path(r'^activity/', include('olympia.activity.api_urls')), re_path(r'^addons/', include(addons_v5)), re_path(r'^applications/', include('olympia.applications.api_urls')), re_path(r'^blocklist/', include('olympia.blocklist.urls')), diff --git a/src/olympia/lib/settings_base.py b/src/olympia/lib/settings_base.py index caf3fe03de..005f861be0 100644 --- a/src/olympia/lib/settings_base.py +++ b/src/olympia/lib/settings_base.py @@ -264,6 +264,7 @@ SUPPORTED_NONAPPS = ( 'abuse', 'admin', 'apps', + 'activity', 'contribute.json', 'developer_agreement', 'developers', @@ -289,6 +290,7 @@ DEFAULT_APP = 'firefox' # This needs to be kept in sync with addons-frontend's validLocaleUrlExceptions # https://github.com/mozilla/addons-frontend/blob/master/config/default-amo.js SUPPORTED_NONLOCALES = ( + 'activity', 'contribute.json', 'google1f3e37b7351799a5.html', 'google231a41e803e464e9.html', diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index b61becfca6..34db9c76a2 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -10,6 +10,7 @@ from django.forms.models import ( modelformset_factory, ) from django.utils.html import format_html, format_html_join +from django.utils.translation import gettext import markupsafe @@ -41,6 +42,8 @@ ACTION_FILTERS = ( ACTION_DICT = dict(approved=amo.LOG.APPROVE_RATING, deleted=amo.LOG.DELETE_RATING) +VALID_ATTACHMENT_EXTENSIONS = ('.txt',) + class RatingModerationLogForm(forms.Form): start = forms.DateField(required=False, label='View entries between') @@ -299,6 +302,19 @@ class ActionChoiceWidget(forms.RadioSelect): return option +def validate_review_attachment(value): + if value: + if not value.name.endswith(VALID_ATTACHMENT_EXTENSIONS): + valid_extensions_string = '(%s)' % ', '.join(VALID_ATTACHMENT_EXTENSIONS) + raise forms.ValidationError( + gettext( + 'Unsupported file type, please upload a ' + 'file {extensions}.'.format(extensions=valid_extensions_string) + ) + ) + return value + + class ReviewForm(forms.Form): # Hack to restore behavior from pre Django 1.10 times. # Django 1.10 enabled `required` rendering for required widgets. That @@ -363,6 +379,11 @@ class ReviewForm(forms.Form): required=True, widget=ReasonsChoiceWidget, ) + attachment_file = forms.FileField( + required=False, validators=[validate_review_attachment] + ) + attachment_input = forms.CharField(required=False, widget=forms.Textarea()) + version_pk = forms.IntegerField(required=False, min_value=1) cinder_jobs_to_resolve = WidgetRenderedModelMultipleChoiceField( label='Outstanding DSA related reports to resolve:', @@ -413,6 +434,10 @@ class ReviewForm(forms.Form): # If the user select a different type of job before changing actions there could # be non-appeal jobs selected as cinder_jobs_to_resolve under resolve_appeal_job # action, or appeal jobs under resolve_reports_job action. So filter them out. + if self.cleaned_data.get('attachment_input') and self.cleaned_data.get( + 'attachment_file' + ): + raise ValidationError('Cannot upload both a file and input.') if self.cleaned_data.get('action') == 'resolve_appeal_job': self.cleaned_data['cinder_jobs_to_resolve'] = [ job diff --git a/src/olympia/reviewers/migrations/0038_auto_20240909_1647.py b/src/olympia/reviewers/migrations/0038_auto_20240909_1647.py new file mode 100644 index 0000000000..1f79f1efa3 --- /dev/null +++ b/src/olympia/reviewers/migrations/0038_auto_20240909_1647.py @@ -0,0 +1,16 @@ +# Generated by Django 4.2.15 on 2024-09-09 16:47 + +from django.db import migrations + +from olympia.core.db.migrations import CreateWaffleSwitch + + +class Migration(migrations.Migration): + + dependencies = [ + ('reviewers', '0037_autoapprovalsummary_is_pending_rejection'), + ] + + operations = [ + CreateWaffleSwitch('enable-activity-log-attachments') + ] \ No newline at end of file diff --git a/src/olympia/reviewers/templates/reviewers/includes/history.html b/src/olympia/reviewers/templates/reviewers/includes/history.html index 94ddf04ac9..36834b278e 100644 --- a/src/olympia/reviewers/templates/reviewers/includes/history.html +++ b/src/olympia/reviewers/templates/reviewers/includes/history.html @@ -14,4 +14,15 @@ {% endif %} {% endif %} + {% if switch_is_active('enable-activity-log-attachments') %} + + {% if record.attachmentlog %} + {% with attachment=record.attachmentlog.file %} +
+ Download Attachment ({{attachment.size|filesizeformat}}) +
+ {% endwith %} + {% endif %} + + {% endif %} diff --git a/src/olympia/reviewers/templates/reviewers/review.html b/src/olympia/reviewers/templates/reviewers/review.html index 6fec4181f6..654b070e78 100644 --- a/src/olympia/reviewers/templates/reviewers/review.html +++ b/src/olympia/reviewers/templates/reviewers/review.html @@ -102,7 +102,7 @@ {% endif %} -
+ {% csrf_token %} @@ -261,6 +261,23 @@ + {% if switch_is_active('enable-activity-log-attachments') %} +
+ +
+ + or Paste from Clipboard +
+ + + {{ form.attachment_input.errors }} + {{ form.attachment_file.errors }} +
+ {% endif %}
{% trans %} diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index 176ec26462..208b14256c 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -1,6 +1,7 @@ import uuid from datetime import datetime +from django.core.files.base import ContentFile from django.utils.encoding import force_str from pyquery import PyQuery as pq @@ -48,9 +49,10 @@ class TestReviewForm(TestCase): self.request = FakeRequest() self.file = self.version.file - def get_form(self, data=None): + def get_form(self, data=None, files=None): return ReviewForm( data=data, + files=files, helper=ReviewHelper( addon=self.addon, version=self.version, user=self.request.user ), @@ -1134,3 +1136,25 @@ class TestReviewForm(TestCase): # only policies that are expose_in_reviewer_tools=True should be included policy_exposed ] + + def test_upload_attachment(self): + self.grant_permission(self.request.user, 'Addons:Review') + attachment = ContentFile('Pseudo File', name='attachment.txt') + data = { + 'action': 'reply', + 'comments': 'lol', + } + files = {'attachment_file': attachment} + + form = self.get_form(data=data, files=files) + assert form.is_valid() + assert not form.errors + + data['attachment_input'] = 'whee' + form = self.get_form(data=data) + assert form.is_valid() + assert not form.errors + + form = self.get_form(data=data, files=files) + assert not form.is_valid() + assert form.errors == {'__all__': ['Cannot upload both a file and input.']} diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index bf1e608964..4f2c5b2fb2 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -5,6 +5,7 @@ from unittest.mock import call, patch from django.conf import settings from django.core import mail +from django.core.files.base import ContentFile from django.core.files.storage import default_storage as storage from django.test.utils import override_settings from django.urls import reverse @@ -17,6 +18,7 @@ from olympia.abuse.models import AbuseReport, CinderDecision, CinderJob, CinderP from olympia.activity.models import ( ActivityLog, ActivityLogToken, + AttachmentLog, CinderPolicyLog, ReviewActionReasonLog, ) @@ -1162,6 +1164,30 @@ class TestReviewHelper(TestReviewHelperBase): assert logs.count() == 1 assert logs[0].user == task_user + def test_log_action_attachment_input(self): + assert AttachmentLog.objects.count() == 0 + data = self.get_data() + text = 'This is input' + data['attachment_input'] = 'This is input' + self.helper.set_data(data) + self.helper.handler.log_action(amo.LOG.REJECT_VERSION) + assert AttachmentLog.objects.count() == 1 + attachment_log = AttachmentLog.objects.first() + file_content = attachment_log.file.read().decode('utf-8') + assert file_content == text + + def test_log_action_attachment_file(self): + assert AttachmentLog.objects.count() == 0 + text = "I'm a text file" + data = self.get_data() + data['attachment_file'] = ContentFile(text, name='attachment.txt') + self.helper.set_data(data) + self.helper.handler.log_action(amo.LOG.REJECT_VERSION) + assert AttachmentLog.objects.count() == 1 + attachment_log = AttachmentLog.objects.first() + file_content = attachment_log.file.read().decode('utf-8') + assert file_content == text + @patch('olympia.reviewers.utils.notify_addon_decision_to_cinder.delay') @patch('olympia.reviewers.utils.resolve_job_in_cinder.delay') def test_notify_decision_calls_resolve_job_in_cinder( diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 36e9a364db..7313203568 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -10,7 +10,7 @@ from django.conf import settings from django.core import mail from django.core.cache import cache from django.core.files import temp -from django.core.files.base import File as DjangoFile +from django.core.files.base import ContentFile, File as DjangoFile from django.db import connection, reset_queries from django.template import defaultfilters from django.test.client import RequestFactory @@ -30,7 +30,7 @@ from olympia.abuse.models import AbuseReport, CinderDecision, CinderJob, CinderP from olympia.access import acl from olympia.access.models import Group, GroupUser from olympia.accounts.serializers import BaseUserSerializer -from olympia.activity.models import ActivityLog, DraftComment +from olympia.activity.models import ActivityLog, AttachmentLog, DraftComment from olympia.addons.models import ( UPCOMING_DUE_DATE_CUT_OFF_DAYS_CONFIG_DEFAULT, Addon, @@ -2611,6 +2611,65 @@ class TestReview(ReviewBase): assert len(mail.outbox) == 1 self.assertTemplateUsed(response, 'activity/emails/from_reviewer.txt') + def test_attachment_input(self): + self.client.post( + self.url, + {'action': 'reply', 'comments': 'hello sailor'}, + ) + # A regular reply does not create an AttachmentLog. + assert AttachmentLog.objects.count() == 0 + text = 'babys first build log' + response = self.client.post( + self.url, + { + 'action': 'reply', + 'comments': 'hello sailor', + 'attachment_input': text, + }, + ) + assert response.status_code == 302 + assert AttachmentLog.objects.count() == 1 + attachment_log = AttachmentLog.objects.first() + file_content = attachment_log.file.read().decode('utf-8') + assert file_content == text + + def test_attachment_valid_upload(self): + assert AttachmentLog.objects.count() == 0 + text = "I'm a text file" + attachment = ContentFile(text, name='attachment.txt') + response = self.client.post( + self.url, + { + 'action': 'reply', + 'comments': 'hello sailor', + 'attachment_file': attachment, + }, + ) + assert response.status_code == 302 + assert AttachmentLog.objects.count() == 1 + attachment_log = AttachmentLog.objects.first() + file_content = attachment_log.file.read().decode('utf-8') + assert file_content == text + + @override_switch('enable-activity-log-attachments', active=True) + def test_attachment_invalid_upload(self): + assert AttachmentLog.objects.count() == 0 + attachment = ContentFile("I'm not a text file", name='attachment.png') + response = self.client.post( + self.url, + { + 'action': 'reply', + 'comments': 'hello sailor', + 'attachment_file': attachment, + }, + ) + assert response.status_code != 302 + assert AttachmentLog.objects.count() == 0 + self.assertIn( + 'Unsupported file type, please upload a file (.txt)', + response.content.decode('utf-8'), + ) + def test_page_title(self): response = self.client.get(self.url) assert response.status_code == 200 @@ -2704,7 +2763,7 @@ class TestReview(ReviewBase): str(author.get_role_display()), self.addon, ) - with self.assertNumQueries(56): + with self.assertNumQueries(57): # FIXME: obviously too high, but it's a starting point. # Potential further optimizations: # - Remove trivial... and not so trivial duplicates @@ -2769,6 +2828,7 @@ class TestReview(ReviewBase): # 54. cinder policies for the policy dropdown # 55. select users by role for this add-on (?) # 56. unreviewed versions in other channel + # 57. attachmentlog response = self.client.get(self.url) assert response.status_code == 200 doc = pq(response.content) @@ -2951,15 +3011,27 @@ class TestReview(ReviewBase): in doc('#versions-history .review-files .listing-header .light').text() ) + @override_switch('enable-activity-log-attachments', active=True) def test_item_history_comment(self): # Add Comment. self.client.post(self.url, {'action': 'comment', 'comments': 'hello sailor'}) + # Add reply with an attachment. + self.client.post( + self.url, + { + 'action': 'reply', + 'comments': 'hello again sailor', + 'attachment_input': 'build log', + }, + ) response = self.client.get(self.url) assert response.status_code == 200 doc = pq(response.content)('#versions-history .review-files') assert doc('th').eq(1).text() == 'Commented' - assert doc('.history-comment').text() == 'hello sailor' + assert doc('.history-comment').eq(0).text() == 'hello sailor' + assert doc('.history-comment').eq(1).text() == 'hello again sailor' + assert len(doc('.download-reply-attachment')) == 1 def test_item_history_pending_rejection(self): response = self.client.get(self.url) @@ -5438,7 +5510,7 @@ class TestReview(ReviewBase): results={'matchedRules': [customs_rule.name]}, ) - with self.assertNumQueries(57): + with self.assertNumQueries(58): # See test_item_history_pagination() for more details about the # queries count. What's important here is that the extra versions # and scanner results don't cause extra queries. diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 4935ce2d22..25f98aa8b6 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -3,6 +3,7 @@ from datetime import datetime, timedelta from django.conf import settings from django.contrib.humanize.templatetags.humanize import naturaltime +from django.core.files.base import ContentFile from django.db.models import Count, F, Q from django.urls import reverse from django.utils.http import urlencode @@ -15,7 +16,7 @@ from olympia import amo from olympia.abuse.models import CinderJob, CinderPolicy from olympia.abuse.tasks import notify_addon_decision_to_cinder, resolve_job_in_cinder from olympia.access import acl -from olympia.activity.models import ActivityLog +from olympia.activity.models import ActivityLog, AttachmentLog from olympia.activity.utils import notify_about_activity_log from olympia.addons.models import Addon, AddonApprovalsCounter, AddonReviewerFlags from olympia.constants.abuse import DECISION_ACTIONS @@ -1017,6 +1018,17 @@ class ReviewBase: kwargs = {'user': user or self.user, 'created': timestamp, 'details': details} self.log_entry = ActivityLog.objects.create(action, *args, **kwargs) + attachment = None + if self.data.get('attachment_file'): + attachment = self.data.get('attachment_file') + elif self.data.get('attachment_input'): + # The name will be overridden later by attachment_upload_path. + attachment = ContentFile( + self.data['attachment_input'], name='attachment.txt' + ) + if attachment is not None: + AttachmentLog.objects.create(activity_log=self.log_entry, file=attachment) + def reviewer_reply(self): # Default to reviewer reply action. action = amo.LOG.REVIEWER_REPLY_VERSION diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py index 96edbefb20..031d573b12 100644 --- a/src/olympia/reviewers/views.py +++ b/src/olympia/reviewers/views.py @@ -527,7 +527,9 @@ def review(request, addon, channel=None): human_review=True, ) form = ReviewForm( - request.POST if request.method == 'POST' else None, helper=form_helper + request.POST if request.method == 'POST' else None, + request.FILES if request.method == 'POST' else None, + helper=form_helper, ) reports = Paginator(AbuseReport.objects.for_addon(addon), 5).page(1) diff --git a/src/olympia/urls.py b/src/olympia/urls.py index 2af1bd5ee6..a00f1da5bf 100644 --- a/src/olympia/urls.py +++ b/src/olympia/urls.py @@ -43,6 +43,8 @@ urlpatterns = [ re_path(r'^uploads/', include(upload_patterns)), # Downloads. re_path(r'^downloads/', include(download_patterns)), + # Activity. + re_path(r'activity/', include('olympia.activity.urls')), # Users re_path(r'', include('olympia.users.urls')), # Developer Hub. diff --git a/static/css/zamboni/reviewers.less b/static/css/zamboni/reviewers.less index f34303c4ff..2b29c56782 100644 --- a/static/css/zamboni/reviewers.less +++ b/static/css/zamboni/reviewers.less @@ -896,6 +896,11 @@ form.review-form .data-toggle { height: 200px; } +#review-actions-form #id_attachment_input { + height: 100px; + width: 30%; +} + #review-actions-form .review-actions-desc { text-align: center; } @@ -1406,3 +1411,12 @@ table.abuse_reports { margin-bottom: 0; } } + +#attachment-type-toggle, #id_attachment_file { + padding: 0.5em; +} + +.review-actions-attachment-reply { + display: flex; + flex-direction: column; +} \ No newline at end of file diff --git a/static/css/zamboni/zamboni.css b/static/css/zamboni/zamboni.css index 357d08adb2..a81961e95e 100644 --- a/static/css/zamboni/zamboni.css +++ b/static/css/zamboni/zamboni.css @@ -724,6 +724,11 @@ th, td { padding: 0.308em 0.537em 0.214em 0.231em; /* 4px 7px 3px 7px */ } +div:has(> .download-reply-attachment) { + display: flex; + flex-direction: column; + text-align: right; +} /** Button =Popups *********/ .install { diff --git a/static/js/zamboni/reviewers.js b/static/js/zamboni/reviewers.js index ea9aa3930e..86044320c9 100644 --- a/static/js/zamboni/reviewers.js +++ b/static/js/zamboni/reviewers.js @@ -301,6 +301,20 @@ function initExtraReviewActions() { }), ); + $('#toggle_attachment_file').on('click', function (e) { + e.preventDefault(); + $('#attachment-type-toggle').addClass('hidden'); + $('#attachment_input_wrapper').addClass('hidden'); + $('#attachment_file_wrapper').removeClass('hidden'); + }); + + $('#toggle_attachment_input').on('click', function (e) { + e.preventDefault(); + $('#attachment-type-toggle').addClass('hidden'); + $('#attachment_file_wrapper').addClass('hidden'); + $('#attachment_input_wrapper').removeClass('hidden'); + }); + // One-off-style buttons. $('.more-actions button.oneoff[data-api-url]').click( _pd(function () {