diff --git a/app/assets/javascripts/discourse/app/lib/transform-post.js b/app/assets/javascripts/discourse/app/lib/transform-post.js index b9695b51fed..8f61645e5d6 100644 --- a/app/assets/javascripts/discourse/app/lib/transform-post.js +++ b/app/assets/javascripts/discourse/app/lib/transform-post.js @@ -39,7 +39,6 @@ export function transformBasicPost(post) { bookmarked: post.bookmarked, bookmarkReminderAt: post.bookmark_reminder_at, bookmarkName: post.bookmark_name, - bookmarkReminderType: post.bookmark_reminder_type, yours: post.yours, shareUrl: post.get("shareUrl"), staff: post.staff, diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index c0645e2ffa2..f8b68bed285 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/app/controllers/bookmarks_controller.rb @@ -14,7 +14,6 @@ class BookmarksController < ApplicationController bookmark = bookmark_manager.create( post_id: params[:post_id], name: params[:name], - reminder_type: params[:reminder_type], reminder_at: params[:reminder_at], options: { auto_delete_preference: params[:auto_delete_preference] || 0 @@ -41,7 +40,6 @@ class BookmarksController < ApplicationController bookmark_manager.update( bookmark_id: params[:id], name: params[:name], - reminder_type: params[:reminder_type], reminder_at: params[:reminder_at], options: { auto_delete_preference: params[:auto_delete_preference] || 0 diff --git a/app/jobs/regular/export_user_archive.rb b/app/jobs/regular/export_user_archive.rb index 7a8e4b113f5..4ff3f1fcb81 100644 --- a/app/jobs/regular/export_user_archive.rb +++ b/app/jobs/regular/export_user_archive.rb @@ -31,7 +31,7 @@ module Jobs auth_tokens: ['id', 'auth_token_hash', 'prev_auth_token_hash', 'auth_token_seen', 'client_ip', 'user_agent', 'seen_at', 'rotated_at', 'created_at', 'updated_at'], auth_token_logs: ['id', 'action', 'user_auth_token_id', 'client_ip', 'auth_token_hash', 'created_at', 'path', 'user_agent'], badges: ['badge_id', 'badge_name', 'granted_at', 'post_id', 'seq', 'granted_manually', 'notification_id', 'featured_rank'], - bookmarks: ['post_id', 'topic_id', 'post_number', 'link', 'name', 'created_at', 'updated_at', 'reminder_type', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'], + bookmarks: ['post_id', 'topic_id', 'post_number', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'], category_preferences: ['category_id', 'category_names', 'notification_level', 'dismiss_new_timestamp'], flags: ['id', 'post_id', 'flag_type', 'created_at', 'updated_at', 'deleted_at', 'deleted_by', 'related_post_id', 'targets_topic', 'was_take_action'], likes: ['id', 'post_id', 'topic_id', 'post_number', 'created_at', 'updated_at', 'deleted_at', 'deleted_by'], @@ -248,7 +248,6 @@ module Jobs bkmk.name, bkmk.created_at, bkmk.updated_at, - Bookmark.reminder_types[bkmk.reminder_type], bkmk.reminder_at, bkmk.reminder_last_sent_at, bkmk.reminder_set_at, diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 69219cfe62c..14f9eba0cda 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -2,7 +2,8 @@ class Bookmark < ActiveRecord::Base self.ignored_columns = [ - "topic_id" # TODO (martin) (2021-12-01): remove + "topic_id", # TODO (martin) (2021-12-01): remove + "reminder_type" # TODO (martin) (2021-12-01): remove ] belongs_to :user @@ -11,6 +12,15 @@ class Bookmark < ActiveRecord::Base delegate :topic_id, to: :post + def self.auto_delete_preferences + @auto_delete_preferences ||= Enum.new( + never: 0, + when_reminder_sent: 1, + on_owner_reply: 2 + ) + end + + # TODO (martin) (2021-12-01) Remove this once plugins are not using it. def self.reminder_types @reminder_types ||= Enum.new( later_today: 1, @@ -24,19 +34,6 @@ class Bookmark < ActiveRecord::Base ) end - def self.auto_delete_preferences - @auto_delete_preferences ||= Enum.new( - never: 0, - when_reminder_sent: 1, - on_owner_reply: 2 - ) - end - - validates :reminder_at, presence: { - message: I18n.t("bookmarks.errors.time_must_be_provided"), - if: -> { reminder_type.present? } - } - validate :unique_per_post_for_user, on: [:create, :update], if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_user_id? } @@ -83,7 +80,7 @@ class Bookmark < ActiveRecord::Base end def no_reminder? - self.reminder_at.blank? && self.reminder_type.blank? + self.reminder_at.blank? end def auto_delete_when_reminder_sent? @@ -101,7 +98,6 @@ class Bookmark < ActiveRecord::Base def clear_reminder! update!( reminder_at: nil, - reminder_type: nil, reminder_last_sent_at: Time.zone.now, reminder_set_at: nil ) @@ -173,7 +169,6 @@ end # user_id :bigint not null # post_id :bigint not null # name :string(100) -# reminder_type :integer # reminder_at :datetime # created_at :datetime not null # updated_at :datetime not null @@ -188,7 +183,6 @@ end # 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/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index d585090beda..8de921eefff 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -53,7 +53,6 @@ class PostSerializer < BasicPostSerializer :bookmarked, :bookmark_reminder_at, :bookmark_id, - :bookmark_reminder_type, :bookmark_name, :bookmark_auto_delete_preference, :raw, @@ -346,10 +345,6 @@ class PostSerializer < BasicPostSerializer bookmarked end - def include_bookmark_reminder_type? - bookmarked - end - def include_bookmark_name? bookmarked end @@ -374,11 +369,6 @@ class PostSerializer < BasicPostSerializer post_bookmark&.reminder_at end - def bookmark_reminder_type - return if post_bookmark.blank? - Bookmark.reminder_types[post_bookmark.reminder_type].to_s - end - def bookmark_name post_bookmark&.name end diff --git a/db/migrate/20210913032326_add_for_topic_to_bookmarks.rb b/db/migrate/20210913032326_add_for_topic_to_bookmarks.rb index 2660cf69a18..84524f570e0 100644 --- a/db/migrate/20210913032326_add_for_topic_to_bookmarks.rb +++ b/db/migrate/20210913032326_add_for_topic_to_bookmarks.rb @@ -4,6 +4,8 @@ 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] + if index_exists?(:bookmarks, [:user_id, :post_id]) + remove_index :bookmarks, [:user_id, :post_id] + end end end diff --git a/db/migrate/20210915215952_mark_reminder_type_as_readonly_for_bookmarks.rb b/db/migrate/20210915215952_mark_reminder_type_as_readonly_for_bookmarks.rb new file mode 100644 index 00000000000..83f171d7cac --- /dev/null +++ b/db/migrate/20210915215952_mark_reminder_type_as_readonly_for_bookmarks.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class MarkReminderTypeAsReadonlyForBookmarks < ActiveRecord::Migration[6.1] + def up + Migration::ColumnDropper.mark_readonly(:bookmarks, :reminder_type) + end + + def down + Migration::ColumnDropper.drop_readonly(:bookmarks, :reminder_type) + end +end diff --git a/db/migrate/20210915222124_drop_reminder_type_index_for_bookmarks.rb b/db/migrate/20210915222124_drop_reminder_type_index_for_bookmarks.rb new file mode 100644 index 00000000000..06e9fb010ea --- /dev/null +++ b/db/migrate/20210915222124_drop_reminder_type_index_for_bookmarks.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class DropReminderTypeIndexForBookmarks < ActiveRecord::Migration[6.1] + def up + if index_exists?(:bookmarks, [:reminder_type]) + remove_index :bookmarks, [:reminder_type] + end + end + + def down + if !index_exists?(:bookmarks, [:reminder_type]) + add_index :bookmarks, :reminder_type + end + end +end diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 89defccfdba..1a3558adad1 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -7,9 +7,9 @@ class BookmarkManager @user = user end - def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil, options: {}) + # TODO (martin) (2021-12-01) Remove reminder_type keyword argument once plugins are not using it. + def create(post_id:, name: nil, reminder_at: nil, reminder_type: nil, options: {}) post = Post.find_by(id: post_id) - reminder_type = parse_reminder_type(reminder_type) # no bookmarking deleted posts or topics raise Discourse::InvalidAccess if post.blank? || post.topic.blank? @@ -23,7 +23,6 @@ class BookmarkManager user_id: @user.id, post: post, name: name, - reminder_type: reminder_type, reminder_at: reminder_at, reminder_set_at: Time.zone.now }.merge(options) @@ -67,16 +66,14 @@ class BookmarkManager BookmarkReminderNotificationHandler.send_notification(bookmark) end - def update(bookmark_id:, name:, reminder_type:, reminder_at:, options: {}) + # TODO (martin) (2021-12-01) Remove reminder_type keyword argument once plugins are not using it. + def update(bookmark_id:, name:, reminder_at:, reminder_type: nil, options: {}) bookmark = find_bookmark_and_check_access(bookmark_id) - reminder_type = parse_reminder_type(reminder_type) - success = bookmark.update( { name: name, reminder_at: reminder_at, - reminder_type: reminder_type, reminder_set_at: Time.zone.now }.merge(options) ) @@ -118,9 +115,4 @@ class BookmarkManager TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic) bookmarks_remaining_in_topic end - - def parse_reminder_type(reminder_type) - return if reminder_type.blank? - reminder_type.is_a?(Integer) ? reminder_type : Bookmark.reminder_types[reminder_type.to_sym] - end end diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index 84ba1348d48..28810ce7758 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -23,7 +23,7 @@ class BookmarkReminderNotificationHandler def self.clear_reminder(bookmark) Rails.logger.debug( - "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder info: #{bookmark.reminder_at} | #{Bookmark.reminder_types[bookmark.reminder_type]}" + "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder at: #{bookmark.reminder_at}" ) bookmark.clear_reminder! diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb index bab3bef8a2c..01b02d507ee 100644 --- a/spec/fabricators/bookmark_fabricator.rb +++ b/spec/fabricators/bookmark_fabricator.rb @@ -4,13 +4,11 @@ Fabricator(:bookmark) do user post { Fabricate(:post) } name "This looked interesting" - reminder_type { Bookmark.reminder_types[:tomorrow] } reminder_at { 1.day.from_now.iso8601 } reminder_set_at { Time.zone.now } end Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do - reminder_type { Bookmark.reminder_types[:next_business_day] } reminder_at do date = if Time.zone.now.friday? Time.zone.now + 3.days diff --git a/spec/jobs/export_user_archive_spec.rb b/spec/jobs/export_user_archive_spec.rb index 810e787d3a1..8f083f172c7 100644 --- a/spec/jobs/export_user_archive_spec.rb +++ b/spec/jobs/export_user_archive_spec.rb @@ -264,7 +264,6 @@ describe Jobs::ExportUserArchive do let(:post3) { Fabricate(:post) } let(:message) { Fabricate(:private_message_topic) } let(:post4) { Fabricate(:post, topic: message) } - let(:reminder_type) { Bookmark.reminder_types[:tomorrow] } let(:reminder_at) { 1.day.from_now } it 'properly includes bookmark records' do @@ -274,9 +273,9 @@ describe Jobs::ExportUserArchive do update1_at = now + 1.hours bkmk1.update(name: 'great food recipe', updated_at: update1_at) - manager.create(post_id: post2.id, name: name, reminder_type: :tomorrow, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }) + manager.create(post_id: post2.id, name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }) twelve_hr_ago = freeze_time now - 12.hours - pending_reminder = manager.create(post_id: post3.id, name: name, reminder_type: :later_today, reminder_at: now - 8.hours) + pending_reminder = manager.create(post_id: post3.id, name: name, reminder_at: now - 8.hours) freeze_time now tau_record = message.topic_allowed_users.create!(user_id: user.id) @@ -296,7 +295,6 @@ describe Jobs::ExportUserArchive do expect(DateTime.parse(data[0]['updated_at'])).to eq(DateTime.parse(update1_at.to_s)) expect(data[1]['name']).to eq(name) - expect(data[1]['reminder_type']).to eq('tomorrow') expect(DateTime.parse(data[1]['reminder_at'])).to eq(DateTime.parse(reminder_at.to_s)) expect(data[1]['auto_delete_preference']).to eq('when_reminder_sent') diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index 5461f8f1209..95600c2be97 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -5,7 +5,6 @@ require 'rails_helper' RSpec.describe BookmarkManager do let(:user) { Fabricate(:user) } - let(:reminder_type) { 'tomorrow' } let(:reminder_at) { 1.day.from_now } fab!(:post) { Fabricate(:post) } let(:name) { 'Check this out!' } @@ -32,7 +31,7 @@ RSpec.describe BookmarkManager do end it "updates the topic user bookmarked column to true if any post is bookmarked" do - subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + subject.create(post_id: post.id, name: name, reminder_at: reminder_at) tu = TopicUser.find_by(user: user) expect(tu.bookmarked).to eq(true) tu.update(bookmarked: false) @@ -41,14 +40,13 @@ RSpec.describe BookmarkManager do expect(tu.bookmarked).to eq(true) end - context "when a reminder time + type is provided" do + context "when a reminder time is provided" do it "saves the values correctly" do - subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + subject.create(post_id: post.id, name: name, reminder_at: reminder_at) bookmark = Bookmark.find_by(user: user) expect(bookmark.reminder_at).to eq_time(reminder_at) expect(bookmark.reminder_set_at).not_to eq(nil) - expect(bookmark.reminder_type).to eq(Bookmark.reminder_types[:tomorrow]) end end @@ -81,21 +79,11 @@ RSpec.describe BookmarkManager do end end - context "when the reminder time is not provided when it needs to be" do - let(:reminder_at) { nil } - it "adds an error to the manager" do - subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) - expect(subject.errors.full_messages).to include( - "Reminder at " + I18n.t("bookmarks.errors.time_must_be_provided") - ) - end - end - context "when the reminder time is in the past" do let(:reminder_at) { 10.days.ago } it "adds an error to the manager" do - subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + subject.create(post_id: post.id, name: name, reminder_at: reminder_at) expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_past_reminder")) end end @@ -104,7 +92,7 @@ RSpec.describe BookmarkManager do let(:reminder_at) { 11.years.from_now } it "adds an error to the manager" do - subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + subject.create(post_id: post.id, name: name, reminder_at: reminder_at) expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future")) end end @@ -169,25 +157,22 @@ RSpec.describe BookmarkManager do let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: user, post: post, name: "Old name") } let(:new_name) { "Some new name" } let(:new_reminder_at) { 10.days.from_now } - let(:new_reminder_type) { Bookmark.reminder_types[:custom] } let(:options) { {} } def update_bookmark subject.update( bookmark_id: bookmark.id, name: new_name, - reminder_type: new_reminder_type, reminder_at: new_reminder_at, options: options ) end - it "saves the time and new reminder type and new name successfully" do + it "saves the time and new name successfully" do update_bookmark bookmark.reload expect(bookmark.name).to eq(new_name) expect(bookmark.reminder_at).to eq_time(new_reminder_at) - expect(bookmark.reminder_type).to eq(new_reminder_type) end context "when options are provided" do @@ -200,15 +185,6 @@ RSpec.describe BookmarkManager do end end - context "if the new reminder type is a string" do - let(:new_reminder_type) { "custom" } - it "is parsed" do - update_bookmark - bookmark.reload - expect(bookmark.reminder_type).to eq(Bookmark.reminder_types[:custom]) - end - end - context "if the bookmark is belonging to some other user" do let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) } it "raises an invalid access error" do diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index 95f77d79cf0..b53ed7b8151 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -168,11 +168,11 @@ RSpec.describe BookmarkQuery do end describe "#list_all ordering" do - let!(:bookmark1) { Fabricate(:bookmark, user: user, updated_at: 1.day.ago, reminder_type: nil, reminder_at: nil) } - let!(:bookmark2) { Fabricate(:bookmark, user: user, updated_at: 2.days.ago, reminder_type: nil, reminder_at: nil) } - let!(:bookmark3) { Fabricate(:bookmark, user: user, updated_at: 6.days.ago, reminder_type: nil, reminder_at: nil) } - let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago, reminder_type: nil, reminder_at: nil) } - let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago, reminder_type: nil, reminder_at: nil) } + let!(:bookmark1) { Fabricate(:bookmark, user: user, updated_at: 1.day.ago, reminder_at: nil) } + let!(:bookmark2) { Fabricate(:bookmark, user: user, updated_at: 2.days.ago, reminder_at: nil) } + let!(:bookmark3) { Fabricate(:bookmark, user: user, updated_at: 6.days.ago, reminder_at: nil) } + let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago, reminder_at: nil) } + let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago, reminder_at: nil) } it "order defaults to updated_at DESC" do expect(bookmark_query.list_all.map(&:id)).to eq([ @@ -185,9 +185,7 @@ RSpec.describe BookmarkQuery do end it "orders by reminder_at, then updated_at" do - bookmark4.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow]) bookmark4.update_column(:reminder_at, 1.day.from_now) - bookmark5.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow]) bookmark5.update_column(:reminder_at, 26.hours.from_now) expect(bookmark_query.list_all.map(&:id)).to eq([ @@ -202,17 +200,14 @@ RSpec.describe BookmarkQuery do it "shows pinned bookmarks first ordered by reminder_at ASC then updated_at DESC" do bookmark3.update_column(:pinned, true) - bookmark3.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow]) bookmark3.update_column(:reminder_at, 1.day.from_now) bookmark4.update_column(:pinned, true) - bookmark4.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow]) bookmark4.update_column(:reminder_at, 28.hours.from_now) bookmark1.update_column(:pinned, true) bookmark2.update_column(:pinned, true) - bookmark5.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow]) bookmark5.update_column(:reminder_at, 1.day.from_now) expect(bookmark_query.list_all.map(&:id)).to eq([ diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb index 1011bb517b3..05e8f2429b7 100644 --- a/spec/requests/bookmarks_controller_spec.rb +++ b/spec/requests/bookmarks_controller_spec.rb @@ -19,7 +19,6 @@ describe BookmarksController do post "/bookmarks.json", params: { post_id: bookmark_post.id, - reminder_type: "tomorrow", reminder_at: (Time.zone.now + 1.day).iso8601 } @@ -39,7 +38,6 @@ describe BookmarksController do it "returns failed JSON with a 400 error" do post "/bookmarks.json", params: { post_id: bookmark_post.id, - reminder_type: "tomorrow", reminder_at: (Time.zone.now + 1.day).iso8601 } post "/bookmarks.json", params: { @@ -62,7 +60,6 @@ describe BookmarksController do it "returns failed JSON with a 400 error" do post "/bookmarks.json", params: { post_id: bookmark_post.id, - reminder_type: "tomorrow", reminder_at: (Time.zone.now + 1.day).iso8601 } @@ -72,20 +69,6 @@ describe BookmarksController do ) end end - - context "if the user provides a reminder type that needs a reminder_at that is missing" do - it "returns failed JSON with a 400 error" do - post "/bookmarks.json", params: { - post_id: bookmark_post.id, - reminder_type: "tomorrow" - } - - expect(response.status).to eq(400) - expect(response.parsed_body['errors'].first).to include( - I18n.t("bookmarks.errors.time_must_be_provided") - ) - end - end end describe "#destroy" do