Skip to content
This repository has been archived by the owner on Mar 15, 2018. It is now read-only.

Commit

Permalink
Added feature profile filters when not desktop (bug 862454)
Browse files Browse the repository at this point in the history
  • Loading branch information
robhudson committed May 31, 2013
1 parent 9f68673 commit d884f7c
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 8 deletions.
20 changes: 16 additions & 4 deletions mkt/search/api.py
Expand Up @@ -3,12 +3,12 @@
from django.conf.urls import url
from django.core.exceptions import ObjectDoesNotExist

import waffle
from tastypie import http
from tastypie.authorization import ReadOnlyAuthorization
from tastypie.throttle import BaseThrottle
from tastypie.utils import trailing_slash
from tower import ugettext as _
import waffle

import amo
from access import acl
Expand All @@ -21,7 +21,8 @@
from mkt.api.authentication import OptionalOAuthAuthentication
from mkt.api.base import CORSResource, MarketplaceResource
from mkt.api.resources import AppResource
from mkt.search.views import _get_query, _filter_search
from mkt.constants.features import FeatureProfile
from mkt.search.views import _filter_search, _get_query
from mkt.search.forms import ApiSearchForm
from mkt.webapps.models import Webapp
from mkt.webapps.utils import es_app_to_dict
Expand Down Expand Up @@ -66,6 +67,7 @@ def get_list(self, request=None, **kwargs):
'type': addon_type,
}

# Allow reviewers and admins to search by statuses other than PUBLIC.
if status and (status == 'any' or status != amo.STATUS_PUBLIC):
if is_admin or is_reviewer:
base_filters['status'] = status
Expand All @@ -74,12 +76,22 @@ def get_list(self, request=None, **kwargs):
content=json.dumps(
{'reason': _('Unauthorized to filter by status.')}))

# Search specific processing of the results.
# Filter by device feature profile.
profile = None
# TODO: Remove uses_es conditional with 'search-api-es' waffle.
if uses_es and request.GET.get('dev') in ('firefoxos', 'android'):
sig = request.GET.get('pro')
if sig:
profile = FeatureProfile.from_signature(sig)

# Filter by region.
region = getattr(request, 'REGION', mkt.regions.WORLDWIDE)

qs = _get_query(region, gaia=request.GAIA, mobile=request.MOBILE,
tablet=request.TABLET, filters=base_filters,
new_idx=True)
qs = _filter_search(request, qs, form_data, region=region)
qs = _filter_search(request, qs, form_data, region=region,
profile=profile)
paginator = self._meta.paginator_class(request.GET, qs,
resource_uri=self.get_resource_list_uri(),
limit=self._meta.limit)
Expand Down
97 changes: 96 additions & 1 deletion mkt/search/tests/test_api.py
Expand Up @@ -15,9 +15,10 @@
from mkt.api.base import list_url
from mkt.api.models import Access, generate
from mkt.api.tests.test_oauth import BaseOAuth, OAuthClient
from mkt.constants.features import FeatureProfile
from mkt.search.forms import DEVICE_CHOICES_IDS
from mkt.site.fixtures import fixture
from mkt.webapps.models import Webapp
from mkt.webapps.models import AppFeatures, Webapp


class TestApi(BaseOAuth, ESTestCase):
Expand Down Expand Up @@ -277,6 +278,100 @@ def test_minimum_price_tier(self):
eq_(len(res2.json['objects']), 0)


class TestApiFeatures(BaseOAuth, ESTestCase):
fixtures = fixture('webapp_337141')

def setUp(self):
self.create_switch('search-api-es')
self.client = OAuthClient(None)
self.url = list_url('search')
self.webapp = Webapp.objects.get(pk=337141)
self.category = Category.objects.create(name='test',
type=amo.ADDON_WEBAPP)
# A typical desktop profile on Firefox with the following features:
# {'apps': True,
# 'audio': True,
# 'battery': True,
# 'device_storage': True,
# 'fullscreen': True,
# 'geolocation': True,
# 'idle': True,
# 'indexeddb': True,
# 'light_events': True,
# 'network_info': True,
# 'orientation': True,
# 'proximity': True,
# 'push': True,
# 'sms': True,
# 'vibrate': True,
# 'video_webm': True,
# 'webaudio': True}
self.profile = '8a7dd46c.32.1'
self.qs = {'q': 'something', 'pro': self.profile, 'dev': 'firefoxos'}

def test_no_features(self):
# Base test to make sure we find the app.
self.webapp.save()
self.refresh('webapp')

res = self.client.get(self.url + (self.qs,))
eq_(res.status_code, 200)
obj = json.loads(res.content)['objects'][0]
eq_(obj['slug'], self.webapp.app_slug)

def test_one_good_feature(self):
# Enable an app feature that matches one in our profile.
AppFeatures.objects.create(version=self.webapp.current_version,
has_geolocation=True)
self.webapp.save()
self.refresh('webapp')

