Make the code manager the default link for reviewer tools (#14442)

2020-06-02 12:56:09 -04:00 · 2020-06-02 12:56:09 -04:00 · 18384a0c3c
--- a/src/olympia/reviewers/migrations/0005_remove_code_manager_waffle_flag.py
+++ b/src/olympia/reviewers/migrations/0005_remove_code_manager_waffle_flag.py
@ -0,0 +1,18 @@
+# Generated by Django 2.2.12 on 2020-06-02 13:28
+
+from django.db import migrations
+
+
+def delete_waffle_flag(apps, schema_editor):
+    Flag = apps.get_model('waffle', 'Flag')
+
+    Flag.objects.filter(name='code-manager').delete()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('reviewers', '0004_remove_autoapprovalsummary_is_listing_disabled'),
+    ]
+
+    operations = [migrations.RunPython(delete_waffle_flag)]
--- a/src/olympia/reviewers/templates/reviewers/includes/files_view.html
+++ b/src/olympia/reviewers/templates/reviewers/includes/files_view.html
@ -11,38 +11,26 @@
    </div>
    <a href="{{ url('devhub.file_validation', addon.slug, file[0].id) }}">{{ _('Validation') }}</a>
    &middot;
-    <a href="{{ url('files.list', file[0].id) }}">{{ _('Contents') }}</a>
-    {% if show_diff and version == latest_version_in_channel %}
+    <a 
+      href="{{ code_manager_url('browse', addon.pk, version.pk) }}"
+      title="{{ _('Browse all files in this version') }}"
+      >
+        {{ _('Contents') }}
+    </a>
+    {% if base_version and version == latest_version_in_channel %}
    &middot;
-    <a class="compare" href="{{ url('files.compare', file[0].id, file_compare(file[0], show_diff)) }}">{{ _('Compare') }}</a>
+    <a
+      class="compare"
+      href="{{ code_manager_url('compare', addon.pk, version.pk, base_version.pk) }}"
+      title="{{ _('Compare this version to the last reviewed version') }}"
+    >
+      {{ _('Compare') }}
+    </a>
    {% endif %}
    {% if file[0].is_webextension %}
    <div class="file-permissions"><strong>{{ _('Permissions:') }}</strong> {{ ', '.join(file[0].webext_permissions_list) or _('None') }}</div>
    {% endif %}
  </span>
 </li>
-{% if waffle.flag('code-manager') %}
-  <li class="code-manager-links">
-    <div>
-      <strong>Code Manager</strong>
-      <img width="25" height="9" src="{{ static('img/reviewers/new.gif') }}">
-    </div>
-    <a
-      href="{{ code_manager_url('/browse/{}/versions/{}/'.format(addon.pk, version.pk)) }}"
-      title="Browse all files in this version"
-    >
-      {{ _('Contents') }}
-    </a>
-    {% if show_diff and version == latest_version_in_channel %}
-      &middot;
-      <a
-        href="{{ code_manager_url('/compare/{}/versions/{}...{}/'.format(addon.pk, show_diff.pk, version.pk)) }}"
-        title="Compare this version to the last reviewed version"
-      >
-        {{ _('Compare') }}
-      </a>
-    {% endif %}
-  </li>
-{% endif %}
 {% endfor %}
 </ul>
--- a/src/olympia/reviewers/templatetags/code_manager.py
+++ b/src/olympia/reviewers/templatetags/code_manager.py
@ -0,0 +1,19 @@
+from django import template
+from django.conf import settings
+
+register = template.Library()
+
+
+@register.simple_tag
+def code_manager_url(page, addon_id, version_id, base_version_id=None,
+                     file=None):
+    # Always return URLs in en-US because the Code Manager is not localized.
+    url = '{}/en-US'.format(settings.CODE_MANAGER_URL)
+    if page == 'browse':
+        url = '{}/browse/{}/versions/{}/'.format(url, addon_id, version_id)
+    else:
+        url = '{}/compare/{}/versions/{}...{}/'.format(
+            url, addon_id, base_version_id, version_id)
+    if file:
+        url = '{}?path={}'.format(url, file)
+    return url
--- a/src/olympia/reviewers/templatetags/jinja_helpers.py
+++ b/src/olympia/reviewers/templatetags/jinja_helpers.py
@ -13,6 +13,7 @@ from olympia.access import acl
 from olympia.addons.templatetags.jinja_helpers import new_context
 from olympia.ratings.permissions import user_can_delete_rating
 from olympia.reviewers.models import ReviewerScore
