From 2364c2653f3288abe4ea6ec78d2826c87d308407 Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Thu, 25 Apr 2019 08:54:50 +0200 Subject: [PATCH] Add bulk insert for UserSystemNotifications, New service for sending emails --- app/models/user_system_notification.rb | 13 +----- ...cation_in_communication_channel_service.rb | 27 ++++++++++++ .../push_to_communication_channel_service.rb | 40 ++++++++++++++++++ .../sync_system_notifications_service.rb | 19 ++++++++- ...dd_unique_index_to_system_notifications.rb | 12 ++++++ db/schema.rb | 5 ++- spec/factories/system_notifications.rb | 2 +- spec/models/user_system_notification_spec.rb | 30 ------------- ...n_in_communication_channel_service_spec.rb | 42 +++++++++++++++++++ ...h_to_communication_channel_service_spec.rb | 36 ++++++++++++++++ .../sync_system_notifications_service_spec.rb | 18 ++++++++ 11 files changed, 198 insertions(+), 46 deletions(-) create mode 100644 app/services/notifications/handle_system_notification_in_communication_channel_service.rb create mode 100644 app/services/notifications/push_to_communication_channel_service.rb create mode 100644 db/migrate/20190424113216_add_unique_index_to_system_notifications.rb create mode 100644 spec/services/notifications/handle_system_notification_in_communication_channel_service_spec.rb create mode 100644 spec/services/notifications/push_to_communication_channel_service_spec.rb diff --git a/app/models/user_system_notification.rb b/app/models/user_system_notification.rb index d98825c1c..cfa45813a 100644 --- a/app/models/user_system_notification.rb +++ b/app/models/user_system_notification.rb @@ -4,8 +4,7 @@ class UserSystemNotification < ApplicationRecord belongs_to :user belongs_to :system_notification - after_create :send_email, - if: proc { |sn| sn.user.system_message_email_notification } + validates :system_notification, uniqueness: { scope: :user } scope :unseen, -> { where(seen_at: nil) } @@ -15,9 +14,7 @@ class UserSystemNotification < ApplicationRecord def self.mark_as_read(notification_id) notification = find_by_system_notification_id(notification_id) - if notification && notification.read_at.nil? - notification.update(read_at: Time.now) - end + notification.update(read_at: Time.now) if notification && notification.read_at.nil? end def self.show_on_login(update_read_time = false) @@ -45,10 +42,4 @@ class UserSystemNotification < ApplicationRecord notification end end - - private - - def send_email - AppMailer.delay.system_notification(user, system_notification) - end end diff --git a/app/services/notifications/handle_system_notification_in_communication_channel_service.rb b/app/services/notifications/handle_system_notification_in_communication_channel_service.rb new file mode 100644 index 000000000..c4c84a9d5 --- /dev/null +++ b/app/services/notifications/handle_system_notification_in_communication_channel_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Notifications + class HandleSystemNotificationInCommunicationChannelService + extend Service + + attr_reader :errors + + def initialize(system_notification) + @system_notification = system_notification + @errors = {} + end + + def call + @system_notification.user_system_notifications.find_each do |usn| + user = usn.user + AppMailer.delay.system_notification(user, @system_notification) if user.system_message_email_notification + end + + self + end + + def succeed? + @errors.none? + end + end +end diff --git a/app/services/notifications/push_to_communication_channel_service.rb b/app/services/notifications/push_to_communication_channel_service.rb new file mode 100644 index 000000000..746e0799e --- /dev/null +++ b/app/services/notifications/push_to_communication_channel_service.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module Notifications + class PushToCommunicationChannelService + extend Service + + WHITELISTED_ITEM_TYPES = %w(SystemNotification).freeze + + attr_reader :errors + + def initialize(item_id:, item_type:) + @item_type = item_type + @item = item_type.constantize.find item_id + @errors = {} + end + + def call + return self unless valid? + + "Notifications::Handle#{@item_type}InCommunicationChannelService".constantize.call(@item) + self + end + + def succeed? + @errors.none? + end + + private + + def valid? + raise 'Dont know how to handle this type of items' unless WHITELISTED_ITEM_TYPES.include?(@item_type) + + if @item.nil? + @errors[:invalid_arguments] = 'Can\'t find item' if @item.nil? + return false + end + true + end + end +end diff --git a/app/services/notifications/sync_system_notifications_service.rb b/app/services/notifications/sync_system_notifications_service.rb index 45020dd81..af3a925a9 100644 --- a/app/services/notifications/sync_system_notifications_service.rb +++ b/app/services/notifications/sync_system_notifications_service.rb @@ -81,12 +81,27 @@ module Notifications .where(source_id: attrs[:source_id]).first_or_initialize(attrs) if n.new_record? - n.users = User.all - n.save! + save_notification n elsif n.last_time_changed_at < attrs[:last_time_changed_at] n.update_attributes!(attrs) end end end + + def save_notification(notification) + ActiveRecord::Base.transaction do + notification.save! + + User.find_in_batches do |user_ids| + user_system_notifications = user_ids.pluck(:id).collect do |item| + Hash[:user_id, item, :system_notification_id, notification.id] + end + UserSystemNotification.import user_system_notifications, validate: false + end + end + + Notifications::PushToCommunicationChannelService.delay.call(item_id: notification.id, + item_type: notification.class.name) + end end end diff --git a/db/migrate/20190424113216_add_unique_index_to_system_notifications.rb b/db/migrate/20190424113216_add_unique_index_to_system_notifications.rb new file mode 100644 index 000000000..7c61427c0 --- /dev/null +++ b/db/migrate/20190424113216_add_unique_index_to_system_notifications.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class AddUniqueIndexToSystemNotifications < ActiveRecord::Migration[5.1] + def change + # remove not unique index and add new with uniq + remove_index :system_notifications, :source_id + add_index :system_notifications, :source_id, unique: true + + add_index :user_system_notifications, %i(user_id system_notification_id), unique: true, + name: 'index_user_system_notifications_on_user_and_notification_id' + end +end diff --git a/db/schema.rb b/db/schema.rb index 3a97121f0..b6afd5523 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190410110605) do +ActiveRecord::Schema.define(version: 20190424113216) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -684,7 +684,7 @@ ActiveRecord::Schema.define(version: 20190410110605) do t.datetime "updated_at", null: false t.index ["last_time_changed_at"], name: "index_system_notifications_on_last_time_changed_at" t.index ["source_created_at"], name: "index_system_notifications_on_source_created_at" - t.index ["source_id"], name: "index_system_notifications_on_source_id" + t.index ["source_id"], name: "index_system_notifications_on_source_id", unique: true end create_table "tables", force: :cascade do |t| @@ -819,6 +819,7 @@ ActiveRecord::Schema.define(version: 20190410110605) do t.index ["read_at"], name: "index_user_system_notifications_on_read_at" t.index ["seen_at"], name: "index_user_system_notifications_on_seen_at" t.index ["system_notification_id"], name: "index_user_system_notifications_on_system_notification_id" + t.index ["user_id", "system_notification_id"], name: "index_user_system_notifications_on_user_and_notification_id", unique: true t.index ["user_id"], name: "index_user_system_notifications_on_user_id" end diff --git a/spec/factories/system_notifications.rb b/spec/factories/system_notifications.rb index db2c23d14..42c7021f0 100644 --- a/spec/factories/system_notifications.rb +++ b/spec/factories/system_notifications.rb @@ -7,7 +7,7 @@ FactoryBot.define do modal_title { Faker::Name.first_name } modal_body { Faker::Lorem.paragraphs(4).map { |pr| "

