diff --git a/Gemfile b/Gemfile index 8fc814dd9..d1b313587 100644 --- a/Gemfile +++ b/Gemfile @@ -11,7 +11,6 @@ group :production do end gem "active_record_union", "~> 1.3" # Adds proper union and union_all methods to ActiveRecord::Relation -gem "activerecord-import", "~> 1.0" # Adds ability to bulk create activerecord objects gem "acts-as-taggable-on", "~> 6.5" # A tagging plugin for Rails applications that allows for custom tagging along dynamic contexts gem "acts_as_follower", github: "thepracticaldev/acts_as_follower", branch: "master" # Allow any model to follow any other model gem "addressable", "~> 2.7" # A replacement for the URI implementation that is part of Ruby's standard library diff --git a/Gemfile.lock b/Gemfile.lock index 7bcc71463..35e79c0c1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -65,8 +65,6 @@ GEM activerecord (6.0.3.1) activemodel (= 6.0.3.1) activesupport (= 6.0.3.1) - activerecord-import (1.0.5) - activerecord (>= 3.2) activestorage (6.0.3.1) actionpack (= 6.0.3.1) activejob (= 6.0.3.1) @@ -873,7 +871,6 @@ PLATFORMS DEPENDENCIES active_record_union (~> 1.3) - activerecord-import (~> 1.0) acts-as-taggable-on (~> 6.5) acts_as_follower! addressable (~> 2.7) diff --git a/app/controllers/credits_controller.rb b/app/controllers/credits_controller.rb index 975974249..70997f59d 100644 --- a/app/controllers/credits_controller.rb +++ b/app/controllers/credits_controller.rb @@ -26,16 +26,24 @@ class CreditsController < ApplicationController return unless make_payment - credit_objects = Array.new(@number_to_purchase) do + credits_attributes = Array.new(@number_to_purchase) do + # unfortunately Rails requires the timestamps to be present and doesn't add them automatically + # see + now = Time.current + attrs = { created_at: now, updated_at: now, cost: cost_per_credit / 100.0 } + if params[:organization_id].present? @purchaser = Organization.find(params[:organization_id]) - Credit.new(organization_id: params[:organization_id], cost: cost_per_credit / 100.0) + attrs[:organization_id] = params[:organization_id] else @purchaser = current_user - Credit.new(user_id: current_user.id, cost: cost_per_credit / 100.0) + attrs[:user_id] = current_user.id end + + attrs end - Credit.import credit_objects + Credit.insert_all(credits_attributes) + @purchaser.credits_count = @purchaser.credits.size @purchaser.spent_credits_count = @purchaser.credits.spent.size @purchaser.unspent_credits_count = @purchaser.credits.unspent.size diff --git a/app/models/credit.rb b/app/models/credit.rb index 7f5a0b116..58df2fa5b 100644 --- a/app/models/credit.rb +++ b/app/models/credit.rb @@ -19,9 +19,20 @@ class Credit < ApplicationRecord end def self.add_to(user_or_org, amount) + return unless amount.positive? + + now = Time.current association_id = "#{user_or_org.class.name.underscore}_id" - credits = Array.new(amount) { new(association_id => user_or_org.id) } - import(credits) + attributes = Array.new(amount) do + { + association_id => user_or_org.id, + :created_at => now, + :updated_at => now + } + end + + insert_all(attributes) + update_cache_columns(user_or_org) end diff --git a/app/services/notifications/notifiable_action/send.rb b/app/services/notifications/notifiable_action/send.rb index fbec4f9be..dcc04330d 100644 --- a/app/services/notifications/notifiable_action/send.rb +++ b/app/services/notifications/notifiable_action/send.rb @@ -21,28 +21,32 @@ module Notifications article: article_data(notifiable) } json_data[:organization] = organization_data(notifiable.organization) if notifiable.organization_id - notifications = [] + + notifications_attributes = [] # followers is an array and not an activerecord object # followers can occasionally be nil because orphaned follows can possibly exist in the db (for now) followers.sort_by(&:updated_at).reverse[0..10_000].each do |follower| - notifications.push Notification.new( + now = Time.current + notifications_attributes.push( user_id: follower.id, notifiable_id: notifiable.id, notifiable_type: notifiable.class.name, action: action, json_data: json_data, - notified_at: Time.current, + created_at: now, + notified_at: now, + updated_at: now, ) end - conflict_target = %i[notifiable_id notifiable_type user_id] - conflict_target << :action if action.present? - index_predicate = "action IS#{action.present? ? ' NOT ' : ' '}NULL" - Notification.import! notifications, - on_duplicate_key_update: { - conflict_target: conflict_target, - index_predicate: index_predicate, - columns: %i[json_data notified_at read] - } + + return if notifications_attributes.blank? + + upsert_index = choose_upsert_index(action) + Notification.upsert_all( + notifications_attributes, + unique_by: upsert_index, + returning: %i[id], + ) end private @@ -54,6 +58,12 @@ module Notifications followers += notifiable.organization.followers_scoped.where(subscription_status: "all_articles").map(&:follower) if notifiable.organization_id followers.uniq.compact end + + def choose_upsert_index(action) + return :index_notifications_on_user_notifiable_and_action_not_null if action.present? + + :index_notifications_on_user_notifiable_action_is_null + end end end end diff --git a/app/services/notifications/reactions/send.rb b/app/services/notifications/reactions/send.rb index 84645ece0..4ffd53c93 100644 --- a/app/services/notifications/reactions/send.rb +++ b/app/services/notifications/reactions/send.rb @@ -55,10 +55,7 @@ module Notifications notification.notified_at = Time.current notification.read = false if json_data[:reaction][:aggregated_siblings].size > previous_siblings_size - # temporarily returning validations to prevent creating duplicate notifications - # notification_id = save_notification(notification) - notification.save! - notification_id = notification.id + notification_id = save_notification(notification_params, notification) OpenStruct.new(action: :saved, notification_id: notification_id) end @@ -68,29 +65,53 @@ module Notifications attr_reader :reaction, :receiver - # when a notification exists in the db already it's safe to just save - # when it doesn't, there could be a race condition when 2 jobs try to create duplicate notifications concurrently - # in this case upsert is used to rely on postgres constraints and update or insert depending on if the record exists in the db at this point - # currently, activerecord-import upsert is used - # when the app is upgraded to Rails 6 this can be refactored to use rails upsert - def save_notification(notification) + # when a notification exists in the DB already it's safe to just save it, + # when it doesn't, there could be a race condition when 2 jobs try to create + # duplicate notifications concurrently, in this case upsert is used to + # rely on PostgreSQL constraints, thus we use `.upsert` + def save_notification(params, notification) if notification.persisted? notification.save! notification.id else - # conflict target and index_predicate specify the index to use - conflict_target = %i[notifiable_id notifiable_type] - conflict_target << :action if notification.action? - conflict_target << :user_id if notification.user_id? - conflict_target << :organization_id if notification.organization_id? - index_predicate = "action IS#{notification.action? ? ' NOT ' : ' '}NULL" - import_result = Notification.import! [notification], - on_duplicate_key_update: { - conflict_target: conflict_target, - index_predicate: index_predicate, - columns: %i[json_data notified_at read] - } - import_result.ids.first + # in the upsert, we are only interested in updating the following columns: + # json_data, notified_at, read and the notification params used by `.find_or_initialize_by` + upsert_columns = %w[json_data notified_at read] + upsert_attributes = notification.attributes.select { |k| upsert_columns.include?(k) } + upsert_attributes.merge!(params) + + # unfortunately Rails requires the timestamps to be present even if unused in case of upsert + # see + now = Time.current + upsert_attributes["created_at"] = upsert_attributes["updated_at"] = now + + # we also need to select the correct index to let PostgreSQL know + # how to determine conflict on rows + upsert_index = choose_upsert_index(notification) + + upsert_result = Notification.upsert( + upsert_attributes, + unique_by: upsert_index, + returning: %i[id], + ) + + upsert_result.to_a.first["id"] + end + end + + def choose_upsert_index(notification) + # if none of these is present, we use the two columns for the upsert + return %i[notifiable_id notifiable_type] unless + notification.action? || notification.user_id? || notification.organization_id? + + if notification.action? + return :index_notifications_on_user_notifiable_and_action_not_null if notification.user_id? + + :index_notifications_on_org_notifiable_and_action_not_null + else + return :index_notifications_on_user_notifiable_action_is_null if notification.user_id? + + :index_notifications_on_org_notifiable_action_is_null end end diff --git a/app/services/podcasts/create_episode.rb b/app/services/podcasts/create_episode.rb index 80f3ee56f..396f4f665 100644 --- a/app/services/podcasts/create_episode.rb +++ b/app/services/podcasts/create_episode.rb @@ -10,27 +10,20 @@ module Podcasts end def call - ep = PodcastEpisode.new - ep.title = item.title - ep.podcast_id = podcast_id - ep.slug = item.title.parameterize - ep.subtitle = item.itunes_subtitle - ep.summary = item.itunes_summary - ep.website_url = item.link - ep.guid = item.guid - get_media_url(ep) if item.enclosure_url - begin - ep.published_at = item.pubDate.to_date - rescue ArgumentError, NoMethodError => e - Rails.logger.error("not a valid date: #{e}") - end - ep.body = item.body - import_result = PodcastEpisode.import! [ep], on_duplicate_key_update: { - conflict_target: %i[media_url], - columns: %i[title slug subtitle summary website_url published_at reachable media_url https body] - } - episode = PodcastEpisode.find(import_result.ids.first) + attributes = podcast_episode_attributes + attributes = add_media_url(attributes) + attributes = add_published_at(attributes) + + upsert_result = PodcastEpisode.upsert( + attributes, + unique_by: :index_podcast_episodes_on_media_url, + returning: %i[id], + ) + + episode = PodcastEpisode.find(upsert_result.to_a.first["id"]) + finalize(episode) + episode end @@ -38,11 +31,40 @@ module Podcasts attr_reader :podcast_id, :item - def get_media_url(episode) + def podcast_episode_attributes + now = Time.current + + { + podcast_id: podcast_id, + title: item.title, + slug: item.title.parameterize, + subtitle: item.itunes_subtitle, + summary: item.itunes_summary, + website_url: item.link, + guid: item.guid, + body: item.body, + created_at: now, + updated_at: now + } + end + + def add_media_url(attributes) + return attributes if item.enclosure_url.blank? + result = GetMediaUrl.call(item.enclosure_url) - episode.reachable = result.reachable - episode.media_url = result.url - episode.https = result.https + + attributes.merge( + reachable: result.reachable, + media_url: result.url, + https: result.https, + ) + end + + def add_published_at(attributes) + attributes.merge(published_at: item.pubDate.to_date) + rescue ArgumentError, NoMethodError => e + Rails.logger.error("not a valid date: #{e}") + attributes end def finalize(episode) diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 36eb7b317..866fece1e 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -21,79 +21,55 @@ RSpec.describe Notification, type: :model do end describe "when trying to create duplicate notifications" do - # Duplicate notifications are not allowed even when validations are skipped - it "doesn't allow to create a duplicate notification via import" do - create(:notification, user: user, notifiable: article, action: "Reaction") - duplicate_notification = build(:notification, user: user, notifiable: article, action: "Reaction") - expect do - described_class.import([duplicate_notification], - on_duplicate_key_update: { - conflict_target: %i[notifiable_id notifiable_type user_id action], - index_predicate: "action IS NOT NULL", - columns: %i[json_data notified_at read] - }) - end.not_to change(described_class, :count) - end + describe "when notifiable is an Article" do + it "doesn't allow to create duplicated article reaction notifications for a user" do + reaction = create(:reaction, reactable: article, user: user2) - it "updates when trying to create a duplicate notification via import" do - notification = create( - :notification, user: user, notifiable: article, action: "Reaction", json_data: { "user_id" => 1 } - ) - duplicate_notification = build( - :notification, user: user, notifiable: article, action: "Reaction", json_data: { "user_id" => 2 } - ) - described_class.import([duplicate_notification], - on_duplicate_key_update: { - conflict_target: %i[notifiable_id notifiable_type user_id action], - index_predicate: "action IS NOT NULL", - columns: %i[json_data notified_at read] - }) - notification.reload - expect(notification.json_data["user_id"]).to eq(2) - end + described_class.send_reaction_notification_without_delay(reaction, article.user) + expect(article.user.notifications.count).to eq(1) - it "doesn't allow to create a duplicate organization notification via import" do - create(:notification, organization: organization, notifiable: article, action: "Reaction") - duplicate_notification = build(:notification, organization: organization, notifiable: article, action: "Reaction") - expect do - described_class.import([duplicate_notification], - on_duplicate_key_update: { - conflict_target: %i[notifiable_id notifiable_type organization_id action], - index_predicate: "action IS NOT NULL", - columns: %i[json_data notified_at read] - }) - end.not_to change(described_class, :count) + expect do + described_class.send_reaction_notification_without_delay(reaction, article.user) + end.not_to change(article.user.notifications, :count) + end + + it "doesn't allow to create duplicated article reaction notifications for an organization" do + article = create(:article, organization: organization) + reaction = create(:reaction, reactable: article, user: user2) + + described_class.send_reaction_notification_without_delay(reaction, article.organization) + expect(article.organization.notifications.count).to eq(1) + + expect do + described_class.send_reaction_notification_without_delay(reaction, article.organization) + end.not_to change(article.organization.notifications, :count) + end end describe "when notifiable is a Comment" do - let!(:comment) { create(:comment, commentable: article) } + it "doesn't allow to create duplicated comment reaction notifications for a user" do + comment = create(:comment, commentable: article) + reaction = create(:reaction, reactable: comment, user: user2) + + described_class.send_reaction_notification_without_delay(reaction, comment.user) + expect(comment.user.notifications.count).to eq(1) - it "doesn't allow to create a duplicate user notification via import when action is nil" do - notification_attributes = { user: user, notifiable: comment, action: nil } - create(:notification, notification_attributes) - duplicate_notification = build(:notification, notification_attributes) expect do - described_class.import([duplicate_notification], - on_duplicate_key_update: { - conflict_target: %i[notifiable_id notifiable_type user_id], - index_predicate: "action IS NULL", - columns: %i[json_data notified_at read] - }) - end.not_to change(described_class, :count) + described_class.send_reaction_notification_without_delay(reaction, comment.user) + end.not_to change(comment.user.notifications, :count) end - it "doesn't allow to create a duplicate org notification via import when action is nil" do - notification_attributes = { organization: organization, notifiable: comment, action: nil } - create(:notification, notification_attributes) - duplicate_notification = build(:notification, notification_attributes) + it "doesn't allow to create duplicated comment reaction notifications for an organization" do + article = create(:article, organization: organization) + comment = create(:comment, commentable: article) + reaction = create(:reaction, reactable: comment, user: user2) + + described_class.send_reaction_notification_without_delay(reaction, article.organization) + expect(article.organization.notifications.count).to eq(1) + expect do - described_class.import([duplicate_notification], - on_duplicate_key_update: { - conflict_target: %i[notifiable_id notifiable_type organization_id], - index_predicate: "action IS NULL", - columns: %i[json_data notified_at read] - }) - end.not_to change(described_class, :count) + described_class.send_reaction_notification_without_delay(reaction, article.organization) + end.not_to change(article.organization.notifications, :count) end end end diff --git a/spec/services/notifications/notifiable_action/send_spec.rb b/spec/services/notifications/notifiable_action/send_spec.rb index 17330ee61..e078566b6 100644 --- a/spec/services/notifications/notifiable_action/send_spec.rb +++ b/spec/services/notifications/notifiable_action/send_spec.rb @@ -52,6 +52,7 @@ RSpec.describe Notifications::NotifiableAction::Send, type: :service do it "doesn't fail if the notification already exists" do notification = create(:notification, user: user2, action: "Published", notifiable: article) result = described_class.call(article, "Published") - expect(result.ids).to include(notification.id) + ids = result.to_a.map { |r| r["id"] } + expect(ids).to include(notification.id) end end diff --git a/vendor/cache/activerecord-import-1.0.5.gem b/vendor/cache/activerecord-import-1.0.5.gem deleted file mode 100644 index 348a5fac3..000000000 Binary files a/vendor/cache/activerecord-import-1.0.5.gem and /dev/null differ