Skip to content
Browse files

Merge branch 'fix-wait-time-632191'

  • Loading branch information...
2 parents 27466c7 + 3272245 commit aa53e28b477ab97a409e6254b4d45b2d7fdfbb69 @kumar303 kumar303 committed Feb 25, 2011
Showing with 208 additions and 6 deletions.
  1. +9 −6 apps/addons/models.py
  2. +108 −0 apps/amo/models.py
  3. +67 −0 apps/amo/tests/test_models.py
  4. +24 −0 apps/devhub/tests/test_views.py
View
15 apps/addons/models.py
@@ -103,7 +103,7 @@ def q(*args, **kw):
disabled_by_user=False, status__in=status)
-class Addon(amo.models.ModelBase):
+class Addon(amo.models.OnChangeMixin, amo.models.ModelBase):
STATUS_CHOICES = amo.STATUS_CHOICES.items()
LOCALES = [(translation.to_locale(k).replace('_', '-'), v) for k, v in
settings.LANGUAGES.items()]
@@ -830,15 +830,18 @@ def version_changed(sender, **kw):
dispatch_uid='version_changed')
-def watch_status(sender, instance, **kw):
+def watch_status(old_attr={}, new_attr={}, instance=None,
+ sender=None, **kw):
"""Set nominationdate if self.status asks for full review."""
- if kw.get('raw'):
+ new_status = new_attr.get('status')
+ if not new_status:
return
addon = instance
stati = (amo.STATUS_NOMINATED, amo.STATUS_LITE_AND_NOMINATED)
- if addon.status in stati:
- addon.nomination_date = datetime.now()
-dbsignals.pre_save.connect(watch_status, sender=Addon)
+ if new_status in stati and old_attr['status'] != new_status:
+ addon.update(nomination_date=datetime.now())
+
+Addon.on_change(watch_status)
class MiniAddonManager(AddonManager):
View
108 apps/amo/models.py
@@ -1,3 +1,4 @@
+from collections import defaultdict
import contextlib
import threading
@@ -157,6 +158,113 @@ def raw(self, raw_query, params=None, *args, **kwargs):
using=self._db, *args, **kwargs)
+class _NoChangeInstance(object):
+ """A proxy for object instances to make safe operations within an
+ OnChangeMixin.on_change() callback.
+ """
+
+ def __init__(self, instance):
+ self.__instance = instance
+
+ def __repr__(self):
+ return u'<%s for %r>' % (self.__class__.__name__, self.__instance)
+
+ def __getattr__(self, attr):
+ return getattr(self.__instance, attr)
+
+ def __setattr__(self, attr, val):
+ if attr.endswith('__instance'):
+ # _NoChangeInstance__instance
+ self.__dict__[attr] = val
+ else:
+ setattr(self.__instance, attr, val)
+
+ def save(self, *args, **kw):
+ kw['_signal'] = False
+ return self.__instance.save(*args, **kw)
+
+ def update(self, *args, **kw):
+ kw['_signal'] = False
+ return self.__instance.update(*args, **kw)
+
+
+_on_change_callbacks = defaultdict(list)
+
+
+# @TODO(Kumar) liberate: move OnChangeMixin Model mixin to nuggets
+class OnChangeMixin(object):
+ """Mixin for a Model that allows you to observe attribute changes.
+
+ Register change observers with::
+
+ class YourModel(amo.models.OnChangeMixin,
+ amo.models.ModelBase):
+ # ...
+ pass
+
+ YourModel.on_change(callback)
+
+ """
+
+ def __init__(self, *args, **kw):
+ super(OnChangeMixin, self).__init__(*args, **kw)
+ self._initial_attr = dict(self.__dict__)
+
+ @classmethod
+ def on_change(cls, callback):
+ """Register a function to call on save or update to respond to changes.
+
+ For example::
+
+ def watch_status(old_attr={}, new_attr={},
+ instance=None, sender=None, **kw):
+ if old_attr.get('status') != new_attr.get('status'):
+ # ...
+ new_instance.save(_signal=False)
+ TheModel.on_change(watch_status)
+
+ .. note::
+
+ Any call to instance.save() or instance.update() within a callback
+ will not trigger any change handlers.
+
+ """
+ _on_change_callbacks[cls].append(callback)
+
+ def _send_changes(self, old_attr, new_attr_kw):
+ new_attr = old_attr.copy()
+ new_attr.update(new_attr_kw)
+ for cb in _on_change_callbacks[self.__class__]:
+ cb(old_attr=old_attr, new_attr=new_attr,
+ instance=_NoChangeInstance(self), sender=self.__class__)
+
+ def save(self, *args, **kw):
+ """
+ Save changes to the model instance.
+
+ If _signal=False is in ``kw`` the on_change() callbacks won't be called.
+ """
+ signal = kw.pop('_signal', True)
+ result = super(OnChangeMixin, self).save(*args, **kw)
+ if signal:
+ self._send_changes(self._initial_attr, dict(self.__dict__))
+ return result
+
+ def update(self, **kw):
+ """
+ Shortcut for doing an UPDATE on this object.
+
+ If _signal=False is in ``kw`` the post_save signal won't be sent.
+ """
+ cls = self.__class__
+ signal = kw.pop('_signal', True)
+ old_attr = dict(self.__dict__)
+ result = cls.objects.filter(pk=self.pk).update(**kw)
+ if signal:
+ self._send_changes(old_attr, kw)
+ return result
+
+
class ModelBase(caching.base.CachingMixin, models.Model):
"""
Base class for AMO models to abstract some common features.
View
67 apps/amo/tests/test_models.py
@@ -1,7 +1,9 @@
from test_utils import TestCase
+from mock import Mock
from nose.tools import eq_
+import amo.models
from amo.models import manual_order
from amo import models as context
from addons.models import Addon
@@ -39,3 +41,68 @@ def test_use_master():
eq_(local.pinned, True)
eq_(local.pinned, True)
eq_(local.pinned, False)
+
+
+class TestModelBase(TestCase):
+ fixtures = ['base/addon_3615']
+
+ def setUp(self):
+ self.saved_cb = amo.models._on_change_callbacks.copy()
+ amo.models._on_change_callbacks.clear()
+ self.cb = Mock()
+ Addon.on_change(self.cb)
+
+ def tearDown(self):
+ amo.models._on_change_callbacks = self.saved_cb
+
+ def test_change_called_on_new_instance_save(self):
+ for create_addon in (Addon, Addon.objects.create):
+ addon = create_addon(site_specific=False, type=amo.ADDON_EXTENSION)
+ addon.site_specific = True
+ addon.save()
+ assert self.cb.called
+ kw = self.cb.call_args[1]
+ eq_(kw['old_attr']['site_specific'], False)
+ eq_(kw['new_attr']['site_specific'], True)
+ eq_(kw['instance'].id, addon.id)
+ eq_(kw['sender'], Addon)
+
+ def test_change_called_on_update(self):
+ addon = Addon.objects.get(pk=3615)
+ addon.update(site_specific=False)
+ assert self.cb.called
+ kw = self.cb.call_args[1]
+ eq_(kw['old_attr']['site_specific'], True)
+ eq_(kw['new_attr']['site_specific'], False)
+ eq_(kw['instance'].id, addon.id)
+ eq_(kw['sender'], Addon)
+
+ def test_change_called_on_save(self):
+ addon = Addon.objects.get(pk=3615)
+ addon.site_specific = False
+ addon.save()
+ assert self.cb.called
+ kw = self.cb.call_args[1]
+ eq_(kw['old_attr']['site_specific'], True)
+ eq_(kw['new_attr']['site_specific'], False)
+ eq_(kw['instance'].id, addon.id)
+ eq_(kw['sender'], Addon)
+
+ def test_change_is_not_recursive(self):
+
+ class fn:
+ called = False
+
+ def callback(old_attr=None, new_attr=None, instance=None,
+ sender=None, **kw):
+ fn.called = True
+ # Both save and update should be protected:
+ instance.update(site_specific=False)
+ instance.save()
+
+ Addon.on_change(callback)
+
+ addon = Addon.objects.get(pk=3615)
+ addon.save()
+ assert fn.called
+ # No exception = pass
View
24 apps/devhub/tests/test_views.py
@@ -2867,6 +2867,18 @@ def test_full_review(self):
assert_close_to_now(addon.nomination_date)
assert_raises(SubmitStep.DoesNotExist, self.get_step)
+ def test_nomination_date_is_only_set_once(self):
+ # This was a regression, see bug 632191.
+ # Nominate:
+ r = self.client.post(self.url, dict(review_type=amo.STATUS_NOMINATED))
+ eq_(r.status_code, 302)
+ nomdate = datetime.now() - timedelta(days=5)
+ self.get_addon().update(nomination_date=nomdate, _signal=False)
+ # Update something else in the addon:
+ self.get_addon().update(slug='foobar')
+ eq_(self.get_addon().nomination_date.timetuple()[0:5],
+ nomdate.timetuple()[0:5])
+
class TestSubmitStep7(TestSubmitBase):
@@ -3806,6 +3818,18 @@ def test_renominate_for_full_review(self):
self.check(amo.STATUS_NULL, self.public_url, amo.STATUS_NOMINATED)
assert_close_to_now(self.get_addon().nomination_date)
+ def test_renomination_resets_nomination_date(self):
+ # Nominate:
+ self.addon.update(status=amo.STATUS_LITE_AND_NOMINATED)
+ # Pretend it was nominated in the past:
+ self.addon.update(nomination_date=datetime.now() - timedelta(days=30),
+ _signal=False)
+ # Reject it:
+ self.addon.update(status=amo.STATUS_NULL)
+ # Re-nominate:
+ self.addon.update(status=amo.STATUS_LITE_AND_NOMINATED)
+ assert_close_to_now(self.get_addon().nomination_date)
+
class TestRedirects(test_utils.TestCase):
fixtures = ['base/apps', 'base/users', 'base/addon_3615']

0 comments on commit aa53e28

Please sign in to comment.
Something went wrong with that request. Please try again.