#{pr}

" }.join } source_created_at { Faker::Time.between(3.days.ago, Date.today) } - source_id { Faker::Number.between(1, 1000) } + source_id { SystemNotification.order(source_id: :desc).first&.source_id.to_i + 1 } last_time_changed_at { Time.now } trait :show_on_login do show_on_login { true } diff --git a/spec/models/user_system_notification_spec.rb b/spec/models/user_system_notification_spec.rb index d838657b5..557291273 100644 --- a/spec/models/user_system_notification_spec.rb +++ b/spec/models/user_system_notification_spec.rb @@ -15,36 +15,6 @@ describe UserSystemNotification do it { is_expected.to belong_to(:system_notification) } end - describe '.create' do - before do - Delayed::Worker.delay_jobs = false - end - - after do - Delayed::Worker.delay_jobs = true - end - - context 'when user has enabled notifications' do - it 'calls send an email on creation' do - allow(user_system_notification.user) - .to receive(:system_message_email_notification).and_return(true) - - expect(user_system_notification).to receive(:send_email) - user_system_notification.save - end - end - - context 'when user has disabled notifications' do - it 'doesn\'t call send an email on createion' do - allow(user_system_notification.user) - .to receive(:system_message_email_notification).and_return(false) - - expect(user_system_notification).not_to receive(:send_email) - user_system_notification.save - end - end - end - describe 'Methods' do let(:notifcation_one) { create :system_notification } let(:notifcation_two) { create :system_notification } diff --git a/spec/services/notifications/handle_system_notification_in_communication_channel_service_spec.rb b/spec/services/notifications/handle_system_notification_in_communication_channel_service_spec.rb new file mode 100644 index 000000000..5d4450ae3 --- /dev/null +++ b/spec/services/notifications/handle_system_notification_in_communication_channel_service_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Notifications::HandleSystemNotificationInCommunicationChannelService do + let(:system_notification) { create :system_notification } + let!(:user_system_notification) do + create :user_system_notification, user: user, system_notification: system_notification + end + let(:user) { create :user } + let(:service_call) do + Notifications::HandleSystemNotificationInCommunicationChannelService.call(system_notification) + end + + before do + Delayed::Worker.delay_jobs = false + end + + after do + Delayed::Worker.delay_jobs = true + end + + context 'when user has enabled notifications' do + it 'calls AppMailer' do + allow_any_instance_of(User).to receive(:system_message_email_notification).and_return(true) + + expect(AppMailer).to receive(:system_notification).and_return(double('Mailer', deliver: true)) + + service_call + end + end + + context 'when user has disabled notifications' do + it 'does not call AppMailer' do + allow_any_instance_of(User).to receive(:system_message_email_notification).and_return(false) + + expect(AppMailer).not_to receive(:system_notification) + + service_call + end + end +end diff --git a/spec/services/notifications/push_to_communication_channel_service_spec.rb b/spec/services/notifications/push_to_communication_channel_service_spec.rb new file mode 100644 index 000000000..753ab13a0 --- /dev/null +++ b/spec/services/notifications/push_to_communication_channel_service_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Notifications::PushToCommunicationChannelService do + let(:system_notification) { create :system_notification } + let(:service_call) do + Notifications::PushToCommunicationChannelService.call(item_id: system_notification.id, + item_type: system_notification.class.name) + end + + context 'when call with valid items' do + it 'call service to to handle sending out' do + expect(Notifications::HandleSystemNotificationInCommunicationChannelService) + .to receive(:call).with(system_notification) + + service_call + end + end + + context 'when call with not valid items' do + it 'returns error with key invalid_arguments when system notification not exists' do + allow(SystemNotification).to receive(:find).and_return(nil) + + expect(service_call.errors).to have_key(:invalid_arguments) + end + + it 'raise error when have not listed object' do + u = create :user + + expect do + Notifications::PushToCommunicationChannelService.call(item_id: u.id, item_type: 'User') + end.to(raise_error('Dont know how to handle this type of items')) + end + end +end diff --git a/spec/services/notifications/sync_system_notifications_service_spec.rb b/spec/services/notifications/sync_system_notifications_service_spec.rb index 6b3312054..a2a2784d9 100644 --- a/spec/services/notifications/sync_system_notifications_service_spec.rb +++ b/spec/services/notifications/sync_system_notifications_service_spec.rb @@ -6,6 +6,8 @@ describe Notifications::SyncSystemNotificationsService do url = 'http://system-notifications-service.test/api/system_notifications' let!(:user) { create :user } let(:service_call) do + allow_any_instance_of(Notifications::PushToCommunicationChannelService).to receive(:call).and_return(nil) + Notifications::SyncSystemNotificationsService.call end @@ -80,6 +82,14 @@ describe Notifications::SyncSystemNotificationsService do expect { service_call }.to change { UserSystemNotification.count }.by(20) end + + it 'calls service to notify users about notification' do + Delayed::Worker.delay_jobs = false + expect(Notifications::PushToCommunicationChannelService).to receive(:call).exactly(10) + + service_call + Delayed::Worker.delay_jobs = true + end end context 'when request is unsuccessful' do @@ -101,5 +111,13 @@ describe Notifications::SyncSystemNotificationsService do expect(service_call.errors).to have_key(:socketerror) end + + it 'does not call service to notify users about notification' do + Delayed::Worker.delay_jobs = false + expect(Notifications::PushToCommunicationChannelService).to_not receive(:call) + + service_call + Delayed::Worker.delay_jobs = true + end end end