Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

List all the icons in API #342

Closed
wants to merge 1 commit into from

3 participants

@darktrojan

see bug 731539 for discussion

@darktrojan

ping?

@clouserw
Owner

Kumar is going to review this next week

@kumar303
Collaborator

Hi. Thanks for all the change so far. Can you rebase/squash all your changes into one commit? Here is some info on how to do that https://blog.mozilla.org/webdev/2011/11/21/git-using-topic-branches-and-interactive-rebasing-effectively/

The reason it's important here is that the API is under heavy load. If we find on dev or prod that it affects performance we may need to revert the commit until it's fixed.

@kumar303
Collaborator

The cron job and model change adds a lot of complexity. Could we start by just adding the new icon method from your first pull request and defer the cron change to another pull request? Your first patch was very close to done, it just needed to use the storage API. Thanks for all your work on this.

@kumar303
Collaborator

Thanks for doing another cleanup. I took a closer look at has_icon_internal() and there's no way we can check the icon dir in a loop (especially when using S3). Can you reapply the cron job patch that makes it a column in the table? Sorry, I know I said not to but we have to make this a column in the table with the kind of traffic the API gets.

@darktrojan darktrojan closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 25, 2012
  1. @darktrojan
This page is out of date. Refresh to see the latest.
View
44 apps/addons/models.py
@@ -11,6 +11,8 @@
from django.conf import settings
from django.core.cache import cache
+from django.core.files.storage import (FileSystemStorage,
+ default_storage as storage)
from django.db import models, transaction
from django.dispatch import receiver
from django.db.models import Q, Max, signals as dbsignals
@@ -28,8 +30,8 @@
from amo.decorators import use_master
from amo.fields import DecimalCharField
from amo.helpers import absolutify, shared_url
-from amo.utils import (cache_ns_key, chunked, JSONEncoder, send_mail, slugify,
- sorted_groupby, to_language, urlparams)
+from amo.utils import (cache_ns_key, chunked, memoize, JSONEncoder, send_mail,
+ slugify, sorted_groupby, to_language, urlparams)
from amo.urlresolvers import get_outgoing_url, reverse
from compat.models import CompatReport
from files.models import File
@@ -757,6 +759,24 @@ def get_icon_dir(self):
return os.path.join(settings.ADDON_ICONS_PATH,
'%s' % (self.id / 1000))
+ def has_icon(self, size):
+ """
+ Returns whether we have an icon of size |size|, to avoid cases
+ where get_icon_url returns a URL to a smaller icon than requested.
+ """
+ # All personas have a 32px icon
+ if self.type == amo.ADDON_PERSONA:
+ return size == 32
+ # Default icons are ignored
+ if not self.icon_type:
+ return False
+ # Built in icons have only 32px and 64px versions
+ if self.icon_type.split('/')[0] == 'icon':
+ return size in (32, 64)
+
+ icon_dir = self.get_icon_dir()
+ return has_icon_internal(self.id, icon_dir, size)
+
def get_icon_url(self, size, use_default=True):
"""
Returns either the addon's icon url.
@@ -1340,6 +1360,26 @@ def has_installed(self, user):
return self.installed.filter(user=user).exists()
+@memoize(prefix='has_icon')
+def has_icon_internal(id, icon_dir, size):
+ """
+ Cached method to do I/O-heavy work of Addon.has_icon.
+ """
+ icon_file = os.path.join(icon_dir, '%s-%s.png' % (id, size))
+ if not storage.exists(icon_file):
+ return False
+
+ smaller_sizes = [s for s in amo.ADDON_ICON_SIZES if s < size]
+ if len(smaller_sizes) == 0:
+ return True
+
+ # Test if the icon we want is the same as the next-largest icon
+ smaller_icon_file = os.path.join(icon_dir,
+ '%s-%s.png' % (id, smaller_sizes[-1]))
+ return (not storage.exists(smaller_icon_file) or
+ storage.size(smaller_icon_file) != storage.size(icon_file))
+
+
class AddonDeviceType(amo.models.ModelBase):
addon = models.ForeignKey(Addon)
device_type = models.PositiveIntegerField(default=amo.DEVICE_DESKTOP,
View
40 apps/addons/tests/test_models.py
@@ -4,6 +4,7 @@
import json
import os
from datetime import datetime, timedelta
+import shutil
import tempfile
from urlparse import urlparse
@@ -23,6 +24,7 @@
from amo import set_user
from amo.helpers import absolutify
from amo.signals import _connect, _disconnect
+from amo.tests.test_helpers import get_image_path
from addons.models import (Addon, AddonCategory, AddonDependency,
AddonDeviceType, AddonRecommendation, AddonType,
AddonUpsell, AddonUser, AppSupport, BlacklistedGuid,
@@ -224,12 +226,16 @@ class TestAddonModels(amo.tests.TestCase):
def setUp(self):
TranslationSequence.objects.create(id=99243)
- # TODO(andym): use Mock appropriately here.
- self.old_version = amo.FIREFOX.latest_version
- amo.FIREFOX.latest_version = '3.6.15'
- def tearDown(self):
- amo.FIREFOX.latest_version = self.old_version
+ patcher = patch.object(amo.FIREFOX, 'latest_version', '3.6.15')
+ patcher.start()
+ self.addCleanup(patcher.stop)
+
+ temp_icons_dir = tempfile.mkdtemp()
+ patcher = patch.object(settings, 'ADDON_ICONS_PATH', temp_icons_dir)
+ patcher.start()
+ self.addCleanup(patcher.stop)
+ self.addCleanup(lambda:shutil.rmtree(temp_icons_dir))
def test_current_version(self):
"""
@@ -385,6 +391,25 @@ def test_incompatible_asterix(self):
a = Addon.objects.get(pk=3615)
eq_(a.incompatible_latest_apps(), [])
+ def test_has_icon(self):
+ addon = Addon.objects.get(pk=3615)
+ dirname = os.path.join(settings.ADDON_ICONS_PATH, '3')
+ os.mkdir(dirname)
+ src = get_image_path('transparent-expected.png')
+ shutil.copy(src, os.path.join(dirname, '3615-32.png'))
+ shutil.copy(src, os.path.join(dirname, '3615-48.png'))
+
+ eq_(addon.has_icon(32), True)
+ eq_(addon.has_icon(48), False)
+ eq_(addon.has_icon(64), False)
+
+ def test_has_icon_default(self):
+ addon = Addon.objects.get(pk=5299)
+ addon.update(icon_type='')
+ eq_(addon.has_icon(32), False)
+ eq_(addon.has_icon(48), False)
+ eq_(addon.has_icon(64), False)
+
def test_icon_url(self):
"""
Tests for various icons.
@@ -1400,6 +1425,11 @@ def setUp(self):
self.persona.footer = 'footer.jpg'
self.persona.save()
+ def test_has_icon(self):
+ eq_(self.addon.has_icon(32), True)
+ eq_(self.addon.has_icon(48), False)
+ eq_(self.addon.has_icon(64), False)
+
def test_image_urls(self):
self.persona.persona_id = 0
self.persona.save()
View
7 apps/api/templates/api/includes/addon.xml
@@ -41,8 +41,11 @@
</license>
{%- endif -%}
{# The 32px icon must be the last icon specified. #}
- <icon size="64">{{ addon.get_icon_url(64, use_default=False) }}</icon>
- <icon size="32">{{ addon.get_icon_url(32, use_default=False) }}</icon>
+ {%- for size in amo.ADDON_ICON_SIZES|reverse %}
+ {%- if addon.has_icon(size) -%}
+ <icon size="{{ size }}">{{ addon.get_icon_url(size) }}</icon>
+ {%- endif -%}
+ {%- endfor -%}
<compatible_applications>
{%- if version -%}
{%- for app in version.compatible_apps.values() %}
View
13 apps/api/tests/test_views.py
@@ -273,9 +273,6 @@ def test_addon_detail(self):
u'<author>55021 \u0627\u0644\u062a\u0637\u0628</author>')
self.assertContains(response, "<summary>Delicious Bookmarks is the")
self.assertContains(response, "<description>This extension integrates")
-
- icon_url = settings.ADDON_ICON_URL % (3, 3615, 32, '')
- self.assertContains(response, '<icon size="32">' + icon_url)
self.assertContains(response, "<application>")
self.assertContains(response, "<name>Firefox</name>")
self.assertContains(response, "<application_id>1</application_id>")
@@ -490,10 +487,18 @@ def test_is_featured(self):
lang=lang, app=app),
'<featured>%s</featured>' % result)
+ @patch.object(Addon, 'has_icon', lambda self, size: size in (32, 48))
+ def test_icon(self):
+ icon_url = settings.ADDON_ICON_URL % (5, 5299, 32, '')
+ self.assertContains(make_call('addon/5299'), '<icon size="32">' + icon_url)
+ icon_url = settings.ADDON_ICON_URL % (5, 5299, 48, '')
+ self.assertContains(make_call('addon/5299'), '<icon size="48">' + icon_url)
+ self.assertNotContains(make_call('addon/5299'), '<icon size="64">')
+
def test_default_icon(self):
addon = Addon.objects.get(pk=5299)
addon.update(icon_type='')
- self.assertContains(make_call('addon/5299'), '<icon size="32"></icon>')
+ self.assertNotContains(make_call('addon/5299'), '<icon')
def test_thumbnail_size(self):
addon = Addon.objects.get(pk=5299)
Something went wrong with that request. Please try again.