diff --git a/src/olympia/activity/models.py b/src/olympia/activity/models.py index 149f7c2afc4..79201710f48 100644 --- a/src/olympia/activity/models.py +++ b/src/olympia/activity/models.py @@ -3,7 +3,7 @@ import uuid from collections import defaultdict from copy import copy -from datetime import datetime +from datetime import date, datetime from inspect import isclass from django.apps import apps @@ -41,6 +41,18 @@ GENERIC_USER_NAME = gettext('Add-ons Review Team') +# Activity ids that are not considered as needing a reply from developers, so +# they are never considered "pending". +NOT_PENDING_IDS = ( + amo.LOG.DEVELOPER_REPLY_VERSION.id, + amo.LOG.APPROVE_VERSION.id, + amo.LOG.REJECT_VERSION.id, + amo.LOG.PRELIMINARY_VERSION.id, + amo.LOG.PRELIMINARY_ADDON_MIGRATED.id, + amo.LOG.NOTES_FOR_REVIEWERS_CHANGED.id, + amo.LOG.SOURCE_CODE_UPLOADED.id, +) + def attachment_upload_path(instance, filename): ext = os.path.splitext(filename)[1] @@ -313,6 +325,38 @@ class ActivityLogQuerySet(BaseQuerySet): def default_transformer(self, logs): ActivityLog.arguments_builder(logs) + def pending_for_developer(self, for_version=None): + """Return ActivityLog that are considered "pending" for developers. + + An Activity will be considered "pending" if it's a review queue + activity not hidden to developers that is more recent that the latest + activity created by a developer/reviewer. Said differently: if a + developer doesn't do something after a reviewer action, that reviewer + action will be considered pending.""" + if for_version is None: + for_version = models.OuterRef('versionlog__version_id') + latest_reply_date = models.functions.Coalesce( + models.Subquery( + self.filter( + action__in=NOT_PENDING_IDS, + versionlog__version=for_version, + ) + .values('created') + .order_by('-created')[:1] + ), + date.min, + ) + return ( + # The subquery needs to run on the already filterd activities we + # care about (it uses `self`, i.e. the current state of the + # queryset), so we need to filter by action first, then trigger the + # subquery, then filter by the result, we can't group all of that + # in a single filter() call. + self.filter(action__in=amo.LOG_REVIEW_QUEUE_DEVELOPER) + .annotate(latest_reply_date=latest_reply_date) + .filter(created__gt=models.F('latest_reply_date')) + ) + class ActivityLogManager(ManagerBase): _queryset_class = ActivityLogQuerySet diff --git a/src/olympia/activity/tests/test_models.py b/src/olympia/activity/tests/test_models.py index 1c9e6f56699..24176800a98 100644 --- a/src/olympia/activity/tests/test_models.py +++ b/src/olympia/activity/tests/test_models.py @@ -99,6 +99,129 @@ def test_rejected_version_still_valid(self): assert self.token.is_valid() +class TestActivityLogManager(TestCase): + def test_pending_for_developer(self): + to_create = ( + # Tests with Developer_Reply + ( + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.DEVELOPER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + 1, + ), + ( + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.DEVELOPER_REPLY_VERSION, + 0, + ), + # Tests with Approval + ( + amo.LOG.APPROVE_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + 2, + ), + ( + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.APPROVE_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + 1, + ), + ( + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.APPROVE_VERSION, + 0, + ), + # Tests with Rejection + ( + amo.LOG.REJECT_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + 2, + ), + ( + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REJECT_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + 1, + ), + ( + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REJECT_VERSION, + 0, + ), + # Test with no approve or reject + ( + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + amo.LOG.REVIEWER_REPLY_VERSION, + 3, + ), + ) + + user = user_factory() + addon = addon_factory() + expected = [] + for action1, action2, action3, count in to_create: + version = version_factory(addon=addon) + logs = ( + ActivityLog.objects.create(action1, addon, version, user=user), + ActivityLog.objects.create(action2, addon, version, user=user), + ActivityLog.objects.create(action3, addon, version, user=user), + ) + logs[-3].update(created=self.days_ago(2)) + logs[-2].update(created=self.days_ago(1)) + logs[-1].update(created=self.days_ago(0)) + if count: + expected.extend(logs[-count:]) + results = list(ActivityLog.objects.for_addons(addon).pending_for_developer()) + assert len(results) == len(expected) + assert set(results) == set(expected) + + def test_with_reply_going_to_multiple_versions_with_developer_reply(self): + user = user_factory() + addon = addon_factory() + v1 = addon.current_version + v2 = version_factory(addon=addon) + # Make a reviewer reply on both versions + grouped_reviewer_reply = ActivityLog.objects.create( + amo.LOG.REVIEWER_REPLY_VERSION, + addon, + v1, + v2, + user=user, + ) + grouped_reviewer_reply.update(created=self.days_ago(42)) + # Make the developer reply only on one of the versions + developer_reply_on_v1 = ActivityLog.objects.create( + amo.LOG.DEVELOPER_REPLY_VERSION, + addon, + v1, + user=user, + ) + developer_reply_on_v1.update(created=self.days_ago(41)) + + # Extra data that shouldn't be relevant + version_factory(addon=addon) + extra_addon = addon_factory() + ActivityLog.objects.create( + amo.LOG.REVIEWER_REPLY_VERSION, + extra_addon, + extra_addon.current_version, + user=user, + ) + results = list( + ActivityLog.objects.for_versions( + addon.versions.all() + ).pending_for_developer() + ) + assert len(results) == 1 + assert results[0] == grouped_reviewer_reply + + class TestActivityLog(TestCase): fixtures = ['base/addon_3615'] diff --git a/src/olympia/activity/tests/test_views.py b/src/olympia/activity/tests/test_views.py index fdfdaf2e154..e1a0ab47619 100644 --- a/src/olympia/activity/tests/test_views.py +++ b/src/olympia/activity/tests/test_views.py @@ -302,7 +302,7 @@ def test_queries(self): 'fiiiine', amo.LOG.REVIEWER_REPLY_VERSION, self.days_ago(0) ) self._login_developer() - with self.assertNumQueries(17): + with self.assertNumQueries(16): # - 2 savepoints because of tests # - 2 user and groups # - 2 addon and its translations @@ -315,8 +315,7 @@ def test_queries(self): # enough yet to pass that to the activity log queryset, it's # difficult since it's not a FK) # - 2 version and its translations (same issue) - # - 2 for highlighting (repeats the query to fetch the activity log - # per version) + # - 1 for highlighting "pending" activities response = self.client.get(self.url) assert response.status_code == 200 diff --git a/src/olympia/activity/utils.py b/src/olympia/activity/utils.py index db71dd457ca..1820b35dc00 100644 --- a/src/olympia/activity/utils.py +++ b/src/olympia/activity/utils.py @@ -394,28 +394,6 @@ def send_activity_mail( ) -NOT_PENDING_IDS = ( - amo.LOG.DEVELOPER_REPLY_VERSION.id, - amo.LOG.APPROVE_VERSION.id, - amo.LOG.REJECT_VERSION.id, - amo.LOG.PRELIMINARY_VERSION.id, - amo.LOG.PRELIMINARY_ADDON_MIGRATED.id, - amo.LOG.NOTES_FOR_REVIEWERS_CHANGED.id, - amo.LOG.SOURCE_CODE_UPLOADED.id, -) - - -def filter_queryset_to_pending_replies(queryset, log_type_ids=NOT_PENDING_IDS): - latest_reply_date = ( - queryset.filter(action__in=log_type_ids) - .values_list('created', flat=True) - .first() - ) - if not latest_reply_date: - return queryset - return queryset.filter(created__gt=latest_reply_date) - - def bounce_mail(message, reason): recipient_header = ( None diff --git a/src/olympia/activity/views.py b/src/olympia/activity/views.py index 2502cc3184f..1eca0697ae1 100644 --- a/src/olympia/activity/views.py +++ b/src/olympia/activity/views.py @@ -27,7 +27,6 @@ from olympia.activity.tasks import process_email from olympia.activity.utils import ( action_from_user, - filter_queryset_to_pending_replies, log_and_notify, ) from olympia.addons.views import AddonChildMixin @@ -96,9 +95,7 @@ def check_permissions(self, request): def get_serializer_context(self): ctx = super().get_serializer_context() ctx['to_highlight'] = list( - filter_queryset_to_pending_replies(self.get_queryset()).values_list( - 'pk', flat=True - ) + self.get_queryset().pending_for_developer().values_list('pk', flat=True) ) return ctx diff --git a/src/olympia/devhub/templates/devhub/versions/list.html b/src/olympia/devhub/templates/devhub/versions/list.html index 216af93013b..3469dd04b8f 100644 --- a/src/olympia/devhub/templates/devhub/versions/list.html +++ b/src/olympia/devhub/templates/devhub/versions/list.html @@ -62,12 +62,10 @@ {{ _('Review History') }} - {% if latest_version_in_channel_including_disabled == version %} - {% set pending_count = pending_activity_log_count_for_developer(version) %} - {% if pending_count > 0 %} - {{ pending_count }} - {% endif %} - {% endif %} + {% set pending_count = pending_activity_log_count_for_developer(version) %} + {% if pending_count > 0 %} + {{ pending_count }} + {% endif %} diff --git a/src/olympia/devhub/templatetags/jinja_helpers.py b/src/olympia/devhub/templatetags/jinja_helpers.py index 8a00229b45c..027093fcd82 100644 --- a/src/olympia/devhub/templatetags/jinja_helpers.py +++ b/src/olympia/devhub/templatetags/jinja_helpers.py @@ -6,7 +6,6 @@ from olympia import amo from olympia.access import acl from olympia.activity.models import ActivityLog -from olympia.activity.utils import filter_queryset_to_pending_replies from olympia.amo.templatetags.jinja_helpers import format_date, new_context, page_title from olympia.files.models import File @@ -88,10 +87,12 @@ def summarize_validation(validation): @library.global_function def pending_activity_log_count_for_developer(version): - alog = ActivityLog.objects.for_versions(version).filter( - action__in=amo.LOG_REVIEW_QUEUE_DEVELOPER - ) - return filter_queryset_to_pending_replies(alog).count() + # unread_count is an annotation set by version_list() view to do this once + # for all versions in the list. We use it if it's present, otherwise fall + # back to the per-version computation. + if hasattr(version, 'unread_count'): + return version.unread_count + return ActivityLog.objects.for_versions(version).pending_for_developer().count() @library.global_function diff --git a/src/olympia/devhub/tests/test_views_versions.py b/src/olympia/devhub/tests/test_views_versions.py index 23a71337bf1..68b16f55361 100644 --- a/src/olympia/devhub/tests/test_views_versions.py +++ b/src/olympia/devhub/tests/test_views_versions.py @@ -282,7 +282,6 @@ def _extra_version_and_file(self, status): version_two = version_factory( addon=self.addon, license=version.license, - version='1.2.3', file_kw={'status': status}, ) return version_two, version_two.file @@ -658,8 +657,14 @@ def test_version_history_mixed_channels(self): ) def test_pending_activity_count(self): + v1 = self.addon.current_version + v1.update(created=self.days_ago(1)) v2, _ = self._extra_version_and_file(amo.STATUS_AWAITING_REVIEW) + v3, _ = self._extra_version_and_file(amo.STATUS_APPROVED) # Add some activity log messages + ActivityLog.objects.create( + amo.LOG.REVIEWER_REPLY_VERSION, v1.addon, v1, user=self.user + ) ActivityLog.objects.create( amo.LOG.REVIEWER_REPLY_VERSION, v2.addon, v2, user=self.user ) @@ -667,17 +672,66 @@ def test_pending_activity_count(self): amo.LOG.REVIEWER_REPLY_VERSION, v2.addon, v2, user=self.user ) - response = self.client.get(self.url) + with self.assertNumQueries(43): + # FIXME: lots of optimizations left to do. That count shouldn't go + # higher. + # 1. SAVEPOINT + # 2. the add-on + # 3. translations for that add-on (default transformer) + # 4. categories for that add-on (default transformer) + # 5. current version for that add-on (default transformer) + # 6. translations for the current version (default transformer) + # 7. applications versions for the current version (default transformer) + # 8. users for that add-on (default transformer) + # 9. previews for that add-on (default transformer) + # 10. current user + # 11. groups for that user + # 12. check on user being an author + # 13. count versions for the add-on for pagination + # 14. RELEASE SAVEPOINT + # 15. Add-ons for that user + # 16. Latest version in listed channel + # 17. Translations for that version + # 18. Latest version in unlisted channel + # 19. Latest public version in listed channel + # 20. Translations for that version + # 21. check on user being an author (dupe) + # 22. site notice + # 23. suppressed email waffle switch check + # 24. 8 latest add-ons from that user for the menu + # 25. translations for those add-ons + # 26. authors for those add-ons + # 27. count of pending activities on latest version + # 28. file validation for that latest version + # 29. is add-on promoted (for deletion warning) + # 30. check on user being an author (dupe) + # 31. versions being displayed w/ pending activities count and file attached + # 32. translations for those versions + # 33. applications versions for those versions + # 34. file validation + # 35. file validation (other version) + # 36. check on user being an author (dupe) + # 37. latest non-disabled version + # 38. translations for that version + # 39. are there versions in unlisted channel + # 40. versions in unlisted channel + # 41. translations for those versions + # 42. latest non-disabled version in unlisted channel + # 43. check on user being an author (dupe) + response = self.client.get(self.url) assert response.status_code == 200 doc = pq(response.content) - # Two versions... - assert doc('.review-history-show').length == 2 - # ...but only one counter, for the latest version + # Three versions... + assert doc('.review-history-show').length == 3 + # ...2 have pending activities pending_activity_count = doc('.review-history-pending-count') - assert pending_activity_count.length == 1 - # There are two activity logs pending - assert pending_activity_count.text() == '2' + assert pending_activity_count.length == 2 + # There are two activity logs pending on v2, one on v1. + pending_activity_count_for_v2 = pending_activity_count[0] + assert pending_activity_count_for_v2.text_content() == '2' + pending_activity_count_for_v1 = pending_activity_count[1] + assert pending_activity_count_for_v1.text_content() == '1' def test_channel_tag(self): self.addon.current_version.update(created=self.days_ago(1)) diff --git a/src/olympia/devhub/views.py b/src/olympia/devhub/views.py index f01bc6c7579..0a794693502 100644 --- a/src/olympia/devhub/views.py +++ b/src/olympia/devhub/views.py @@ -10,7 +10,7 @@ from django.core.exceptions import PermissionDenied from django.core.files.storage import default_storage as storage from django.db import transaction -from django.db.models import Count +from django.db.models import Count, F, Func, OuterRef, Subquery from django.http import JsonResponse from django.shortcuts import get_object_or_404, redirect from django.template import loader @@ -1267,7 +1267,30 @@ def check_validation_override(request, form, addon, version): @dev_required def version_list(request, addon_id, addon): - qs = addon.versions.order_by('-created') + unread_count = ( + ( + ActivityLog.objects.all() + # There are 2 subquery: the one in pending_for_developer() to + # determine the date that determines whether an activity is pending + # or not, and then that queryset which is applied for each version. + # That means the version filtering needs to be applied twice: for + # both the date threshold (inner subquery, so the version id to + # refer to is the parent of the parent) and the unread count itself + # ("regular" subquery so the version id to refer to is just the + # parent). + .pending_for_developer(for_version=OuterRef(OuterRef('id'))) + # pending_for_developer() evaluates the queryset it's called from + # so we have to apply our second filter w/ OuterRef *after* calling + # it, otherwise OuterRef would point to the wrong parent. + .filter(versionlog__version=OuterRef('id')) + .values('id') + ) + .annotate(count=Func(F('id'), function='COUNT')) + .values('count') + ) + qs = addon.versions.annotate(unread_count=Subquery(unread_count)).order_by( + '-created' + ) versions = amo_utils.paginate(request, qs) is_admin = acl.action_allowed_for(request.user, amo.permissions.REVIEWS_ADMIN)