Allow reviewers and developers to reply to older versions (#22661)

* Allow reviewers and developers to reply to older versions

src/olympia/activity/tests/test_views.py

@@ -150,15 +150,22 @@ class ReviewNotesViewSetDetailMixin(LogMixin):
         self.version.delete()
 
         # There was a listed version, it has been deleted but still, it was
-        # there, so listed reviewers should still be able to access.
+        # there, so listed reviewers should still be able to access if they
+        # have Addons:ViewDeleted
         self._login_reviewer()
         response = self.client.get(self.url)
+        assert response.status_code == 404
+
+        user = UserProfile.objects.get(username='reviewer')
+        self.grant_permission(user, 'Addons:ViewDeleted')
+        response = self.client.get(self.url)
         assert response.status_code == 200
 
     def test_deleted_version_developer(self):
         self.version.delete()
         self._login_developer()
-        self._test_url()
+        response = self.client.get(self.url)
+        assert response.status_code == 404
 
     def test_get_version_not_found(self):
         self._login_reviewer(permission='*:*')
@@ -503,7 +510,7 @@ class TestReviewNotesViewSetCreate(TestCase):
         assert response.status_code == 404
         assert not self.get_review_activity_queryset().exists()
 
-    def test_reply_to_deleted_version_is_400(self):
+    def test_reply_to_deleted_version_is_404(self):
         old_version = self.addon.find_latest_version(channel=amo.CHANNEL_LISTED)
         new_version = version_factory(addon=self.addon)
         old_version.delete()
@@ -516,10 +523,10 @@ class TestReviewNotesViewSetCreate(TestCase):
         self.grant_permission(self.user, 'Addons:Review')
         self.client.login_api(self.user)
         response = self._post_reply()
-        assert response.status_code == 400
+        assert response.status_code == 404
         assert not self.get_review_activity_queryset().exists()
 
-    def test_cant_reply_to_old_version(self):
+    def test_can_reply_to_old_version(self):
         old_version = self.addon.find_latest_version(channel=amo.CHANNEL_LISTED)
         old_version.update(created=self.days_ago(1))
         new_version = version_factory(addon=self.addon)
@@ -537,10 +544,10 @@ class TestReviewNotesViewSetCreate(TestCase):
         assert response.status_code == 201
         assert self.get_review_activity_queryset().count() == 1
 
-        # The check we can't reply to the old version
+        # The check we can reply to the old version
         response = self._post_reply()
-        assert response.status_code == 400
-        assert self.get_review_activity_queryset().count() == 1
+        assert response.status_code == 201
+        assert self.get_review_activity_queryset().count() == 2
 
     def test_developer_can_reply_to_disabled_version(self):
         self.version.file.update(status=amo.STATUS_DISABLED)

src/olympia/activity/views.py

@@ -4,7 +4,7 @@ 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 _
+from django.utils.translation import gettext_lazy as _
 
 from rest_framework import exceptions, status
 from rest_framework.decorators import (
@@ -37,6 +37,7 @@ from olympia.api.permissions import (
     AllowListedViewerOrReviewer,
     AllowUnlistedViewerOrReviewer,
     AnyOf,
+    GroupPermission,
 )
 
 
@@ -65,8 +66,9 @@ class VersionReviewNotesViewSet(
         if not hasattr(self, 'version_object'):
             addon = self.get_addon_object()
             self.version_object = get_object_or_404(
-                # Fetch the version without transforms, using the addon related
-                # manager to avoid reloading it from the database.
+                # Fetch the version without transforms, we don't need the extra
+                # data (and the addon property will be set on the version since
+                # we're using the addon.versions manager).
                 addon.versions(manager='unfiltered_for_relations')
                 .all()
                 .no_transforms(),
@@ -75,10 +77,20 @@ class VersionReviewNotesViewSet(
         return self.version_object
 
     def check_object_permissions(self, request, obj):
-        """Check object permissions against the Addon, not the ActivityLog."""
+        # Permissions checks are all done in check_permissions(), there are no
+        # checks to be done for an individual activity log.
+        pass
+
+    def check_permissions(self, request):
         # Just loading the add-on object triggers permission checks, because
         # the implementation in AddonChildMixin calls AddonViewSet.get_object()
         self.get_addon_object()
+        # The only thing left to test is that the Version is not deleted.
+        version = self.get_version_object()
+        if version.deleted and not GroupPermission(
+            amo.permissions.ADDONS_VIEW_DELETED
+        ).has_object_permission(request, self, version):
+            raise http.Http404
 
     def get_serializer_context(self):
         ctx = super().get_serializer_context()
@@ -91,13 +103,6 @@ class VersionReviewNotesViewSet(
 
     def create(self, request, *args, **kwargs):
         version = self.get_version_object()
-        latest_version = version.addon.find_latest_version(
-            channel=version.channel, exclude=()
-        )
-        if version != latest_version:
-            raise exceptions.ParseError(
-                gettext('Only latest versions of addons can have notes added.')
-            )
         serializer = ActivityLogSerializerForComments(data=request.data)
         serializer.is_valid(raise_exception=True)
         activity_object = log_and_notify(

src/olympia/devhub/templates/devhub/versions/list.html

@@ -89,9 +89,7 @@
          data-is-current="{{ (version == addon.current_version)|int }}">x</a>
     </td>
   </tr>
-  {% set can_request_review=addon.can_request_review() %}
-  {% set can_cancel=(not addon.is_disabled and addon.status==amo.STATUS_NOMINATED) %}
-  {% if full_info and check_addon_ownership(request.user, addon, allow_developer=True) and version.channel == amo.CHANNEL_LISTED and (can_request_review or can_cancel) %}
+  {% if full_info and version.channel == amo.CHANNEL_LISTED and (can_request_review or can_cancel) and check_addon_ownership(request.user, addon, allow_developer=True) %}
     <tr>
       <td colspan="0" class="version-status-actions item-actions">
         {% if can_request_review %}
@@ -123,20 +121,14 @@
                   <pre>$comments</pre>
               </div>
           </div>
-          {% if latest_version_in_channel_including_disabled == version %}
-            <div class="dev-review-reply">
-              <form class="dev-review-reply-form" action="{{ drf_url('version-reviewnotes-list', addon.id, version.id) }}"
-                    data-session-id="{{ session_id }}" data-history="#{{ version.id }}-review-history" data-no-csrf>
-                  {# This is not a django form, an XHR goes through to the form action on submit #}
-                  <textarea maxlength="{{ comments_maxlength }}" name="comments" placeholder="{{ _('Leave a reply') }}"></textarea>
-                  <button type="submit" class="submit" >{{ _('Reply') }}</button>
-              </form>
-            </div>
-          {% else %}
-            <div>
-              <a href="#" class="review-history-show" data-version="{{ latest_version_in_channel_including_disabled.id }}">{{ _('Leave a reply') }}</a>
-            </div>
-          {% endif %}
+          <div class="dev-review-reply">
+            <form class="dev-review-reply-form" action="{{ drf_url('version-reviewnotes-list', addon.id, version.id) }}"
+                  data-session-id="{{ session_id }}" data-history="#{{ version.id }}-review-history" data-no-csrf>
+                {# This is not a django form, an XHR goes through to the form action on submit #}
+                <textarea maxlength="{{ comments_maxlength }}" name="comments" placeholder="{{ _('Leave a reply') }}"></textarea>
+                <button type="submit" class="submit" >{{ _('Reply') }}</button>
+            </form>
+          </div>
       </td>
   </tr>
 {% endmacro %}

src/olympia/devhub/tests/test_views_versions.py

@@ -599,16 +599,13 @@ class TestVersion(TestCase):
         doc = pq(response.content)
 
         show_links = doc('.review-history-show')
-        assert show_links.length == 3
+        assert show_links.length == 2
         assert show_links[0].attrib['data-div'] == '#%s-review-history' % v1.id
-        assert not show_links[1].attrib.get('data-div')
-        assert show_links[2].attrib['data-div'] == '#%s-review-history' % v2.id
+        assert show_links[1].attrib['data-div'] == '#%s-review-history' % v2.id
 
-        # All 3 links will have a 'data-version' attribute.
+        # Both links will have a 'data-version' attribute.
         assert show_links[0].attrib['data-version'] == str(v1.id)
-        # But the 2nd link will point to the latest version in the channel.
         assert show_links[1].attrib['data-version'] == str(v2.id)
-        assert show_links[2].attrib['data-version'] == str(v2.id)
 
         # Test review history
         review_history_td = doc('#%s-review-history' % v1.id)[0]
@@ -625,17 +622,22 @@ class TestVersion(TestCase):
         # No counter, because we don't have any pending activity to show.
         assert pending_activity_count.length == 0
 
-        reply_api_url = absolutify(
-            reverse_ns('version-reviewnotes-list', args=[self.addon.id, v2.id])
-        )
-        # Reply box div is there (only one)
-        assert doc('.dev-review-reply-form').length == 1
-        review_form = doc('.dev-review-reply-form')[0]
-        assert review_form.attrib['action'] == reply_api_url
-        assert review_form.attrib['data-session-id'] == self.client.session.session_key
-        assert review_form.attrib['data-history'] == '#%s-review-history' % v2.id
-        textarea = doc('.dev-review-reply-form textarea')[0]
-        assert textarea.attrib['maxlength'] == '100000'
+        # Reply box div is there for each version
+        assert doc('.dev-review-reply-form').length == 2
+        for idx, version in enumerate([v1, v2]):
+            reply_api_url = absolutify(
+                reverse_ns('version-reviewnotes-list', args=[self.addon.id, version.pk])
+            )
+            review_form = doc('.dev-review-reply-form')[idx]
+            assert review_form.attrib['action'] == reply_api_url
+            assert (
+                review_form.attrib['data-session-id'] == self.client.session.session_key
+            )
+            assert (
+                review_form.attrib['data-history'] == '#%s-review-history' % version.pk
+            )
+            textarea = doc('.dev-review-reply-form textarea')[idx]
+            assert textarea.attrib['maxlength'] == '100000'
 
     def test_version_history_mixed_channels(self):
         v1 = self.version
@@ -669,9 +671,9 @@ class TestVersion(TestCase):
         assert response.status_code == 200
         doc = pq(response.content)
 
-        # Two versions, but three review-history-show because one reply link.
-        assert doc('.review-history-show').length == 3
-        # Two versions, but only one counter, for the latest/deleted version
+        # Two versions...
+        assert doc('.review-history-show').length == 2
+        # ...but only one counter, for the latest version
         pending_activity_count = doc('.review-history-pending-count')
         assert pending_activity_count.length == 1
         # There are two activity logs pending

src/olympia/devhub/views.py

@@ -1273,10 +1273,12 @@ def version_list(request, addon_id, addon):
 
     data = {
         'addon': addon,
-        'versions': versions,
-        'session_id': request.session.session_key,
-        'is_admin': is_admin,
+        'can_request_review': addon.can_request_review(),
+        'can_cancel': not addon.is_disabled and addon.status == amo.STATUS_NOMINATED,
         'comments_maxlength': CommentLog._meta.get_field('comments').max_length,
+        'is_admin': is_admin,
+        'session_id': request.session.session_key,
+        'versions': versions,
     }
     return TemplateResponse(request, 'devhub/versions/list.html', context=data)
 

src/olympia/reviewers/forms.py

@@ -125,23 +125,33 @@ class VersionsChoiceWidget(forms.SelectMultiple):
                 'block_multiple_versions',
                 'confirm_multiple_versions',
                 'reject_multiple_versions',
+                'reply',
             ],
             amo.STATUS_AWAITING_REVIEW: [
                 'approve_multiple_versions',
                 'reject_multiple_versions',
+                'reply',
+            ],
+            amo.STATUS_DISABLED: [
+                'unreject_multiple_versions',
+                'reply',
             ],
-            amo.STATUS_DISABLED: ['unreject_multiple_versions'],
         },
         amo.CHANNEL_LISTED: {
             amo.STATUS_APPROVED: [
                 'block_multiple_versions',
                 'reject_multiple_versions',
+                'reply',
             ],
             amo.STATUS_AWAITING_REVIEW: [
                 'approve_multiple_versions',
                 'reject_multiple_versions',
+                'reply',
+            ],
+            amo.STATUS_DISABLED: [
+                'unreject_multiple_versions',
+                'reply',
             ],
-            amo.STATUS_DISABLED: ['unreject_multiple_versions'],
         },
     }
 
@@ -408,7 +418,8 @@ class ReviewForm(forms.Form):
 
     def is_valid(self):
         # Some actions do not require comments and reasons.
-        action = self.helper.actions.get(self.data.get('action'))
+        selected_action = self.data.get('action')
+        action = self.helper.actions.get(selected_action)
         if action:
             if not action.get('comments', True):
                 self.fields['comments'].required = False
@@ -422,7 +433,7 @@ class ReviewForm(forms.Form):
                     self.fields['reasons'].required = True
                 if action.get('allows_policies'):
                     self.fields['cinder_policies'].required = True
-            if self.data.get('action') == 'resolve_appeal_job':
+            if selected_action == 'resolve_appeal_job':
                 self.fields['appeal_action'].required = True
         result = super().is_valid()
         if result:

src/olympia/reviewers/tests/test_forms.py

@@ -308,7 +308,7 @@ class TestReviewForm(TestCase):
         assert list(choices.queryset)[0] == self.reason_all
         assert list(choices.queryset)[1] == self.reason_theme
 
-    def test_reasons_not_required_for_reply(self):
+    def test_reasons_not_required_for_reply_but_versions_is(self):
         self.grant_permission(self.request.user, 'Addons:Review')
         form = self.get_form()
         assert not form.is_bound
@@ -316,6 +316,7 @@ class TestReviewForm(TestCase):
             data={
                 'action': 'reply',
                 'comments': 'lol',
+                'versions': [self.version.pk],
             }
         )
         assert form.helper.actions['reply']['requires_reasons'] is False
@@ -323,6 +324,18 @@ class TestReviewForm(TestCase):
         assert form.is_valid()
         assert not form.errors
 
+        form = self.get_form(
+            data={
+                'action': 'reply',
+                'comments': 'lol',
+            }
+        )
+        assert form.is_bound
+        assert not form.is_valid()
+        assert form.errors == {
+            'versions': ['This field is required.'],
+        }
+
     def test_reasons_required_for_reject_multiple_versions(self):
         self.grant_permission(self.request.user, 'Addons:Review')
         form = self.get_form()
@@ -589,6 +602,7 @@ class TestReviewForm(TestCase):
                         canned_response='reason 1',
                     )
                 ],
+                'versions': [self.version.pk],
             }
         )
         form.helper.actions['reply']['comments'] = False
@@ -619,6 +633,7 @@ class TestReviewForm(TestCase):
             expected_select_data_value = [
                 'reject_multiple_versions',
                 'set_needs_human_review_multiple_versions',
+                'reply',
             ]
         # We hide some of the versions using JavaScript + some data attributes on each
         # <option>.
@@ -676,6 +691,7 @@ class TestReviewForm(TestCase):
             # That version is approved.
             'block_multiple_versions',
             'reject_multiple_versions',
+            'reply',
             'set_needs_human_review_multiple_versions',
         ]
         assert option1.attrib.get('value') == str(self.version.pk)
