RE: Add validations to Shelf model (#14998)

* Draft pull request - validator for criteria

* Updated validation for criteria; revised models, test_admin, conftest

* Added additional criteria validation tests to test_admin

* Replaced validator with clean_criteria method

* Removed responses for shelves

* Revised clean_criteria method and added tests to test_admin.py

* Revised shelf types in models.py

* Added message to inform admin the total addons returned after successfully saving a shelf

* Updated success message and respective tests

* Created test_forms, revised forms, admin, test admin

* Updated settings to add INTERNAL_SITE_URL and nginx to ALLOWED_HOSTS

* Revised baseUrl, removed results/save method - to add later

* Revised settings (dev, stage, prod) to move INTERNAL_SITE_URL near EXTERNAL_SITE_URL, moved 'nginx' to list from settings_base.py

* Formatting

settings.py

@@ -76,12 +76,13 @@ ES_DEFAULT_NUM_REPLICAS = 0
 SITE_URL = os.environ.get('OLYMPIA_SITE_URL') or 'http://localhost:8000'
 DOMAIN = SERVICES_DOMAIN = urlparse(SITE_URL).netloc
 SERVICES_URL = SITE_URL
+INTERNAL_SITE_URL = 'http://nginx'
 EXTERNAL_SITE_URL = SITE_URL
 
 CODE_MANAGER_URL = (
     os.environ.get('CODE_MANAGER_URL') or 'http://localhost:3000')
 
-ALLOWED_HOSTS = ALLOWED_HOSTS + [SERVICES_DOMAIN]
+ALLOWED_HOSTS = ALLOWED_HOSTS + [SERVICES_DOMAIN, 'nginx']
 
 # Default AMO user id to use for tasks (from users.json fixture in zadmin).
 TASK_USER_ID = 10968

src/olympia/conf/dev/settings.py

@@ -32,6 +32,8 @@ API_THROTTLING = False
 DOMAIN = env('DOMAIN', default='addons-dev.allizom.org')
 SERVER_EMAIL = 'zdev@addons.mozilla.org'
 SITE_URL = 'https://' + DOMAIN
+INTERNAL_SITE_URL = env('INTERNAL_SITE_URL',
+                        default='https://addons-dev.allizom.org')
 EXTERNAL_SITE_URL = env('EXTERNAL_SITE_URL',
                         default='https://addons-dev.allizom.org')
 SERVICES_URL = env('SERVICES_URL',

src/olympia/conf/prod/settings.py

@@ -20,6 +20,8 @@ CDN_HOST = 'https://addons.cdn.mozilla.net'
 DOMAIN = env('DOMAIN', default='addons.mozilla.org')
 SERVER_EMAIL = 'zprod@addons.mozilla.org'
 SITE_URL = 'https://' + DOMAIN
+INTERNAL_SITE_URL = env('INTERNAL_SITE_URL',
+                        default='https://addons.mozilla.org')
 EXTERNAL_SITE_URL = env('EXTERNAL_SITE_URL',
                         default='https://addons.mozilla.org')
 SERVICES_URL = env('SERVICES_URL',

src/olympia/conf/stage/settings.py

@@ -30,6 +30,8 @@ API_THROTTLING = True
 DOMAIN = env('DOMAIN', default='addons.allizom.org')
 SERVER_EMAIL = 'zstage@addons.mozilla.org'
 SITE_URL = 'https://' + DOMAIN
+INTERNAL_SITE_URL = env('INTERNAL_SITE_URL',
+                        default='https://addons.allizom.org')
 EXTERNAL_SITE_URL = env('EXTERNAL_SITE_URL',
                         default='https://addons.allizom.org')
 SERVICES_URL = env('SERVICES_URL',

src/olympia/discovery/tests/test_admin.py

@@ -1,14 +1,17 @@
 # -*- coding: utf-8 -*-
 from pyquery import PyQuery as pq
+from unittest import mock
 
 import os
+import responses
 
 from olympia.amo.storage_utils import copy_stored_file
-from olympia.amo.tests import TestCase, addon_factory, user_factory
+from olympia.amo.tests import TestCase, addon_factory, reverse_ns, user_factory
 from olympia.amo.tests.test_helpers import get_uploaded_file
 from olympia.amo.urlresolvers import django_reverse, reverse
 from django.conf import settings
 from django.core.files.images import get_image_dimensions
+
 from olympia.discovery.models import DiscoveryItem
 from olympia.hero.models import (
     PrimaryHeroImage, SecondaryHero, SecondaryHeroModule)
@@ -890,6 +893,15 @@ class TestShelfAdmin(TestCase):
         self.list_url = reverse(
             'admin:discovery_shelfmodule_changelist')
         self.detail_url_name = 'admin:discovery_shelfmodule_change'
+        self.success_message = (
+            '"Recommended extensions" was changed successfully.')
+
+        criteria_sea = '?recommended=true&sort=random&type=extension'
+        responses.add(
+            responses.GET,
+            reverse_ns('addon-search') + criteria_sea,
+            status=200,
+            json={'count': 103})
 
     def test_can_see_shelf_module_in_admin_with_discovery_edit(self):
         user = user_factory()
@@ -918,9 +930,9 @@ class TestShelfAdmin(TestCase):
 
     def test_can_edit_with_discovery_edit_permission(self):
         item = Shelf.objects.create(
-            title='Recommended extensions',
+            title='Popular extensions',
             shelf_type='extension',
-            criteria='/this/is/the/criteria',
+            criteria='?sort=users&type=extension',
             footer_text='See more',
             footer_pathname='/this/is/the/pathname')
         detail_url = reverse(self.detail_url_name, args=(item.pk,))
@@ -931,32 +943,39 @@ class TestShelfAdmin(TestCase):
         response = self.client.get(detail_url, follow=True)
         assert response.status_code == 200
         content = response.content.decode('utf-8')
-        assert 'Recommended extensions' in content
+        assert 'Popular extensions' in content
 
-        response = self.client.post(
+        with mock.patch('olympia.shelves.forms.ShelfForm.clean') as mock_clean:
-            detail_url,
+            mock_clean.return_value = {
-            {
+                'title': 'Recommended extensions',
-                'title': 'Popular extensions',
                 'shelf_type': 'extension',
-                'criteria': '/this/is/the/criteria',
+                'criteria': (
+                    '?recommended=true&sort=random&type=extension'),
                 'footer_text': 'See more',
                 'footer_pathname': '/this/is/the/pathname'
-            }, follow=True)
+            }
-        assert response.status_code == 200
+
-        item.reload()
+            response = self.client.post(
-        assert Shelf.objects.count() == 1
+                detail_url,
-        assert item.title == 'Popular extensions'
+                mock_clean.return_value,
+                follow=True)
+            assert response.status_code == 200
+            item.reload()
+            assert Shelf.objects.count() == 1
+            assert item.title == 'Recommended extensions'
+            assert item.shelf_type == 'extension'
+            assert item.criteria == (
+                '?recommended=true&sort=random&type=extension')
 
     def test_can_delete_with_discovery_edit_permission(self):
         item = Shelf.objects.create(
             title='Recommended extensions',
             shelf_type='extension',
-            criteria='/this/is/the/criteria',
+            criteria='?recommended=true&sort=random&type=extension',
             footer_text='See more',
             footer_pathname='/this/is/the/pathname')
         delete_url = reverse(
-            'admin:discovery_shelfmodule_delete', args=(item.pk,)
+            'admin:discovery_shelfmodule_delete', args=(item.pk,))
-        )
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Discovery:Edit')
@@ -981,20 +1000,29 @@ class TestShelfAdmin(TestCase):
         response = self.client.get(add_url, follow=True)
         assert response.status_code == 200
         assert Shelf.objects.count() == 0
-        response = self.client.post(
+
-            add_url,
+        with mock.patch('olympia.shelves.forms.ShelfForm.clean') as mock_clean:
-            {
+            mock_clean.return_value = {
                 'title': 'Recommended extensions',
                 'shelf_type': 'extension',
-                'criteria': '/this/is/the/criteria',
+                'criteria':
+                    ('?recommended=true&sort=random&type=extension'),
                 'footer_text': 'See more',
-                'footer_pathname': '/this/is/the/pathname'
+                'footer_pathname': '/this/is/the/pathname',
-            },
+            }
-            follow=True)
+
-        assert response.status_code == 200
+            response = self.client.post(
-        assert Shelf.objects.count() == 1
+                add_url,
-        item = Shelf.objects.get()
+                mock_clean.return_value,
-        assert item.title == 'Recommended extensions'
+                follow=True)
+
+            assert response.status_code == 200
+            assert Shelf.objects.count() == 1
+            item = Shelf.objects.get()
+            assert item.title == 'Recommended extensions'
+            assert item.shelf_type == 'extension'
+            assert item.criteria == (
+                '?recommended=true&sort=random&type=extension')
 
     def test_can_not_add_without_discovery_edit_permission(self):
         add_url = reverse('admin:discovery_shelfmodule_add')
@@ -1008,7 +1036,8 @@ class TestShelfAdmin(TestCase):
             {
                 'title': 'Recommended extensions',
                 'shelf_type': 'extension',
-                'criteria': '/this/is/the/criteria',
+                'criteria':
+                '?recommended=true&sort=random&type=extension',
                 'footer_text': 'See more',
                 'footer_pathname': '/this/is/the/pathname'
             },
@@ -1020,12 +1049,11 @@ class TestShelfAdmin(TestCase):
         item = Shelf.objects.create(
             title='Recommended extensions',
             shelf_type='extension',
-            criteria='/this/is/the/criteria',
+            criteria='?recommended=true&sort=random&type=extension',
             footer_text='See more',
             footer_pathname='/this/is/the/pathname')
         detail_url = reverse(
-            'admin:discovery_shelfmodule_change', args=(item.pk,)
+            'admin:discovery_shelfmodule_change', args=(item.pk,))
-        )
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.client.login(email=user.email)
@@ -1037,7 +1065,8 @@ class TestShelfAdmin(TestCase):
             {
                 'title': 'Popular extensions',
                 'shelf_type': 'extension',
-                'criteria': '/this/is/the/criteria',
+                'criteria':
+                '?recommended=true&sort=users&type=extension',
                 'footer_text': 'See more',
                 'footer_pathname': '/this/is/the/pathname'
             }, follow=True)
@@ -1050,12 +1079,11 @@ class TestShelfAdmin(TestCase):
         item = Shelf.objects.create(
             title='Recommended extensions',
             shelf_type='extension',
-            criteria='/this/is/the/criteria',
+            criteria='?recommended=true&sort=random&type=extension',
             footer_text='See more',
             footer_pathname='/this/is/the/pathname')
         delete_url = reverse(
-            'admin:discovery_shelfmodule_delete', args=(item.pk,)
+            'admin:discovery_shelfmodule_delete', args=(item.pk,))
-        )
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.client.login(email=user.email)