+from olympia.reviewers.templatetags import code_manager
 from olympia.versions.models import Version


@ -185,7 +186,7 @@ def all_distinct_files(context, version):
        addon=context.get('addon'),
        # This allows the template to call waffle.flag().
        request=context.get('request'),
-        show_diff=context.get('show_diff'),
+        base_version=context.get('base_version'),
        version=version))


@ -221,13 +222,10 @@ def is_expired_lock(context, lock):


@library.global_function
-def code_manager_url(path):
-    if not path.startswith('/'):
-        raise ValueError(
-            'Expected a relative path; got: "{}"'.format(path)
-        )
-    # Always return URLs in en-US because the Code Manager is not localized.
-    return '{}/en-US{}'.format(settings.CODE_MANAGER_URL, path)
+def code_manager_url(
+        page, addon_id, version_id, base_version_id=None):
+    return code_manager.code_manager_url(
+        page, addon_id, version_id, base_version_id)


@library.global_function
--- a/src/olympia/reviewers/tests/test_code_manager_tags.py
+++ b/src/olympia/reviewers/tests/test_code_manager_tags.py
@ -0,0 +1,51 @@
+from django.test.utils import override_settings
+
+from olympia.reviewers.templatetags import code_manager
+from olympia.amo.tests import TestCase
+
+
+class TestCodeManagerUrl(TestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.addon_id = 1
+        self.base_version_id = 2
+        self.cm_url = 'http://code-manager'
+        self.file = 'somefile.js'
+        self.version_id = 3
+
+    def test_create_a_browse_url(self):
+        with override_settings(CODE_MANAGER_URL=self.cm_url):
+            assert code_manager.code_manager_url(
+                'browse', self.addon_id, self.version_id) == (
+                '{}/en-US/browse/{}/versions/{}/'.format(
+                    self.cm_url, self.addon_id, self.version_id)
+            )
+
+    def test_create_a_compare_url(self):
+        with override_settings(CODE_MANAGER_URL=self.cm_url):
+            assert code_manager.code_manager_url(
+                'compare', self.addon_id, self.version_id,
+                self.base_version_id) == (
+                '{}/en-US/compare/{}/versions/{}...{}/'.format(
+                    self.cm_url, self.addon_id, self.base_version_id,
+                    self.version_id)
+            )
+
+    def test_create_a_browse_url_with_file(self):
+        with override_settings(CODE_MANAGER_URL=self.cm_url):
+            assert code_manager.code_manager_url(
+                'browse', self.addon_id, self.version_id, file=self.file) == (
+                '{}/en-US/browse/{}/versions/{}/?path={}'.format(
+                    self.cm_url, self.addon_id, self.version_id, self.file)
+            )
+
+    def test_create_a_compare_url_with_file(self):
+        with override_settings(CODE_MANAGER_URL=self.cm_url):
+            assert code_manager.code_manager_url(
+                'compare', self.addon_id, self.version_id,
+                self.base_version_id, self.file) == (
+                '{}/en-US/compare/{}/versions/{}...{}/?path={}'.format(
+                    self.cm_url, self.addon_id, self.base_version_id,
+                    self.version_id, self.file)
+            )
--- a/src/olympia/reviewers/tests/test_jinja_helpers.py
+++ b/src/olympia/reviewers/tests/test_jinja_helpers.py
@ -1,13 +1,12 @@
 # -*- coding: utf-8 -*-
 import pytest

-from django.test.utils import override_settings
-
 from olympia import amo
 from olympia.addons.models import Addon
 from olympia.amo.tests import TestCase
 from olympia.files.models import File
-from olympia.reviewers.templatetags import jinja_helpers
+from olympia.reviewers.templatetags import (
+    code_manager, jinja_helpers)
 from olympia.versions.models import Version


@ -79,12 +78,7 @@ def test_file_review_status_handles_invalid_status_id():


 def test_create_a_code_manager_url():
-    with override_settings(CODE_MANAGER_URL='http://code-manager'):
-        assert jinja_helpers.code_manager_url('/some/path') == (
-            'http://code-manager/en-US/some/path'
-        )
-
-
-def test_code_manager_url_expects_a_relative_path():
-    with pytest.raises(ValueError):
-        jinja_helpers.code_manager_url('http://code-manager/some/path')
+    assert jinja_helpers.code_manager_url(
+        'browse', addon_id=1, base_version_id=2, version_id=3
+    ) == code_manager.code_manager_url(
+        'browse', addon_id=1, base_version_id=2, version_id=3)
--- a/src/olympia/reviewers/tests/test_views.py
+++ b/src/olympia/reviewers/tests/test_views.py
@ -22,7 +22,6 @@ from rest_framework.test import APIRequestFactory
 from freezegun import freeze_time
 from lxml.html import HTMLParser, fromstring
 from pyquery import PyQuery as pq
-from waffle.testutils import override_flag

 from olympia import amo, core, ratings
 from olympia.abuse.models import AbuseReport
@ -49,6 +48,7 @@ from olympia.ratings.models import Rating, RatingFlag
 from olympia.reviewers.models import (
    AutoApprovalSummary, CannedResponse, ReviewerScore, ReviewerSubscription,
    Whiteboard)
+from olympia.reviewers.templatetags.jinja_helpers import code_manager_url
 from olympia.reviewers.utils import ContentReviewTable
 from olympia.reviewers.views import _queue
 from olympia.reviewers.serializers import CannedResponseSerializer
@ -3064,7 +3064,6 @@ class ReviewBase(QueueTest):
        return data


-@override_flag('code-manager', active=False)
 class TestReview(ReviewBase):

    def test_reviewer_required(self):
@ -4286,26 +4285,31 @@ class TestReview(ReviewBase):
        first_file = self.addon.current_version.files.all()[0]
        first_file.update(status=amo.STATUS_APPROVED)
        self.addon.current_version.update(created=self.days_ago(2))
+        first_version_pk = self.addon.current_version.pk

        new_version = version_factory(addon=self.addon, version='0.2')
-        new_file = new_version.files.all()[0]
        self.addon.update(_current_version=new_version)
        assert self.addon.current_version == new_version

        response = self.client.get(self.url)
        assert response.status_code == 200
        doc = pq(response.content)
-        assert response.context['show_diff']
+        assert response.context['base_version']
        links = doc('#versions-history .file-info .compare')
+
        expected = [
-            reverse('files.compare', args=[new_file.pk, first_file.pk]),
+            code_manager_url(
+                'compare', addon_id=self.addon.pk,
+                base_version_id=first_version_pk, version_id=new_version.pk),
        ]
+
        check_links(expected, links, verify=False)

    def test_compare_link_auto_approved_ignored(self):
        first_file = self.addon.current_version.files.all()[0]
        first_file.update(status=amo.STATUS_APPROVED)
        self.addon.current_version.update(created=self.days_ago(3))
+        first_version_pk = self.addon.current_version.pk

        interim_version = version_factory(addon=self.addon, version='0.2')
        interim_version.update(created=self.days_ago(2))
@ -4313,7 +4317,6 @@ class TestReview(ReviewBase):
            version=interim_version, verdict=amo.AUTO_APPROVED)

        new_version = version_factory(addon=self.addon, version='0.3')
