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

Updated collection device filtering to work for android (bug 975063) #1819

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 18 additions & 6 deletions mkt/collections/serializers.py
Expand Up @@ -42,8 +42,8 @@ class CollectionAppSerializer(SimpleAppSerializer):

class CollectionESAppSerializer(SimpleESAppSerializer):
"""
Like CollectionAppSerializer, but used when fetching apps from ES instead of
the database.
Like CollectionAppSerializer, but used when fetching apps from ES instead
of the database.
"""
pass

Expand All @@ -65,15 +65,27 @@ def to_native_es(self, value):
ser = CollectionESAppSerializer(value, context=self.context, many=True)
return ser.data

def _get_device(self, request):
# Fireplace sends `dev` and `device`. See the API docs. When
# `dev` is 'android' we also need to check `device` to pick a device
# object.
dev = request.GET.get('dev')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dev and device are a bit confusing but I don't suppose it can be changed here. Maybe a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will surely change when we collapse android mobile and tablet together into a single device. I plan to work on that next.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment or TODO?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API docs say:
device (string) – Filters by supported device. One of ‘desktop’, ‘mobile’, ‘tablet’, or ‘firefoxos’.
dev (string) – Enables filtering by device profile if either ‘firefoxos’ or ‘android’.

I actually don't get why these are separate.

device = request.GET.get('device')

if dev == 'android' and device:
dev = '%s-%s' % (dev, device)

return amo.DEVICE_LOOKUP.get(dev)

def field_to_native(self, obj, field_name):
if not hasattr(self, 'context') or not 'request' in self.context:
raise ImproperlyConfigured('Pass request in self.context when'
' using CollectionMembershipField.')

request = self.context['request']

# Having 'use-es-for-apps' in the context means the parent view wants us
# to use ES to fetch the apps. If that key is present, check that we
# Having 'use-es-for-apps' in the context means the parent view wants
# us to use ES to fetch the apps. If that key is present, check that we
# have a view in the context and that the waffle flag is active. If
# everything checks out, bypass the db and use ES to fetch apps for a
# nice performance boost.
Expand All @@ -83,7 +95,7 @@ def field_to_native(self, obj, field_name):
qs = get_component(obj, self.source)

# Filter apps based on device and feature profiles.
device = amo.DEVICE_LOOKUP.get(request.GET.get('dev'))
device = self._get_device(request)
profile = get_feature_profile(request)
if device and device != amo.DEVICE_DESKTOP:
qs = qs.filter(addondevicetype__device_type=device.id)
Expand All @@ -103,7 +115,7 @@ def field_to_native_es(self, obj, request):
"""
profile = get_feature_profile(request)
region = self.context['view'].get_region(request)
device = amo.DEVICE_LOOKUP.get(request.GET.get('dev'))
device = self._get_device(request)

_rget = lambda d: getattr(request, d, False)
qs = Webapp.from_search(request, region=region, gaia=_rget('GAIA'),
Expand Down
23 changes: 14 additions & 9 deletions mkt/collections/tests/test_serializers.py
Expand Up @@ -67,8 +67,10 @@ def test_to_native(self):
eq_(data[1]['id'], int(self.app2.id))
eq_(data[1]['resource_uri'], self.app2.get_api_url(pk=self.app2.pk))

def _field_to_native_profile(self, profile='0.0', dev='firefoxos'):
request = self.get_request({'pro': profile, 'dev': dev})
def _field_to_native_profile(self, **kwargs):
query = {'pro': '0.0', 'dev': 'firefoxos'}
query.update(kwargs)
request = self.get_request(query)
self.field.parent = self.collection
self.field.source = 'apps'
self.field.context['request'] = request
Expand Down Expand Up @@ -106,23 +108,24 @@ def test_app_pending(self):
eq_(len(result), 0)

def test_field_to_native_profile(self):
result = self._field_to_native_profile(self.profile)
result = self._field_to_native_profile(pro=self.profile)
eq_(len(result), 1)
eq_(int(result[0]['id']), self.app.id)

def test_field_to_native_profile_mismatch(self):
self.app.current_version.features.update(has_geolocation=True)
result = self._field_to_native_profile(self.profile)
result = self._field_to_native_profile(pro=self.profile)
eq_(len(result), 0)

def test_field_to_native_invalid_profile(self):
result = self._field_to_native_profile('muahahah')
result = self._field_to_native_profile(pro='muahahah')
# Ensure that no filtering took place.
eq_(len(result), 1)
eq_(int(result[0]['id']), self.app.id)

def test_field_to_native_device_filter(self):
result = self._field_to_native_profile('muahahah', 'android-mobile')
result = self._field_to_native_profile(pro='muahahah', dev='android',
device='mobile')
eq_(len(result), 0)


Expand All @@ -149,12 +152,14 @@ def setUp(self):
AddonUser.objects.create(addon=self.app, user=self.user)
self.refresh('webapp')

def _field_to_native_profile(self, profile='0.0', dev='firefoxos'):
def _field_to_native_profile(self, **kwargs):
"""
Like _field_to_native_profile in BaseTestCollectionMembershipField,
but calling field_to_native_es directly.
"""
request = self.get_request({'pro': profile, 'dev': dev})
query = {'pro': '0.0', 'dev': 'firefoxos'}
query.update(kwargs)
request = self.get_request(query)
self.field.context['request'] = request
return self.field.field_to_native_es(self.collection, request)

Expand All @@ -163,7 +168,7 @@ def test_field_to_native_profile_mismatch(self):
# FIXME: a simple refresh() wasn't enough, don't we reindex apps when
# feature profiles change ? Investigate.
self.reindex(self.app.__class__, 'webapp')
result = self._field_to_native_profile(self.profile)
result = self._field_to_native_profile(pro=self.profile)
eq_(len(result), 0)

def test_ordering(self):
Expand Down