@@ -686,6 +702,7 @@ class TestReviewForm(TestCase):
             # That version is pending.
             'approve_multiple_versions',
             'reject_multiple_versions',
+            'reply',
             'set_needs_human_review_multiple_versions',
         ]
         assert option2.attrib.get('value') == str(pending_version.pk)
@@ -697,6 +714,7 @@ class TestReviewForm(TestCase):
             # but it was never signed so it doesn't get
             # set_needs_human_review_multiple_versions
             'unreject_multiple_versions',
+            'reply',
         ]
         assert option3.attrib.get('value') == str(rejected_version.pk)
 
@@ -717,6 +735,7 @@ class TestReviewForm(TestCase):
                 'clear_pending_rejection_multiple_versions',
                 'clear_needs_human_review_multiple_versions',
                 'set_needs_human_review_multiple_versions',
+                'reply',
             ]
         )
 
@@ -797,6 +816,7 @@ class TestReviewForm(TestCase):
             'block_multiple_versions',
             'confirm_multiple_versions',
             'reject_multiple_versions',
+            'reply',
             'set_needs_human_review_multiple_versions',
         ]
         assert option1.attrib.get('value') == str(self.version.pk)
@@ -807,6 +827,7 @@ class TestReviewForm(TestCase):
             # That version is pending.
             'approve_multiple_versions',
             'reject_multiple_versions',
