diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 8f31be4dbc7..cc165412555 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -77,6 +77,15 @@ class Bookmark < ActiveRecord::Base self.auto_delete_preference == Bookmark.auto_delete_preferences[:on_owner_reply] end + def clear_reminder! + update( + reminder_at: nil, + reminder_type: nil, + reminder_last_sent_at: Time.zone.now, + reminder_set_at: nil + ) + end + scope :pending_reminders, ->(before_time = Time.now.utc) do where("reminder_at IS NOT NULL AND reminder_at <= :before_time", before_time: before_time) end diff --git a/db/migrate/20200728022830_sync_topic_user_bookmarked_column_with_bookmarks.rb b/db/migrate/20200728022830_sync_topic_user_bookmarked_column_with_bookmarks.rb new file mode 100644 index 00000000000..76dfde6c5cd --- /dev/null +++ b/db/migrate/20200728022830_sync_topic_user_bookmarked_column_with_bookmarks.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class SyncTopicUserBookmarkedColumnWithBookmarks < ActiveRecord::Migration[6.0] + def up + should_be_bookmarked_sql = <<~SQL + UPDATE topic_users SET bookmarked = true WHERE id IN ( + SELECT topic_users.id + FROM topic_users + INNER JOIN bookmarks ON bookmarks.user_id = topic_users.user_id AND + bookmarks.topic_id = topic_users.topic_id + WHERE NOT topic_users.bookmarked + ) AND NOT bookmarked + SQL + DB.exec(should_be_bookmarked_sql) + + # post_action_type_id 1 is bookmark + should_not_be_bookmarked_sql = <<~SQL + UPDATE topic_users SET bookmarked = FALSE WHERE ID IN ( + SELECT DISTINCT topic_users.id FROM topic_users + LEFT JOIN bookmarks ON bookmarks.topic_id = topic_users.topic_id AND bookmarks.user_id = topic_users.user_id + LEFT JOIN post_actions ON post_actions.user_id = topic_users.user_id AND post_actions.post_action_type_id = 1 AND post_actions.post_id IN (SELECT id FROM posts WHERE posts.topic_id = topic_users.topic_id) + WHERE topic_users.bookmarked = true AND (bookmarks.id IS NULL AND post_actions.id IS NULL)) + SQL + DB.exec(should_not_be_bookmarked_sql) + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 90088181f63..773713a1fe7 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -29,7 +29,7 @@ class BookmarkManager return add_errors_from(bookmark) end - update_topic_user_bookmarked(topic: post.topic, bookmarked: true) + update_topic_user_bookmarked(post.topic) bookmark end @@ -42,16 +42,14 @@ class BookmarkManager bookmark.destroy - bookmarks_remaining_in_topic = Bookmark.exists?(topic_id: bookmark.topic_id, user: @user) - if !bookmarks_remaining_in_topic - update_topic_user_bookmarked(topic: bookmark.topic, bookmarked: false) - end + bookmarks_remaining_in_topic = update_topic_user_bookmarked(bookmark.topic) { topic_bookmarked: bookmarks_remaining_in_topic } end - def destroy_for_topic(topic) + def destroy_for_topic(topic, filter = {}, opts = {}) topic_bookmarks = Bookmark.where(user_id: @user.id, topic_id: topic.id) + topic_bookmarks = topic_bookmarks.where(filter) Bookmark.transaction do topic_bookmarks.each do |bookmark| @@ -59,7 +57,7 @@ class BookmarkManager bookmark.destroy end - update_topic_user_bookmarked(topic: topic, bookmarked: false) + update_topic_user_bookmarked(topic, opts) end end @@ -94,8 +92,14 @@ class BookmarkManager private - def update_topic_user_bookmarked(topic:, bookmarked:) - TopicUser.change(@user.id, topic, bookmarked: bookmarked) + def update_topic_user_bookmarked(topic, opts = {}) + # PostCreator can specify whether auto_track is enabled or not, don't want to + # create a TopicUser in that case + bookmarks_remaining_in_topic = Bookmark.exists?(topic_id: topic.id, user: @user) + return bookmarks_remaining_in_topic if opts.key?(:auto_track) && !opts[:auto_track] + + TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic) + bookmarks_remaining_in_topic end def parse_reminder_type(reminder_type) diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index b1f32cee618..0ea65413bb6 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -11,7 +11,7 @@ class BookmarkReminderNotificationHandler create_notification(bookmark) if bookmark.delete_when_reminder_sent? - return bookmark.destroy + BookmarkManager.new(bookmark.user).destroy(bookmark.id) end clear_reminder(bookmark) @@ -23,12 +23,7 @@ class BookmarkReminderNotificationHandler "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder info: #{bookmark.reminder_at} | #{Bookmark.reminder_types[bookmark.reminder_type]}" ) - bookmark.update( - reminder_at: nil, - reminder_type: nil, - reminder_last_sent_at: Time.zone.now, - reminder_set_at: nil - ) + bookmark.clear_reminder! end def self.create_notification(bookmark) diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 8f890d83c82..5bac660899b 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -405,10 +405,11 @@ class PostCreator def delete_owned_bookmarks return if !@post.topic_id - @user.bookmarks.where( - topic_id: @post.topic_id, - auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] - ).destroy_all + BookmarkManager.new(@user).destroy_for_topic( + Topic.with_deleted.find(@post.topic_id), + { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] }, + @opts + ) end def handle_spam diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 636bf9c5df5..e013559003e 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -653,12 +653,22 @@ describe PostCreator do before do Fabricate(:bookmark, topic: topic, user: user, auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) Fabricate(:bookmark, topic: topic, user: user, auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) + TopicUser.create!(topic: topic, user: user, bookmarked: true) + end + + it "deletes the bookmarks, but not the ones without an auto_delete_preference" do Fabricate(:bookmark, topic: topic, user: user) Fabricate(:bookmark, user: user) - end - it "deletes the bookmarks" do creator.create expect(Bookmark.where(user: user).count).to eq(2) + expect(TopicUser.find_by(topic: topic, user: user).bookmarked).to eq(true) + end + + context "when there are no bookmarks left in the topic" do + it "sets TopicUser.bookmarked to false" do + creator.create + expect(TopicUser.find_by(topic: topic, user: user).bookmarked).to eq(false) + end end end diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb index 21470898812..c8e5ef806f7 100644 --- a/spec/lib/bookmark_reminder_notification_handler_spec.rb +++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb @@ -35,11 +35,31 @@ RSpec.describe BookmarkReminderNotificationHandler do end context "when the auto_delete_preference is when_reminder_sent" do - it "deletes the bookmark after the reminder gets sent" do + before do + TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true) bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent]) + end + + it "deletes the bookmark after the reminder gets sent" do subject.send_notification(bookmark) expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) end + + it "changes the TopicUser bookmarked column to false" do + subject.send_notification(bookmark) + expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(false) + end + + context "if there are still other bookmarks in the topic" do + before do + Fabricate(:bookmark, topic: bookmark.topic, post: Fabricate(:post, topic: bookmark.topic), user: user) + end + + it "does not change the TopicUser bookmarked column to false" do + subject.send_notification(bookmark) + expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(true) + end + end end context "when the post has been deleted" do