diff --git a/app/models/article.rb b/app/models/article.rb index 61daf0870..8acd9bc1a 100644 --- a/app/models/article.rb +++ b/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/ diff --git a/db/migrate/20210707053923_add_unique_index_to_articles_feed_source_url_where_published.rb b/db/migrate/20210707053923_add_unique_index_to_articles_feed_source_url_where_published.rb new file mode 100755 index 000000000..6146a7594 --- /dev/null +++ b/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 diff --git a/db/migrate/20210712173206_add_unique_index_to_articles_canonical_url_where_published.rb b/db/migrate/20210712173206_add_unique_index_to_articles_canonical_url_where_published.rb new file mode 100644 index 000000000..36d8bb59c --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 239e2e0db..005b21f2a 100644 --- a/db/schema.rb +++ b/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" diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 2dbc6370d..039e669f1 100644 --- a/spec/models/article_spec.rb +++ b/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 diff --git a/spec/services/feeds/import_spec.rb b/spec/services/feeds/import_spec.rb index 01d9d9f64..1cad0ae97 100644 --- a/spec/services/feeds/import_spec.rb +++ b/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