[api][webui] Use a polymorphic relationship in Notifications

src/api/app/jobs/cleanup_notifications.rb

@@ -1,5 +1,5 @@
 class CleanupNotifications < ApplicationJob
   def perform
-    Notifications::RssFeedItem.cleanup
+    Notification::RssFeedItem.cleanup
   end
 end

src/api/app/jobs/send_event_emails.rb

@@ -25,7 +25,7 @@ def perform
 
   def create_rss_notifications(event)
     event.subscriptions.each do |subscription|
-      Notifications::RssFeedItem.create(
+      Notification::RssFeedItem.create(
         subscriber: subscription.subscriber,
         event_type: event.eventtype,
         event_payload: event.payload,

src/api/app/models/group.rb

@@ -11,7 +11,7 @@ class Group < ApplicationRecord
   has_many :event_subscriptions, dependent: :destroy, inverse_of: :group
   has_many :reviews, dependent: :nullify, as: :reviewable
 
-  has_many :rss_feed_items, -> { order(created_at: :desc) }, class_name: 'Notifications::RssFeedItem', dependent: :destroy
+  has_many :rss_feed_items, -> { order(created_at: :desc) }, class_name: 'Notification::RssFeedItem', as: :subscriber, dependent: :destroy
 
   validates :title,
             format: { with:    %r{\A[\w\.\-]*\z},

src/api/app/models/notifications/base.rb → src/api/app/models/notification.rb

@@ -1,6 +1,4 @@
-class Notifications::Base < ApplicationRecord
-  self.table_name = "notifications"
-
+class Notification < ApplicationRecord
   belongs_to :subscriber, polymorphic: true
 
   serialize :event_payload, Hash
@@ -15,18 +13,17 @@ def event
 # Table name: notifications
 #
 #  id                         :integer          not null, primary key
-#  user_id                    :integer          indexed
-#  group_id                   :integer          indexed
 #  type                       :string(255)      not null
 #  event_type                 :string(255)      not null
 #  event_payload              :text(65535)      not null
 #  subscription_receiver_role :string(255)      not null
 #  delivered                  :boolean          default(FALSE)
 #  created_at                 :datetime         not null
 #  updated_at                 :datetime         not null
+#  subscriber_type            :string(255)      indexed => [subscriber_id]
+#  subscriber_id              :integer          indexed => [subscriber_type]
 #
 # Indexes
 #
-#  index_notifications_on_group_id  (group_id)
-#  index_notifications_on_user_id   (user_id)
+#  index_notifications_on_subscriber_type_and_subscriber_id  (subscriber_type,subscriber_id)
 #

src/api/app/models/notifications/rss_feed_item.rb → src/api/app/models/notification/rss_feed_item.rb

@@ -1,4 +1,4 @@
-class Notifications::RssFeedItem < Notifications::Base
+class Notification::RssFeedItem < Notification
   MAX_ITEMS_PER_USER = 10
   MAX_ITEMS_PER_GROUP = 10
 
@@ -36,18 +36,17 @@ def description
 # Table name: notifications
 #
 #  id                         :integer          not null, primary key
-#  user_id                    :integer          indexed
-#  group_id                   :integer          indexed
 #  type                       :string(255)      not null
 #  event_type                 :string(255)      not null
 #  event_payload              :text(65535)      not null
 #  subscription_receiver_role :string(255)      not null
 #  delivered                  :boolean          default(FALSE)
 #  created_at                 :datetime         not null
 #  updated_at                 :datetime         not null
+#  subscriber_type            :string(255)      indexed => [subscriber_id]
+#  subscriber_id              :integer          indexed => [subscriber_type]
 #
 # Indexes
 #
-#  index_notifications_on_group_id  (group_id)
-#  index_notifications_on_user_id   (user_id)
+#  index_notifications_on_subscriber_type_and_subscriber_id  (subscriber_type,subscriber_id)
 #

src/api/app/models/user.rb

@@ -54,7 +54,7 @@ class User < ApplicationRecord
   # users have 0..1 user_registration records assigned to them
   has_one :user_registration
 
-  has_many :rss_feed_items, -> { order(created_at: :desc) }, class_name: 'Notifications::RssFeedItem', dependent: :destroy
+  has_many :rss_feed_items, -> { order(created_at: :desc) }, class_name: 'Notification::RssFeedItem', as: :subscriber, dependent: :destroy
 
   scope :all_without_nobody, -> { where("login != ?", nobody_login) }
 

src/api/db/migrate/20170619111734_alter_notifications_to_use_polymorphic.rb

@@ -0,0 +1,15 @@
+class AlterNotificationsToUsePolymorphic < ActiveRecord::Migration[5.0]
+  def change
+    add_reference :notifications, :subscriber, polymorphic: true
+
+    Notification.find_in_batches batch_size: 500 do |batch|
+      batch.each do |notification|
+        notification.subscriber = notification.user || notification.group
+        notification.save!
+      end
+    end
+
+    remove_reference :notifications, :group
+    remove_reference :notifications, :user
+  end
+end

src/api/db/structure.sql

@@ -713,18 +713,17 @@ CREATE TABLE `messages` (
 
 CREATE TABLE `notifications` (
   `id` int(11) NOT NULL AUTO_INCREMENT,
-  `user_id` int(11) DEFAULT NULL,
-  `group_id` int(11) DEFAULT NULL,
   `type` varchar(255) NOT NULL,
   `event_type` varchar(255) NOT NULL,
   `event_payload` text NOT NULL,
   `subscription_receiver_role` varchar(255) NOT NULL,
   `delivered` tinyint(1) DEFAULT '0',
   `created_at` datetime NOT NULL,
   `updated_at` datetime NOT NULL,
+  `subscriber_type` varchar(255) DEFAULT NULL,
+  `subscriber_id` int(11) DEFAULT NULL,
   PRIMARY KEY (`id`),
-  KEY `index_notifications_on_user_id` (`user_id`),
-  KEY `index_notifications_on_group_id` (`group_id`)
+  KEY `index_notifications_on_subscriber_type_and_subscriber_id` (`subscriber_type`,`subscriber_id`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
 CREATE TABLE `package_issues` (
@@ -1514,6 +1513,5 @@ INSERT INTO `schema_migrations` (version) VALUES
 ('6'),
 ('7'),
 ('8'),
-('9');
-
-
+('9'),
+('20170619111734');

src/api/spec/factories/notification.rb

@@ -1,10 +1,10 @@
 FactoryGirl.define do
-  factory :notification, class: 'Notifications::Base' do
+  factory :notification do
     event_type 'FakeEventType'
     event_payload 'FakeJsonPayload'
     subscription_receiver_role 'owner'
   end
 
-  factory :rss_notification, parent: :notification, class: 'Notifications::RssFeedItem' do
+  factory :rss_notification, parent: :notification, class: 'Notification::RssFeedItem' do
   end
 end

src/api/spec/jobs/send_event_emails_spec.rb

@@ -30,27 +30,27 @@
     end
 
     it "creates an rss notification for user's email" do
-      notification = Notifications::Base.find_by(subscriber: user)
+      notification = Notification.find_by(subscriber: user)
 
-      expect(notification.type).to eq('Notifications::RssFeedItem')
+      expect(notification.type).to eq('Notification::RssFeedItem')
       expect(notification.event_type).to eq('Event::CommentForProject')
       expect(notification.event_payload['comment_body']).to include('how are things?')
       expect(notification.subscription_receiver_role).to eq('all')
       expect(notification.delivered).to be_falsey
     end
 
     it "creates an rss notification for group's email" do
-      notification = Notifications::RssFeedItem.find_by(subscriber: group)
+      notification = Notification::RssFeedItem.find_by(subscriber: group)
 
-      expect(notification.type).to eq('Notifications::RssFeedItem')
+      expect(notification.type).to eq('Notification::RssFeedItem')
       expect(notification.event_type).to eq('Event::CommentForProject')
       expect(notification.event_payload['comment_body']).to include('how are things?')
       expect(notification.subscription_receiver_role).to eq('all')
       expect(notification.delivered).to be_falsey
     end
 
     it 'only creates two notifications' do
-      expect(Notifications::Base.count).to eq(2)
+      expect(Notification.count).to eq(2)
     end
   end
 end

src/api/spec/models/notification_rss_feed_item.rb

@@ -1,82 +1,85 @@
 require "rails_helper"
 
-RSpec.describe Notifications::RssFeedItem do
+RSpec.describe Notification::RssFeedItem do
+  let(:payload) { { comment: 'SuperFakeComment', requestid: 1 } }
+  let(:delete_package_event) { Event::DeletePackage.new(payload) }
+
   describe '.cleanup' do
     let(:user) { create(:confirmed_user) }
     let(:user2) { create(:confirmed_user) }
     let(:group) { create(:group, users: [user, user2]) }
     let(:unconfirmed_user) { create(:user) }
-    let(:max_items_per_user) { Notifications::RssFeedItem::MAX_ITEMS_PER_USER }
-    let(:max_items_per_group) { Notifications::RssFeedItem::MAX_ITEMS_PER_GROUP }
+    let(:max_items_per_user) { Notification::RssFeedItem::MAX_ITEMS_PER_USER }
+    let(:max_items_per_group) { Notification::RssFeedItem::MAX_ITEMS_PER_GROUP }
     let(:greater_than_max_items_per_user) { max_items_per_user + 3 }
     let(:greater_than_max_items_per_group) { max_items_per_group + 5 }
 
     context 'without any RSS items' do
-      it { expect { Notifications::RssFeedItem.cleanup }.not_to change { Notifications::RssFeedItem.count } }
+      it { expect { Notification::RssFeedItem.cleanup }.not_to change { Notification::RssFeedItem.count } }
     end
 
     context 'with a single users' do
       context 'and not enough RSS items' do
         before do
-          create_list(:rss_notification, max_items_per_user, user: user)
+          create_list(:rss_notification, max_items_per_user, subscriber: user)
         end
 
-        it { expect { Notifications::RssFeedItem.cleanup }.not_to change { Notifications::RssFeedItem.count } }
+        it { expect { Notification::RssFeedItem.cleanup }.not_to change { Notification::RssFeedItem.count } }
       end
 
       context 'and enough RSS items' do
         context 'for an active user' do
           before do
-            create_list(:rss_notification, greater_than_max_items_per_user, user: user)
+            create_list(:rss_notification, greater_than_max_items_per_user, subscriber: user)
           end
 
-          it { expect { Notifications::RssFeedItem.cleanup }.to change { Notifications::RssFeedItem.count }.by(-3) }
+          it { expect { Notification::RssFeedItem.cleanup }.to change { Notification::RssFeedItem.count }.by(-3) }
         end
 
         context 'for a non active user' do
           before do
-            create_list(:rss_notification, greater_than_max_items_per_user, user: unconfirmed_user)
+            create_list(:rss_notification, greater_than_max_items_per_user, subscriber: unconfirmed_user)
           end
 
-          it { expect { Notifications::RssFeedItem.cleanup }.to change { Notifications::RssFeedItem.count }.by(-greater_than_max_items_per_user) }
+          it { expect { Notification::RssFeedItem.cleanup }.to change { Notification::RssFeedItem.count }.by(-greater_than_max_items_per_user) }
         end
       end
     end
 
     context 'with multiple users' do
       context 'all of them active' do
         before do
-          create_list(:rss_notification, greater_than_max_items_per_user, user: user)
-          create_list(:rss_notification, greater_than_max_items_per_user, user: user2)
+          create_list(:rss_notification, greater_than_max_items_per_user, subscriber: user)
+          create_list(:rss_notification, greater_than_max_items_per_user, subscriber: user2)
         end
 
-        it { expect { Notifications::RssFeedItem.cleanup }.to change { user.rss_feed_items.count }.by(-3) }
-        it { expect { Notifications::RssFeedItem.cleanup }.to change { user2.rss_feed_items.count }.by(-3) }
+        it { expect { Notification::RssFeedItem.cleanup }.to change { user.rss_feed_items.count }.by(-3) }
+        it { expect { Notification::RssFeedItem.cleanup }.to change { user2.rss_feed_items.count }.by(-3) }
       end
 
       context 'being a mixture of active and non active users' do
         before do
-          create_list(:rss_notification, greater_than_max_items_per_user, user: user)
-          create_list(:rss_notification, greater_than_max_items_per_user, user: unconfirmed_user)
+          create_list(:rss_notification, greater_than_max_items_per_user, subscriber: user)
+          create_list(:rss_notification, greater_than_max_items_per_user, subscriber: unconfirmed_user)
         end
 
-        it { expect { Notifications::RssFeedItem.cleanup }.to change { user.rss_feed_items.count }.by(-3) }
-        it { expect { Notifications::RssFeedItem.cleanup }.to change { unconfirmed_user.rss_feed_items.count }.by(-greater_than_max_items_per_user) }
+        it { expect { Notification::RssFeedItem.cleanup }.to change { user.rss_feed_items.count }.by(-3) }
+        it { expect { Notification::RssFeedItem.cleanup }.to change { unconfirmed_user.rss_feed_items.count }.by(-greater_than_max_items_per_user) }
       end
     end
 
     context 'with users and group notifications' do
       before do
-        create_list(:rss_notification, greater_than_max_items_per_user, user: user)
-        create_list(:rss_notification, greater_than_max_items_per_user, user: user2)
-        create_list(:rss_notification, greater_than_max_items_per_user, user: unconfirmed_user)
-        create_list(:rss_notification, greater_than_max_items_per_group, group: group)
+        create_list(:rss_notification, greater_than_max_items_per_user, subscriber: user)
+        create_list(:rss_notification, greater_than_max_items_per_user, subscriber: user2)
+        create_list(:rss_notification, greater_than_max_items_per_user, subscriber: unconfirmed_user)
+        create_list(:rss_notification, greater_than_max_items_per_group, subscriber: group)
       end
 
-      it { expect { Notifications::RssFeedItem.cleanup }.to change { user.rss_feed_items.count }.by(-3) }
-      it { expect { Notifications::RssFeedItem.cleanup }.to change { user2.rss_feed_items.count }.by(-3) }
-      it { expect { Notifications::RssFeedItem.cleanup }.to change { group.rss_feed_items.count }.by(-5) }
-      it { expect { Notifications::RssFeedItem.cleanup }.to change { unconfirmed_user.rss_feed_items.count }.by(-greater_than_max_items_per_user) }
+      it { expect { Notification::RssFeedItem.cleanup }.to change { user.rss_feed_items.count }.by(-3) }
+      it { expect { Notification::RssFeedItem.cleanup }.to change { user2.rss_feed_items.count }.by(-3) }
+      it { expect { Notification::RssFeedItem.cleanup }.to change { group.rss_feed_items.count }.by(-5) }
+      it { expect { Notification::RssFeedItem.cleanup }.to change { unconfirmed_user.rss_feed_items.count }.by(-greater_than_max_items_per_user) }
     end
   end
 end

src/api/spec/models/notification_spec.rb

@@ -0,0 +1,13 @@
+require "rails_helper"
+
+RSpec.describe Notification do
+  let(:payload) { {comment: 'SuperFakeComment', requestid: 1} }
+  let(:delete_package_event) { Event::DeletePackage.new(payload) }
+
+  describe '#event' do
+    subject { create(:rss_notification, event_type: 'Event::DeletePackage', event_payload: payload).event }
+
+    it { expect(subject.class).to eq(delete_package_event.class) }
+    it { expect(subject.payload).to eq(delete_package_event.payload) }
+  end
+end

src/api/spec/models/user_spec.rb

@@ -727,4 +727,60 @@
       end
     end
   end
+
+  describe '#combined_rss_feed_items' do
+    let(:max_items_per_user) { Notification::RssFeedItem::MAX_ITEMS_PER_USER }
+    let(:max_items_per_group) { Notification::RssFeedItem::MAX_ITEMS_PER_GROUP }
+    let(:group) { create(:group) }
+    let!(:groups_user) { create(:groups_user, user: confirmed_user, group: group) }
+
+    context 'with a lot notifications from the user' do
+      before do
+        create_list(:rss_notification, max_items_per_group, subscriber: group)
+        create_list(:rss_notification, max_items_per_user + 5, subscriber: confirmed_user)
+        create_list(:rss_notification, 3, subscriber: user)
+      end
+
+      subject { confirmed_user.combined_rss_feed_items }
+
+      it { expect(subject.count).to be(max_items_per_user) }
+      it { expect(subject.any? { |x| x.subscriber != confirmed_user }).to be_falsey }
+      it { expect(subject.any? { |x| x.subscriber == group }).to be_falsey }
+      it { expect(subject.any? { |x| x.subscriber == user }).to be_falsey }
+    end
+
+    context 'with a lot notifications from the group' do
+      before do
+        create_list(:rss_notification, 5, subscriber: confirmed_user)
+        create_list(:rss_notification, max_items_per_group - 1, subscriber: group)
+        create_list(:rss_notification, 3, subscriber: user)
+      end
+
+      subject { confirmed_user.combined_rss_feed_items }
+
+      it { expect(subject.count).to be(max_items_per_user) }
+      it { expect(subject.select { |x| x.subscriber == confirmed_user }.length).to eq(1) }
+      it { expect(subject.select { |x| x.subscriber == group }.length).to eq(max_items_per_user - 1) }
+      it { expect(subject.any? { |x| x.subscriber == user }).to be_falsey }
+    end
+
+    context 'with a notifications mixed' do
+      let(:batch) { max_items_per_user / 4 }
+
+      before do
+        create_list(:rss_notification, max_items_per_user + batch, subscriber: confirmed_user)
+        create_list(:rss_notification, batch, subscriber: group)
+        create_list(:rss_notification, batch, subscriber: confirmed_user)
+        create_list(:rss_notification, batch, subscriber: group)
+        create_list(:rss_notification, 3, subscriber: user)
+      end
+
+      subject { confirmed_user.combined_rss_feed_items }
+
+      it { expect(subject.count).to be(max_items_per_user) }
+      it { expect(subject.select { |x| x.subscriber == confirmed_user }.length).to be >= batch * 2 }
+      it { expect(subject.select { |x| x.subscriber == group }.length).to eq(batch * 2) }
+      it { expect(subject.any? { |x| x.subscriber == user }).to be_falsey }
+    end
+  end
 end