src/olympia/shelves/admin.py

@@ -1,6 +1,9 @@
 from django.contrib import admin
 
+from .forms import ShelfForm
+
 
 class ShelfAdmin(admin.ModelAdmin):
     list_display = ('title', 'shelf_type')
     actions = ['delete_selected']
+    form = ShelfForm

src/olympia/shelves/forms.py

@@ -0,0 +1,53 @@
+import requests
+
+from rest_framework.reverse import reverse as drf_reverse
+
+from django import forms
+from django.conf import settings
+
+from olympia.shelves.models import Shelf
+
+
+class ShelfForm(forms.ModelForm):
+    class Meta:
+        model = Shelf
+        fields = ('title', 'shelf_type', 'criteria',
+                  'footer_text', 'footer_pathname',)
+
+    def clean(self):
+        data = self.cleaned_data
+        criteria = data['criteria']
+        baseUrl = settings.INTERNAL_SITE_URL
+
+        if data['shelf_type'] in ('extension', 'search', 'theme'):
+            api = drf_reverse('v4:addon-search')
+        elif data['shelf_type'] == 'categories':
+            api = drf_reverse('v4:category-list')
+        elif data['shelf_type'] == 'collections':
+            api = drf_reverse('v4:collection-list')
+        elif data['shelf_type'] == 'recommendations':
+            api = drf_reverse('v4:addon-recommendations')
+
+        url = baseUrl + api + criteria
+
+        try:
+            response = requests.get(url)
+            results = response.json()
+
+            if response.status_code == 404:
+                raise forms.ValidationError('Check criteria - No data found')
+            if response.status_code != 200:
+                raise forms.ValidationError(
+                    'Check criteria - %s' % results[0])
+
+            # Value of results is either dict or list depending on the endpoint
+            if 'count' in results:
+                if results.get('count', 0) == 0:
+                    raise forms.ValidationError(
+                        'Check criteria parameters - e.g., "type"')
+            else:
+                if len(results) == 0:
+                    raise forms.ValidationError(
+                        'Check criteria - No data found')
+        except requests.exceptions.ConnectionError:
+            raise forms.ValidationError('Connection Error')

