diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 0b1a266ada9..6cf254972ac 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1598,7 +1598,10 @@ class UsersController < ApplicationController end end format.ics do - @bookmark_reminders = Bookmark.where(user_id: user.id).where.not(reminder_at: nil).joins(:topic) + @bookmark_reminders = Bookmark.with_reminders + .where(user_id: user.id) + .includes(:topic) + .order(:reminder_at) end end end diff --git a/app/jobs/regular/sync_topic_user_bookmarked.rb b/app/jobs/regular/sync_topic_user_bookmarked.rb index cf8db86c582..bd630253e82 100644 --- a/app/jobs/regular/sync_topic_user_bookmarked.rb +++ b/app/jobs/regular/sync_topic_user_bookmarked.rb @@ -11,7 +11,7 @@ module Jobs INNER JOIN posts ON posts.id = b.post_id WHERE NOT topic_users.bookmarked AND posts.deleted_at IS NULL AND - topic_users.topic_id = b.topic_id AND + topic_users.topic_id = posts.topic_id AND topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} SQL @@ -22,7 +22,7 @@ module Jobs SELECT COUNT(*) FROM bookmarks INNER JOIN posts ON posts.id = bookmarks.post_id - WHERE bookmarks.topic_id = topic_users.topic_id + WHERE posts.topic_id = topic_users.topic_id AND bookmarks.user_id = topic_users.user_id AND posts.deleted_at IS NULL ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 6bc1a016cdf..74a0d27bcb9 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -1,9 +1,15 @@ # frozen_string_literal: true class Bookmark < ActiveRecord::Base + self.ignored_columns = [ + "topic_id" # TODO (martin) (2021-12-01): remove + ] + belongs_to :user belongs_to :post - belongs_to :topic + has_one :topic, through: :post + + delegate :topic_id, to: :post def self.reminder_types @reminder_types ||= Enum.new( @@ -39,16 +45,6 @@ class Bookmark < ActiveRecord::Base validate :bookmark_limit_not_reached validates :name, length: { maximum: 100 } - # we don't care whether the post or topic is deleted, - # they hold important information about the bookmark - def post - Post.unscoped { super } - end - - def topic - Topic.unscoped { super } - end - def unique_per_post_for_user if Bookmark.exists?(user_id: user_id, post_id: post_id) self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post")) @@ -91,6 +87,10 @@ class Bookmark < ActiveRecord::Base self.auto_delete_preference == Bookmark.auto_delete_preferences[:on_owner_reply] end + def reminder_at_ics(offset: 0) + (reminder_at + offset).strftime(I18n.t("datetime_formats.formats.calendar_ics")) + end + def clear_reminder! update!( reminder_at: nil, @@ -100,19 +100,34 @@ class Bookmark < ActiveRecord::Base ) end + scope :with_reminders, -> do + where("reminder_at IS NOT NULL") + 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) + with_reminders.where("reminder_at <= :before_time", before_time: before_time) end scope :pending_reminders_for_user, ->(user) do pending_reminders.where(user: user) end + scope :for_user_in_topic, ->(user_id, topic_id) { + joins(:post).where(user_id: user_id, posts: { topic_id: topic_id }) + } + def self.count_per_day(opts = nil) opts ||= {} result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago) - result = result.where('bookmarks.created_at <= ?', opts[:end_date]) if opts[:end_date] - result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id])) if opts[:category_id] + + if opts[:end_date] + result = result.where('bookmarks.created_at <= ?', opts[:end_date]) + end + + if opts[:category_id] + result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id])) + end + result.group('date(bookmarks.created_at)') .order('date(bookmarks.created_at)') .count @@ -127,9 +142,9 @@ class Bookmark < ActiveRecord::Base topics_deleted = DB.query(<<~SQL, grace_time: grace_time) DELETE FROM bookmarks b USING topics t, posts p - WHERE (b.topic_id = t.id AND b.post_id = p.id) + WHERE (t.id = p.topic_id AND b.post_id = p.id) AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time) - RETURNING b.topic_id + RETURNING t.id AS topic_id SQL topics_deleted_ids = topics_deleted.map(&:topic_id).uniq @@ -145,7 +160,6 @@ end # # id :bigint not null, primary key # user_id :bigint not null -# topic_id :bigint not null # post_id :bigint not null # name :string(100) # reminder_type :integer diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 7762f149f01..3c280eceff5 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -176,6 +176,10 @@ class PostMover DiscourseEvent.trigger(:post_moved, new_post, original_topic.id) + # we don't want to keep the old topic's OP bookmarked when we are + # moving it into a new topic + Bookmark.where(post_id: post.id).update_all(post_id: new_post.id) + new_post end @@ -479,8 +483,6 @@ class PostMover end def update_bookmarks - Bookmark.where(post_id: post_ids).update_all(topic_id: @destination_topic.id) - DB.after_commit do Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @original_topic.id) Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @destination_topic.id) diff --git a/app/models/topic.rb b/app/models/topic.rb index 6b89da4f873..95cb1b94dfa 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -203,7 +203,7 @@ class Topic < ActiveRecord::Base belongs_to :category has_many :category_users, through: :category has_many :posts - has_many :bookmarks + has_many :bookmarks, through: :posts has_many :ordered_posts, -> { order(post_number: :asc) }, class_name: "Post" has_many :topic_allowed_users has_many :topic_allowed_groups diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb index 39a494c5792..e724931c5dd 100644 --- a/app/serializers/user_bookmark_serializer.rb +++ b/app/serializers/user_bookmark_serializer.rb @@ -30,12 +30,16 @@ class UserBookmarkSerializer < ApplicationSerializer :post_user_avatar_template, :post_user_name + def topic_id + post.topic_id + end + def topic - @topic ||= object.topic || Topic.unscoped.find(object.topic_id) + @topic ||= object.topic end def post - @post ||= object.post || Post.unscoped.find(object.post_id) + @post ||= object.post end def closed diff --git a/app/views/users/bookmarks.ics.erb b/app/views/users/bookmarks.ics.erb index cfb50701ae4..a0c93c13881 100644 --- a/app/views/users/bookmarks.ics.erb +++ b/app/views/users/bookmarks.ics.erb @@ -4,9 +4,9 @@ PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_versi <% @bookmark_reminders.each do |bookmark| %> BEGIN:VEVENT UID:bookmark_reminder_#<%= bookmark.id %>@<%= Discourse.current_hostname %> -DTSTAMP:<%= bookmark.updated_at.strftime("%Y%m%dT%H%M%SZ") %> -DTSTART:<%= bookmark.reminder_at.strftime("%Y%m%dT%H%M%SZ") %> -DTEND:<%= (bookmark.reminder_at + 1.hour).strftime("%Y%m%dT%H%M%SZ") %> +DTSTAMP:<%= bookmark.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics")) %> +DTSTART:<%= bookmark.reminder_at_ics %> +DTEND:<%= bookmark.reminder_at_ics(offset: 1.hour) %> SUMMARY:<%= bookmark.name.presence || bookmark.topic.title %> DESCRIPTION:<%= Discourse.base_url %>/t/-/<%= bookmark.topic_id %> URL:<%= Discourse.base_url %>/t/-/<%= bookmark.topic_id %> diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5666cf7920c..a1af3b477c5 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -19,6 +19,8 @@ en: long: "%B %-d, %Y, %l:%M%P" # Format directives: https://ruby-doc.org/core/Time.html#method-i-strftime no_day: "%B %Y" + # Format directives: https://ruby-doc.org/core/Time.html#method-i-strftime + calendar_ics: "%Y%m%dT%H%M%SZ" date: # Do not remove the brackets and commas and do not translate the first month name. It should be "null". month_names: diff --git a/db/post_migrate/20210909041448_make_topic_id_nullable_for_bookmarks.rb b/db/post_migrate/20210909041448_make_topic_id_nullable_for_bookmarks.rb new file mode 100644 index 00000000000..7e8c75294d2 --- /dev/null +++ b/db/post_migrate/20210909041448_make_topic_id_nullable_for_bookmarks.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class MakeTopicIdNullableForBookmarks < ActiveRecord::Migration[6.1] + def up + change_column_null :bookmarks, :topic_id, true + Migration::ColumnDropper.mark_readonly(:bookmarks, :topic_id) + end + + def down + Migration::ColumnDropper.drop_readonly(:bookmarks, :topic_id) + change_column_null :bookmarks, :topic_id, false + end +end diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 6bd8f58316a..89defccfdba 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -21,7 +21,6 @@ class BookmarkManager bookmark = Bookmark.create( { user_id: @user.id, - topic: post.topic, post: post, name: name, reminder_type: reminder_type, @@ -50,7 +49,7 @@ class BookmarkManager end def destroy_for_topic(topic, filter = {}, opts = {}) - topic_bookmarks = Bookmark.where(user_id: @user.id, topic_id: topic.id) + topic_bookmarks = Bookmark.for_user_in_topic(@user.id, topic.id) topic_bookmarks = topic_bookmarks.where(filter) Bookmark.transaction do @@ -113,7 +112,7 @@ class BookmarkManager 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) + bookmarks_remaining_in_topic = Bookmark.for_user_in_topic(@user.id, topic.id).exists? return bookmarks_remaining_in_topic if opts.key?(:auto_track) && !opts[:auto_track] TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic) diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 02dee3b8d21..01d839554a5 100644 --- a/lib/bookmark_query.rb +++ b/lib/bookmark_query.rb @@ -69,9 +69,8 @@ class BookmarkQuery def user_bookmarks Bookmark.where(user: @user) - .includes(topic: :tags) .includes(post: :user) - .references(:topic) + .includes(post: { topic: :tags }) .references(:post) end end diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index 55c8247811b..84ba1348d48 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -4,9 +4,12 @@ class BookmarkReminderNotificationHandler def self.send_notification(bookmark) return if bookmark.blank? Bookmark.transaction do - if bookmark.post.blank? || bookmark.post.deleted_at.present? + # we don't send reminders for deleted posts or topics, + # just as we don't allow creation of bookmarks for deleted + # posts or topics + if bookmark.post.blank? || bookmark.topic.blank? clear_reminder(bookmark) - elsif bookmark.topic + else create_notification(bookmark) if bookmark.auto_delete_when_reminder_sent? diff --git a/lib/tasks/bookmarks.rake b/lib/tasks/bookmarks.rake index 87fe005b075..0b40eb38f6a 100644 --- a/lib/tasks/bookmarks.rake +++ b/lib/tasks/bookmarks.rake @@ -31,7 +31,7 @@ task "bookmarks:sync_to_table" => :environment do |_t, args| post_action_bookmarks.each do |pab| now = Time.zone.now - bookmarks_to_create << "(#{pab.topic_id}, #{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')" + bookmarks_to_create << "(#{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')" end create_bookmarks(bookmarks_to_create) @@ -54,7 +54,7 @@ def create_bookmarks(bookmarks_to_create) # DB.exec( <<~SQL - INSERT INTO bookmarks (topic_id, post_id, user_id, created_at, updated_at) + INSERT INTO bookmarks (post_id, user_id, created_at, updated_at) VALUES #{bookmarks_to_create.join(",\n")} ON CONFLICT DO NOTHING SQL diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 0e72a2b14b0..b91ea3af1f0 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -392,6 +392,7 @@ class TopicView def has_bookmarks? return false if @user.blank? + return false if @topic.trashed? @topic.bookmarks.exists?(user_id: @user.id) end diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index bf677b39df9..4948eb05483 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -786,13 +786,13 @@ describe PostCreator do context "when the user has bookmarks with auto_delete_preference on_owner_reply" 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]) + Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) + Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: topic), 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, post: Fabricate(:post, topic: topic), user: user) Fabricate(:bookmark, user: user) creator.create expect(Bookmark.where(user: user).count).to eq(2) diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index ac4e91d746a..671c5f9f454 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -424,16 +424,25 @@ describe TopicView do end context "when the topic is deleted" do - it "gets the first post bookmark reminder at for the user" do + it "returns nil" do topic_view = TopicView.new(topic, user) PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy topic.reload - first, second = topic_view.bookmarked_posts + expect(topic_view.bookmarked_posts).to eq(nil) + end + end + + context "when one of the posts is deleted" do + it "does not return that post's bookmark" do + topic_view = TopicView.new(topic, user) + PostDestroyer.new(Fabricate(:admin), topic.posts.second).destroy + topic.reload + + expect(topic_view.bookmarked_posts.length).to eq(1) + first = topic_view.bookmarked_posts.first expect(first[:post_id]).to eq(bookmark1.post_id) expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at) - expect(second[:post_id]).to eq(bookmark2.post_id) - expect(second[:reminder_at]).to eq_time(bookmark2.reminder_at) end end end diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb index ede44dfecae..bab3bef8a2c 100644 --- a/spec/fabricators/bookmark_fabricator.rb +++ b/spec/fabricators/bookmark_fabricator.rb @@ -3,7 +3,6 @@ Fabricator(:bookmark) do user post { Fabricate(:post) } - topic { |attrs| attrs[:post].topic } name "This looked interesting" reminder_type { Bookmark.reminder_types[:tomorrow] } reminder_at { 1.day.from_now.iso8601 } diff --git a/spec/jobs/sync_topic_user_bookmarked_spec.rb b/spec/jobs/sync_topic_user_bookmarked_spec.rb index 4d4cd107251..5a30f558382 100644 --- a/spec/jobs/sync_topic_user_bookmarked_spec.rb +++ b/spec/jobs/sync_topic_user_bookmarked_spec.rb @@ -15,8 +15,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true) tu5 = Fabricate(:topic_user, bookmarked: false) - Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample) - Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample) + Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) subject.execute(topic_id: topic.id) @@ -34,8 +34,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false) tu2 = Fabricate(:topic_user, topic: topic, bookmarked: true) - Fabricate(:bookmark, user: tu1.user, topic: topic, post: post1) - Fabricate(:bookmark, user: tu2.user, topic: topic, post: post1) + Fabricate(:bookmark, user: tu1.user, post: post1) + Fabricate(:bookmark, user: tu2.user, post: post1) post1.trash! @@ -57,8 +57,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true) tu5 = Fabricate(:topic_user, bookmarked: false) - Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample) - Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample) + Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) subject.execute diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index 11cbf642b16..5461f8f1209 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -18,7 +18,7 @@ RSpec.describe BookmarkManager do bookmark = Bookmark.find_by(user: user) expect(bookmark.post_id).to eq(post.id) - expect(bookmark.topic_id).to eq(post.topic_id) + expect(bookmark.topic.id).to eq(post.topic_id) end it "when topic is deleted it raises invalid access from guardian check" do @@ -65,7 +65,7 @@ RSpec.describe BookmarkManager do context "when the bookmark already exists for the user & post" do before do - Bookmark.create(post: post, user: user, topic: post.topic) + Bookmark.create(post: post, user: user) end it "adds an error to the manager" do @@ -228,19 +228,19 @@ RSpec.describe BookmarkManager do describe ".destroy_for_topic" do let!(:topic) { Fabricate(:topic) } - let!(:bookmark1) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) } - let!(:bookmark2) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) } + let!(:bookmark1) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) } + let!(:bookmark2) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) } it "destroys all bookmarks for the topic for the specified user" do subject.destroy_for_topic(topic) - expect(Bookmark.where(user: user, topic: topic).length).to eq(0) + expect(Bookmark.for_user_in_topic(user.id, topic.id).length).to eq(0) end it "does not destroy any other user's topic bookmarks" do user2 = Fabricate(:user) - Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user2) + Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user2) subject.destroy_for_topic(topic) - expect(Bookmark.where(user: user2, topic: topic).length).to eq(1) + expect(Bookmark.for_user_in_topic(user2.id, topic.id).length).to eq(1) end it "updates the topic user bookmarked column to false" do diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index 5511164d018..95f77d79cf0 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -45,7 +45,7 @@ RSpec.describe BookmarkQuery do before do @post = Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs")) @bookmark3 = Fabricate(:bookmark, user: user, name: "Check up later") - @bookmark4 = Fabricate(:bookmark, user: user, post: @post, topic: @post.topic) + @bookmark4 = Fabricate(:bookmark, user: user, post: @post) end it "can search by bookmark name" do @@ -90,7 +90,7 @@ RSpec.describe BookmarkQuery do context "for a private message topic bookmark" do let(:pm_topic) { Fabricate(:private_message_topic) } before do - bookmark1.update(topic: pm_topic, post: Fabricate(:post, topic: pm_topic)) + bookmark1.update(post: Fabricate(:post, topic: pm_topic)) TopicUser.change(user.id, pm_topic.id, total_msecs_viewed: 1) end @@ -160,7 +160,7 @@ RSpec.describe BookmarkQuery do Topic.expects(:preload_custom_fields) expect( bookmark_query.list_all.find do |b| - b.topic_id = bookmark1.topic_id + b.topic.id = bookmark1.topic.id end.topic.custom_fields['test_field'] ).not_to eq(nil) end diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb index c8e5ef806f7..136776805e2 100644 --- a/spec/lib/bookmark_reminder_notification_handler_spec.rb +++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb @@ -30,10 +30,33 @@ RSpec.describe BookmarkReminderNotificationHandler do it "clears the reminder" do subject.send_notification(bookmark) - bookmark.reload expect(bookmark.reload.no_reminder?).to eq(true) end + context "when the topic is deleted" do + before do + bookmark.topic.trash! + bookmark.reload + end + + it "does not send a notification and clears the reminder" do + expect { subject.send_notification(bookmark) }.not_to change { Notification.count } + expect(bookmark.reload.no_reminder?).to eq(true) + end + end + + context "when the post is deleted" do + before do + bookmark.post.trash! + bookmark.reload + end + + it "does not send a notification and clears the reminder" do + expect { subject.send_notification(bookmark) }.not_to change { Notification.count } + expect(bookmark.reload.no_reminder?).to eq(true) + end + end + context "when the auto_delete_preference is when_reminder_sent" do before do TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true) @@ -52,7 +75,7 @@ RSpec.describe BookmarkReminderNotificationHandler do 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) + Fabricate(:bookmark, post: Fabricate(:post, topic: bookmark.topic), user: user) end it "does not change the TopicUser bookmarked column to false" do diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index 401712c2ed1..d617e026f27 100644 --- a/spec/models/bookmark_spec.rb +++ b/spec/models/bookmark_spec.rb @@ -7,12 +7,11 @@ describe Bookmark do context 'validations' do it 'does not allow user to bookmark a post twice' do - bookmark = Fabricate(:bookmark, post: post, topic: post.topic) + bookmark = Fabricate(:bookmark, post: post) user = bookmark.user bookmark_2 = Fabricate.build(:bookmark, post: post, - topic: post.topic, user: user ) @@ -22,7 +21,7 @@ describe Bookmark do describe "#cleanup!" do it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do - bookmark = Fabricate(:bookmark, post: post, topic: post.topic) + bookmark = Fabricate(:bookmark, post: post) bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) post.trash! post.update(deleted_at: 4.days.ago) @@ -33,10 +32,10 @@ describe Bookmark do it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do post2 = Fabricate(:post) - bookmark = Fabricate(:bookmark, post: post, topic: post.topic) + bookmark = Fabricate(:bookmark, post: post) bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) - bookmark3 = Fabricate(:bookmark, post: post2, topic: post2.topic) - bookmark4 = Fabricate(:bookmark, post: post2, topic: post2.topic) + bookmark3 = Fabricate(:bookmark, post: post2) + bookmark4 = Fabricate(:bookmark, post: post2) post.trash! post.update(deleted_at: 4.days.ago) post2.trash! @@ -51,8 +50,8 @@ describe Bookmark do end it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do - bookmark = Fabricate(:bookmark, post: post, topic: post.topic) - bookmark2 = Fabricate(:bookmark, topic: post.topic, post: Fabricate(:post, topic: post.topic)) + bookmark = Fabricate(:bookmark, post: post) + bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) bookmark3 = Fabricate(:bookmark) post.topic.trash! post.topic.update(deleted_at: 4.days.ago) @@ -63,7 +62,7 @@ describe Bookmark do end it "does not delete bookmarks attached to posts that are not deleted or that have not met the 3 day grace period" do - bookmark = Fabricate(:bookmark, post: post, topic: post.topic) + bookmark = Fabricate(:bookmark, post: post) bookmark2 = Fabricate(:bookmark) Bookmark.cleanup! expect(Bookmark.find(bookmark.id)).to eq(bookmark) @@ -73,6 +72,57 @@ describe Bookmark do expect(Bookmark.find_by(id: bookmark2.id)).to eq(bookmark2) end + describe "#count_per_day" do + let(:category) { Fabricate(:category) } + let(:topic_in_category) { Fabricate(:topic, category: category) } + let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) } + let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) } + let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) } + let!(:bookmark4) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) } + let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) } + + it "gets the count of bookmarks grouped by date within the last 30 days by default" do + expect(Bookmark.count_per_day).to eq({ + 1.day.ago.to_date => 1, + 2.days.ago.to_date => 1, + 3.days.ago.to_date => 2 + }) + end + + it "respects the start_date option" do + expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({ + 1.day.ago.to_date => 1, + }) + end + + it "respects the since_days_ago option" do + expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({ + 1.day.ago.to_date => 1, + }) + end + + it "respects the end_date option" do + expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({ + 2.days.ago.to_date => 1, + 3.days.ago.to_date => 2, + }) + end + + it "respects the category_id option" do + expect(Bookmark.count_per_day(category_id: category.id)).to eq({ + 3.days.ago.to_date => 1, + }) + end + + it "does not include deleted posts or topics" do + bookmark4.post.trash! + expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) + bookmark4.post.recover! + bookmark4.topic.trash! + expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) + end + end + describe "bookmark limits" do fab!(:user) { Fabricate(:user) } diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 3df580a71fd..a689c35ab6f 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -396,10 +396,10 @@ describe PostMover do it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do some_user = Fabricate(:user) new_post = Fabricate(:post, topic: p1.topic) - bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: some_user) - bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4) - bookmark3 = Fabricate(:bookmark, topic: p4.topic, post: p4) - bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post) + bookmark1 = Fabricate(:bookmark, post: p1, user: some_user) + bookmark2 = Fabricate(:bookmark, post: p4) + bookmark3 = Fabricate(:bookmark, post: p4) + bookmark4 = Fabricate(:bookmark, post: new_post) new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name") expect(bookmark1.reload.topic_id).to eq(new_topic.id) expect(bookmark2.reload.topic_id).to eq(new_topic.id) @@ -412,11 +412,11 @@ describe PostMover do user1 = Fabricate(:user) user2 = Fabricate(:user) - bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: user1) - bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user1) + bookmark1 = Fabricate(:bookmark, post: p1, user: user1) + bookmark2 = Fabricate(:bookmark, post: p4, user: user1) - bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: user2) - bookmark4 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user2) + bookmark3 = Fabricate(:bookmark, post: p3, user: user2) + bookmark4 = Fabricate(:bookmark, post: p4, user: user2) tu1 = Fabricate( :topic_user, @@ -441,6 +441,10 @@ describe PostMover do new_topic_user1 = TopicUser.find_by(topic: new_topic, user: user1) new_topic_user2 = TopicUser.find_by(topic: new_topic, user: user2) + original_topic_id = p1.topic_id + expect(p1.reload.topic_id).to eq(original_topic_id) + expect(p4.reload.topic_id).to eq(new_topic.id) + expect(tu1.reload.bookmarked).to eq(false) expect(tu2.reload.bookmarked).to eq(true) expect(new_topic_user1.bookmarked).to eq(true) @@ -724,10 +728,10 @@ describe PostMover do it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do some_user = Fabricate(:user) new_post = Fabricate(:post, topic: p3.topic) - bookmark1 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: some_user) - bookmark2 = Fabricate(:bookmark, topic: p3.topic, post: p3) - bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3) - bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post) + bookmark1 = Fabricate(:bookmark, post: p3, user: some_user) + bookmark2 = Fabricate(:bookmark, post: p3) + bookmark3 = Fabricate(:bookmark, post: p3) + bookmark4 = Fabricate(:bookmark, post: new_post) topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id) expect(bookmark1.reload.topic_id).to eq(destination_topic.id) expect(bookmark2.reload.topic_id).to eq(destination_topic.id) diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index ec5bf414631..59f76e51413 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -564,7 +564,7 @@ describe PostsController do describe "#destroy_bookmark" do fab!(:post) { Fabricate(:post) } - fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post, topic: post.topic) } + fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post) } before do sign_in(user) @@ -578,7 +578,7 @@ describe PostsController do context "when the user still has bookmarks in the topic" do before do - Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic), topic: post.topic) + Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic)) end it "marks topic_bookmarked as true" do delete "/posts/#{post.id}/bookmark.json" diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index f904694a6bd..c6becd3ed38 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -3108,9 +3108,9 @@ RSpec.describe TopicsController do it "deletes all the bookmarks for the user in the topic" do sign_in(user) post = create_post - Fabricate(:bookmark, post: post, topic: post.topic, user: user) + Fabricate(:bookmark, post: post, user: user) put "/t/#{post.topic_id}/remove_bookmarks.json" - expect(Bookmark.where(user: user, topic: topic).count).to eq(0) + expect(Bookmark.for_user_in_topic(user.id, post.topic_id).count).to eq(0) end end end @@ -3130,7 +3130,7 @@ RSpec.describe TopicsController do it "errors if the topic is already bookmarked for the user" do post = create_post - Bookmark.create(post: post, user: user, topic: post.topic) + Bookmark.create(post: post, user: user) put "/t/#{post.topic_id}/bookmark.json" expect(response.status).to eq(400) @@ -3143,14 +3143,14 @@ RSpec.describe TopicsController do put "/t/#{post.topic_id}/bookmark.json" expect(response.status).to eq(200) - bookmarks_for_topic = Bookmark.where(topic: post.topic, user: user) + bookmarks_for_topic = Bookmark.for_user_in_topic(user.id, post.topic_id) expect(bookmarks_for_topic.count).to eq(1) expect(bookmarks_for_topic.first.post_id).to eq(post.id) end it "errors if the topic is already bookmarked for the user" do post = create_post - Bookmark.create(post: post, topic: post.topic, user: user) + Bookmark.create(post: post, user: user) put "/t/#{post.topic_id}/bookmark.json" expect(response.status).to eq(400) diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index b9aa3a23d62..dcd52eeeacb 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5061,6 +5061,39 @@ describe UsersController do expect(response.parsed_body['user_bookmark_list']['bookmarks'].map { |b| b['id'] }).to match_array([bookmark1.id, bookmark2.id]) end + it "returns an .ics file of bookmark reminders for the user in date order" do + bookmark1.update!(name: nil, reminder_at: 1.day.from_now) + bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now) + + sign_in(user) + get "/u/#{user.username}/bookmarks.ics" + expect(response.status).to eq(200) + expect(response.body).to eq(<<~ICS) + BEGIN:VCALENDAR + VERSION:2.0 + PRODID:-//Discourse//#{Discourse.current_hostname}//#{Discourse.full_version}//EN + BEGIN:VEVENT + UID:bookmark_reminder_##{bookmark1.id}@#{Discourse.current_hostname} + DTSTAMP:#{bookmark1.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics"))} + DTSTART:#{bookmark1.reminder_at_ics} + DTEND:#{bookmark1.reminder_at_ics(offset: 1.hour)} + SUMMARY:#{bookmark1.topic.title} + DESCRIPTION:#{Discourse.base_url}/t/-/#{bookmark1.topic_id} + URL:#{Discourse.base_url}/t/-/#{bookmark1.topic_id} + END:VEVENT + BEGIN:VEVENT + UID:bookmark_reminder_##{bookmark2.id}@#{Discourse.current_hostname} + DTSTAMP:#{bookmark2.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics"))} + DTSTART:#{bookmark2.reminder_at_ics} + DTEND:#{bookmark2.reminder_at_ics(offset: 1.hour)} + SUMMARY:Some bookmark note + DESCRIPTION:#{Discourse.base_url}/t/-/#{bookmark2.topic_id} + URL:#{Discourse.base_url}/t/-/#{bookmark2.topic_id} + END:VEVENT + END:VCALENDAR + ICS + end + it "does not show another user's bookmarks" do sign_in(user) get "/u/#{bookmark3.user.username}/bookmarks.json" diff --git a/spec/serializers/user_bookmark_serializer_spec.rb b/spec/serializers/user_bookmark_serializer_spec.rb index a192cae9124..4225a2ed12a 100644 --- a/spec/serializers/user_bookmark_serializer_spec.rb +++ b/spec/serializers/user_bookmark_serializer_spec.rb @@ -5,7 +5,7 @@ require 'rails_helper' RSpec.describe UserBookmarkSerializer do let(:user) { Fabricate(:user) } let(:post) { Fabricate(:post, user: user) } - let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post, topic: post.topic) } + let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post) } let(:bookmark_list) { BookmarkQuery.new(user: bookmark.user).list_all.to_ary } it "serializes all properties correctly" do