From 49a70a37f10edb05fd98ac9935f72aeb00a842d2 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 19 Aug 2022 09:35:25 +1000 Subject: [PATCH] FIX: Remove last_unread_post excerpt logic for bookmarks (#17979) The logic to determine what post excerpt to show for a topic-level bookmark based on the last unread post was complex and slow, so we decided to remove it and always just use the first post excerpt. This commit also fixes an issue where a couple of instances of for_topic were missed when doing the Bookmarkable refactors, so: 1. Clicking the topic bookmark link was not taking the user to the last unread post 2. When replying to a topic where there was a topic level bookmark with the auto delete preference of "on owner reply", we were not removing the bookmark from the UI correctly. A test has been added for the former, the latter would be quite time-consuming to test and not really worth it considering it's quite an edge case UI bug. --- .../discourse/app/controllers/topic.js | 5 ++- .../discourse/app/models/bookmark.js | 3 +- .../tests/unit/models/bookmark-test.js | 42 +++++++++++++++++++ .../user_topic_bookmark_serializer.rb | 30 ++----------- app/services/topic_bookmarkable.rb | 2 +- spec/requests/users_controller_spec.rb | 39 +---------------- 6 files changed, 55 insertions(+), 66 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/unit/models/bookmark-test.js diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index 30becd174d3..3eee01298fe 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -240,7 +240,10 @@ export default Controller.extend(bufferedProperty("model"), { this.model.removeBookmark(post.bookmark_id); }); } - const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true); + const forTopicBookmark = this.model.bookmarks.findBy( + "bookmarkable_type", + "Topic" + ); if ( forTopicBookmark?.auto_delete_preference === AUTO_DELETE_PREFERENCES.ON_OWNER_REPLY diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index cd8c81dfc9a..ff8dda8a4e7 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -143,7 +143,8 @@ const Bookmark = RestModel.extend({ // for topic level bookmarks we want to jump to the last unread post URL, // which the topic-link helper does by default if no linked post number is // provided - const linkedPostNumber = this.for_topic ? null : this.linked_post_number; + const linkedPostNumber = + this.bookmarkable_type === "Topic" ? null : this.linked_post_number; return Topic.create({ id: this.topic_id, diff --git a/app/assets/javascripts/discourse/tests/unit/models/bookmark-test.js b/app/assets/javascripts/discourse/tests/unit/models/bookmark-test.js new file mode 100644 index 00000000000..01b453eb145 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/unit/models/bookmark-test.js @@ -0,0 +1,42 @@ +import { module, test } from "qunit"; +import Bookmark from "discourse/models/bookmark"; + +module("Unit | Model | bookmark", function () { + test("topicForList - Topic bookmarkable", function (assert) { + let bookmark = Bookmark.create({ + id: 1, + bookmarkable_type: "Topic", + bookmarkable_id: 999, + linked_post_number: null, + topic_id: 999, + fancy_title: "Some test topic", + last_read_post_number: 23, + highest_post_number: 30, + }); + + assert.strictEqual( + bookmark.topicForList.linked_post_number, + null, + "linked_post_number is null" + ); + }); + + test("topicForList - Post bookmarkable", function (assert) { + let bookmark = Bookmark.create({ + id: 1, + bookmarkable_type: "Post", + bookmarkable_id: 999, + linked_post_number: 787, + topic_id: 999, + fancy_title: "Some test topic", + last_read_post_number: 23, + highest_post_number: 30, + }); + + assert.strictEqual( + bookmark.topicForList.linked_post_number, + 787, + "linked_post_number is correct" + ); + }); +}); diff --git a/app/serializers/user_topic_bookmark_serializer.rb b/app/serializers/user_topic_bookmark_serializer.rb index 8aa78bc88b3..84d14d1620a 100644 --- a/app/serializers/user_topic_bookmark_serializer.rb +++ b/app/serializers/user_topic_bookmark_serializer.rb @@ -1,12 +1,11 @@ # frozen_string_literal: true class UserTopicBookmarkSerializer < UserPostTopicBookmarkBaseSerializer - # it does not matter what the linked post number is for topic bookmarks, - # on the client we always take the user to the last unread post in the - # topic when the bookmark URL is clicked - attributes :last_read_post_number + # NOTE: It does not matter what the linked post number is for topic bookmarks, + # on the client we always take the user to the last unread post in the + # topic when the bookmark URL is clicked def linked_post_number 1 end @@ -28,28 +27,7 @@ class UserTopicBookmarkSerializer < UserPostTopicBookmarkBaseSerializer end def cooked - @cooked ||= \ - if last_read_post_number.present? - for_topic_cooked_post - else - first_post.cooked - end - end - - def for_topic_cooked_post - post_number = [last_read_post_number + 1, highest_post_number].min - sorted_regular_posts = topic.posts.sort_by(&:post_number).select do |post| - post.post_type == Post.types[:regular] - end - first_unread_post = sorted_regular_posts.find do |post| - post.post_number >= post_number - end - - # if first_unread_cooked is blank this likely means that the last - # read post was either deleted or is a small action post. - # in this case we should just get the last regular post and - # use that for the cooked value so we have something to show - (first_unread_post || sorted_regular_posts.last).cooked + first_post.cooked end def bookmarkable_user diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index 67c474664c1..5a5276f64a7 100644 --- a/app/services/topic_bookmarkable.rb +++ b/app/services/topic_bookmarkable.rb @@ -12,7 +12,7 @@ class TopicBookmarkable < BaseBookmarkable end def self.preload_associations - [:tags, { posts: :user }] + [:tags, { first_post: :user }] end def self.perform_custom_preload!(topic_bookmarks, guardian) diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 466273e16a5..88ff88ce945 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5485,10 +5485,7 @@ RSpec.describe UsersController do let!(:post) { Fabricate(:post, topic: topic) } let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, bookmarkable: topic) } - it "uses the last_read_post_number + 1 for the bookmarks excerpt" do - next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable) - Fabricate(:post_with_external_links, topic: bookmark.bookmarkable) - bookmark.reload + it "uses the first post of the topic for the bookmarks excerpt" do TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number }) sign_in(user) @@ -5496,42 +5493,10 @@ RSpec.describe UsersController do get "/u/#{user.username}/bookmarks.json" expect(response.status).to eq(200) bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"] - expected_excerpt = PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true) + expected_excerpt = PrettyText.excerpt(topic.first_post.cooked, 300, keep_emoji_images: true) expect(bookmark_list.first["excerpt"]).to eq(expected_excerpt) end - it "does not use a small post for the last unread cooked post" do - small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable) - next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable) - Fabricate(:post_with_external_links, topic: bookmark.bookmarkable) - bookmark.reload - TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number }) - - sign_in(user) - - get "/u/#{user.username}/bookmarks.json" - expect(response.status).to eq(200) - bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"] - - expect(bookmark_list.first["excerpt"]).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true)) - end - - it "handles the last read post in the topic being a small post by getting the last read regular post" do - last_regular_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable) - small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable) - bookmark.reload - topic.reload - TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: small_action_post.post_number }) - - sign_in(user) - - get "/u/#{user.username}/bookmarks.json" - expect(response.status).to eq(200) - bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"] - - expect(bookmark_list.first["excerpt"]).to eq(PrettyText.excerpt(last_regular_post.cooked, 300, keep_emoji_images: true)) - end - describe "bookmarkable_url" do context "with the link_to_first_unread_post option" do it "is a full topic URL to the first unread post in the topic when the option is set" do