src/olympia/shelves/migrations/0003_auto_20200720_1509.py

@@ -0,0 +1,24 @@
+# Generated by Django 2.2.14 on 2020-07-20 15:09
+
+from django.db import migrations, models
+import olympia.shelves.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('shelves', '0002_auto_20200716_1254'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='shelf',
+            name='criteria',
+            field=models.CharField(help_text='e.g., ?recommended=true&sort=random&type=extension', max_length=200,),
+        ),
+        migrations.AlterField(
+            model_name='shelf',
+            name='shelf_type',
+            field=models.CharField(choices=[('categories', 'categories'), ('collections', 'collections'), ('extension', 'extension'), ('recommendations', 'recommendations'), ('search', 'search'), ('theme', 'theme')], max_length=200, verbose_name='type'),
+        ),
+    ]

src/olympia/shelves/models.py

@@ -2,8 +2,8 @@ from django.db import models
 
 from olympia.amo.models import ModelBase
 
-SHELF_TYPES = (
+SHELF_TYPES = ('categories', 'collections', 'extension',
-    'category', 'collection', 'extension', 'recommended', 'search', 'theme')
+               'recommendations', 'search', 'theme')
 
 SHELF_TYPE_CHOICES = tuple((ty, ty) for ty in SHELF_TYPES)
 