-        new_file = new_version.files.all()[0]

        self.addon.update(_current_version=new_version)
        assert self.addon.current_version == new_version
@ -4321,13 +4324,15 @@ class TestReview(ReviewBase):
        response = self.client.get(self.url)
        assert response.status_code == 200
        doc = pq(response.content)
-        assert response.context['show_diff']
+        assert response.context['base_version']
        links = doc('#versions-history .file-info .compare')
        # Comparison should be between the last version and the first,
        # ignoring the interim version because it was auto-approved and not
        # manually confirmed by a human.
        expected = [
-            reverse('files.compare', args=[new_file.pk, first_file.pk]),
+            code_manager_url(
+                'compare', addon_id=self.addon.pk,
+                base_version_id=first_version_pk, version_id=new_version.pk),
        ]
        check_links(expected, links, verify=False)

@ -4338,7 +4343,6 @@ class TestReview(ReviewBase):

        confirmed_version = version_factory(addon=self.addon, version='0.2')
        confirmed_version.update(created=self.days_ago(2))
-        confirmed_file = confirmed_version.files.all()[0]
        AutoApprovalSummary.objects.create(
            verdict=amo.AUTO_APPROVED, version=confirmed_version,
            confirmed=True)
@ -4349,7 +4353,6 @@ class TestReview(ReviewBase):
            version=interim_version, verdict=amo.AUTO_APPROVED)

        new_version = version_factory(addon=self.addon, version='0.4')
