Allow multiple users to fetch all articles from the same RSS feed (#14160)

* fix: artiles from the same feed can be fetched from multiple accounts

* undo unintentional changes to schema.rb

* modify: article_spec and import_spec tests

* fix: failing test

* modify: import_spec:213 for user.setting

* fix: failing test import_spec:213

* add: unique index on articles canonical_url where published

* revert: schema.rb

* modify: unique url error message to include admin email

* modify: custom error message

* modify: .update to .update! (bang)

app/models/article.rb

@@ -27,6 +27,8 @@ class Article < ApplicationRecord
 
   # The date that we began limiting the number of user mentions in an article.
   MAX_USER_MENTION_LIVE_AT = Time.utc(2021, 4, 7).freeze
+  UNIQUE_URL_ERROR = "has already been taken. " \
+                     "Email #{ForemInstance.email} for further details.".freeze
 
   has_one :discussion_lock, dependent: :destroy
 
@@ -54,10 +56,14 @@ class Article < ApplicationRecord
   validates :body_markdown, uniqueness: { scope: %i[user_id title] }
   validates :boost_states, presence: true
   validates :cached_tag_list, length: { maximum: 126 }
-  validates :canonical_url, uniqueness: { allow_nil: true }
+  validates :canonical_url,
+            uniqueness: { allow_nil: true, scope: :published, message: UNIQUE_URL_ERROR },
+            if: :published?
   validates :canonical_url, url: { allow_blank: true, no_local: true, schemes: %w[https http] }
   validates :comments_count, presence: true
-  validates :feed_source_url, uniqueness: { allow_nil: true }
+  validates :feed_source_url,
+            uniqueness: { allow_nil: true, scope: :published, message: UNIQUE_URL_ERROR },
+            if: :published?
   validates :feed_source_url, url: { allow_blank: true, no_local: true, schemes: %w[https http] }
   validates :main_image, url: { allow_blank: true, schemes: %w[https http] }
   validates :main_image_background_hex_color, format: /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/

db/migrate/20210707053923_add_unique_index_to_articles_feed_source_url_where_published.rb

@@ -0,0 +1,19 @@
+class AddUniqueIndexToArticlesFeedSourceUrlWherePublished < ActiveRecord::Migration[6.1]
+  disable_ddl_transaction!
+  
+  def up
+    if index_exists?(:articles, :feed_source_url, unique: true)
+      remove_index :articles, column: :feed_source_url, unique: true, algorithm: :concurrently
+    end
+
+    add_index :articles, :feed_source_url, unique: true, where: "published is true", algorithm: :concurrently
+  end
+
+  def down
+    if index_exists?(:articles, :feed_source_url, unique: true, where: "published is true")
+      remove_index :articles, :feed_source_url, unique: true, where: "published is true", algorithm: :concurrently
+    end
+
+    add_index :articles, column: :feed_source_url, unique: true, algorithm: :concurrently
+  end
+end

db/migrate/20210712173206_add_unique_index_to_articles_canonical_url_where_published.rb

@@ -0,0 +1,19 @@
+class AddUniqueIndexToArticlesCanonicalUrlWherePublished < ActiveRecord::Migration[6.1]
+  disable_ddl_transaction!
+
+  def up
+    if index_exists?(:articles, :canonical_url, unique: true)
+      remove_index :articles, column: :canonical_url, unique: true, algorithm: :concurrently
+    end
+
+    add_index :articles, :canonical_url, unique: true, where: "published is true", algorithm: :concurrently
+  end
+
+  def down
+    if index_exists?(:articles, :canonical_url, unique: true, where: "published is true")
+      remove_index :articles, :canonical_url, unique: true, where: "published is true", algorithm: :concurrently
+    end
+
+    add_index :articles, column: :canonical_url, unique: true, algorithm: :concurrently
+  end
+end

db/schema.rb

@@ -156,12 +156,12 @@ ActiveRecord::Schema.define(version: 2021_07_15_211923) do
     t.index "user_id, title, digest(body_markdown, 'sha512'::text)", name: "index_articles_on_user_id_and_title_and_digest_body_markdown", unique: true
     t.index ["boost_states"], name: "index_articles_on_boost_states", using: :gin
     t.index ["cached_tag_list"], name: "index_articles_on_cached_tag_list", opclass: :gin_trgm_ops, using: :gin
-    t.index ["canonical_url"], name: "index_articles_on_canonical_url", unique: true
+    t.index ["canonical_url"], name: "index_articles_on_canonical_url", unique: true, where: "(published IS TRUE)"
     t.index ["collection_id"], name: "index_articles_on_collection_id"
     t.index ["comment_score"], name: "index_articles_on_comment_score"
     t.index ["comments_count"], name: "index_articles_on_comments_count"
     t.index ["featured_number"], name: "index_articles_on_featured_number"
-    t.index ["feed_source_url"], name: "index_articles_on_feed_source_url", unique: true
+    t.index ["feed_source_url"], name: "index_articles_on_feed_source_url", unique: true, where: "(published IS TRUE)"
     t.index ["hotness_score", "comments_count"], name: "index_articles_on_hotness_score_and_comments_count"
     t.index ["hotness_score"], name: "index_articles_on_hotness_score"
     t.index ["path"], name: "index_articles_on_path"

spec/models/article_spec.rb

@@ -50,8 +50,6 @@ RSpec.describe Article, type: :model do
     it { is_expected.to validate_presence_of(:title) }
     it { is_expected.to validate_presence_of(:user_id) }
 
-    it { is_expected.to validate_uniqueness_of(:canonical_url).allow_nil }
-    it { is_expected.to validate_uniqueness_of(:feed_source_url).allow_nil }
     it { is_expected.to validate_uniqueness_of(:slug).scoped_to(:user_id) }
 
     it { is_expected.not_to allow_value("foo").for(:main_image_background_hex_color) }
@@ -1309,4 +1307,45 @@ RSpec.describe Article, type: :model do
       expect { article.update_score }.not_to change { article.reload.hotness_score }
     end
   end
+
+  describe "#feed_source_url and canonical_url must be unique for published articles" do
+    let(:url) { "http://www.example.com" }
+
+    it "is valid when both articles are drafts" do
+      body_markdown = "---\ntitle: Title\npublished: false\ncanonical_url: #{url}\n---\n\n"
+      create(:article, body_markdown: body_markdown, feed_source_url: url)
+      another_article = build(:article, body_markdown: body_markdown, feed_source_url: url)
+
+      expect(another_article).to be_valid
+    end
+
+    it "is valid when first article is a draft, second is published" do
+      body_markdown = "---\ntitle: Title\npublished: false\ncanonical_url: #{url}\n---\n\n"
+      create(:article, body_markdown: body_markdown, feed_source_url: url)
+      body_markdown = "---\ntitle: Title\npublished: true\ncanonical_url: #{url}\n---\n\n"
+      another_article = build(:article, body_markdown: body_markdown, feed_source_url: url)
+
+      expect(another_article).to be_valid
+    end
+
+    it "is valid when first article is published, second is draft" do
+      body_markdown = "---\ntitle: Title\npublished: true\ncanonical_url: #{url}\n---\n\n"
+      create(:article, body_markdown: body_markdown, feed_source_url: url)
+      body_markdown = "---\ntitle: Title\npublished: false\ncanonical_url: #{url}\n---\n\n"
+      another_article = build(:article, body_markdown: body_markdown, feed_source_url: url)
+
+      expect(another_article).to be_valid
+    end
+
+    it "is not valid when both articles are published" do
+      body_markdown = "---\ntitle: Title\npublished: true\ncanonical_url: #{url}\n---\n\n"
+      create(:article, body_markdown: body_markdown, feed_source_url: url)
+      another_article = build(:article, body_markdown: body_markdown, feed_source_url: url)
+      error_message = "has already been taken. " \
+                      "Email #{ForemInstance.email} for further details."
+      expect(another_article).not_to be_valid
+      expect(another_article.errors.messages[:canonical_url]).to include(error_message)
+      expect(another_article.errors.messages[:feed_source_url]).to include(error_message)
+    end
+  end
 end

spec/services/feeds/import_spec.rb

@@ -209,4 +209,25 @@ RSpec.describe Feeds::Import, type: :service, vcr: true do
       expect(body_markdown).to include(expected_image_markdown)
     end
   end
+
+  context "when multiple users fetch from the same feed_url" do
+    it "fetches the articles in both accounts (if feed_mark_canonical = false)" do
+      rss_feed_user1 = Users::Setting.find_by(feed_url: link).user
+      rss_feed_user2 = create(:user)
+      rss_feed_user2.setting.update!(feed_url: link)
+      expect { described_class.call(users: User.where(id: Users::Setting.where(feed_url: link).select(:user_id))) }
+        .to change(rss_feed_user1.articles, :count).by(10)
+        .and change(rss_feed_user2.articles, :count).by(10)
+    end
+
+    it "fetches the articles in both accounts (if feed_mark_canonical = true)" do
+      rss_feed_user1 = Users::Setting.find_by(feed_url: link).user
+      rss_feed_user1.setting.update!(feed_mark_canonical: true)
+      rss_feed_user2 = create(:user)
+      rss_feed_user2.setting.update!(feed_url: link, feed_mark_canonical: true)
+      expect { described_class.call(users: User.where(id: Users::Setting.where(feed_url: link).select(:user_id))) }
+        .to change(rss_feed_user1.articles, :count).by(10)
+        .and change(rss_feed_user2.articles, :count).by(10)
+    end
+  end
 end