From d1d2298a4cee0b021db66ff116a665ee8ae27111 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 15 Sep 2021 11:29:22 +1000 Subject: [PATCH] DEV: Add for_topic column to bookmarks (#14343) This new column will be used to indicate that a bookmark is at the topic level. The first post of a topic can be bookmarked twice after this change -- with for_topic set to true and with for_topic set to false. A later PR will use this column for logic to bookmark the topic, and then topic-level bookmark links will take you to the last unread post in the topic. See also 22208836c5f2adcf8c85a62d27b67968743b20f0 --- app/models/bookmark.rb | 28 ++++++++++---- ...210913032326_add_for_topic_to_bookmarks.rb | 9 +++++ spec/models/bookmark_spec.rb | 38 ++++++++++++++++++- 3 files changed, 65 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20210913032326_add_for_topic_to_bookmarks.rb diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 74a0d27bcb..69219cfe62 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -46,7 +46,14 @@ class Bookmark < ActiveRecord::Base validates :name, length: { maximum: 100 } def unique_per_post_for_user - if Bookmark.exists?(user_id: user_id, post_id: post_id) + is_first_post = Post.where(id: post_id).pluck_first(:post_number) == 1 + exists = if is_first_post + Bookmark.exists?(user_id: user_id, post_id: post_id, for_topic: for_topic) + else + Bookmark.exists?(user_id: user_id, post_id: post_id) + end + + if exists self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post")) end end @@ -116,6 +123,10 @@ class Bookmark < ActiveRecord::Base joins(:post).where(user_id: user_id, posts: { topic_id: topic_id }) } + def self.find_for_topic_by_user(topic_id, user_id) + for_user_in_topic(user_id, topic_id).where(for_topic: true).first + end + def self.count_per_day(opts = nil) opts ||= {} result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago) @@ -170,14 +181,15 @@ end # reminder_set_at :datetime # auto_delete_preference :integer default(0), not null # pinned :boolean default(FALSE) +# for_topic :boolean default(FALSE), not null # # Indexes # -# index_bookmarks_on_post_id (post_id) -# index_bookmarks_on_reminder_at (reminder_at) -# index_bookmarks_on_reminder_set_at (reminder_set_at) -# index_bookmarks_on_reminder_type (reminder_type) -# index_bookmarks_on_topic_id (topic_id) -# index_bookmarks_on_user_id (user_id) -# index_bookmarks_on_user_id_and_post_id (user_id,post_id) UNIQUE +# index_bookmarks_on_post_id (post_id) +# index_bookmarks_on_reminder_at (reminder_at) +# index_bookmarks_on_reminder_set_at (reminder_set_at) +# index_bookmarks_on_reminder_type (reminder_type) +# index_bookmarks_on_topic_id (topic_id) +# index_bookmarks_on_user_id (user_id) +# index_bookmarks_on_user_id_and_post_id_and_for_topic (user_id,post_id,for_topic) UNIQUE # diff --git a/db/migrate/20210913032326_add_for_topic_to_bookmarks.rb b/db/migrate/20210913032326_add_for_topic_to_bookmarks.rb new file mode 100644 index 0000000000..2660cf69a1 --- /dev/null +++ b/db/migrate/20210913032326_add_for_topic_to_bookmarks.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddForTopicToBookmarks < ActiveRecord::Migration[6.1] + def change + add_column :bookmarks, :for_topic, :boolean, default: false, null: false + add_index :bookmarks, [:user_id, :post_id, :for_topic], unique: true + remove_index :bookmarks, [:user_id, :post_id] + end +end diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index d617e026f2..2e7386dafb 100644 --- a/spec/models/bookmark_spec.rb +++ b/spec/models/bookmark_spec.rb @@ -5,8 +5,8 @@ require 'rails_helper' describe Bookmark do fab!(:post) { Fabricate(:post) } - context 'validations' do - it 'does not allow user to bookmark a post twice' do + context "validations" do + it "does not allow user to bookmark a post twice, enforces unique bookmark per post, user, and for_topic" do bookmark = Fabricate(:bookmark, post: post) user = bookmark.user @@ -17,6 +17,40 @@ describe Bookmark do expect(bookmark_2.valid?).to eq(false) end + + it "allows a user to bookmark a post twice if it is the first post and for_topic is different" do + post.update!(post_number: 1) + bookmark = Fabricate(:bookmark, post: post, for_topic: false) + user = bookmark.user + + bookmark_2 = Fabricate(:bookmark, + post: post, + user: user, + for_topic: true + ) + + expect(bookmark_2.valid?).to eq(true) + + bookmark_3 = Fabricate.build(:bookmark, + post: post, + user: user, + for_topic: true + ) + + expect(bookmark_3.valid?).to eq(false) + end + end + + describe "#find_for_topic_by_user" do + it "gets the for_topic bookmark for a user for a specific topic" do + user = Fabricate(:user) + post.update!(post_number: 1) + bookmark = Fabricate(:bookmark, user: user) + bookmark_2 = Fabricate(:bookmark, user: user, post: post, for_topic: true) + expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(bookmark_2) + bookmark_2.update!(for_topic: false) + expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(nil) + end end describe "#cleanup!" do