Updated waiting times column on review queues (bug 793914)

For the pending and updated queues, we now use version.nomination.  And
version.nomination is set when app status goes to PENDING. For escalated
and re-review queues we use the queue's created stamp.

apps/amo/tests/__init__.py

@@ -217,7 +217,7 @@ def decorated(request, *args, **kwargs):
 
 
 def days_ago(days):
-    return datetime.now() - timedelta(days=days)
+    return datetime.now().replace(microsecond=0) - timedelta(days=days)
 
 
 class TestCase(RedisTest, test_utils.TestCase):
@@ -382,6 +382,8 @@ def assertCloseToNow(self, dt, now=None):
         """
         Make sure the datetime is within a minute from `now`.
         """
+        if not dt:
+            raise AssertionError('Expected datetime, got: %s' % dt)
         dt_later_ts = time.mktime((dt + timedelta(minutes=1)).timetuple())
         dt_earlier_ts = time.mktime((dt - timedelta(minutes=1)).timetuple())
         if not now:

apps/versions/models.py

@@ -1,4 +1,5 @@
 # -*- coding: utf-8 -*-
+import datetime
 import os
 
 import django.dispatch
@@ -453,14 +454,32 @@ def inherit_nomination(sender, instance, **kw):
     """
     if kw.get('raw'):
         return
-    if (instance.nomination is None
-        and instance.addon.status in (amo.STATUS_NOMINATED,
-                                      amo.STATUS_LITE_AND_NOMINATED)
-        and not instance.is_beta):
-        last_ver = (Version.objects.filter(addon=instance.addon)
-                    .exclude(nomination=None).order_by('-nomination'))
-        if last_ver.exists():
-            instance.update(nomination=last_ver[0].nomination)
+    addon = instance.addon
+    if (addon.type == amo.ADDON_WEBAPP and addon.is_packaged):
+        # If prior version's file is pending, inherit nomination. Otherwise,
+        # set nomination to now.
+        last_ver = (Version.objects.filter(addon=addon)
+                                   .exclude(pk=instance.pk)
+                                   .order_by('-nomination'))
+        if (last_ver.exists() and
+            last_ver[0].all_files[0].status == amo.STATUS_PENDING):
+            instance.update(nomination=last_ver[0].nomination, _signal=False)
+            log.debug('[Webapp:%s] Inheriting nomination from prior pending '
+                      'version' % addon.id)
+        elif addon.status == amo.STATUS_PUBLIC and not instance.nomination:
+            log.debug('[Webapp:%s] Setting nomination date to now for new '
+                      'version.' % addon.id)
+            instance.update(nomination=datetime.datetime.now(), _signal=False)
+    else:
+        if (instance.nomination is None
+            and addon.status in (amo.STATUS_NOMINATED,
+                                 amo.STATUS_LITE_AND_NOMINATED)
+            and not instance.is_beta):
+            last_ver = (Version.objects.filter(addon=addon)
+                        .exclude(nomination=None).order_by('-nomination'))
+            if last_ver.exists():
+                instance.update(nomination=last_ver[0].nomination,
+                                _signal=False)
 
 
 def update_incompatible_versions(sender, instance, **kw):
@@ -507,25 +526,27 @@ def clear_compatversion_cache_on_delete(sender, instance, **kw):
 
 
 version_uploaded = django.dispatch.Signal()
-models.signals.post_save.connect(update_status, sender=Version,
-                                 dispatch_uid='version_update_status')
-models.signals.post_save.connect(inherit_nomination, sender=Version,
-                                 dispatch_uid='version_inherit_nomination')
-models.signals.post_delete.connect(update_status, sender=Version,
-                                   dispatch_uid='version_update_status')
-models.signals.post_save.connect(update_incompatible_versions, sender=Version,
-                                 dispatch_uid='version_update_incompat')
-models.signals.post_delete.connect(update_incompatible_versions,
-                                   sender=Version,
-                                   dispatch_uid='version_update_incompat')
-models.signals.pre_delete.connect(cleanup_version, sender=Version,
-                                  dispatch_uid='cleanup_version')
-models.signals.post_save.connect(clear_compatversion_cache_on_save,
-                                 sender=Version,
-                                 dispatch_uid='clear_compatversion_cache_save')
-models.signals.post_delete.connect(clear_compatversion_cache_on_delete,
-                                   sender=Version,
-                                   dispatch_uid='clear_compatversion_cache_del')
+models.signals.post_save.connect(
+    update_status, sender=Version, dispatch_uid='version_update_status')
+models.signals.post_save.connect(
+    inherit_nomination, sender=Version,
+    dispatch_uid='version_inherit_nomination')
+models.signals.post_delete.connect(
+    update_status, sender=Version, dispatch_uid='version_update_status')
+models.signals.post_save.connect(
+    update_incompatible_versions, sender=Version,
+    dispatch_uid='version_update_incompat')
+models.signals.post_delete.connect(
+    update_incompatible_versions, sender=Version,
+    dispatch_uid='version_update_incompat')
+models.signals.pre_delete.connect(
+    cleanup_version, sender=Version, dispatch_uid='cleanup_version')
+models.signals.post_save.connect(
+    clear_compatversion_cache_on_save, sender=Version,
+    dispatch_uid='clear_compatversion_cache_save')
+models.signals.post_delete.connect(
+    clear_compatversion_cache_on_delete, sender=Version,
+    dispatch_uid='clear_compatversion_cache_del')
 
 
 class LicenseManager(amo.models.ManagerBase):

migrations/547-set-version-nominations-for-apps.sql

@@ -0,0 +1,5 @@
+UPDATE versions, addons
+SET versions.nomination=versions.created
+WHERE versions.addon_id=addons.id AND
+      addons.addontype_id=11 AND
+      versions.nomination IS NULL;

mkt/reviewers/tests/test_views.py

@@ -218,9 +218,11 @@ class TestAppQueue(AppReviewerTest, AccessMixin, FlagsMixin, SearchMixin,
 
     def setUp(self):
         self.apps = [app_factory(name='XXX',
-                                 status=amo.STATUS_PENDING),
+                                 status=amo.STATUS_PENDING,
+                                 version_kw={'nomination': self.days_ago(2)}),
                      app_factory(name='YYY',
-                                 status=amo.STATUS_PENDING),
+                                 status=amo.STATUS_PENDING,
+                                 version_kw={'nomination': self.days_ago(1)}),
                      app_factory(name='ZZZ')]
         self.apps[0].update(created=self.days_ago(2))
         self.apps[1].update(created=self.days_ago(1))
@@ -478,10 +480,12 @@ class TestUpdateQueue(AppReviewerTest, AccessMixin, FlagsMixin, SearchMixin,
     def setUp(self):
         app1 = app_factory(is_packaged=True, name='XXX',
                            version_kw={'version': '1.0',
-                                       'created': self.days_ago(2)})
+                                       'created': self.days_ago(2),
+                                       'nomination': self.days_ago(2)})
         app2 = app_factory(is_packaged=True, name='YYY',
                            version_kw={'version': '1.0',
-                                       'created': self.days_ago(2)})
+                                       'created': self.days_ago(2),
+                                       'nomination': self.days_ago(2)})
 
         version_factory(addon=app1, version='1.1', created=self.days_ago(1),
                         nomination=self.days_ago(1),
@@ -498,8 +502,8 @@ def review_url(self, app):
         return reverse('reviewers.apps.review', args=[app.app_slug])
 
     def test_template_links(self):
-        self.apps[0].versions.latest().files.update(created=self.days_ago(2))
-        self.apps[1].versions.latest().files.update(created=self.days_ago(1))
+        self.apps[0].versions.latest().update(nomination=self.days_ago(2))
+        self.apps[1].versions.latest().update(nomination=self.days_ago(1))
         r = self.client.get(self.url)
         eq_(r.status_code, 200)
         links = pq(r.content)('#addon-queue tbody')('tr td:nth-of-type(2) a')
@@ -594,8 +598,8 @@ def test_escalated_not_in_queue(self):
     def test_order(self):
         self.apps[0].update(created=self.days_ago(10))
         self.apps[1].update(created=self.days_ago(5))
-        self.apps[0].versions.latest().files.update(created=self.days_ago(1))
-        self.apps[1].versions.latest().files.update(created=self.days_ago(4))
+        self.apps[0].versions.latest().update(nomination=self.days_ago(1))
+        self.apps[1].versions.latest().update(nomination=self.days_ago(4))
         res = self.client.get(self.url)
         apps = list(res.context['addons'])
         eq_(apps[0].app, self.apps[1])

mkt/reviewers/views.py

@@ -333,9 +333,8 @@ def _do_sort(request, qs):
     """Column sorting logic based on request GET parameters."""
     sort, order = clean_sort_param(request)
 
-    if qs.model in [EscalationQueue, RereviewQueue] and sort != 'created':
-        # For some queues, want to use queue's created field, not the app's
-        # created field.
+    if qs.model is not Webapp and sort != 'created':
+        # For when `Webapp` isn't the base model of the queryset.
         sort = 'addon__' + sort
 
     if order == 'asc':
@@ -355,13 +354,16 @@ def _do_sort(request, qs):
 @permission_required('Apps', 'Review')
 def queue_apps(request):
     excluded_ids = EscalationQueue.uncached.values_list('addon', flat=True)
-    qs = (Webapp.uncached.filter(type=amo.ADDON_WEBAPP,
-                                 disabled_by_user=False,
-                                 status=amo.STATUS_PENDING)
-                         .exclude(id__in=excluded_ids))
-
-    qs, search_form = _get_search_form(request, _do_sort(request, qs))
-    apps = [QueuedApp(app, app.created) for app in qs]
+    qs = (Version.uncached.filter(addon__type=amo.ADDON_WEBAPP,
+                                  addon__disabled_by_user=False,
+                                  addon__status=amo.STATUS_PENDING)
+                          .exclude(addon__id__in=excluded_ids)
+                          .order_by('nomination', 'created')
+                          .select_related('addon').no_transforms())
+
+    qs, search_form = _queue_to_apps(request, qs)
+    apps = [QueuedApp(app, app.current_version.nomination)
+            for app in Webapp.version_and_file_transformer(qs)]
 
     return _queue(request, apps, 'pending', search_form)
 
@@ -403,7 +405,7 @@ def queue_updates(request):
                          .filter(id__in=addon_ids))
     qs, search_form = _get_search_form(request, _do_sort(request, qs))
 
-    apps = [QueuedApp(app, app.all_versions[0].all_files[0].created)
+    apps = [QueuedApp(app, app.all_versions[0].nomination)
             for app in Webapp.version_and_file_transformer(qs)]
     apps = sorted(apps, key=lambda a: a.created)
     return _queue(request, apps, 'updates', search_form)

mkt/webapps/models.py

@@ -752,6 +752,26 @@ def update_cached_manifests(sender, **kw):
         update_cached_manifests.delay(sender.id)
 
 
+@Webapp.on_change
+def watch_status(old_attr={}, new_attr={}, instance=None, sender=None, **kw):
+    """Set nomination date when app is pending review."""
+    new_status = new_attr.get('status')
+    if not new_status:
+        return
+    addon = instance
+    if new_status == amo.STATUS_PENDING and old_attr['status'] != new_status:
+        # We always set nomination date when app switches to PENDING, even if
+        # previously rejected.
+        try:
+            latest = addon.versions.latest()
+            log.debug('[Webapp:%s] Setting nomination date to now.' % addon.id)
+            latest.update(nomination=datetime.datetime.now())
+        except Version.DoesNotExist:
+            log.debug('[Webapp:%s] Missing version, no nomination set.'
+                      % addon.id)
+            pass
+
+
 class ImageAsset(amo.models.ModelBase):
     addon = models.ForeignKey(Addon, related_name='image_assets')
     filetype = models.CharField(max_length=25, default='image/png')

mkt/webapps/tests/test_models.py

@@ -379,6 +379,39 @@ def test_app_type_hosted(self):
     def test_app_type_packaged(self):
         eq_(Webapp(is_packaged=True).app_type, 'packaged')
 
+    def test_nomination_new(self):
+        app = app_factory()
+        app.update(status=amo.STATUS_NULL)
+        app.versions.latest().update(nomination=None)
+        app.update(status=amo.STATUS_PENDING)
+        assert app.versions.latest().nomination
+
+    def test_nomination_rejected(self):
+        app = app_factory()
+        app.update(status=amo.STATUS_REJECTED)
+        app.versions.latest().update(nomination=self.days_ago(1))
+        app.update(status=amo.STATUS_PENDING)
+        self.assertCloseToNow(app.versions.latest().nomination)
+
+    def test_nomination_pkg_pending_new_version(self):
+        # New versions while pending inherit version nomination.
+        app = app_factory()
+        app.update(status=amo.STATUS_PENDING, is_packaged=True)
+        old_ver = app.versions.latest()
+        old_ver.update(nomination=self.days_ago(1))
+        old_ver.all_files[0].update(status=amo.STATUS_PENDING)
+        v = Version.objects.create(addon=app, version='1.9')
+        eq_(v.nomination, old_ver.nomination)
+
+    def test_nomination_pkg_public_new_version(self):
+        # New versions while public get a new version nomination.
+        app = app_factory()
+        app.update(is_packaged=True)
+        old_ver = app.versions.latest()
+        old_ver.update(nomination=self.days_ago(1))
+        v = Version.objects.create(addon=app, version='1.9')
+        self.assertCloseToNow(v.nomination)
+
 
 class TestPackagedAppManifestUpdates(amo.tests.TestCase):
     # Note: More extensive tests for `Addon.update_names` are in the Addon