+            'reply',
             'set_needs_human_review_multiple_versions',
         ]
         assert option2.attrib.get('value') == str(pending_version.pk)
@@ -818,6 +839,7 @@ class TestReviewForm(TestCase):
             # but it was never signed so it doesn't get
             # set_needs_human_review_multiple_versions
             'unreject_multiple_versions',
+            'reply',
         ]
         assert option3.attrib.get('value') == str(rejected_version.pk)
 
@@ -1143,6 +1165,7 @@ class TestReviewForm(TestCase):
         data = {
             'action': 'reply',
             'comments': 'lol',
+            'versions': [self.version.pk],
         }
         files = {'attachment_file': attachment}
 

src/olympia/reviewers/tests/test_utils.py

@@ -214,7 +214,9 @@ class TestReviewHelper(TestReviewHelperBase):
     def test_process_action_good(self):
         self.grant_permission(self.user, 'Addons:Review')
         self.helper = self.get_helper()
-        self.helper.set_data({'action': 'reply', 'comments': 'foo'})
+        self.helper.set_data(
+            {'action': 'reply', 'comments': 'foo', 'versions': [self.review_version]}
+        )
         self.helper.process()
         assert len(mail.outbox) == 1
 
@@ -1228,6 +1230,7 @@ class TestReviewHelper(TestReviewHelperBase):
 
     def test_send_reviewer_reply(self):
         self.setup_data(amo.STATUS_APPROVED)
