RE: Add addon_type property to shelf module, remove search-themes shelf endpoint type (#16967)

* Remove search-themes endpoint choice, added addon_type field

* Updated test_admin.py

* Updated doc, models, forms (and test), test_serializers, test_admin, migration file

* Fixed lint issues

* Updated serializer to reflect addon_type choice name instead of integer

* Fixed lint issues

* Updated addon_type from int in test_admin
This commit is contained in:
Lisa Chan 2021-04-21 12:30:41 -04:00 коммит произвёл GitHub
Родитель 5ca3557581
Коммит 4e5dc5cdcb
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
8 изменённых файлов: 143 добавлений и 72 удалений

Просмотреть файл

@ -100,6 +100,7 @@ small number of shelves this endpoint is not paginated.
:>json object|null results[].headline: The title of the shelf. (See :ref:`translated fields <api-overview-translations>`. Note: when ``lang`` is not specified, not all locales will be returned, unlike other translated fields).
:>json string results[].url: The configured URL using the shelf's endpoint and criteria; links to the shelf's returned add-ons.
:>json string results[].endpoint: The :ref:`endpoint type <shelf-endpoint-type>` selected for the shelf.
:>json string results[].addon_type: The :ref:`add-on type <addon-detail-type>` selected for the shelf.
:>json string results[].criteria: The criteria for the addons in the shelf.
:>json object|null results[].footer: The optional footer to be displayed with the shelf.
:>json string results[].footer.url: The optional url for the footer text.
@ -113,14 +114,13 @@ small number of shelves this endpoint is not paginated.
Possible values for the ``endpoint`` field:
============== ====================================================================
============== ====================================================
Value Description
============== ====================================================================
search an :ref:`addon search<addon-search>`, excluding ``?type=statictheme``
search-themes an :ref:`addon search<addon-search>`, with ``?type=statictheme``
============== ====================================================
search an :ref:`addon search<addon-search>`
collections a mozilla :ref:`collection<collection-addon-list>`.
The collection slug will be in ``criteria``
============== ====================================================================
============== ====================================================
----------------------------------

Просмотреть файл

@ -7,6 +7,7 @@ from django.urls import reverse
import responses
from pyquery import PyQuery as pq
from olympia import amo
from olympia.amo.storage_utils import copy_stored_file
from olympia.amo.tests import TestCase, addon_factory, reverse_ns, user_factory
from olympia.amo.tests.test_helpers import get_uploaded_file
@ -881,6 +882,7 @@ class TestShelfAdmin(TestCase):
item = Shelf.objects.create(
title='Popular extensions',
endpoint='search',
addon_type=amo.ADDON_EXTENSION,
criteria='?sort=users&type=extension',
footer_text='See more',
footer_pathname='/this/is/the/pathname',
@ -898,6 +900,7 @@ class TestShelfAdmin(TestCase):
mock_clean.return_value = {
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': ('?promoted=recommended&sort=random&type=extension'),
'footer_text': 'See more',
'footer_pathname': '/this/is/the/pathname',
@ -912,6 +915,7 @@ class TestShelfAdmin(TestCase):
assert Shelf.objects.count() == 1
assert item.title == 'Recommended extensions'
assert item.endpoint == 'search'
assert item.addon_type == amo.ADDON_EXTENSION
assert item.criteria == ('?promoted=recommended&sort=random&type=extension')
assert item.addon_count == item.get_count() == 2
@ -919,6 +923,7 @@ class TestShelfAdmin(TestCase):
item = Shelf.objects.create(
title='Popular extensions',
endpoint='search',
addon_type=amo.ADDON_EXTENSION,
criteria='?sort=users&type=extension',
footer_text='See more',
footer_pathname='/this/is/the/pathname',
@ -930,6 +935,7 @@ class TestShelfAdmin(TestCase):
data = {
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': ('?promoted=recommended&sort=random&type=extension'),
'footer_text': 'See more',
'footer_pathname': '/this/is/the/pathname',
@ -962,6 +968,7 @@ class TestShelfAdmin(TestCase):
item = Shelf.objects.create(
title='Recommended extensions',
endpoint='search',
addon_type=amo.ADDON_EXTENSION,
criteria='?promoted=recommended&sort=random&type=extension',
footer_text='See more',
footer_pathname='/this/is/the/pathname',
@ -993,6 +1000,7 @@ class TestShelfAdmin(TestCase):
mock_clean.return_value = {
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': ('?promoted=recommended&sort=random&type=extension'),
'footer_text': 'See more',
'footer_pathname': '/this/is/the/pathname',
@ -1006,6 +1014,7 @@ class TestShelfAdmin(TestCase):
item = Shelf.objects.get()
assert item.title == 'Recommended extensions'
assert item.endpoint == 'search'
assert item.addon_type == amo.ADDON_EXTENSION
assert item.criteria == ('?promoted=recommended&sort=random&type=extension')
def test_can_not_add_without_discovery_edit_permission(self):
@ -1019,6 +1028,7 @@ class TestShelfAdmin(TestCase):
{
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': '?promoted=recommended&sort=random&type=extension',
'footer_text': 'See more',
'footer_pathname': '/this/is/the/pathname',
@ -1033,6 +1043,7 @@ class TestShelfAdmin(TestCase):
item = Shelf.objects.create(
title='Recommended extensions',
endpoint='search',
addon_type=amo.ADDON_EXTENSION,
criteria='?promoted=recommended&sort=random&type=extension',
footer_text='See more',
footer_pathname='/this/is/the/pathname',
@ -1048,6 +1059,7 @@ class TestShelfAdmin(TestCase):
{
'title': 'Popular extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': '?promoted=recommended&sort=users&type=extension',
'footer_text': 'See more',
'footer_pathname': '/this/is/the/pathname',
@ -1064,6 +1076,7 @@ class TestShelfAdmin(TestCase):
item = Shelf.objects.create(
title='Recommended extensions',
endpoint='search',
addon_type=amo.ADDON_EXTENSION,
criteria='?promoted=recommended&sort=random&type=extension',
footer_text='See more',
footer_pathname='/this/is/the/pathname',
@ -1090,6 +1103,7 @@ class TestHomepageShelvesAdmin(TestCase):
self.shelf = Shelf.objects.create(
title='Populâr themes',
endpoint='search',
addon_type=amo.ADDON_STATICTHEME,
criteria='?sort=users&type=statictheme',
)

Просмотреть файл

@ -7,6 +7,7 @@ from django.conf import settings
from django.urls import NoReverseMatch, reverse
import olympia.core.logger
from olympia import amo
from olympia.shelves.models import Shelf
@ -19,6 +20,7 @@ class ShelfForm(forms.ModelForm):
fields = (
'title',
'endpoint',
'addon_type',
'criteria',
'addon_count',
'footer_text',
@ -30,24 +32,27 @@ class ShelfForm(forms.ModelForm):
base_url = settings.INTERNAL_SITE_URL
endpoint = data.get('endpoint')
addon_type = data.get('addon_type')
criteria = data.get('criteria')
if criteria is None:
return
if endpoint in ('search', 'search-themes'):
params = criteria[1:].split('&')
if addon_type == amo.ADDON_EXTENSION and 'type=statictheme' in params:
raise forms.ValidationError(
'Use "Theme (Static)" in Addon type field for type=statictheme.'
)
elif addon_type == amo.ADDON_STATICTHEME and 'type=statictheme' not in params:
raise forms.ValidationError(
'Check fields - for "Theme (Static)" addon type, use type=statictheme. '
'For non theme addons, use "Extension" in Addon type field, '
'not "Theme (Static)".'
)
if endpoint == 'search':
if not criteria.startswith('?') or criteria.count('?') > 1:
raise forms.ValidationError('Check criteria field.')
params = criteria[1:].split('&')
if endpoint == 'search' and 'type=statictheme' in params:
raise forms.ValidationError(
'Use "search-themes" endpoint for type=statictheme.'
)
elif endpoint == 'search-themes' and 'type=statictheme' not in params:
raise forms.ValidationError(
'Don`t use "search-themes" endpoint for non themes. '
'Use "search".'
)
api = reverse(f'{api_settings.DEFAULT_VERSION}:addon-search')
url = base_url + api + criteria
elif endpoint == 'collections':

Просмотреть файл

@ -0,0 +1,28 @@
# Generated by Django 2.2.20 on 2021-04-16 22:25
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('shelves', '0013_auto_20210325_1320'),
]
operations = [
migrations.AddField(
model_name='shelf',
name='addon_type',
field=models.PositiveIntegerField(choices=[(1, 'Extension'), (2, 'Deprecated Complete Theme'), (3, 'Dictionary'), (4, 'Search Engine'), (5, 'Language Pack (Application)'), (6, 'Language Pack (Add-on)'), (7, 'Plugin'), (9, 'Deprecated LWT'), (10, 'Theme (Static)')], db_column='addontype_id', default=1),
),
migrations.AlterField(
model_name='shelf',
name='addon_count',
field=models.PositiveSmallIntegerField(default=0, help_text='0 means the default number (4, or 3 for themes) of add-ons will be included in shelf responses. Set to override.'),
),
migrations.AlterField(
model_name='shelf',
name='endpoint',
field=models.CharField(choices=[('collections', 'collections'), ('search', 'search')], db_column='shelf_type', max_length=200),
),
]

Просмотреть файл

@ -1,8 +1,9 @@
from django.db import models
from olympia import amo
from olympia.amo.models import ModelBase
ENDPOINTS = ('collections', 'search', 'search-themes')
ENDPOINTS = ('collections', 'search')
ENDPOINT_CHOICES = tuple((ty, ty) for ty in ENDPOINTS)
@ -29,9 +30,14 @@ class Shelf(ModelBase):
)
addon_count = models.PositiveSmallIntegerField(
default=0,
help_text='0 means the default number (4, or 3 for search-themes) of add-ons '
help_text='0 means the default number (4, or 3 for themes) of add-ons '
'will be included in shelf responses. Set to override.',
)
addon_type = models.PositiveIntegerField(
choices=amo.ADDON_TYPE.items(),
db_column='addontype_id',
default=amo.ADDON_EXTENSION,
)
class Meta:
verbose_name_plural = 'shelves'
@ -40,7 +46,9 @@ class Shelf(ModelBase):
return self.title
def get_count(self):
return self.addon_count or (3 if self.endpoint in ('search-themes',) else 4)
return self.addon_count or (
3 if self.addon_type == amo.ADDON_STATICTHEME else 4
)
class ShelfManagement(ModelBase):

Просмотреть файл

@ -6,10 +6,12 @@ from rest_framework import serializers
from rest_framework.request import Request as DRFRequest
from rest_framework.reverse import reverse as drf_reverse
from olympia import amo
from olympia.addons.views import AddonSearchView
from olympia.api.fields import (
AbsoluteOutgoingURLField,
GetTextTranslationSerializerField,
ReverseChoiceField,
)
from olympia.bandwagon.views import CollectionAddonViewSet
from olympia.hero.serializers import CTAField
@ -27,6 +29,7 @@ class ShelfSerializer(serializers.ModelSerializer):
url = serializers.SerializerMethodField()
footer = ShelfFooterField(source='*')
addons = serializers.SerializerMethodField()
addon_type = ReverseChoiceField(choices=list(amo.ADDON_TYPE_CHOICES_API.items()))
class Meta:
model = Shelf
@ -34,13 +37,14 @@ class ShelfSerializer(serializers.ModelSerializer):
'title',
'url',
'endpoint',
'addon_type',
'criteria',
'footer',
'addons',
]
def get_url(self, obj):
if obj.endpoint in ('search', 'search-themes'):
if obj.endpoint == 'search':
api = drf_reverse('addon-search', request=self.context.get('request'))
url = api + obj.criteria
elif obj.endpoint == 'collections':
@ -67,7 +71,7 @@ class ShelfSerializer(serializers.ModelSerializer):
orginal_get = real_request.GET
real_request.GET = real_request.GET.copy()
if obj.endpoint in ('search', 'search-themes'):
if obj.endpoint == 'search':
criteria = obj.criteria.strip('?')
params = dict(parse.parse_qsl(criteria))
request.GET.update(params)

Просмотреть файл

@ -3,6 +3,7 @@ import responses
from django.conf import settings
from django.core.exceptions import ValidationError
from olympia import amo
from olympia.amo.tests import TestCase, reverse_ns
from olympia.shelves.forms import ShelfForm
@ -10,10 +11,10 @@ from olympia.shelves.forms import ShelfForm
class TestShelfForm(TestCase):
def setUp(self):
self.criteria_sea = '?promoted=recommended&sort=random&type=extension'
self.criteria_theme = '?sort=users&type=statictheme'
self.criteria_col = 'password-managers'
self.criteria_col_404 = 'passwordmanagers'
self.criteria_not_200 = '?sort=user&type=extension'
self.criteria_empty = '?sort=users&type=theme'
responses.add(
responses.GET,
@ -51,18 +52,13 @@ class TestShelfForm(TestCase):
status=400,
json=['Invalid "sort" parameter.'],
)
responses.add(
responses.GET,
reverse_ns('addon-search') + self.criteria_empty,
status=200,
json={'count': 0},
)
def test_clean_search(self):
form = ShelfForm(
{
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_sea,
'addon_count': '0',
},
@ -77,6 +73,7 @@ class TestShelfForm(TestCase):
{
'title': 'Password managers (Collections)',
'endpoint': 'collections',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_col,
'addon_count': '0',
},
@ -89,6 +86,7 @@ class TestShelfForm(TestCase):
{
'title': '',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_sea,
'addon_count': '0',
},
@ -101,6 +99,7 @@ class TestShelfForm(TestCase):
{
'title': 'Recommended extensions',
'endpoint': '',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_sea,
'addon_count': '0',
},
@ -108,10 +107,24 @@ class TestShelfForm(TestCase):
assert not form.is_valid()
assert form.errors == {'endpoint': ['This field is required.']}
def test_clean_form_is_missing_addon_type_field(self):
form = ShelfForm(
{
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': '',
'criteria': self.criteria_sea,
'addon_count': '0',
},
)
assert not form.is_valid()
assert form.errors == {'addon_type': ['This field is required.']}
def test_clean_form_is_missing_addon_count_field(self):
data = {
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_sea,
}
form = ShelfForm(data)
@ -137,6 +150,7 @@ class TestShelfForm(TestCase):
{
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': '',
'addon_count': '0',
},
@ -149,6 +163,7 @@ class TestShelfForm(TestCase):
{
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': '..?recommended-true',
'addon_count': '0',
},
@ -163,6 +178,7 @@ class TestShelfForm(TestCase):
{
'title': 'Recommended extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': '??recommended-true',
'addon_count': '0',
},
@ -172,43 +188,12 @@ class TestShelfForm(TestCase):
form.clean()
assert exc.exception.message == ('Check criteria field.')
def test_clean_searchtheme_criteria_theme_used_for_statictheme_type(self):
form = ShelfForm(
{
'title': 'Recommended extensions',
'endpoint': 'search',
'criteria': '?recommended=true&type=statictheme',
'addon_count': '0',
}
)
assert not form.is_valid()
with self.assertRaises(ValidationError) as exc:
form.clean()
assert exc.exception.message == (
'Use "search-themes" endpoint for type=statictheme.'
)
def test_clean_searchtheme_criteria_theme_not_used_for_other_type(self):
form = ShelfForm(
{
'title': 'Recommended extensions',
'endpoint': 'search-themes',
'criteria': '?recommended=true&type=extension',
'addon_count': '0',
}
)
assert not form.is_valid()
with self.assertRaises(ValidationError) as exc:
form.clean()
assert exc.exception.message == (
'Don`t use "search-themes" endpoint for non themes. Use "search".'
)
def test_clean_form_throws_error_for_NoReverseMatch(self):
form = ShelfForm(
{
'title': 'New collection',
'endpoint': 'collections',
'addon_type': amo.ADDON_EXTENSION,
'criteria': '/',
'addon_count': '0',
},
@ -224,6 +209,7 @@ class TestShelfForm(TestCase):
data = {
'title': 'Password manager (Collections)',
'endpoint': 'collections',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_col_404,
'addon_count': '0',
}
@ -235,8 +221,9 @@ class TestShelfForm(TestCase):
def test_clean_returns_not_200(self):
data = {
'title': 'Popular themes',
'title': 'Popular extensions',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_not_200,
'addon_count': '0',
}
@ -246,11 +233,12 @@ class TestShelfForm(TestCase):
form.clean()
assert exc.exception.message == ('Check criteria - Invalid "sort" parameter.')
def test_clean_returns_empty(self):
def test_clean_themes_addontype_used_for_statictheme_type(self):
data = {
'title': 'Popular themes',
'title': 'Recommended extensions',
'endpoint': 'search',
'criteria': self.criteria_empty,
'addon_type': amo.ADDON_STATICTHEME,
'criteria': self.criteria_sea,
'addon_count': '0',
}
form = ShelfForm(data)
@ -258,5 +246,23 @@ class TestShelfForm(TestCase):
with self.assertRaises(ValidationError) as exc:
form.clean()
assert exc.exception.message == (
'No add-ons found. Check criteria parameters - e.g., "type"'
'Check fields - for "Theme (Static)" addon type, use type=statictheme. '
'For non theme addons, use "Extension" in Addon type field, '
'not "Theme (Static)".'
)
def test_clean_extensions_addontype_not_used_for_statictheme_type(self):
data = {
'title': 'Popular themes',
'endpoint': 'search',
'addon_type': amo.ADDON_EXTENSION,
'criteria': self.criteria_theme,
'addon_count': '0',
}
form = ShelfForm(data)
assert not form.is_valid()
with self.assertRaises(ValidationError) as exc:
form.clean()
assert exc.exception.message == (
'Use "Theme (Static)" in Addon type field for type=statictheme.'
)

Просмотреть файл

@ -98,7 +98,8 @@ class TestShelvesSerializer(ESTestCase):
def setUp(self):
self.search_pop_thm = Shelf.objects.create(
title='Populâr themes',
endpoint='search-themes',
endpoint='search',
addon_type=amo.ADDON_STATICTHEME,
criteria='?sort=users&type=statictheme',
footer_text='See more populâr themes',
footer_pathname='/themes/',
@ -107,6 +108,7 @@ class TestShelvesSerializer(ESTestCase):
self.search_rec_ext = Shelf.objects.create(
title='Recommended extensions',
endpoint='search',
addon_type=amo.ADDON_EXTENSION,
criteria='?promoted=recommended&sort=random&type=extension',
footer_text='See more recommended extensions',
)
@ -114,6 +116,7 @@ class TestShelvesSerializer(ESTestCase):
self.collections_shelf = Shelf.objects.create(
title='Enhanced privacy extensions',
endpoint='collections',
addon_type=amo.ADDON_EXTENSION,
criteria='privacy-matters',
footer_text='See more enhanced privacy extensions',
footer_pathname='https://blog.mozilla.org/addons',
@ -132,7 +135,7 @@ class TestShelvesSerializer(ESTestCase):
return ShelfSerializer(instance, context=context).data
def _get_result_url(self, instance):
if instance.endpoint in ('search', 'search-themes'):
if instance.endpoint == 'search':
return reverse_ns('addon-search') + instance.criteria
elif instance.endpoint == 'collections':
return reverse_ns(
@ -149,6 +152,7 @@ class TestShelvesSerializer(ESTestCase):
data = self.serialize(self.search_rec_ext)
assert data['title'] == {'en-US': 'Recommended extensions'}
assert data['endpoint'] == 'search'
assert data['addon_type'] == 'extension'
assert data['criteria'] == '?promoted=recommended&sort=random&type=extension'
assert data['footer']['text'] == {'en-US': 'See more recommended extensions'}
assert data['footer']['url'] is None
@ -157,7 +161,8 @@ class TestShelvesSerializer(ESTestCase):
def test_basic_themes(self):
data = self.serialize(self.search_pop_thm)
assert data['title'] == {'en-US': 'Populâr themes'}
assert data['endpoint'] == 'search-themes'
assert data['endpoint'] == 'search'
assert data['addon_type'] == 'statictheme'
assert data['criteria'] == '?sort=users&type=statictheme'
assert data['footer']['text'] == {'en-US': 'See more populâr themes'}
assert data['footer']['url'] == 'http://testserver/themes/'
@ -198,21 +203,22 @@ class TestShelvesSerializer(ESTestCase):
shelf = Shelf(
title='Populâr stuff',
endpoint='search',
addon_type=amo.ADDON_EXTENSION,
criteria='?sort=users&type=extension',
)
data = self.serialize(shelf)
# non-themes are limited to 4 by default
# extensions are limited to 4 by default
assert len(data['addons']) == 4
shelf.endpoint = 'search-themes'
shelf.addon_type = amo.ADDON_STATICTHEME
shelf.criteria = '?sort=users&type=statictheme'
data = self.serialize(shelf)
# themes are limited to 3 by default
assert len(data['addons']) == 3
# double check by changing the endpoint without changing the criteria
shelf.endpoint = 'search'
# double check by changing the addon_type without changing the criteria
shelf.addon_type = amo.ADDON_EXTENSION
data = self.serialize(shelf)
assert len(data['addons']) == 4