@@ -13,8 +13,8 @@ class Shelf(ModelBase):
     shelf_type = models.CharField(
         max_length=200, choices=SHELF_TYPE_CHOICES, verbose_name='type')
     criteria = models.CharField(
-        max_length=200, blank=True,
+        max_length=200,
-        help_text="e.g., search/?recommended=true&sort=random&type=extension")
+        help_text="e.g., ?recommended=true&sort=random&type=extension")
     footer_text = models.CharField(
         max_length=200, blank=True,
         help_text="e.g., See more recommended extensions")

src/olympia/shelves/tests/test_forms.py

@@ -0,0 +1,118 @@
+import responses
+
+from rest_framework.reverse import reverse as drf_reverse
+
+from django.conf import settings
+from django.core.exceptions import ValidationError
+
+from olympia.amo.tests import TestCase
+from ..forms import ShelfForm
+
+
+class TestShelfForm(TestCase):
+    def setUp(self):
+        self.criteria_sea = '?recommended=true&sort=random&type=extension'
+        self.criteria_cat = '?slug=alerts-updates'
+        self.criteria_rec = '?recommended=true'
+        self.criteria_404 = 'sort=users&type=statictheme'
+        self.criteria_not_200 = '?sort=user&type=statictheme'
+        self.criteria_empty = '?sort=users&type=theme'
+        baseUrl = settings.INTERNAL_SITE_URL
+
+        responses.add(
+            responses.GET,
+            baseUrl + drf_reverse('v4:addon-search') +
+            self.criteria_sea,
+            status=200,
+            json={'count': 103})
+        responses.add(
+            responses.GET,
+            baseUrl + drf_reverse('v4:category-list') +
+            self.criteria_cat,
+            status=200,
+            json=[{'id': 1}, {'id': 2}])
+        responses.add(
+            responses.GET,
+            baseUrl + drf_reverse('v4:addon-recommendations') +
+            self.criteria_rec,
+            status=200,
+            json={'count': 4})
+        responses.add(
+            responses.GET,
+            baseUrl + drf_reverse('v4:addon-search') +
+            self.criteria_404,
+            status=404,
+            json={"detail": "Not found."}),
+        responses.add(
+            responses.GET,
+            baseUrl + drf_reverse('v4:addon-search') +
+            self.criteria_not_200,
+            status=400,
+            json=['Invalid \"sort\" parameter.'])
+        responses.add(
+            responses.GET,
+            baseUrl + drf_reverse('v4:addon-search') +
+            self.criteria_empty,
+            status=200,
+            json={'count': 0})
+
+    def test_clean_search(self):
+        form = ShelfForm({
+            'title': 'Recommended extensions',
+            'shelf_type': 'extension',
+            'criteria': self.criteria_sea})
+        assert form.is_valid(), form.errors
+        assert form.cleaned_data['criteria'] == (
+            '?recommended=true&sort=random&type=extension')
+
+    def test_clean_categories(self):
+        form = ShelfForm({
+            'title': 'Alerts & Updates (Categories)',
+            'shelf_type': 'categories',
+            'criteria': self.criteria_cat})
+        assert form.is_valid(), form.errors
+        assert form.cleaned_data['criteria'] == '?slug=alerts-updates'
+
+    def test_clean_recommendations(self):
+        form = ShelfForm({
+            'title': 'Recommended Add-ons',
+            'shelf_type': 'recommendations',
+            'criteria': self.criteria_rec})
+        assert form.is_valid(), form.errors
+        assert form.cleaned_data['criteria'] == '?recommended=true'
+
+    def test_clean_returns_404(self):
+        data = {
+            'title': 'Popular themes',
+            'shelf_type': 'theme',
+            'criteria': self.criteria_404}
+        form = ShelfForm(data)
+        form.is_valid()
+        with self.assertRaises(ValidationError) as exc:
+            form.clean()
+        assert exc.exception.message == (
+            u'Check criteria - No data found')
+
+    def test_clean_returns_not_200(self):
+        data = {
+            'title': 'Popular themes',
+            'shelf_type': 'theme',
+            'criteria': self.criteria_not_200}
+        form = ShelfForm(data)
+        form.is_valid()
+        with self.assertRaises(ValidationError) as exc:
+            form.clean()
+        assert exc.exception.message == (
+            u'Check criteria - Invalid \"sort\" parameter.')
+
+    def test_clean_returns_empty(self):
+        data = {
+            'title': 'Popular themes',
+            'shelf_type': 'theme',
+            'criteria': self.criteria_empty}
+        form = ShelfForm(data)
+        form.is_valid()
+        with self.assertRaises(ValidationError) as exc:
+            form.clean()
+        assert exc.exception.message == (
+            u'Check criteria parameters - e.g., "type"')