Simplify ActivityLog.objects.for_*() methods and implement for_versions() (#15106)

* Simplify ActivityLog.objects.for_*() methods and implement for_versions()

The simplification makes us do JOINs instead of an extra query and avoids
a costly evaluation of the results just to build a list of pks when there
are many logs returned.

for_versions() replaces for_version(), works the same way as for_addons(),
if a single Version instance is passed it converts it to a tuple of one
item behind the scenes.

* Fix number of queries expected

* Filter on blocklog__guid instead of blocklog__block__guid since that's possible

src/olympia/activity/models.py

@@ -218,18 +218,13 @@ class ActivityLogManager(ManagerBase):
         if isinstance(addons, Addon):
             addons = (addons,)
 
-        vals = (AddonLog.objects.filter(addon__in=addons)
-                .values_list('activity_log', flat=True))
+        return self.filter(addonlog__addon__in=addons)
 
-        if vals:
-            return self.filter(pk__in=list(vals))
-        else:
-            return self.none()
+    def for_versions(self, versions):
+        if isinstance(versions, Version):
+            versions = (versions,)
 
-    def for_version(self, version):
-        vals = (VersionLog.objects.filter(version=version)
-                .values_list('activity_log', flat=True))
-        return self.filter(pk__in=list(vals))
+        return self.filter(versionlog__version__in=versions)
 
     def for_groups(self, groups):
         if isinstance(groups, Group):
@@ -238,19 +233,13 @@ class ActivityLogManager(ManagerBase):
         return self.filter(grouplog__group__in=groups)
 
     def for_user(self, user):
-        vals = (UserLog.objects.filter(user=user)
-                .values_list('activity_log', flat=True))
-        return self.filter(pk__in=list(vals))
+        return self.filter(userlog__user=user)
 
     def for_block(self, block):
-        vals = (BlockLog.objects.filter(block=block)
-                .values_list('activity_log', flat=True))
-        return self.filter(pk__in=list(vals))
+        return self.filter(blocklog__block=block)
 
     def for_guidblock(self, guid):
-        vals = (BlockLog.objects.filter(guid=guid)
-                .values_list('activity_log', flat=True))
-        return self.filter(pk__in=list(vals))
+        return self.filter(blocklog__guid=guid)
 
     def for_developer(self):
         return self.exclude(action__in=constants.activity.LOG_ADMINS +

src/olympia/activity/tests/test_models.py

@@ -219,10 +219,21 @@ class TestActivityLog(TestCase):
         version = Version.objects.all()[0]
         ActivityLog.create(amo.LOG.REJECT_VERSION, version.addon, version,
                            user=self.request.user)
-        entries = ActivityLog.objects.for_version(version)
+        entries = ActivityLog.objects.for_versions(version)
         assert len(entries) == 1
         assert version.get_url_path() in str(entries[0])
 
+    def test_version_log_multiple(self):
+        addon = Addon.objects.get()
+        version = version_factory(addon=addon)
+        addon_factory()  # To create an extra unrelated version
+        for version in Version.objects.all():
+            ActivityLog.create(
+                amo.LOG.REJECT_VERSION, version.addon, version,
+                user=self.request.user)
+        entries = ActivityLog.objects.for_versions(addon.versions.all())
+        assert len(entries) == 2
+
     def test_version_log_unlisted_addon(self):
         version = Version.objects.all()[0]
         # Get the url before the addon is changed to unlisted.
@@ -230,7 +241,7 @@ class TestActivityLog(TestCase):
         self.make_addon_unlisted(version.addon)
         ActivityLog.create(amo.LOG.REJECT_VERSION, version.addon, version,
                            user=self.request.user)
-        entries = ActivityLog.objects.for_version(version)
+        entries = ActivityLog.objects.for_versions(version)
         assert len(entries) == 1
         assert url_path not in str(entries[0])
 

src/olympia/activity/tests/test_views.py

@@ -197,13 +197,12 @@ class TestReviewNotesViewSetList(ReviewNotesViewSetDetailMixin, TestCase):
         self.note4 = self.log(u'fiiiine', amo.LOG.REVIEWER_REPLY_VERSION,
                               self.days_ago(0))
         self._login_developer()
-        with self.assertNumQueries(19):
+        with self.assertNumQueries(17):
             # - 2 savepoints because of tests
             # - 2 user and groups
             # - 2 addon and its translations
             # - 1 addon author lookup (permission check)
             # - 1 version (no transforms at all)
-            # - 1 activity log version to find relevant activity logs
             # - 1 count of activity logs
             # - 1 activity logs themselves
             # - 1 user
@@ -211,7 +210,7 @@ class TestReviewNotesViewSetList(ReviewNotesViewSetDetailMixin, TestCase):
             #   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)
-            # - 3 for highlighting (repeats the query to fetch the activity log
+            # - 2 for highlighting (repeats the query to fetch the activity log
             #   per version)
             response = self.client.get(self.url)
             assert response.status_code == 200

src/olympia/activity/utils.py

@@ -305,7 +305,7 @@ def notify_about_activity_log(addon, version, note, perm_setting=None,
         # for automated messages), build the context for them and send them
         # their copy.
         log_users = {
-            alog.user for alog in ActivityLog.objects.for_version(version) if
+            alog.user for alog in ActivityLog.objects.for_versions(version) if
             acl.is_user_any_kind_of_reviewer(alog.user)}
         reviewers = log_users - addon_authors - task_user - {note.user}
         reviewer_context_dict = author_context_dict.copy()

src/olympia/activity/views.py

@@ -34,7 +34,7 @@ class VersionReviewNotesViewSet(AddonChildMixin, ListModelMixin,
     serializer_class = ActivityLogSerializer
 
     def get_queryset(self):
-        alog = ActivityLog.objects.for_version(self.get_version_object())
+        alog = ActivityLog.objects.for_versions(self.get_version_object())
         return alog.filter(action__in=amo.LOG_REVIEW_QUEUE_DEVELOPER)
 
     def get_addon_object(self):

src/olympia/blocklist/tests/test_admin.py

@@ -291,11 +291,11 @@ class TestBlocklistSubmissionAdmin(TestCase):
             action=log.action).last()
         assert block_log_by_guid == log
 
-        assert log == ActivityLog.objects.for_version(first_version).last()
-        assert log == ActivityLog.objects.for_version(second_version).last()
-        assert log == ActivityLog.objects.for_version(
+        assert log == ActivityLog.objects.for_versions(first_version).last()
+        assert log == ActivityLog.objects.for_versions(second_version).last()
+        assert log == ActivityLog.objects.for_versions(
             deleted_addon_version).last()
-        assert not ActivityLog.objects.for_version(pending_version).exists()
+        assert not ActivityLog.objects.for_versions(pending_version).exists()
         assert [msg.message for msg in response.context['messages']] == [
             'The blocklist submission "No Sign-off: guid@; dfd; some reason" '
             'was added successfully.']
@@ -494,7 +494,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         block_log = ActivityLog.objects.for_block(new_block).filter(
             action=add_log.action).last()
         assert block_log == add_log
-        vlog = ActivityLog.objects.for_version(
+        vlog = ActivityLog.objects.for_versions(
             new_addon.current_version).last()
         assert vlog == add_log
 
@@ -522,7 +522,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         block_log = ActivityLog.objects.for_block(existing_and_partial).filter(
             action=edit_log.action).last()
         assert block_log == edit_log
-        vlog = ActivityLog.objects.for_version(
+        vlog = ActivityLog.objects.for_versions(
             partial_addon.current_version).last()
         assert vlog == edit_log
 
@@ -533,7 +533,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert existing_and_full.url != 'dfd'
         assert not ActivityLog.objects.for_addons(
             existing_and_full.addon).exists()
-        assert not ActivityLog.objects.for_version(
+        assert not ActivityLog.objects.for_versions(
             existing_and_full.addon.current_version).exists()
 
         assert submission.input_guids == (
@@ -678,7 +678,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         block_log = ActivityLog.objects.for_block(new_block).filter(
             action=log.action).last()
         assert block_log == log
-        vlog = ActivityLog.objects.for_version(
+        vlog = ActivityLog.objects.for_versions(
             new_addon.current_version).last()
         assert vlog == log
 
@@ -701,7 +701,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         block_log = ActivityLog.objects.for_block(existing_zero_to_max).filter(
             action=log.action).last()
         assert block_log == log
-        vlog = ActivityLog.objects.for_version(
+        vlog = ActivityLog.objects.for_versions(
             existing_zero_to_max.addon.current_version).last()
         assert vlog == log
 
@@ -713,7 +713,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert existing_one_to_ten.in_legacy_blocklist is False
         assert not ActivityLog.objects.for_addons(
             existing_one_to_ten.addon).exists()
-        assert not ActivityLog.objects.for_version(
+        assert not ActivityLog.objects.for_versions(
             existing_one_to_ten.addon.current_version).exists()
 
         submission = BlocklistSubmission.objects.get()
@@ -1212,7 +1212,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         block_log = ActivityLog.objects.for_block(new_block).filter(
             action=add_log.action).last()
         assert block_log == add_log
-        vlog = ActivityLog.objects.for_version(addon.current_version).last()
+        vlog = ActivityLog.objects.for_versions(addon.current_version).last()
         assert vlog == add_log
 
         assert signoff_log.action == amo.LOG.BLOCKLIST_SIGNOFF.id
@@ -1557,7 +1557,7 @@ class TestBlockAdminEdit(TestCase):
         block_log_by_guid = ActivityLog.objects.for_guidblock('guid@').filter(
             action=log.action).last()
         assert block_log_by_guid == log
-        vlog = ActivityLog.objects.for_version(
+        vlog = ActivityLog.objects.for_versions(
             self.addon.current_version).last()
         assert vlog == log
 
@@ -2050,7 +2050,7 @@ class TestBlockAdminDelete(TestCase):
         else:
             assert add_log.details['signoff_state'] == 'No Sign-off'
             assert 'signoff_by' not in add_log.details
-        vlog = ActivityLog.objects.for_version(
+        vlog = ActivityLog.objects.for_versions(
             block_from_addon.current_version).last()
         assert vlog == add_log
 

src/olympia/devhub/templatetags/jinja_helpers.py

@@ -130,6 +130,6 @@ def version_disabled(version):
 
 @library.global_function
 def pending_activity_log_count_for_developer(version):
-    alog = ActivityLog.objects.for_version(version).filter(
+    alog = ActivityLog.objects.for_versions(version).filter(
         action__in=amo.LOG_REVIEW_QUEUE_DEVELOPER)
     return filter_queryset_to_pending_replies(alog).count()