fix notification eager loading for non journal notifications

* date alert notifications do not have a journal attached so the resource can not be assigned from that. * the eager loading wrapper became corrupted once the details were removed from the notification representer. The code has been adapted so that no more n+1 queries occur.
2 years ago · aff9842550
parent fce7780490
commit aff9842550
4 changed files with 62 additions and 68 deletions
--- a/lib/api/v3/notifications/notification_eager_loading_wrapper.rb
+++ b/lib/api/v3/notifications/notification_eager_loading_wrapper.rb
@ -32,20 +32,22 @@ module API
      class NotificationEagerLoadingWrapper < API::V3::Utilities::EagerLoading::EagerLoadingWrapper
        class << self
          def wrap(notifications)
-            API::V3::Activities::ActivityEagerLoadingWrapper.wrap(notifications.map(&:journal))
-            set_resource(notifications)
-
-            super
+            notifications
+              .includes(API::V3::Notifications::NotificationRepresenter.to_eager_load)
+              .to_a
+              .tap { |loaded_notifications| set_resource(loaded_notifications) }
          end

          private

-          # Copy the resource over from the journal.
-          # Those two should always be the same.
-          # The journable will be loaded within the ActivityEagerLoadingWrapper.
+          # The resource cannot be loaded by rails eager loading means (include)
+          # because it is a polymorphic association. That being as it is, currently only
+          # work packages are assigned.
          def set_resource(notifications)
-            notifications.select { |n| n.journal.present? }.each do |notification|
-              notification.resource = notification.journal.journable
+            work_packages_by_id = WorkPackage.where(id: notifications.pluck(:resource_id).uniq).index_by(&:id)
+
+            notifications.each do |notification|
+              notification.resource = work_packages_by_id[notification.resource_id]
            end
          end
        end
--- a/lib/api/v3/notifications/notification_representer.rb
+++ b/lib/api/v3/notifications/notification_representer.rb
@ -91,7 +91,7 @@ module API
          'Notification'
        end

-        self.to_eager_load = %i[project actor]
+        self.to_eager_load = %i[project actor journal]
      end
    end
  end
--- a/lib/api/v3/notifications/notifications_api.rb
+++ b/lib/api/v3/notifications/notifications_api.rb
@ -45,7 +45,6 @@ module API
            def notification_scope
              ::Notification
                .visible(current_user)
-                .includes(NotificationRepresenter.to_eager_load)
                .where
                .not(read_ian: nil)
            end
--- a/spec/requests/api/v3/notifications/index_resource_spec.rb
+++ b/spec/requests/api/v3/notifications/index_resource_spec.rb
@ -38,23 +38,23 @@ describe ::API::V3::Notifications::NotificationsAPI,
           member_in_project: work_package.project,
           member_with_permissions: %i[view_work_packages])
  end
-  shared_let(:notification1) do
+  shared_let(:mentioned_notification) do
    create(:notification,
           recipient:,
           resource: work_package,
           project: work_package.project,
           journal: work_package.journals.first)
  end
-  shared_let(:notification2) do
+  shared_let(:date_alert_notification) do
    create(:notification,
           recipient:,
+           reason: :date_alert_start_date,
           resource: work_package,
           project: work_package.project,
-           journal: work_package.journals.first)
+           journal: nil,
+           actor: nil)
  end

-  let(:notifications) { [notification1, notification2] }
-
  let(:filters) { nil }

  let(:send_request) do
@ -67,8 +67,6 @@ describe ::API::V3::Notifications::NotificationsAPI,
  end

  before do
-    notifications
-
    login_as current_user
    additional_setup

@ -78,13 +76,13 @@ describe ::API::V3::Notifications::NotificationsAPI,
  describe 'as the user with notifications' do
    let(:current_user) { recipient }

-    it_behaves_like 'API V3 collection response', 2, 2, 'Notification'
+    it_behaves_like 'API V3 collection response', 2, 2, 'Notification' do
+      let(:elements) { [date_alert_notification, mentioned_notification] }
+    end

    context 'with a readIAN filter' do
      let(:nil_notification) { create(:notification, recipient:, read_ian: nil) }