+        self.helper.handler.data['versions'] = [self.addon.versions.get()]
         self.helper.handler.reviewer_reply()
 
         assert len(mail.outbox) == 1
@@ -1236,6 +1239,32 @@ class TestReviewHelper(TestReviewHelperBase):
 
         assert self.check_log_count(amo.LOG.REVIEWER_REPLY_VERSION.id) == 1
 
+    def test_send_reviewer_reply_multiple_versions(self):
+        new_version = version_factory(addon=self.addon, version='3.0')
+        new_version2 = version_factory(addon=self.addon, version='3.2')
+        self.setup_data(amo.STATUS_APPROVED)
+        self.helper.handler.data['versions'] = [new_version, new_version2]
+        self.helper.handler.reviewer_reply()
+
+        # Should result in a single activity...
+        assert self.check_log_count(amo.LOG.REVIEWER_REPLY_VERSION.id) == 1
+        activity = (
+            ActivityLog.objects.for_addons(self.addon)
+            .filter(action=amo.LOG.REVIEWER_REPLY_VERSION.id)
+            .get()
+        )
+        assert [new_version, new_version2] == list(
+            vlog.version
+            for vlog in activity.versionlog_set.all().order_by('version__pk')
+        )
+
+        # ... but 2 emails, because we're sending them version per version.
+        assert len(mail.outbox) == 2
+        assert mail.outbox[0].subject == 'Mozilla Add-ons: Delicious Bookmarks 3.0'
+        assert 'foo' in mail.outbox[0].body
+        assert mail.outbox[1].subject == 'Mozilla Add-ons: Delicious Bookmarks 3.2'
+        assert 'foo' in mail.outbox[1].body
+
     def test_email_no_name(self):
         self.addon.name.delete()
         self.addon.refresh_from_db()