res = self.client.get(self.url + (self.qs,))
eq_(res.status_code, 200)
obj = json.loads(res.content)['objects'][0]
eq_(obj['slug'], self.webapp.app_slug)

def test_one_bad_feature(self):
# Enable an app feature that doesn't match one in our profile.
AppFeatures.objects.create(version=self.webapp.current_version,
has_pay=True)
self.webapp.save()
self.refresh('webapp')

res = self.client.get(self.url + (self.qs,))
eq_(res.status_code, 200)
objs = json.loads(res.content)['objects']
eq_(len(objs), 0)

def test_all_good_features(self):
# Enable app features so they exactly match our device profile.
fp = FeatureProfile.from_signature(self.profile)
AppFeatures.objects.create(
version=self.webapp.current_version,
**dict(('has_%s' % k, v) for k, v in fp.items()))
self.webapp.save()
self.refresh('webapp')

res = self.client.get(self.url + (self.qs,))
eq_(res.status_code, 200)
obj = json.loads(res.content)['objects'][0]
eq_(obj['slug'], self.webapp.app_slug)

def test_bad_profile_on_desktop(self):
# Enable an app feature that doesn't match one in our profile.
qs = self.qs.copy()
del qs['dev'] # Desktop doesn't send a device.
AppFeatures.objects.create(version=self.webapp.current_version,
has_pay=True)
self.webapp.save()
self.refresh('webapp')

res = self.client.get(self.url + (qs,))
eq_(res.status_code, 200)
obj = json.loads(res.content)['objects'][0]
eq_(obj['slug'], self.webapp.app_slug)


class TestApiReviewer(BaseOAuth, ESTestCase):
fixtures = fixture('webapp_337141', 'user_2519')

Expand Down
22 changes: 19 additions & 3 deletions mkt/search/views.py
@@ -1,13 +1,14 @@
from django.shortcuts import redirect

import jingo
from elasticutils.contrib.django import F
from tower import ugettext as _

import amo
import amo.utils
from amo.decorators import json_view
from apps.addons.models import Category
from apps.search.views import WebappSuggestionsAjax, _get_locale_analyzer
from apps.search.views import _get_locale_analyzer, WebappSuggestionsAjax

import mkt
from mkt.constants import regions
Expand Down Expand Up @@ -91,8 +92,15 @@ def name_query(q):


def _filter_search(request, qs, query, filters=None, sorting=None,
sorting_default='-popularity', region=None):
"""Filter an ES queryset based on a list of filters."""
sorting_default='-popularity', region=None, profile=None):
"""
Filter an ES queryset based on a list of filters.
If `profile` (a FeatureProfile object) is provided we filter by the
profile. If you don't want to filter by these don't pass it. I.e. do the
device detection for when this happens elsewhere.
"""
# Intersection of the form fields present and the filters we want to apply.
filters = filters or DEFAULT_FILTERS
sorting = sorting or DEFAULT_SORTING
Expand Down Expand Up @@ -144,6 +152,14 @@ def _filter_search(request, qs, query, filters=None, sorting=None,
# don't list apps that require carrier billing to buy.
if not region.supports_carrier_billing:
qs = qs.filter(carrier_billing_only=False)

if profile:
f = F()
for k, v in profile.iteritems():
if not v:
f &= ~F(**{'features.has_%s' % k: True})

This comment has been minimized.

Copy link
@hannosch

hannosch Jun 4, 2013

Contributor

Why do you search for "not True" instead of checking the field for "False"?

This comment has been minimized.

Copy link
@robhudson

robhudson Jun 4, 2013

Author Member

Mentally I was thinking about this as "For any features my device doesn't support, exclude apps that require those", which translated to exclude where True. But you're right, filtering where False is the same logic. Usually when you invert logic you need to toggle AND and OR, but in this case the AND still applies because we want to match all features not just some.

I changed the line to:
f &= F(**{'features.has_%s' % k: False})

Tests still pass and ES query looks better. Thanks!

This comment has been minimized.

Copy link
@cvan

cvan Jun 4, 2013

Contributor

the reason I thought was that if we add a new feature, it won't fail on the query. with this new query, it would - right? but either way, we probably need ES schematic.

This comment has been minimized.

Copy link
@robhudson

robhudson Jun 4, 2013

Author Member

I don't think it affects apps with no features specified. By default they're all False. E.g. if my device doesn't support SMS we're filtering where has_sms=False (vs excluding has_sms=True). New apps default to has_sms=False so those still get found. Only apps that explicitly state that they require SMS would get filtered out, which is what we want.

This comment has been minimized.

Copy link
@robhudson

robhudson Jun 4, 2013

Author Member

For features we do have, we aren't filtering by so they can be either True or False.

This comment has been minimized.

Copy link
@cvan

cvan Jun 4, 2013

Contributor

I meant new capabilities that we add, not necessarily new apps - but yeah I see what you mean.

qs = qs.filter(f)

return qs


Expand Down

0 comments on commit d884f7c

Please sign in to comment.