-        new_file = new_version.files.all()[0]

        self.addon.update(_current_version=new_version)
        assert self.addon.current_version == new_version
@ -4357,14 +4360,17 @@ class TestReview(ReviewBase):
        response = self.client.get(self.url)
        assert response.status_code == 200
        doc = pq(response.content)
-        assert response.context['show_diff']
+        assert response.context['base_version']
        links = doc('#versions-history .file-info .compare')
        # Comparison should be between the last version and the second,
        # ignoring the third version because it was auto-approved and not
        # manually confirmed by a human (the second was auto-approved but
        # was manually confirmed).
        expected = [
-            reverse('files.compare', args=[new_file.pk, confirmed_file.pk]),
+            code_manager_url(
+                'compare', addon_id=self.addon.pk,
+                base_version_id=confirmed_version.pk,
+                version_id=new_version.pk),
        ]
        check_links(expected, links, verify=False)

@ -4375,13 +4381,11 @@ class TestReview(ReviewBase):

        confirmed_version = version_factory(addon=self.addon, version='0.2')
        confirmed_version.update(created=self.days_ago(2))
-        confirmed_file = confirmed_version.files.all()[0]
        AutoApprovalSummary.objects.create(
            verdict=amo.NOT_AUTO_APPROVED, version=confirmed_version
        )

        new_version = version_factory(addon=self.addon, version='0.3')
-        new_file = new_version.files.all()[0]

        self.addon.update(_current_version=new_version)
        assert self.addon.current_version == new_version
@ -4389,12 +4393,15 @@ class TestReview(ReviewBase):
        response = self.client.get(self.url)
        assert response.status_code == 200
        doc = pq(response.content)
-        assert response.context['show_diff']
+        assert response.context['base_version']
        links = doc('#versions-history .file-info .compare')
        # Comparison should be between the last version and the second,
        # because second was approved by human before auto-approval ran on it
        expected = [
-            reverse('files.compare', args=[new_file.pk, confirmed_file.pk]),
+            code_manager_url(
+                'compare', addon_id=self.addon.pk,
+                base_version_id=confirmed_version.pk,
+                version_id=new_version.pk),
        ]
        check_links(expected, links, verify=False)

@ -5394,61 +5401,6 @@ class TestAbuseReportsView(ReviewerTest):
        assert response.status_code == 200


-@override_flag('code-manager', active=True)
-class TestCodeManagerLinks(ReviewBase):
-
-    def get_links(self):
-        response = self.client.get(self.url)
-        assert response.status_code == 200
-
-        doc = pq(response.content)
-        return doc.find('.code-manager-links')
-
-    def test_link_to_contents(self):
-        links = self.get_links()
-
-        contents = links.find('a').eq(0)
-        assert contents.text() == "Contents"
-        assert contents.attr('href').endswith(
-            '/browse/{}/versions/{}/'.format(
-                self.addon.pk, self.version.pk
-            )
-        )
-
-        # There should only be one Contents link for the version.
-        assert links.find('a').length == 1
-
-    def test_link_to_version_comparison(self):
-        last_version = self.addon.current_version
-        last_version.files.update(status=amo.STATUS_APPROVED)
-        last_version.update(created=self.days_ago(2))
-
-        new_version = version_factory(addon=self.addon, version='0.2')
-        self.addon.update(_current_version=new_version)
-
-        links = self.get_links()
-
-        compare = links.find('a').eq(2)
-        assert compare.text() == "Compare"
-        assert compare.attr('href').endswith(
-            '/compare/{}/versions/{}...{}/'.format(
-                self.addon.pk,
-                last_version.pk,
-                new_version.pk,
-            )
-        )
-
-        # There should be three links:
-        # 1. The first version's Contents link
-        # 2. The second version's Contents link
-        # 3. The second version's Compare link
-        assert links.find('a').length == 3
-
-    def test_hide_links_when_flag_is_inactive(self):
-        with override_flag('code-manager', active=False):
-            assert self.get_links() == []
-
-
 class TestReviewPending(ReviewBase):

    def setUp(self):