-      let(:notifications) { [notification1, notification2, nil_notification] }
-
      let(:filters) do
        [
          {
@ -99,21 +97,26 @@ describe ::API::V3::Notifications::NotificationsAPI,

      context 'with the filter being set to false' do
        it_behaves_like 'API V3 collection response', 2, 2, 'Notification' do
-          let(:elements) { [notification2, notification1] }
+          let(:elements) { [date_alert_notification, mentioned_notification] }
        end
      end
    end

    context 'with a resource filter' do
-      let(:notification3) { create(:notification, recipient:) }
-      let(:notifications) { [notification1, notification2, notification3] }
+      shared_let(:other_work_package) { create(:work_package, project: work_package.project) }
+      shared_let(:other_resource_notification) do
+        create(:notification,
+               recipient:,
+               project: work_package.project,
+               resource: other_work_package)
+      end

      let(:filters) do
        [
          {
            'resourceId' => {
              'operator' => '=',
-              'values' => [work_package.id.to_s]
+              'values' => [other_work_package.id.to_s]
            }
          },
          {
@ -125,41 +128,42 @@ describe ::API::V3::Notifications::NotificationsAPI,
        ]
      end

-      context 'with the filter being set to false' do
-        it_behaves_like 'API V3 collection response', 2, 2, 'Notification' do
-          let(:elements) { [notification2, notification1] }
-        end
+      it_behaves_like 'API V3 collection response', 1, 1, 'Notification' do
+        let(:elements) { [other_resource_notification] }
      end
    end

    context 'with a project filter' do
-      let(:other_work_package) { create(:work_package) }
-      let(:notification3) do
+      shared_let(:other_project) do
+        create(:project,
+               members: { recipient => recipient.members.first.roles })
+      end
+      shared_let(:other_work_package) { create(:work_package, project: other_project) }
+      shared_let(:other_project_notification) do
        create(:notification,
               recipient:,
               resource: other_work_package,
-               project: other_work_package.project)
+               project: other_project)
      end
-      let(:notifications) { [notification1, notification2, notification3] }

      let(:filters) do
        [
          {
            'project' => {
              'operator' => '=',
-              'values' => [work_package.project_id.to_s]
+              'values' => [other_work_package.project_id.to_s]
            }
          }
        ]
      end

-      it_behaves_like 'API V3 collection response', 2, 2, 'Notification' do
-        let(:elements) { [notification2, notification1] }
+      it_behaves_like 'API V3 collection response', 1, 1, 'Notification' do
+        let(:elements) { [other_project_notification] }
      end
    end

    context 'with a reason filter' do
-      let(:notification3) do
+      shared_let(:assigned_notification) do
        create(:notification,
               reason: :assigned,
               recipient:,
@ -167,7 +171,7 @@ describe ::API::V3::Notifications::NotificationsAPI,
               project: work_package.project,
               journal: work_package.journals.first)
      end
-      let(:notification4) do
+      shared_let(:responsible_notification) do
        create(:notification,
               reason: :responsible,
               recipient:,
@ -175,21 +179,20 @@ describe ::API::V3::Notifications::NotificationsAPI,
               project: work_package.project,
               journal: work_package.journals.first)
      end
-      let(:notifications) { [notification1, notification2, notification3, notification4] }

      let(:filters) do
        [
          {
            'reason' => {
              'operator' => '=',
-              'values' => [notification1.reason.to_s, notification4.reason.to_s]
+              'values' => [mentioned_notification.reason.to_s, responsible_notification.reason.to_s, 'dateAlert']
            }
          }
        ]
      end

      it_behaves_like 'API V3 collection response', 3, 3, 'Notification' do
-        let(:elements) { [notification4, notification2, notification1] }
+        let(:elements) { [responsible_notification, date_alert_notification, mentioned_notification] }
      end

      context 'with an invalid reason' do
@ -216,9 +219,9 @@ describe ::API::V3::Notifications::NotificationsAPI,
    end

    context 'with a non ian notification' do
-      let(:wiki_page) { create(:wiki_page_with_content) }
+      shared_let(:wiki_page) { create(:wiki_page_with_content) }

-      let(:non_ian_notification) do
+      shared_let(:non_ian_notification) do
        create(:notification,
               read_ian: nil,
               recipient:,
@ -227,15 +230,13 @@ describe ::API::V3::Notifications::NotificationsAPI,
               journal: wiki_page.content.journals.first)
      end

-      let(:notifications) { [notification2, notification1, non_ian_notification] }
-
      it_behaves_like 'API V3 collection response', 2, 2, 'Notification' do
-        let(:elements) { [notification2, notification1] }
+        let(:elements) { [date_alert_notification, mentioned_notification] }
      end
    end

    context 'with a reason groupBy' do
-      let(:responsible_notification) do
+      shared_let(:responsible_notification) do
        create(:notification,
               recipient:,
               reason: :responsible,
@ -244,24 +245,14 @@ describe ::API::V3::Notifications::NotificationsAPI,
               journal: work_package.journals.first)
      end

-      let(:start_date_notification) do
-        create(:notification,
-               recipient:,
-               reason: :date_alert_start_date,
-               resource: work_package,
-               project: work_package.project)
-      end
-
-      let(:due_date_notification) do
+      shared_let(:due_date_alert_notification) do
        create(:notification,
               recipient:,
               reason: :date_alert_due_date,
               resource: work_package,
-               project: work_package.project)
-      end
-
-      let(:notifications) do
-        [notification1, notification2, responsible_notification, start_date_notification, due_date_notification]
+               project: work_package.project,
+               journal: nil,
+               actor: nil)
      end

      let(:send_request) do
@ -270,7 +261,11 @@ describe ::API::V3::Notifications::NotificationsAPI,

      let(:groups) { parsed_response['groups'] }

-      it_behaves_like 'API V3 collection response', 5, 5, 'Notification'
+      it_behaves_like 'API V3 collection response', 4, 4, 'Notification' do
+        let(:elements) do
+          [mentioned_notification, responsible_notification, date_alert_notification, due_date_alert_notification]
+        end
+      end

      it 'contains the reason groups', :aggregate_failures do
        expect(groups).to be_a Array
@ -278,19 +273,19 @@ describe ::API::V3::Notifications::NotificationsAPI,

        keyed = groups.index_by { |el| el['value'] }
        expect(keyed.keys).to contain_exactly 'mentioned', 'responsible', 'dateAlert'
-        expect(keyed['mentioned']['count']).to eq 2
+        expect(keyed['mentioned']['count']).to eq 1
        expect(keyed['responsible']['count']).to eq 1
        expect(keyed['dateAlert']['count']).to eq 2
      end
    end

    context 'with a project groupBy' do
-      let(:other_project) do
+      shared_let(:other_project) do
        create(:project,
               members: { recipient => recipient.members.first.roles })
      end
-      let(:work_package2) { create(:work_package, project: other_project) }
-      let(:other_project_notification) do
+      shared_let(:work_package2) { create(:work_package, project: other_project) }
+      shared_let(:other_project_notification) do
        create(:notification,
               resource: work_package2,
               project: other_project,
@ -299,8 +294,6 @@ describe ::API::V3::Notifications::NotificationsAPI,
               journal: work_package2.journals.first)
      end

-      let(:notifications) { [notification1, notification2, other_project_notification] }
-
      let(:send_request) do
        get api_v3_paths.path_for :notifications, group_by: :project
      end