diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index 09810cae00c..2124aa3cfba 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/app/controllers/bookmarks_controller.rb @@ -6,26 +6,22 @@ class BookmarksController < ApplicationController def create params.require(:bookmarkable_id) params.require(:bookmarkable_type) + params.permit(:bookmarkable_id, :bookmarkable_type, :name, :reminder_at, :auto_delete_preference) RateLimiter.new( current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i ).performed! bookmark_manager = BookmarkManager.new(current_user) - - create_params = { + bookmark = bookmark_manager.create_for( + bookmarkable_id: params[:bookmarkable_id], + bookmarkable_type: params[:bookmarkable_type], name: params[:name], reminder_at: params[:reminder_at], options: { - auto_delete_preference: params[:auto_delete_preference] || 0 + auto_delete_preference: params[:auto_delete_preference], + save_user_preferences: true } - } - - bookmark = bookmark_manager.create_for( - **create_params.merge( - bookmarkable_id: params[:bookmarkable_id], - bookmarkable_type: params[:bookmarkable_type] - ) ) if bookmark_manager.errors.empty? @@ -43,6 +39,7 @@ class BookmarksController < ApplicationController def update params.require(:id) + params.permit(:id, :name, :reminder_at, :auto_delete_preference) bookmark_manager = BookmarkManager.new(current_user) bookmark_manager.update( @@ -50,7 +47,8 @@ class BookmarksController < ApplicationController name: params[:name], reminder_at: params[:reminder_at], options: { - auto_delete_preference: params[:auto_delete_preference] || 0 + auto_delete_preference: params[:auto_delete_preference], + save_user_preferences: true } ) diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 4117d314d7d..d7d7d2aaa97 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -40,8 +40,8 @@ class BookmarkManager # this is used to determine when to delete a bookmark # automatically. def create_for(bookmarkable_id:, bookmarkable_type:, name: nil, reminder_at: nil, options: {}) - bookmarkable = bookmarkable_type.constantize.find_by(id: bookmarkable_id) registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type) + bookmarkable = registered_bookmarkable.model.find_by(id: bookmarkable_id) registered_bookmarkable.validate_before_create(@guardian, bookmarkable) bookmark = Bookmark.create( @@ -51,13 +51,13 @@ class BookmarkManager name: name, reminder_at: reminder_at, reminder_set_at: Time.zone.now - }.merge(options) + }.merge(bookmark_model_options_with_defaults(options)) ) return add_errors_from(bookmark) if bookmark.errors.any? registered_bookmarkable.after_create(@guardian, bookmark, options) - update_user_option(bookmark) + update_user_option(bookmark, options) bookmark end @@ -102,14 +102,14 @@ class BookmarkManager { name: name, reminder_set_at: Time.zone.now, - }.merge(options) + }.merge(bookmark_model_options_with_defaults(options)) ) if bookmark.errors.any? return add_errors_from(bookmark) end - update_user_option(bookmark) + update_user_option(bookmark, options) success end @@ -142,7 +142,18 @@ class BookmarkManager TopicUser.change(@user.id, topic, bookmarked: Bookmark.for_user_in_topic(@user.id, topic.id).exists?) end - def update_user_option(bookmark) - @user.user_option.update!(bookmark_auto_delete_preference: bookmark.auto_delete_preference) + def update_user_option(bookmark, options) + return if !options[:save_user_preferences] + @user.user_option.update!( + bookmark_auto_delete_preference: bookmark.auto_delete_preference + ) + end + + def bookmark_model_options_with_defaults(options) + if options[:auto_delete_preference].blank? + options[:auto_delete_preference] = Bookmark.auto_delete_preferences[:never] + end + + options.slice(:auto_delete_preference, :pinned) end end diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index 3b8e9cf75d1..4ea077e7739 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -227,6 +227,11 @@ RSpec.describe BookmarkManager do expect(tu.bookmarked).to eq(true) end + it "sets auto_delete_preference to never by default" do + bookmark = subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) + expect(bookmark.auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:never]) + end + context "when a reminder time is provided" do it "saves the values correctly" do subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) @@ -302,12 +307,41 @@ RSpec.describe BookmarkManager do end end - it "saves user's preference" do - subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }) + it "does not save user preference by default" do + user.user_option.update(bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) + subject.create_for( + bookmarkable_id: post.id, + bookmarkable_type: "Post", + options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] } + ) + expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply]) + + bookmark = Bookmark.find_by(user: user) + subject.update( + bookmark_id: bookmark.id, + name: "test", + reminder_at: 1.day.from_now, + options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] } + ) + expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply]) + end + + it "saves user's preference when save_user_preferences option is specified" do + user.user_option.update(bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) + subject.create_for( + bookmarkable_id: post.id, + bookmarkable_type: "Post", + options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent], save_user_preferences: true } + ) expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:when_reminder_sent]) bookmark = Bookmark.find_by(user: user) - subject.update(bookmark_id: bookmark.id, name: "test", reminder_at: 1.day.from_now, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] }) + subject.update( + bookmark_id: bookmark.id, + name: "test", + reminder_at: 1.day.from_now, + options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply], save_user_preferences: true } + ) expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply]) end end