--- a/src/olympia/reviewers/views.py
+++ b/src/olympia/reviewers/views.py
@ -822,7 +822,7 @@ def review(request, addon, channel=None):

    try:
        # Find the previously approved version to compare to.
-        show_diff = version and (
+        base_version = version and (
            addon.versions.exclude(id=version.id).filter(
                # We're looking for a version that was either manually approved
                # (either it has no auto approval summary, or it has one but
@ -839,7 +839,7 @@ def review(request, addon, channel=None):
                created__lt=version.created,
                files__status=amo.STATUS_APPROVED).latest())
    except Version.DoesNotExist:
-        show_diff = None
+        base_version = None

    # The actions we shouldn't show a minimal form for.
    actions_full = [
@ -917,7 +917,8 @@ def review(request, addon, channel=None):
        deleted_addon_ids=deleted_addon_ids, flags=flags,
        form=form, is_admin=is_admin, is_recommendable=is_recommendable,
        name_translations=name_translations, now=datetime.now(),
-        num_pages=num_pages, pager=pager, reports=reports, show_diff=show_diff,
+        num_pages=num_pages, pager=pager, reports=reports,
+        base_version=base_version,
        subscribed=ReviewerSubscription.objects.filter(
            user=request.user, addon=addon).exists(),
        unlisted=(channel == amo.RELEASE_CHANNEL_UNLISTED),
--- a/src/olympia/scanners/admin.py
+++ b/src/olympia/scanners/admin.py
@ -435,7 +435,9 @@ class AbstractScannerResultAdminMixin(admin.ModelAdmin):
                        'files': files_by_matched_rules[rule.name],
                    }
                    for rule in obj.matched_rules.all()
-                ]
+                ],
+                'addon_id': obj.version.addon.pk if obj.version else None,
+                'version_id': obj.version.pk if obj.version else None,
            },
        )

--- a/src/olympia/scanners/templates/admin/scanners/scannerresult/formatted_matched_rules_with_files.html
+++ b/src/olympia/scanners/templates/admin/scanners/scannerresult/formatted_matched_rules_with_files.html
@ -1,4 +1,5 @@
 {% load admin_urls %}