src/olympia/reviewers/tests/test_views.py

@@ -2605,7 +2605,12 @@ class TestReview(ReviewBase):
         )
         response = self.client.post(
             self.url,
-            {'action': 'reply', 'comments': 'hello sailor', 'reasons': [reason.id]},
+            {
+                'action': 'reply',
+                'comments': 'hello sailor',
+                'reasons': [reason.id],
+                'versions': [self.version.pk],
+            },
         )
         assert response.status_code == 302
         assert len(mail.outbox) == 1
@@ -2614,7 +2619,11 @@ class TestReview(ReviewBase):
     def test_attachment_input(self):
         self.client.post(
             self.url,
-            {'action': 'reply', 'comments': 'hello sailor'},
+            {
+                'action': 'reply',
+                'comments': 'hello sailor',
+                'versions': [self.version.pk],
+            },
         )
         # A regular reply does not create an AttachmentLog.
         assert AttachmentLog.objects.count() == 0
@@ -2624,6 +2633,7 @@ class TestReview(ReviewBase):
             {
                 'action': 'reply',
                 'comments': 'hello sailor',
+                'versions': [self.version.pk],
                 'attachment_input': text,
             },
         )
@@ -2642,6 +2652,7 @@ class TestReview(ReviewBase):
             {
                 'action': 'reply',
                 'comments': 'hello sailor',
+                'versions': [self.version.pk],
                 'attachment_file': attachment,
             },
         )
@@ -2660,6 +2671,7 @@ class TestReview(ReviewBase):
             {
                 'action': 'reply',
                 'comments': 'hello sailor',
+                'versions': [self.version.pk],
                 'attachment_file': attachment,
             },
         )
@@ -3021,6 +3033,7 @@ class TestReview(ReviewBase):
             {
                 'action': 'reply',
                 'comments': 'hello again sailor',
+                'versions': [self.version.pk],
                 'attachment_input': 'build log',
             },
         )
@@ -3852,6 +3865,7 @@ class TestReview(ReviewBase):
             'applications': 'something',
             'comments': 'something',
             'reasons': [reason.id],
+            'versions': [version.pk],
         }
 
         self.client.post(url, data)
@@ -5167,7 +5181,11 @@ class TestReview(ReviewBase):
 
         assert doc('select#id_versions.data-toggle')[0].attrib['data-value'].split(
             ' '
-        ) == ['reject_multiple_versions', 'set_needs_human_review_multiple_versions']
+        ) == [
+            'reject_multiple_versions',
+            'set_needs_human_review_multiple_versions',
+            'reply',
+        ]
 
         assert (
             doc('select#id_versions.data-toggle option')[0].text
@@ -5227,6 +5245,7 @@ class TestReview(ReviewBase):
             'block_multiple_versions',
             'confirm_multiple_versions',
             'set_needs_human_review_multiple_versions',
+            'reply',
         ]
 
         assert doc('.data-toggle.review-comments')[0].attrib['data-value'].split(
@@ -5646,6 +5665,7 @@ class TestReview(ReviewBase):
                 'action': 'reply',
                 'comments': 'Reply!',
                 'reasons': [reason.id],
+                'versions': [self.version.pk],
             },
             follow=True,
         )

src/olympia/reviewers/utils.py

@@ -702,9 +702,10 @@ class ReviewHelper:
             'method': self.handler.reviewer_reply,
             'label': 'Reviewer reply',
             'details': (
-                'This will send a message to the developer. '
-                'You will be notified when they reply.'
+                'This will send a message to the developer, attached to the '
+                'selected version(s). You will be notified when they reply.'
             ),
+            'multiple_versions': True,
             'minimal': True,
             'available': (
                 self.version is not None
@@ -1032,14 +1033,18 @@ class ReviewBase:
     def reviewer_reply(self):
         # Default to reviewer reply action.
         action = amo.LOG.REVIEWER_REPLY_VERSION
+        self.version = None
+        self.file = None
+        versions = self.data['versions']
         log.info(
-            'Sending reviewer reply for %s to authors and other'
-            'recipients' % self.addon
-        )
-        self.log_action(action)
-        notify_about_activity_log(
-            self.addon, self.version, self.log_entry, perm_setting='individual_contact'
+            'Sending reviewer reply for %s versions %s to authors and other'
+            'recipients' % (self.addon, map(str, versions))
         )
+        self.log_action(action, versions=versions)
+        for version in versions:
+            notify_about_activity_log(
+                self.addon, version, self.log_entry, perm_setting='individual_contact'
+            )
 
     def sign_file(self):
         assert not (self.version and self.version.is_blocked)