+{% load code_manager %}
 {# This template can be used for both scanner results and scanner query results. Keep it generic. #}

 <table>
@ -14,7 +15,9 @@
    <td>
      {% for file in rule.files %}
        {% if file_id %}
-          <a href="{{ external_site_url }}{% url 'files.list' file_id 'file' file %}">{{ file }}</a>
+          <a href="{% code_manager_url 'browse' addon_id version_id file=file %}">
+            {{ file }}
+          </a>
        {% else %}
          {{ file }}
        {% endif %}
--- a/src/olympia/scanners/templates/admin/scanners/scannerresult/formatted_matching_files.html
+++ b/src/olympia/scanners/templates/admin/scanners/scannerresult/formatted_matching_files.html
@ -1,4 +1,5 @@
 {% load admin_urls %}
+{% load code_manager %}
 {# This template can be used for both scanner results and scanner query results. Keep it generic. #}

 <ul>
@ -6,7 +7,7 @@
    {% for file in rule.files %}
      <li>
        {% if file_id %}
-          <a href="{{ external_site_url }}{% url 'files.list' file_id 'file' file %}">{{ file }}</a>
+          <a href="{% code_manager_url 'browse' addon_id version_id file=file %}">{{ file }}</a>
        {% else %}
          {{ file }}
        {% endif %}
--- a/src/olympia/scanners/tests/test_admin.py
+++ b/src/olympia/scanners/tests/test_admin.py
@ -38,6 +38,7 @@ from olympia.constants.scanners import (
    YARA,
 )
 from olympia.files.models import FileUpload
+from olympia.reviewers.templatetags.code_manager import code_manager_url
 from olympia.scanners.admin import (
    ExcludeMatchedRuleFilter,
    MatchesFilter,
@ -221,17 +222,13 @@ class TestScannerResultAdmin(TestCase):
        result.add_yara_result(rule=rule.name, meta={'filename': filename})
        result.save()

-        external_site_url = 'http://example.org'
        file_id = version.all_files[0].id
        assert file_id is not None
-        expect_file_item = '<a href="{}{}">{}</a>'.format(
-            external_site_url,
-            reverse('files.list', args=[file_id, 'file', filename]),
-            filename
-        )
-        with override_settings(EXTERNAL_SITE_URL=external_site_url):
-            assert (expect_file_item in
-                    self.admin.formatted_matched_rules_with_files(result))
+
+        expect_file_item = code_manager_url(
+            'browse', version.addon.pk, version.pk, file=filename)
+        assert (expect_file_item in
+                self.admin.formatted_matched_rules_with_files(result))

    def test_formatted_matched_rules_with_files_without_version(self):
        result = ScannerResult.objects.create(scanner=YARA)
@ -245,7 +242,7 @@ class TestScannerResultAdmin(TestCase):
                self.admin.formatted_matched_rules_with_files(result))
        # ...but we do not add a link to it because there is no associated
        # version.
-        assert ('/file/' not in
+        assert ('/browse/' not in
                self.admin.formatted_matched_rules_with_files(result))

    def test_formatted_score_when_scanner_is_not_mad_or_customs(self):
@ -1500,16 +1497,11 @@ class TestScannerQueryResultAdmin(TestCase):
        rule_url = reverse(
            'admin:scanners_scannerqueryrule_change', args=(rule.pk,))

-        external_site_url = 'http://example.org'
        file_id = version.all_files[0].id
        assert file_id is not None
-        expect_file_item = '<a href="{}{}">{}</a>'.format(
-            external_site_url,
-            reverse('files.list', args=[file_id, 'file', filename]),
-            filename
-        )
-        with override_settings(EXTERNAL_SITE_URL=external_site_url):
-            content = self.admin.formatted_matched_rules_with_files(result)
+        expect_file_item = code_manager_url(
+            'browse', version.addon.pk, version.pk, file=filename)
+        content = self.admin.formatted_matched_rules_with_files(result)
        assert expect_file_item in content
        assert rule_url in content

@ -1522,14 +1514,16 @@ class TestScannerQueryResultAdmin(TestCase):
        result1.add_yara_result(
            rule=rule.name, meta={'filename': 'some/file/somewhere.js'})
        result1.add_yara_result(
-            rule=rule.name, meta={'filename': 'another/file/somewhereelse.js'})
+            rule=rule.name,
+            meta={'filename': 'another/file/somewhereelse.js'})
        result1.save()
        result2 = ScannerQueryResult.objects.create(
            scanner=YARA, version=addon_factory().current_version,
            created=self.days_ago(1),
        )
        result2.add_yara_result(
-            rule=rule.name, meta={'filename': 'a/file/from/another_addon.js'})
+            rule=rule.name,
+            meta={'filename': 'a/file/from/another_addon.js'})
        result2.save()
        response = self.client.get(self.list_url)
        assert response.status_code == 200
@ -1537,15 +1531,15 @@ class TestScannerQueryResultAdmin(TestCase):
        links = doc('.field-matching_filenames a')
        assert len(links) == 3
        expected = [
-            'http://testserver{}'.format(reverse('files.list', args=[
-                result1.version.all_files[0].pk, 'file',
-                'some/file/somewhere.js'])),
-            'http://testserver{}'.format(reverse('files.list', args=[
-                result1.version.all_files[0].pk, 'file',
-                'another/file/somewhereelse.js'])),
-            'http://testserver{}'.format(reverse('files.list', args=[
-                result2.version.all_files[0].pk, 'file',
-                'a/file/from/another_addon.js'])),
+            code_manager_url(
+                'browse', result1.version.addon.pk, result1.version.pk,
+                file='some/file/somewhere.js'),
+            code_manager_url(
+                'browse', result1.version.addon.pk, result1.version.pk,
+                file='another/file/somewhereelse.js'),
+            code_manager_url(
+                'browse', result2.version.addon.pk, result2.version.pk,
+                file='a/file/from/another_addon.js'),
        ]
        assert [link.attrib['href'] for link in links] == expected

--- a/static/img/reviewers/new.gif
+++ b/static/img/reviewers/new.gif