diff --git a/app/assets/javascripts/discourse/app/components/bookmark-list.js b/app/assets/javascripts/discourse/app/components/bookmark-list.js
index f96c4aa94a5..03f2b863629 100644
--- a/app/assets/javascripts/discourse/app/components/bookmark-list.js
+++ b/app/assets/javascripts/discourse/app/components/bookmark-list.js
@@ -86,23 +86,19 @@ export default Component.extend(Scrolling, {
@action
editBookmark(bookmark) {
- openBookmarkModal(
- bookmark,
- {
- onAfterSave: (savedData) => {
- this.appEvents.trigger(
- "bookmarks:changed",
- savedData,
- bookmark.attachedTo()
- );
- this.reload();
- },
- onAfterDelete: () => {
- this.reload();
- },
+ openBookmarkModal(bookmark, {
+ onAfterSave: (savedData) => {
+ this.appEvents.trigger(
+ "bookmarks:changed",
+ savedData,
+ bookmark.attachedTo()
+ );
+ this.reload();
},
- { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks }
- );
+ onAfterDelete: () => {
+ this.reload();
+ },
+ });
},
@action
diff --git a/app/assets/javascripts/discourse/app/components/bookmark.js b/app/assets/javascripts/discourse/app/components/bookmark.js
index 9d7a8eb01c6..5a7b20b8845 100644
--- a/app/assets/javascripts/discourse/app/components/bookmark.js
+++ b/app/assets/javascripts/discourse/app/components/bookmark.js
@@ -113,8 +113,12 @@ export default Component.extend({
},
_loadPostLocalDates() {
+ if (this.model.bookmarkableType !== "Post") {
+ return;
+ }
+
let postEl = document.querySelector(
- `[data-post-id="${this.model.postId}"]`
+ `[data-post-id="${this.model.bookmarkableId}"]`
);
let localDateEl;
if (postEl) {
@@ -156,14 +160,8 @@ export default Component.extend({
auto_delete_preference: this.autoDeletePreference,
};
- if (this.siteSettings.use_polymorphic_bookmarks) {
- data.bookmarkable_id = this.model.bookmarkableId;
- data.bookmarkable_type = this.model.bookmarkableType;
- } else {
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- data.post_id = this.model.postId;
- data.for_topic = this.model.forTopic;
- }
+ data.bookmarkable_id = this.model.bookmarkableId;
+ data.bookmarkable_type = this.model.bookmarkableType;
if (this.editingExistingBookmark) {
return ajax(`/bookmarks/${this.model.id}`, {
@@ -191,15 +189,8 @@ export default Component.extend({
name: this.model.name,
};
- if (this.siteSettings.use_polymorphic_bookmarks) {
- data.bookmarkable_id = this.model.bookmarkableId;
- data.bookmarkable_type = this.model.bookmarkableType;
- } else {
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- data.post_id = this.model.postId;
- data.for_topic = this.model.forTopic;
- data.topic_id = this.model.topicId;
- }
+ data.bookmarkable_id = this.model.bookmarkableId;
+ data.bookmarkable_type = this.model.bookmarkableType;
this.afterSave(data);
},
diff --git a/app/assets/javascripts/discourse/app/controllers/bookmark.js b/app/assets/javascripts/discourse/app/controllers/bookmark.js
index fc1737a8df8..c05fcda47ad 100644
--- a/app/assets/javascripts/discourse/app/controllers/bookmark.js
+++ b/app/assets/javascripts/discourse/app/controllers/bookmark.js
@@ -11,28 +11,9 @@ export function openBookmarkModal(
onCloseWithoutSaving: null,
onAfterSave: null,
onAfterDelete: null,
- },
- options = {
- use_polymorphic_bookmarks: false,
}
) {
return new Promise((resolve) => {
- const modalTitle = () => {
- if (options.use_polymorphic_bookmarks) {
- return I18n.t(bookmark.id ? "bookmarks.edit" : "bookmarks.create");
- } else if (bookmark.for_topic) {
- return I18n.t(
- bookmark.id
- ? "post.bookmarks.edit_for_topic"
- : "post.bookmarks.create_for_topic"
- );
- } else {
- return I18n.t(
- bookmark.id ? "post.bookmarks.edit" : "post.bookmarks.create"
- );
- }
- };
-
const model = {
id: bookmark.id,
reminderAt: bookmark.reminder_at,
@@ -40,19 +21,14 @@ export function openBookmarkModal(
name: bookmark.name,
};
- if (options.use_polymorphic_bookmarks) {
- model.bookmarkableId = bookmark.bookmarkable_id;
- model.bookmarkableType = bookmark.bookmarkable_type;
- } else {
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- model.postId = bookmark.post_id;
- model.topicId = bookmark.topic_id;
- model.forTopic = bookmark.for_topic;
- }
+ model.bookmarkableId = bookmark.bookmarkable_id;
+ model.bookmarkableType = bookmark.bookmarkable_type;
let modalController = showModal("bookmark", {
model,
- titleTranslated: modalTitle(),
+ titleTranslated: I18n.t(
+ bookmark.id ? "bookmarks.edit" : "bookmarks.create"
+ ),
modalClass: "bookmark-with-reminder",
});
modalController.setProperties({
diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js
index ab42d954fa3..f50bc723c02 100644
--- a/app/assets/javascripts/discourse/app/controllers/topic.js
+++ b/app/assets/javascripts/discourse/app/controllers/topic.js
@@ -767,38 +767,7 @@ export default Controller.extend(bufferedProperty("model"), {
}
},
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
toggleBookmark(post) {
- if (!this.currentUser) {
- return bootbox.alert(I18n.t("bookmarks.not_bookmarked"));
- } else if (post) {
- const bookmarkForPost = this.model.bookmarks.find(
- (bookmark) => bookmark.post_id === post.id && !bookmark.for_topic
- );
- return this._modifyPostBookmark(
- bookmarkForPost ||
- Bookmark.create({
- post_id: post.id,
- topic_id: post.topic_id,
- for_topic: false,
- auto_delete_preference: this.currentUser
- .bookmark_auto_delete_preference,
- }),
- post
- );
- } else {
- return this._toggleTopicLevelBookmark().then((changedIds) => {
- if (!changedIds) {
- return;
- }
- changedIds.forEach((id) =>
- this.appEvents.trigger("post-stream:refresh", { id })
- );
- });
- }
- },
-
- toggleBookmarkPolymorphic(post) {
if (!this.currentUser) {
return bootbox.alert(I18n.t("bookmarks.not_bookmarked"));
} else if (post) {
@@ -818,16 +787,14 @@ export default Controller.extend(bufferedProperty("model"), {
post
);
} else {
- return this._toggleTopicLevelBookmarkPolymorphic().then(
- (changedIds) => {
- if (!changedIds) {
- return;
- }
- changedIds.forEach((id) =>
- this.appEvents.trigger("post-stream:refresh", { id })
- );
+ return this._toggleTopicLevelBookmark().then((changedIds) => {
+ if (!changedIds) {
+ return;
}
- );
+ changedIds.forEach((id) =>
+ this.appEvents.trigger("post-stream:refresh", { id })
+ );
+ });
}
},
@@ -1285,56 +1252,46 @@ export default Controller.extend(bufferedProperty("model"), {
},
_modifyTopicBookmark(bookmark) {
- return openBookmarkModal(
- bookmark,
- {
- onAfterSave: (savedData) => {
- this._syncBookmarks(savedData);
- this.model.set("bookmarking", false);
- this.model.set("bookmarked", true);
- this.model.incrementProperty("bookmarksWereChanged");
- this.appEvents.trigger(
- "bookmarks:changed",
- savedData,
- bookmark.attachedTo()
- );
+ return openBookmarkModal(bookmark, {
+ onAfterSave: (savedData) => {
+ this._syncBookmarks(savedData);
+ this.model.set("bookmarking", false);
+ this.model.set("bookmarked", true);
+ this.model.incrementProperty("bookmarksWereChanged");
+ this.appEvents.trigger(
+ "bookmarks:changed",
+ savedData,
+ bookmark.attachedTo()
+ );
- // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed.
- this.appEvents.trigger("topic:bookmark-toggled");
- },
- onAfterDelete: (topicBookmarked, bookmarkId) => {
- this.model.removeBookmark(bookmarkId);
- },
+ // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed.
+ this.appEvents.trigger("topic:bookmark-toggled");
},
- { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks }
- );
+ onAfterDelete: (topicBookmarked, bookmarkId) => {
+ this.model.removeBookmark(bookmarkId);
+ },
+ });
},
_modifyPostBookmark(bookmark, post) {
- return openBookmarkModal(
- bookmark,
- {
- onCloseWithoutSaving: () => {
- post.appEvents.trigger("post-stream:refresh", {
- id: this.siteSettings.use_polymorphic_bookmarks
- ? bookmark.bookmarkable_id
- : bookmark.post_id,
- });
- },
- onAfterSave: (savedData) => {
- this._syncBookmarks(savedData);
- this.model.set("bookmarking", false);
- post.createBookmark(savedData);
- this.model.afterPostBookmarked(post, savedData);
- return [post.id];
- },
- onAfterDelete: (topicBookmarked, bookmarkId) => {
- this.model.removeBookmark(bookmarkId);
- post.deleteBookmark(topicBookmarked);
- },
+ return openBookmarkModal(bookmark, {
+ onCloseWithoutSaving: () => {
+ post.appEvents.trigger("post-stream:refresh", {
+ id: bookmark.bookmarkable_id,
+ });
},
- { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks }
- );
+ onAfterSave: (savedData) => {
+ this._syncBookmarks(savedData);
+ this.model.set("bookmarking", false);
+ post.createBookmark(savedData);
+ this.model.afterPostBookmarked(post, savedData);
+ return [post.id];
+ },
+ onAfterDelete: (topicBookmarked, bookmarkId) => {
+ this.model.removeBookmark(bookmarkId);
+ post.deleteBookmark(topicBookmarked);
+ },
+ });
},
_syncBookmarks(data) {
@@ -1352,7 +1309,6 @@ export default Controller.extend(bufferedProperty("model"), {
}
},
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
async _toggleTopicLevelBookmark() {
if (this.model.bookmarking) {
return Promise.resolve();
@@ -1362,40 +1318,6 @@ export default Controller.extend(bufferedProperty("model"), {
return this._maybeClearAllBookmarks();
}
- if (this.model.bookmarkCount === 1) {
- const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true);
- if (forTopicBookmark) {
- return this._modifyTopicBookmark(forTopicBookmark);
- } else {
- const bookmark = this.model.bookmarks[0];
- const post = await this.model.postById(bookmark.post_id);
- return this._modifyPostBookmark(bookmark, post);
- }
- }
-
- if (this.model.bookmarkCount === 0) {
- const firstPost = await this.model.firstPost();
- return this._modifyTopicBookmark(
- Bookmark.create({
- post_id: firstPost.id,
- topic_id: this.model.id,
- for_topic: true,
- auto_delete_preference: this.currentUser
- .bookmark_auto_delete_preference,
- })
- );
- }
- },
-
- async _toggleTopicLevelBookmarkPolymorphic() {
- if (this.model.bookmarking) {
- return Promise.resolve();
- }
-
- if (this.model.bookmarkCount > 1) {
- return this._maybeClearAllBookmarks();
- }
-
if (this.model.bookmarkCount === 1) {
const topicBookmark = this.model.bookmarks.findBy(
"bookmarkable_type",
diff --git a/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js b/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js
index b1f330c5c73..83e5a63b40a 100644
--- a/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js
+++ b/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js
@@ -11,8 +11,7 @@ const DEFER_PRIORITY = 500;
export default {
name: "topic-footer-buttons",
- initialize(container) {
- const siteSettings = container.lookup("site-settings:main");
+ initialize() {
registerTopicFooterButton({
id: "share-and-invite",
icon: "d-topic-share",
@@ -99,12 +98,9 @@ export default {
if (this.topic.bookmarkCount === 0) {
return I18n.t("bookmarked.help.bookmark");
} else if (this.topic.bookmarkCount === 1) {
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- const anyTopicBookmarks = this.topic.bookmarks.some((bookmark) => {
- return siteSettings.use_polymorphic_bookmarks
- ? bookmark.for_topic
- : bookmark.bookmarkable_type === "Topic";
- });
+ const anyTopicBookmarks = this.topic.bookmarks.some(
+ (bookmark) => bookmark.bookmarkable_type === "Topic"
+ );
if (anyTopicBookmarks) {
return I18n.t("bookmarked.help.edit_bookmark_for_topic");
@@ -119,10 +115,7 @@ export default {
return I18n.t("bookmarked.help.unbookmark");
}
},
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- action: siteSettings.use_polymorphic_bookmarks
- ? "toggleBookmarkPolymorphic"
- : "toggleBookmark",
+ action: "toggleBookmark",
dropdown() {
return this.site.mobileView;
},
diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js
index c38a5b69cc6..f2f1b2f6d6e 100644
--- a/app/assets/javascripts/discourse/app/models/bookmark.js
+++ b/app/assets/javascripts/discourse/app/models/bookmark.js
@@ -39,18 +39,10 @@ const Bookmark = RestModel.extend({
},
attachedTo() {
- if (this.siteSettings.use_polymorphic_bookmarks) {
- return {
- target: this.bookmarkable_type.toLowerCase(),
- targetId: this.bookmarkable_id,
- };
- }
-
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- if (this.for_topic) {
- return { target: "topic", targetId: this.topic_id };
- }
- return { target: "post", targetId: this.post_id };
+ return {
+ target: this.bookmarkable_type.toLowerCase(),
+ targetId: this.bookmarkable_id,
+ };
},
togglePin() {
@@ -161,9 +153,6 @@ const Bookmark = RestModel.extend({
@discourseComputed("bookmarkable_type")
bookmarkableTopicAlike(bookmarkable_type) {
- if (!this.siteSettings.use_polymorphic_bookmarks) {
- return true;
- }
return ["Topic", "Post"].includes(bookmarkable_type);
},
});
diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js
index e6b960f4eac..9b4e5c8efcc 100644
--- a/app/assets/javascripts/discourse/app/models/topic.js
+++ b/app/assets/javascripts/discourse/app/models/topic.js
@@ -397,15 +397,7 @@ const Topic = RestModel.extend({
this.set(
"bookmarks",
this.bookmarks.filter((bookmark) => {
- // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- if (
- (!this.siteSettings.use_polymorphic_bookmarks &&
- bookmark.id === id &&
- bookmark.for_topic) ||
- (this.siteSettings.use_polymorphic_bookmarks &&
- bookmark.id === id &&
- bookmark.bookmarkable_type === "Topic")
- ) {
+ if (bookmark.id === id && bookmark.bookmarkable_type === "Topic") {
// TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed.
this.appEvents.trigger("topic:bookmark-toggled");
this.appEvents.trigger(
@@ -425,11 +417,9 @@ const Topic = RestModel.extend({
clearBookmarks() {
this.toggleProperty("bookmarked");
- const postIds = this.siteSettings.use_polymorphic_bookmarks
- ? this.bookmarks
- .filterBy("bookmarkable_type", "Post")
- .mapBy("bookmarkable_id")
- : this.bookmarks.mapBy("post_id");
+ const postIds = this.bookmarks
+ .filterBy("bookmarkable_type", "Post")
+ .mapBy("bookmarkable_id");
postIds.forEach((postId) => {
const loadedPost = this.postStream.findLoadedPost(postId);
if (loadedPost) {
diff --git a/app/assets/javascripts/discourse/app/templates/topic.hbs b/app/assets/javascripts/discourse/app/templates/topic.hbs
index 3b74568e5b8..dd268baff07 100644
--- a/app/assets/javascripts/discourse/app/templates/topic.hbs
+++ b/app/assets/javascripts/discourse/app/templates/topic.hbs
@@ -223,7 +223,6 @@
recoverPost=(action "recoverPost")
expandHidden=(action "expandHidden")
toggleBookmark=(action "toggleBookmark")
- toggleBookmarkPolymorphic=(action "toggleBookmarkPolymorphic")
togglePostType=(action "togglePostType")
rebakePost=(action "rebakePost")
changePostOwner=(action "changePostOwner")
@@ -363,7 +362,6 @@
convertToPublicTopic=(action "convertToPublicTopic")
convertToPrivateMessage=(action "convertToPrivateMessage")
toggleBookmark=(action "toggleBookmark")
- toggleBookmarkPolymorphic=(action "toggleBookmarkPolymorphic")
showFlagTopic=(route-action "showFlagTopic")
toggleArchiveMessage=(action "toggleArchiveMessage")
editFirstPost=(action "editFirstPost")
diff --git a/app/assets/javascripts/discourse/app/widgets/post-menu.js b/app/assets/javascripts/discourse/app/widgets/post-menu.js
index 1f542697ecb..9023b0ed767 100644
--- a/app/assets/javascripts/discourse/app/widgets/post-menu.js
+++ b/app/assets/javascripts/discourse/app/widgets/post-menu.js
@@ -355,9 +355,7 @@ registerButton(
return {
id: attrs.bookmarked ? "unbookmark" : "bookmark",
- action: siteSettings.use_polymorphic_bookmarks
- ? "toggleBookmarkPolymorphic"
- : "toggleBookmark",
+ action: "toggleBookmark",
title,
titleOptions,
className: classNames.join(" "),
diff --git a/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js b/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js
index 16888bee7e7..4be4dfbfca6 100644
--- a/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js
+++ b/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js
@@ -47,15 +47,25 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", {
// for topic level bookmarks we want to jump to the last unread post
// instead of the OP
let postNumber;
- if (bookmark.for_topic) {
+ if (bookmark.bookmarkable_type === "Topic") {
postNumber = bookmark.last_read_post_number + 1;
} else {
postNumber = bookmark.linked_post_number;
}
+ let href;
+ if (
+ bookmark.bookmarkable_type === "Topic" ||
+ bookmark.bookmarkable_type === "Post"
+ ) {
+ href = postUrl(bookmark.slug, bookmark.topic_id, postNumber);
+ } else {
+ href = bookmark.bookmarkable_type;
+ }
+
return this.attach("quick-access-item", {
icon: this.icon(bookmark),
- href: postUrl(bookmark.slug, bookmark.topic_id, postNumber),
+ href,
title: bookmark.name,
content: bookmark.title,
username: bookmark.post_user_username,
diff --git a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js
index 0b09dc508cb..0ee4fda4dfc 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js
@@ -83,16 +83,15 @@ acceptance("Bookmarking", function (needs) {
function handleRequest(request) {
const data = helper.parsePostData(request.requestBody);
- if (data.post_id === "398") {
- if (data.for_topic === "true") {
- return helper.response({ id: 3, success: "OK" });
- } else {
- return helper.response({ id: 1, success: "OK" });
- }
- } else if (data.post_id === "419") {
+ if (data.bookmarkable_id === "398" && data.bookmarkable_type === "Post") {
+ return helper.response({ id: 1, success: "OK" });
+ } else if (data.bookmarkable_type === "Topic") {
+ return helper.response({ id: 3, success: "OK" });
+ } else if (
+ data.bookmarkable_id === "419" &&
+ data.bookmarkable_type === "Post"
+ ) {
return helper.response({ id: 2, success: "OK" });
- } else {
- throw new Error("Pretender: unknown post_id");
}
}
server.post("/bookmarks", handleRequest);
@@ -368,13 +367,6 @@ acceptance("Bookmarking", function (needs) {
test("Creating and editing a topic level bookmark", async function (assert) {
await visit("/t/internationalization-localization/280");
await click("#topic-footer-button-bookmark");
-
- assert.strictEqual(
- query("#discourse-modal-title").innerText,
- I18n.t("post.bookmarks.create_for_topic"),
- "The create modal says creating a topic bookmark"
- );
-
await click("#save-bookmark");
assert.notOk(
@@ -389,13 +381,6 @@ acceptance("Bookmarking", function (needs) {
);
await click("#topic-footer-button-bookmark");
-
- assert.strictEqual(
- query("#discourse-modal-title").innerText,
- I18n.t("post.bookmarks.edit_for_topic"),
- "The edit modal says editing a topic bookmark"
- );
-
await fillIn("input#bookmark-name", "Test name");
await click("#tap_tile_tomorrow");
diff --git a/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js
index 749f16fa0ab..cafbe1a5672 100644
--- a/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js
+++ b/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js
@@ -427,7 +427,9 @@ export default {
created_at: "2020-04-07T05:30:40.446Z",
topic_id: 119,
linked_post_number: 1,
- post_id: 281,
+ bookmarkable_id: 281,
+ bookmarkable_type: "Post",
+ bookmarkable_url: "http://localhost:4200/t/yelling-topic-title/119",
name: "test",
reminder_at: null,
title: "Yelling topic title :/",
diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index 9aabad449b0..09810cae00c 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -4,12 +4,8 @@ class BookmarksController < ApplicationController
requires_login
def create
- if SiteSetting.use_polymorphic_bookmarks
- params.require(:bookmarkable_id)
- params.require(:bookmarkable_type)
- else
- params.require(:post_id)
- end
+ params.require(:bookmarkable_id)
+ params.require(:bookmarkable_type)
RateLimiter.new(
current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i
@@ -25,21 +21,12 @@ class BookmarksController < ApplicationController
}
}
- if SiteSetting.use_polymorphic_bookmarks
- bookmark = bookmark_manager.create_for(
- **create_params.merge(
- bookmarkable_id: params[:bookmarkable_id],
- bookmarkable_type: params[:bookmarkable_type]
- )
+ bookmark = bookmark_manager.create_for(
+ **create_params.merge(
+ bookmarkable_id: params[:bookmarkable_id],
+ bookmarkable_type: params[:bookmarkable_type]
)
- else
- bookmark = bookmark_manager.create(
- **create_params.merge(
- post_id: params[:post_id],
- for_topic: params[:for_topic] == "true",
- )
- )
- end
+ )
if bookmark_manager.errors.empty?
return render json: success_json.merge(id: bookmark.id)
diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
index f1292d4fe08..b75866ba71e 100644
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@@ -539,7 +539,11 @@ class PostsController < ApplicationController
def destroy_bookmark
params.require(:post_id)
- bookmark_id = Bookmark.where(post_id: params[:post_id], user_id: current_user.id).pluck_first(:id)
+ bookmark_id = Bookmark.where(
+ bookmarkable_id: params[:post_id],
+ bookmarkable_type: "Post",
+ user_id: current_user.id
+ ).pluck_first(:id)
destroyed_bookmark = BookmarkManager.new(current_user).destroy(bookmark_id)
render json: success_json.merge(BookmarkManager.bookmark_metadata(destroyed_bookmark, current_user))
diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 5e4c9639560..d30a1981e02 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -608,10 +608,9 @@ class TopicsController < ApplicationController
def bookmark
topic = Topic.find(params[:topic_id].to_i)
- first_post = topic.ordered_posts.first
bookmark_manager = BookmarkManager.new(current_user)
- bookmark_manager.create(post_id: first_post.id)
+ bookmark_manager.create_for(bookmarkable_id: topic.id, bookmarkable_type: "Topic")
if bookmark_manager.errors.any?
return render_json_error(bookmark_manager, status: 400)
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 67f78b1bbba..6976cdb2d12 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1725,20 +1725,13 @@ class UsersController < ApplicationController
end
end
format.ics do
- if SiteSetting.use_polymorphic_bookmarks
- @bookmark_reminders = Bookmark.with_reminders
- .where(user_id: user.id)
- .order(:reminder_at)
- .map do |bookmark|
- bookmark.registered_bookmarkable.serializer.new(
- bookmark, scope: user_guardian, root: false
- )
- end
- else
- @bookmark_reminders = Bookmark.with_reminders
- .where(user_id: user.id)
- .includes(:topic)
- .order(:reminder_at)
+ @bookmark_reminders = Bookmark.with_reminders
+ .where(user_id: user.id)
+ .order(:reminder_at)
+ .map do |bookmark|
+ bookmark.registered_bookmarkable.serializer.new(
+ bookmark, scope: user_guardian, root: false
+ )
end
end
end
diff --git a/app/jobs/regular/export_user_archive.rb b/app/jobs/regular/export_user_archive.rb
index c07590c8af4..3e76285f9d7 100644
--- a/app/jobs/regular/export_user_archive.rb
+++ b/app/jobs/regular/export_user_archive.rb
@@ -31,9 +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'],
- # TODO (martin) [POLYBOOK] - Remove the duplication when polymorphic bookmarks are implemented
- 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'],
- bookmarks_polymorphic: ['bookmarkable_id', 'bookmarkable_type', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'],
+ bookmarks: ['bookmarkable_id', 'bookmarkable_type', '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'],
@@ -50,16 +48,7 @@ module Jobs
components = []
COMPONENTS.each do |name|
- export_method = \
- if name == "bookmarks"
- if SiteSetting.use_polymorphic_bookmarks
- "bookmarks_polymorphic_export"
- else
- "bookmarks_export"
- end
- else
- "#{name}_export"
- end
+ export_method = "#{name}_export"
h = { name: name, method: :"#{export_method}" }
h[:filetype] = :csv
filetype_method = :"#{name}_filetype"
@@ -240,8 +229,8 @@ module Jobs
end
end
- def bookmarks_polymorphic_export
- return enum_for(:bookmarks_polymorphic_export) unless block_given?
+ def bookmarks_export
+ return enum_for(:bookmarks_export) unless block_given?
@current_user.bookmarks.where.not(bookmarkable_type: nil).order(:id).each do |bookmark|
link = ''
@@ -268,32 +257,6 @@ module Jobs
end
end
- # TODO (martin) [POLYBOOK] No longer relevant after polymorphic bookmarks implemented
- def bookmarks_export
- return enum_for(:bookmarks_export) unless block_given?
-
- @current_user.bookmarks.joins(:post).order(:id).each do |bookmark|
- link = ''
- if guardian.can_see_bookmarkable?(bookmark)
- link = bookmark.post.full_url
- end
-
- yield [
- bookmark.post_id,
- bookmark.topic_id,
- bookmark.post&.post_number,
- link,
- bookmark.name,
- bookmark.created_at,
- bookmark.updated_at,
- bookmark.reminder_at,
- bookmark.reminder_last_sent_at,
- bookmark.reminder_set_at,
- Bookmark.auto_delete_preferences[bookmark.auto_delete_preference],
- ]
- end
- end
-
def category_preferences_export
return enum_for(:category_preferences_export) unless block_given?
@@ -434,12 +397,6 @@ module Jobs
end
end
header_array.push("group_names")
- elsif entity == 'bookmarks'
- if SiteSetting.use_polymorphic_bookmarks
- header_array = HEADER_ATTRS_FOR['bookmarks_polymorphic']
- else
- header_array = HEADER_ATTRS_FOR['bookmarks']
- end
else
header_array = HEADER_ATTRS_FOR[entity]
end
diff --git a/app/jobs/regular/sync_topic_user_bookmarked.rb b/app/jobs/regular/sync_topic_user_bookmarked.rb
index 2710861c59d..0633e472ff8 100644
--- a/app/jobs/regular/sync_topic_user_bookmarked.rb
+++ b/app/jobs/regular/sync_topic_user_bookmarked.rb
@@ -5,8 +5,7 @@ module Jobs
def execute(args = {})
topic_id = args[:topic_id]
- if SiteSetting.use_polymorphic_bookmarks
- DB.exec(<<~SQL, topic_id: topic_id)
+ DB.exec(<<~SQL, topic_id: topic_id)
UPDATE topic_users SET bookmarked = true
FROM bookmarks AS b
INNER JOIN posts ON posts.id = b.bookmarkable_id AND b.bookmarkable_type = 'Post'
@@ -14,9 +13,9 @@ module Jobs
posts.deleted_at IS NULL 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
+ SQL
- DB.exec(<<~SQL, topic_id: topic_id)
+ DB.exec(<<~SQL, topic_id: topic_id)
UPDATE topic_users SET bookmarked = false
WHERE topic_users.bookmarked AND
(
@@ -27,31 +26,7 @@ module Jobs
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" : ""}
- SQL
- else
- DB.exec(<<~SQL, topic_id: topic_id)
- UPDATE topic_users SET bookmarked = true
- FROM bookmarks AS b
- 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 = posts.topic_id AND
- topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
- SQL
-
- DB.exec(<<~SQL, topic_id: topic_id)
- UPDATE topic_users SET bookmarked = false
- WHERE topic_users.bookmarked AND
- (
- SELECT COUNT(*)
- FROM bookmarks
- INNER JOIN posts ON posts.id = bookmarks.post_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" : ""}
- SQL
- end
+ SQL
end
end
end
diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index 9d8e86b838b..a74abbbac5e 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -34,16 +34,8 @@ class Bookmark < ActiveRecord::Base
end
belongs_to :user
- belongs_to :post
- has_one :topic, through: :post
belongs_to :bookmarkable, polymorphic: true
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- def topic_id
- return if SiteSetting.use_polymorphic_bookmarks
- post.topic_id
- end
-
def self.auto_delete_preferences
@auto_delete_preferences ||= Enum.new(
never: 0,
@@ -57,16 +49,6 @@ class Bookmark < ActiveRecord::Base
bookmarks_relation.select { |bm| bm.bookmarkable_type == type }
end
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- 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? }
-
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- validate :for_topic_must_use_first_post,
- on: [:create, :update],
- if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_for_topic? }
-
validate :polymorphic_columns_present, on: [:create, :update]
validate :valid_bookmarkable_type, on: [:create, :update]
@@ -85,41 +67,17 @@ class Bookmark < ActiveRecord::Base
end
def polymorphic_columns_present
- return if !SiteSetting.use_polymorphic_bookmarks
return if self.bookmarkable_id.present? && self.bookmarkable_type.present?
self.errors.add(:base, I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
end
def unique_per_bookmarkable
- return if !SiteSetting.use_polymorphic_bookmarks
return if !Bookmark.exists?(user_id: user_id, bookmarkable_id: bookmarkable_id, bookmarkable_type: bookmarkable_type)
self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked", type: bookmarkable_type))
end
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- def unique_per_post_for_user
- return if SiteSetting.use_polymorphic_bookmarks
-
- exists = if is_for_first_post?
- Bookmark.exists?(user_id: user_id, post_id: post_id, for_topic: for_topic)
- else
- Bookmark.exists?(user_id: user_id, post_id: post_id)
- end
-
- if exists
- self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post"))
- end
- end
-
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- def for_topic_must_use_first_post
- if !is_for_first_post? && self.for_topic
- self.errors.add(:base, I18n.t("bookmarks.errors.for_topic_must_use_first_post"))
- end
- end
-
def ensure_sane_reminder_at_time
return if reminder_at.blank?
if reminder_at < Time.zone.now
@@ -145,27 +103,15 @@ class Bookmark < ActiveRecord::Base
end
def valid_bookmarkable_type
- return if !SiteSetting.use_polymorphic_bookmarks
return if Bookmark.valid_bookmarkable_types.include?(self.bookmarkable_type)
self.errors.add(:base, I18n.t("bookmarks.errors.invalid_bookmarkable", type: self.bookmarkable_type))
end
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- def is_for_first_post?
- @is_for_first_post ||= new_record? ? Post.exists?(id: post_id, post_number: 1) : post.post_number == 1
- end
-
def auto_delete_when_reminder_sent?
self.auto_delete_preference == Bookmark.auto_delete_preferences[:when_reminder_sent]
end
- # TODO (martin) [POLYBOOK] This is only relevant for post/topic bookmarkables, need to
- # think of a way to do this gracefully.
- def auto_delete_on_owner_reply?
- self.auto_delete_preference == Bookmark.auto_delete_preferences[:on_owner_reply]
- end
-
def auto_clear_reminder_when_reminder_sent?
self.auto_delete_preference == Bookmark.auto_delete_preferences[:clear_reminder]
end
@@ -194,26 +140,16 @@ class Bookmark < ActiveRecord::Base
end
scope :for_user_in_topic, ->(user_id, topic_id) {
- if SiteSetting.use_polymorphic_bookmarks
- joins("LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
- .joins("LEFT JOIN topics ON topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic'")
- .where(
- "bookmarks.user_id = :user_id AND (topics.id = :topic_id OR posts.topic_id = :topic_id)",
- user_id: user_id, topic_id: topic_id
- )
- else
- joins(:post).where(user_id: user_id, posts: { topic_id: topic_id })
- end
+ joins("LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
+ .joins("LEFT JOIN topics ON (topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic') OR
+ (topics.id = posts.topic_id)")
+ .where(
+ "bookmarks.user_id = :user_id AND (topics.id = :topic_id OR posts.topic_id = :topic_id)
+ AND posts.deleted_at IS NULL AND topics.deleted_at IS NULL",
+ user_id: user_id, topic_id: topic_id
+ )
}
- def self.find_for_topic_by_user(topic_id, user_id)
- if SiteSetting.use_polymorphic_bookmarks
- find_by(user_id: user_id, bookmarkable_id: topic_id, bookmarkable_type: "Topic")
- else
- for_user_in_topic(user_id, topic_id).where(for_topic: true).first
- end
- end
-
def self.count_per_day(opts = nil)
opts ||= {}
result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago)
@@ -223,15 +159,11 @@ class Bookmark < ActiveRecord::Base
end
if opts[:category_id]
- if SiteSetting.use_polymorphic_bookmarks
- result = result
- .joins("LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
- .joins("LEFT JOIN topics ON (topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic') OR (topics.id = posts.topic_id)")
- .where("topics.deleted_at IS NULL AND posts.deleted_at IS NULL")
- .merge(Topic.in_category_and_subcategories(opts[:category_id]))
- else
- result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id]))
- end
+ result = result
+ .joins("LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
+ .joins("LEFT JOIN topics ON (topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic') OR (topics.id = posts.topic_id)")
+ .where("topics.deleted_at IS NULL AND posts.deleted_at IS NULL")
+ .merge(Topic.in_category_and_subcategories(opts[:category_id]))
end
result.group('date(bookmarks.created_at)')
@@ -248,7 +180,10 @@ class Bookmark < ActiveRecord::Base
topics_deleted = DB.query(<<~SQL, grace_time: grace_time)
DELETE FROM bookmarks b
USING topics t, posts p
- WHERE (t.id = p.topic_id AND b.post_id = p.id)
+ WHERE (t.id = p.topic_id AND (
+ (b.bookmarkable_id = p.id AND b.bookmarkable_type = 'Post') OR
+ (b.bookmarkable_id = p.id AND b.bookmarkable_type = 'Topic')
+ ))
AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time)
RETURNING t.id AS topic_id
SQL
diff --git a/app/models/post.rb b/app/models/post.rb
index d956255c9fd..4f81ce7939e 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -47,10 +47,7 @@ class Post < ActiveRecord::Base
has_one :post_stat
- # TODO (martin) [POLYBOOK]
- # When we are ready we can add as: :bookmarkable here to use the
- # polymorphic association.
- has_many :bookmarks
+ has_many :bookmarks, as: :bookmarkable
has_one :incoming_email
diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb
index 1a07f7f77fc..f8e77c7cd6d 100644
--- a/app/models/post_mover.rb
+++ b/app/models/post_mover.rb
@@ -178,10 +178,7 @@ class PostMover
# we don't want to keep the old topic's OP bookmarked when we are
# moving it into a new topic
- #
- # TODO (martin) [POLYBOOK] This will need to be restructured for polymorphic
- # bookmarks when edge cases are handled.
- Bookmark.where(post_id: post.id).update_all(post_id: new_post.id)
+ Bookmark.where(bookmarkable: post).update_all(bookmarkable_id: new_post.id)
new_post
end
diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb
index bfb37faf26a..472e3e87bb0 100644
--- a/app/serializers/post_serializer.rb
+++ b/app/serializers/post_serializer.rb
@@ -369,18 +369,10 @@ class PostSerializer < BasicPostSerializer
end
def post_bookmark
- if SiteSetting.use_polymorphic_bookmarks
- if @topic_view.present?
- @post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.bookmarkable == object }
- else
- @post_bookmark ||= Bookmark.find_by(user: scope.user, bookmarkable: object)
- end
+ if @topic_view.present?
+ @post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.bookmarkable == object }
else
- if @topic_view.present?
- @post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.post_id == object.id && !bookmark.for_topic }
- else
- @post_bookmark ||= object.bookmarks.find_by(user: scope.user, for_topic: false)
- end
+ @post_bookmark ||= Bookmark.find_by(user: scope.user, bookmarkable: object)
end
end
diff --git a/app/serializers/user_bookmark_list_serializer.rb b/app/serializers/user_bookmark_list_serializer.rb
index b8e472d15b7..60859f557bc 100644
--- a/app/serializers/user_bookmark_list_serializer.rb
+++ b/app/serializers/user_bookmark_list_serializer.rb
@@ -4,12 +4,8 @@ class UserBookmarkListSerializer < ApplicationSerializer
attributes :more_bookmarks_url, :bookmarks
def bookmarks
- if SiteSetting.use_polymorphic_bookmarks
- object.bookmarks.map do |bm|
- bm.registered_bookmarkable.serializer.new(bm, scope: scope, root: false)
- end
- else
- object.bookmarks.map { |bm| UserBookmarkSerializer.new(bm, scope: scope, root: false) }
+ object.bookmarks.map do |bm|
+ bm.registered_bookmarkable.serializer.new(bm, scope: scope, root: false)
end
end
diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb
index 0f8386703ba..0992830d9b2 100644
--- a/app/serializers/user_bookmark_serializer.rb
+++ b/app/serializers/user_bookmark_serializer.rb
@@ -3,6 +3,9 @@
require_relative 'post_item_excerpt'
# TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
+#
+# This must be deleted after the plugins relying on it are updated once
+# polymorphic bookmarks become the norm.
class UserBookmarkSerializer < ApplicationSerializer
include PostItemExcerpt
include TopicTagsMixin
diff --git a/app/views/users/bookmarks.ics.erb b/app/views/users/bookmarks.ics.erb
index 8b0b6be6c96..2c80cc951b8 100644
--- a/app/views/users/bookmarks.ics.erb
+++ b/app/views/users/bookmarks.ics.erb
@@ -1,4 +1,3 @@
-<% if SiteSetting.use_polymorphic_bookmarks %>
BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_version %>//EN
@@ -14,21 +13,3 @@ URL:<%= bookmark.bookmarkable_url %>
END:VEVENT
<% end %>
END:VCALENDAR
-
-<% else %>
-BEGIN:VCALENDAR
-VERSION:2.0
-PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_version %>//EN
-<% @bookmark_reminders.each do |bookmark| %>
-BEGIN:VEVENT
-UID:bookmark_reminder_#<%= bookmark.id %>@<%= Discourse.current_hostname %>
-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 %>
-END:VEVENT
-<% end %>
-END:VCALENDAR
-<% end %>
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 6d287f87e6a..4e254263add 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -2403,7 +2403,7 @@ uncategorized:
use_polymorphic_bookmarks:
client: true
- default: false
+ default: true
hidden: true
suggest_weekends_in_date_pickers:
diff --git a/db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb b/db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb
new file mode 100644
index 00000000000..f35aaaefe17
--- /dev/null
+++ b/db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+class BackfillPolymorphicBookmarksAndMakeDefault < ActiveRecord::Migration[7.0]
+ def up
+ DB.exec(<<~SQL)
+ UPDATE site_settings
+ SET value = 't'
+ WHERE name = 'use_polymorphic_bookmarks'
+ SQL
+
+ DB.exec(<<~SQL)
+ UPDATE bookmarks
+ SET bookmarkable_id = post_id, bookmarkable_type = 'Post'
+ WHERE NOT bookmarks.for_topic AND bookmarkable_id IS NULL
+ SQL
+
+ DB.exec(<<~SQL)
+ UPDATE bookmarks
+ SET bookmarkable_id = posts.topic_id, bookmarkable_type = 'Topic'
+ FROM posts
+ WHERE bookmarks.for_topic AND posts.id = bookmarks.post_id AND bookmarkable_id IS NULL
+ SQL
+ end
+
+ def down
+ raise ActiveRecord::IrreversibleMigration
+ end
+end
diff --git a/db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb b/db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb
new file mode 100644
index 00000000000..d92e7aa05c7
--- /dev/null
+++ b/db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class BackfillPolymorphicBookmarks < ActiveRecord::Migration[7.0]
+ def up
+ DB.exec(<<~SQL)
+ UPDATE bookmarks
+ SET bookmarkable_id = post_id, bookmarkable_type = 'Post'
+ WHERE NOT bookmarks.for_topic AND bookmarkable_id IS NULL
+ SQL
+
+ DB.exec(<<~SQL)
+ UPDATE bookmarks
+ SET bookmarkable_id = posts.topic_id, bookmarkable_type = 'Topic'
+ FROM posts
+ WHERE bookmarks.for_topic AND posts.id = bookmarks.post_id AND bookmarkable_id IS NULL
+ SQL
+ end
+
+ def down
+ raise ActiveRecord::IrreversibleMigration
+ end
+end
diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb
index 11a6186e086..4117d314d7d 100644
--- a/lib/bookmark_manager.rb
+++ b/lib/bookmark_manager.rb
@@ -9,18 +9,37 @@ class BookmarkManager
end
def self.bookmark_metadata(bookmark, user)
- if SiteSetting.use_polymorphic_bookmarks
- bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user)
- else
- { topic_bookmarked: Bookmark.for_user_in_topic(user.id, bookmark.topic.id).exists? }
- end
+ bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user)
end
- # TODO (martin) [POLYBOOK] This will be used in place of #create once
- # polymorphic bookmarks are implemented.
+ ##
+ # Creates a bookmark for a registered bookmarkable (see Bookmark.register_bookmarkable
+ # and RegisteredBookmarkable for details on this).
+ #
+ # Only allows creation of bookmarks for records the user
+ # can access via Guardian.
+ #
+ # Any ActiveModel validation errors raised by the Bookmark model are
+ # hoisted to the instance of this class for further reporting.
+ #
+ # Before creation validations, after create callbacks, and after delete
+ # callbacks are all RegisteredBookmarkable specific and should be defined
+ # there.
+ #
+ # @param [Integer] bookmarkable_id The ID of the ActiveRecord model to attach the bookmark to.
+ # @param [String] bookmarkable_type The class name of the ActiveRecord model to attach the bookmark to.
+ # @param [String] name A short note for the bookmark, shown on the user bookmark list
+ # and on hover of reminder notifications.
+ # @param reminder_at The datetime when a bookmark reminder should be sent after.
+ # Note this is not the exact time a reminder will be sent, as
+ # we send reminders on a rolling schedule.
+ # See Jobs::BookmarkReminderNotifications
+ # @params options Additional options when creating a bookmark
+ # - auto_delete_preference:
+ # See Bookmark.auto_delete_preferences,
+ # this is used to determine when to delete a bookmark
+ # automatically.
def create_for(bookmarkable_id:, bookmarkable_type:, name: nil, reminder_at: nil, options: {})
- raise NotImplementedError if !SiteSetting.use_polymorphic_bookmarks
-
bookmarkable = bookmarkable_type.constantize.find_by(id: bookmarkable_id)
registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type)
registered_bookmarkable.validate_before_create(@guardian, bookmarkable)
@@ -43,78 +62,12 @@ class BookmarkManager
bookmark
end
- ##
- # Creates a bookmark for a post where both the post and the topic are
- # not deleted. Only allows creation of bookmarks for posts the user
- # can access via Guardian.
- #
- # Any ActiveModel validation errors raised by the Bookmark model are
- # hoisted to the instance of this class for further reporting.
- #
- # Also handles setting the associated TopicUser.bookmarked value for
- # the post's topic for the user that is creating the bookmark.
- #
- # @param post_id A post ID for a post that is not deleted.
- # @param name A short note for the bookmark, shown on the user bookmark list
- # and on hover of reminder notifications.
- # @param reminder_at The datetime when a bookmark reminder should be sent after.
- # Note this is not the exact time a reminder will be sent, as
- # we send reminders on a rolling schedule.
- # See Jobs::BookmarkReminderNotifications
- # @param for_topic Whether we are creating a topic-level bookmark which
- # has different behaviour in the UI. Only bookmarks for
- # posts with post_number 1 can be marked as for_topic.
- # @params options Additional options when creating a bookmark
- # - auto_delete_preference:
- # See Bookmark.auto_delete_preferences,
- # this is used to determine when to delete a bookmark
- # automatically.
- def create(
- post_id:,
- name: nil,
- reminder_at: nil,
- for_topic: false,
- options: {}
- )
- post = Post.find_by(id: post_id)
- if post.blank? ||
- post.topic.blank? ||
- !@guardian.can_see_topic?(post.topic) ||
- !@guardian.can_see_post?(post)
- raise Discourse::InvalidAccess
- end
-
- bookmark = Bookmark.create(
- {
- user_id: @user.id,
- post: post,
- name: name,
- reminder_at: reminder_at,
- reminder_set_at: Time.zone.now,
- for_topic: for_topic
- }.merge(options)
- )
-
- if bookmark.errors.any?
- return add_errors_from(bookmark)
- end
-
- update_topic_user_bookmarked(post.topic)
- update_user_option(bookmark)
-
- bookmark
- end
-
def destroy(bookmark_id)
bookmark = find_bookmark_and_check_access(bookmark_id)
bookmark.destroy
- if SiteSetting.use_polymorphic_bookmarks
- bookmark.registered_bookmarkable.after_destroy(@guardian, bookmark)
- else
- update_topic_user_bookmarked(bookmark.topic)
- end
+ bookmark.registered_bookmarkable.after_destroy(@guardian, bookmark)
bookmark
end
@@ -134,8 +87,7 @@ class BookmarkManager
end
def self.send_reminder_notification(id)
- bookmark = Bookmark.find_by(id: id)
- BookmarkReminderNotificationHandler.new(bookmark).send_notification
+ BookmarkReminderNotificationHandler.new(Bookmark.find_by(id: id)).send_notification
end
def update(bookmark_id:, name:, reminder_at:, options: {})
diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb
index 16f5536d1ee..9cbc7b6abe5 100644
--- a/lib/bookmark_query.rb
+++ b/lib/bookmark_query.rb
@@ -10,9 +10,7 @@ class BookmarkQuery
end
def self.preload(bookmarks, object)
- if SiteSetting.use_polymorphic_bookmarks
- preload_polymorphic_associations(bookmarks)
- end
+ preload_polymorphic_associations(bookmarks)
if @preload
@preload.each { |preload| preload.call(bookmarks, object) }
end
@@ -37,34 +35,6 @@ class BookmarkQuery
end
def list_all
- return polymorphic_list_all if SiteSetting.use_polymorphic_bookmarks
-
- topics = Topic.listable_topics.secured(@guardian)
- pms = Topic.private_messages_for_user(@user)
- results = list_all_results(topics, pms)
-
- results = results.order(
- "(CASE WHEN bookmarks.pinned THEN 0 ELSE 1 END),
- bookmarks.reminder_at ASC,
- bookmarks.updated_at DESC"
- )
-
- if @params[:q].present?
- results = search(results, @params[:q])
- end
-
- if @page.positive?
- results = results.offset(@page * @params[:per_page])
- end
-
- results = results.limit(@limit).to_a
- BookmarkQuery.preload(results, self)
- results
- end
-
- private
-
- def polymorphic_list_all
search_term = @params[:q]
ts_query = search_term.present? ? Search.ts_query(term: search_term) : nil
search_term_wildcard = search_term.present? ? "%#{search_term}%" : nil
@@ -109,27 +79,4 @@ class BookmarkQuery
BookmarkQuery.preload(results, self)
results
end
-
- def list_all_results(topics, pms)
- results = base_bookmarks.merge(topics.or(pms))
- results = results.merge(Post.secured(@guardian))
- results = @guardian.filter_allowed_categories(results)
- results
- end
-
- def base_bookmarks
- Bookmark.where(user: @user)
- .includes(post: :user)
- .includes(post: { topic: :tags })
- .includes(topic: :topic_users)
- .references(:post)
- .where(topic_users: { user_id: @user.id })
- end
-
- def search(results, term)
- bookmark_ts_query = Search.ts_query(term: term)
- results
- .joins("LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.post_id")
- .where("bookmarks.name ILIKE :q OR #{bookmark_ts_query} @@ post_search_data.search_data", q: "%#{term}%")
- end
end
diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb
index d4dfd901566..13f5b2966e1 100644
--- a/lib/bookmark_reminder_notification_handler.rb
+++ b/lib/bookmark_reminder_notification_handler.rb
@@ -10,13 +10,10 @@ class BookmarkReminderNotificationHandler
def send_notification
return if bookmark.blank?
Bookmark.transaction do
- # TODO (martin) [POLYBOOK] Can probably change this to call the
- # can_send_reminder? on the registered bookmarkable directly instead
- # of having can_send_reminder?
- if !can_send_reminder?
+ if !bookmark.registered_bookmarkable.can_send_reminder?(bookmark)
clear_reminder
else
- create_notification
+ bookmark.registered_bookmarkable.send_reminder_notification(bookmark)
if bookmark.auto_delete_when_reminder_sent?
BookmarkManager.new(bookmark.user).destroy(bookmark.id)
@@ -40,29 +37,4 @@ class BookmarkReminderNotificationHandler
bookmark.clear_reminder!
end
-
- def can_send_reminder?
- if SiteSetting.use_polymorphic_bookmarks
- bookmark.registered_bookmarkable.can_send_reminder?(bookmark)
- else
- bookmark.post.present? && bookmark.topic.present?
- end
- end
-
- def create_notification
- if SiteSetting.use_polymorphic_bookmarks
- bookmark.registered_bookmarkable.send_reminder_notification(bookmark)
- else
- bookmark.user.notifications.create!(
- notification_type: Notification.types[:bookmark_reminder],
- topic_id: bookmark.topic_id,
- post_number: bookmark.post.post_number,
- data: {
- topic_title: bookmark.topic.title,
- display_username: bookmark.user.username,
- bookmark_name: bookmark.name
- }.to_json
- )
- end
- end
end
diff --git a/lib/guardian/bookmark_guardian.rb b/lib/guardian/bookmark_guardian.rb
index fb0bc5370ea..a3703ed9ebd 100644
--- a/lib/guardian/bookmark_guardian.rb
+++ b/lib/guardian/bookmark_guardian.rb
@@ -10,10 +10,6 @@ module BookmarkGuardian
end
def can_see_bookmarkable?(bookmark)
- if SiteSetting.use_polymorphic_bookmarks?
- return bookmark.registered_bookmarkable.can_see?(self, bookmark)
- end
-
- self.can_see_post?(bookmark.post)
+ bookmark.registered_bookmarkable.can_see?(self, bookmark)
end
end
diff --git a/lib/search.rb b/lib/search.rb
index 8a92641d399..f2cc711880a 100644
--- a/lib/search.rb
+++ b/lib/search.rb
@@ -459,16 +459,12 @@ class Search
# search based on a RegisteredBookmarkable's #search_query method.
advanced_filter(/^in:(bookmarks)$/i) do |posts, match|
if @guardian.user
- if SiteSetting.use_polymorphic_bookmarks
- posts.where(<<~SQL)
- posts.id IN (
- SELECT bookmarkable_id FROM bookmarks
- WHERE bookmarks.user_id = #{@guardian.user.id} AND bookmarks.bookmarkable_type = 'Post'
- )
- SQL
- else
- posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})")
- end
+ posts.where(<<~SQL)
+ posts.id IN (
+ SELECT bookmarkable_id FROM bookmarks
+ WHERE bookmarks.user_id = #{@guardian.user.id} AND bookmarks.bookmarkable_type = 'Post'
+ )
+ SQL
end
end
diff --git a/lib/topic_view.rb b/lib/topic_view.rb
index 7589bbe19b9..c3363715d60 100644
--- a/lib/topic_view.rb
+++ b/lib/topic_view.rb
@@ -413,15 +413,9 @@ class TopicView
end
def bookmarks
- if SiteSetting.use_polymorphic_bookmarks
- @bookmarks ||= Bookmark.for_user_in_topic(@user, @topic.id).select(
- :id, :bookmarkable_id, :bookmarkable_type, :reminder_at, :name, :auto_delete_preference
- )
- else
- @bookmarks ||= @topic.bookmarks.where(user: @user).joins(:topic).select(
- :id, :post_id, "topics.id AS topic_id", :for_topic, :reminder_at, :name, :auto_delete_preference
- )
- end
+ @bookmarks ||= Bookmark.for_user_in_topic(@user, @topic.id).select(
+ :id, :bookmarkable_id, :bookmarkable_type, :reminder_at, :name, :auto_delete_preference
+ )
end
MAX_PARTICIPANTS = 24
diff --git a/plugins/discourse-narrative-bot/plugin.rb b/plugins/discourse-narrative-bot/plugin.rb
index 989f6ef19eb..c0fa830128a 100644
--- a/plugins/discourse-narrative-bot/plugin.rb
+++ b/plugins/discourse-narrative-bot/plugin.rb
@@ -281,10 +281,8 @@ after_initialize do
self.add_model_callback(Bookmark, :after_commit, on: :create) do
if self.user.enqueue_narrative_bot_job?
- if SiteSetting.use_polymorphic_bookmarks && self.bookmarkable_type == "Post"
+ if self.bookmarkable_type == "Post"
Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.bookmarkable_id, input: "bookmark")
- elsif !SiteSetting.use_polymorphic_bookmarks && self.post.present?
- Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: "bookmark")
end
end
end
diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb
index ccebc8f6fbc..f2aea1e2e10 100644
--- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb
+++ b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb
@@ -249,13 +249,6 @@ describe DiscourseNarrativeBot::NewUserNarrative do
end
it 'adds an after commit model callback to bookmark' do
- Jobs.run_later!
- bookmark = Fabricate(:bookmark, post: Fabricate(:post))
- expect_job_enqueued(job: :bot_input, args: { user_id: bookmark.user_id, post_id: bookmark.post_id, input: "bookmark" })
- end
-
- it 'adds an after commit model callback to bookmark for polymorphic bookmarks (but only for post polymorphic bookmarks)' do
- SiteSetting.use_polymorphic_bookmarks = true
Jobs.run_later!
bookmark = Fabricate(:bookmark, bookmarkable: Fabricate(:post))
expect_job_enqueued(job: :bot_input, args: { user_id: bookmark.user_id, post_id: bookmark.bookmarkable_id, input: "bookmark" })
diff --git a/script/import_scripts/base.rb b/script/import_scripts/base.rb
index 67fb5cbc578..1345f206d78 100644
--- a/script/import_scripts/base.rb
+++ b/script/import_scripts/base.rb
@@ -624,12 +624,7 @@ class ImportScripts::Base
else
begin
manager = BookmarkManager.new(user)
-
- if SiteSetting.use_polymorphic_bookmarks
- bookmark = manager.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post")
- else
- bookmark = manager.create(post_id: post.id)
- end
+ bookmark = manager.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post")
created += 1 if manager.errors.none?
skipped += 1 if manager.errors.any?
diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb
index 6cbabbfa00f..30dafdf0f11 100644
--- a/spec/fabricators/bookmark_fabricator.rb
+++ b/spec/fabricators/bookmark_fabricator.rb
@@ -2,28 +2,10 @@
Fabricator(:bookmark) do
user
- post {
- if !SiteSetting.use_polymorphic_bookmarks
- Fabricate(:post)
- end
- }
name "This looked interesting"
reminder_at { 1.day.from_now.iso8601 }
reminder_set_at { Time.zone.now }
- bookmarkable {
- if SiteSetting.use_polymorphic_bookmarks
- Fabricate(:post)
- end
- }
-
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- before_create do |bookmark|
- if bookmark.bookmarkable_id.present? || bookmark.bookmarkable.present?
- bookmark.post = nil
- bookmark.post_id = nil
- bookmark.for_topic = false
- end
- end
+ bookmarkable { Fabricate(:post) }
end
Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do
diff --git a/spec/jobs/bookmark_reminder_notifications_spec.rb b/spec/jobs/bookmark_reminder_notifications_spec.rb
index 5953b5a34d5..6a68eb439e7 100644
--- a/spec/jobs/bookmark_reminder_notifications_spec.rb
+++ b/spec/jobs/bookmark_reminder_notifications_spec.rb
@@ -67,9 +67,9 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
end
it 'will not send notification when topic is not available' do
- bookmark1.topic.destroy
- bookmark2.topic.destroy
- bookmark3.topic.destroy
+ bookmark1.bookmarkable.topic.destroy
+ bookmark2.bookmarkable.topic.destroy
+ bookmark3.bookmarkable.topic.destroy
expect { subject.execute }.not_to change { Notification.where(notification_type: Notification.types[:bookmark_reminder]).count }
end
end
diff --git a/spec/jobs/export_user_archive_spec.rb b/spec/jobs/export_user_archive_spec.rb
index 13c582d348a..27b490904d8 100644
--- a/spec/jobs/export_user_archive_spec.rb
+++ b/spec/jobs/export_user_archive_spec.rb
@@ -280,20 +280,20 @@ describe Jobs::ExportUserArchive do
let(:post4) { Fabricate(:post, topic: private_message_topic) }
let(:reminder_at) { 1.day.from_now }
- it 'properly includes bookmark records' do
+ it "properly includes bookmark records" do
now = freeze_time '2017-03-01 12:00'
- bookmark1 = manager.create(post_id: post1.id, name: name)
+ bookmark1 = manager.create_for(bookmarkable_id: post1.id, bookmarkable_type: "Post", name: name)
update1_at = now + 1.hours
bookmark1.update(name: 'great food recipe', updated_at: update1_at)
- manager.create(post_id: post2.id, name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
+ manager.create_for(bookmarkable_id: post2.id, bookmarkable_type: "Post", 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_at: now - 8.hours)
+ pending_reminder = manager.create_for(bookmarkable_id: post3.id, bookmarkable_type: "Post", name: name, reminder_at: now - 8.hours)
freeze_time now
tau_record = private_message_topic.topic_allowed_users.create!(user_id: user.id)
- manager.create(post_id: post4.id, name: name)
+ manager.create_for(bookmarkable_id: post4.id, bookmarkable_type: "Post", name: name)
tau_record.destroy!
BookmarkReminderNotificationHandler.new(pending_reminder).send_notification
@@ -302,9 +302,8 @@ describe Jobs::ExportUserArchive do
expect(data.length).to eq(4)
- expect(data[0]['post_id']).to eq(post1.id.to_s)
- expect(data[0]['topic_id']).to eq(topic1.id.to_s)
- expect(data[0]['post_number']).to eq('5')
+ expect(data[0]['bookmarkable_id']).to eq(post1.id.to_s)
+ expect(data[0]['bookmarkable_type']).to eq("Post")
expect(data[0]['link']).to eq(post1.full_url)
expect(DateTime.parse(data[0]['updated_at'])).to eq(DateTime.parse(update1_at.to_s))
@@ -316,56 +315,10 @@ describe Jobs::ExportUserArchive do
expect(DateTime.parse(data[2]['reminder_last_sent_at'])).to eq(DateTime.parse(now.to_s))
expect(data[2]['reminder_set_at']).to eq('')
- expect(data[3]['topic_id']).to eq(private_message_topic.id.to_s)
+ expect(data[3]['bookmarkable_id']).to eq(post4.id.to_s)
+ expect(data[3]['bookmarkable_type']).to eq("Post")
expect(data[3]['link']).to eq('')
end
-
- context "for polymorphic bookmarks" do
- let(:component) { 'bookmarks_polymorphic' }
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
- it "properly includes bookmark records" do
- now = freeze_time '2017-03-01 12:00'
-
- bookmark1 = manager.create_for(bookmarkable_id: post1.id, bookmarkable_type: "Post", name: name)
- update1_at = now + 1.hours
- bookmark1.update(name: 'great food recipe', updated_at: update1_at)
-
- manager.create_for(bookmarkable_id: post2.id, bookmarkable_type: "Post", 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_for(bookmarkable_id: post3.id, bookmarkable_type: "Post", name: name, reminder_at: now - 8.hours)
- freeze_time now
-
- tau_record = private_message_topic.topic_allowed_users.create!(user_id: user.id)
- manager.create_for(bookmarkable_id: post4.id, bookmarkable_type: "Post", name: name)
- tau_record.destroy!
-
- BookmarkReminderNotificationHandler.new(pending_reminder).send_notification
-
- data, _csv_out = make_component_csv
-
- expect(data.length).to eq(4)
-
- expect(data[0]['bookmarkable_id']).to eq(post1.id.to_s)
- expect(data[0]['bookmarkable_type']).to eq("Post")
- expect(data[0]['link']).to eq(post1.full_url)
- expect(DateTime.parse(data[0]['updated_at'])).to eq(DateTime.parse(update1_at.to_s))
-
- expect(data[1]['name']).to eq(name)
- 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')
-
- expect(DateTime.parse(data[2]['created_at'])).to eq(DateTime.parse(twelve_hr_ago.to_s))
- expect(DateTime.parse(data[2]['reminder_last_sent_at'])).to eq(DateTime.parse(now.to_s))
- expect(data[2]['reminder_set_at']).to eq('')
-
- expect(data[3]['bookmarkable_id']).to eq(post4.id.to_s)
- expect(data[3]['bookmarkable_type']).to eq("Post")
- expect(data[3]['link']).to eq('')
- end
- end
end
context 'category_preferences' do
diff --git a/spec/jobs/sync_topic_user_bookmarked_spec.rb b/spec/jobs/sync_topic_user_bookmarked_spec.rb
index 3b8b52a1ffd..3938085fe38 100644
--- a/spec/jobs/sync_topic_user_bookmarked_spec.rb
+++ b/spec/jobs/sync_topic_user_bookmarked_spec.rb
@@ -13,8 +13,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
fab!(:tu5) { Fabricate(:topic_user, topic: topic, bookmarked: true) }
it "corrects all topic_users.bookmarked records for the topic" do
- Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample)
- Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample)
+ Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
+ Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
subject.execute(topic_id: topic.id)
@@ -26,8 +26,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
end
it "does not consider topic as bookmarked if the bookmarked post is deleted" do
- Fabricate(:bookmark, user: tu1.user, post: post1)
- Fabricate(:bookmark, user: tu2.user, post: post1)
+ Fabricate(:bookmark, user: tu1.user, bookmarkable: post1)
+ Fabricate(:bookmark, user: tu2.user, bookmarkable: post1)
post1.trash!
@@ -38,8 +38,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
end
it "works when no topic id is provided (runs for all topics)" do
- Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample)
- Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample)
+ Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
+ Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
subject.execute
@@ -49,48 +49,4 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
expect(tu4.reload.bookmarked).to eq(true)
expect(tu5.reload.bookmarked).to eq(false)
end
-
- context "for polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
- it "corrects all topic_users.bookmarked records for the topic" do
- Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
- Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
-
- subject.execute(topic_id: topic.id)
-
- expect(tu1.reload.bookmarked).to eq(true)
- expect(tu2.reload.bookmarked).to eq(false)
- expect(tu3.reload.bookmarked).to eq(false)
- expect(tu4.reload.bookmarked).to eq(true)
- expect(tu5.reload.bookmarked).to eq(false)
- end
-
- it "does not consider topic as bookmarked if the bookmarked post is deleted" do
- Fabricate(:bookmark, user: tu1.user, bookmarkable: post1)
- Fabricate(:bookmark, user: tu2.user, bookmarkable: post1)
-
- post1.trash!
-
- subject.execute(topic_id: topic.id)
-
- expect(tu1.reload.bookmarked).to eq(false)
- expect(tu2.reload.bookmarked).to eq(false)
- end
-
- it "works when no topic id is provided (runs for all topics)" do
- Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
- Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
-
- subject.execute
-
- expect(tu1.reload.bookmarked).to eq(true)
- expect(tu2.reload.bookmarked).to eq(false)
- expect(tu3.reload.bookmarked).to eq(false)
- expect(tu4.reload.bookmarked).to eq(true)
- expect(tu5.reload.bookmarked).to eq(false)
- end
- end
end
diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb
index b44925046be..3b8e9cf75d1 100644
--- a/spec/lib/bookmark_manager_spec.rb
+++ b/spec/lib/bookmark_manager_spec.rb
@@ -9,175 +9,24 @@ RSpec.describe BookmarkManager do
subject { described_class.new(user) }
- describe ".create" do
- it "creates the bookmark for the user" do
- subject.create(post_id: post.id, name: name)
- bookmark = Bookmark.find_by(user: user)
-
- expect(bookmark.post_id).to eq(post.id)
- expect(bookmark.topic_id).to eq(post.topic_id)
- end
-
- it "allows creating a bookmark for the topic and for the first post" do
- subject.create(post_id: post.id, name: name, for_topic: true)
- bookmark = Bookmark.find_by(user: user, post_id: post.id, for_topic: true)
-
- expect(bookmark.post_id).to eq(post.id)
- expect(bookmark.topic_id).to eq(post.topic_id)
- expect(bookmark.for_topic).to eq(true)
-
- subject.create(post_id: post.id, name: name)
- bookmark = Bookmark.find_by(user: user, post_id: post.id, for_topic: false)
-
- expect(bookmark.post_id).to eq(post.id)
- expect(bookmark.topic_id).to eq(post.topic_id)
- expect(bookmark.for_topic).to eq(false)
- end
-
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- # Topic bookmarks will be distinct, not attached to a post.
- it "errors when creating a for_topic bookmark for a post that is not the first one" do
- subject.create(post_id: Fabricate(:post, topic: post.topic).id, name: name, for_topic: true)
- expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.for_topic_must_use_first_post"))
- end
-
- it "when topic is deleted it raises invalid access" do
- post.topic.trash!
- expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
- end
-
- it "when post is deleted it raises invalid access" do
- post.trash!
- expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
- 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_at: reminder_at)
- tu = TopicUser.find_by(user: user)
- expect(tu.bookmarked).to eq(true)
- tu.update(bookmarked: false)
- subject.create(post_id: Fabricate(:post, topic: post.topic).id)
- tu.reload
- expect(tu.bookmarked).to eq(true)
- end
-
- context "when a reminder time is provided" do
- it "saves the values correctly" do
- 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)
- end
- end
-
- context "when options are provided" do
- let(:options) { { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] } }
-
- it "saves any additional options successfully" do
- subject.create(post_id: post.id, name: name, options: options)
- bookmark = Bookmark.find_by(user: user)
-
- expect(bookmark.auto_delete_preference).to eq(1)
- end
- end
-
- context "when the bookmark already exists for the user & post" do
- before do
- Bookmark.create(post: post, user: user)
- end
-
- it "adds an error to the manager" do
- subject.create(post_id: post.id)
- expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked_post"))
- end
- end
-
- context "when the bookmark name is too long" do
- it "adds an error to the manager" do
- subject.create(post_id: post.id, name: "test" * 100)
- expect(subject.errors.full_messages).to include("Name is too long (maximum is 100 characters)")
- 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_at: reminder_at)
- expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_past_reminder"))
- end
- end
-
- context "when the reminder time is far-flung (> 10 years from now)" 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_at: reminder_at)
- expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future"))
- end
- end
-
- context "when the post is inaccessible for the user" do
- before do
- post.trash!
- end
- it "raises an invalid access error" do
- expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
- end
- end
-
- context "when the topic is inaccessible for the user" do
- before do
- post.topic.update(category: Fabricate(:private_category, group: Fabricate(:group)))
- end
- it "raises an invalid access error" do
- expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
- end
- end
-
- it "saves user's preference" do
- subject.create(post_id: post.id, 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[:when_reminder_sent])
-
- bookmark = Bookmark.find_by(user: user)
- subject.update(bookmark_id: bookmark, name: "test", reminder_at: 1.day.from_now, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] })
- expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
- end
- end
-
describe ".destroy" do
- let!(:bookmark) { Fabricate(:bookmark, user: user, post: post) }
+ let!(:bookmark) { Fabricate(:bookmark, user: user, bookmarkable: post) }
it "deletes the existing bookmark" do
subject.destroy(bookmark.id)
expect(Bookmark.exists?(id: bookmark.id)).to eq(false)
end
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
context "if the bookmark is the last one bookmarked in the topic" do
it "marks the topic user bookmarked column as false" do
- TopicUser.create(user: user, topic: bookmark.post.topic, bookmarked: true)
+ TopicUser.create(user: user, topic: post.topic, bookmarked: true)
subject.destroy(bookmark.id)
tu = TopicUser.find_by(user: user)
expect(tu.bookmarked).to eq(false)
end
end
- context "if the bookmark is the last one bookmarked in the topic (polymorphic)" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
- it "marks the topic user bookmarked column as false" do
- poly_bookmark = Fabricate(:bookmark, user: user, bookmarkable: post)
- TopicUser.create(user: user, topic: post.topic, bookmarked: true)
- subject.destroy(poly_bookmark.id)
- tu = TopicUser.find_by(user: user)
- expect(tu.bookmarked).to eq(false)
- end
- end
-
context "if the bookmark is belonging to some other user" do
- let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) }
+ let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), bookmarkable: post) }
it "raises an invalid access error" do
expect { subject.destroy(bookmark.id) }.to raise_error(Discourse::InvalidAccess)
end
@@ -191,7 +40,7 @@ RSpec.describe BookmarkManager do
end
describe ".update" do
- let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: user, post: post, name: "Old name") }
+ let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: user, bookmarkable: post, name: "Old name") }
let(:new_name) { "Some new name" }
let(:new_reminder_at) { 10.days.from_now }
let(:options) { {} }
@@ -230,7 +79,7 @@ RSpec.describe BookmarkManager do
end
context "if the bookmark is belonging to some other user" do
- let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) }
+ let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), bookmarkable: post) }
it "raises an invalid access error" do
expect { update_bookmark }.to raise_error(Discourse::InvalidAccess)
end
@@ -247,35 +96,6 @@ RSpec.describe BookmarkManager do
end
describe ".destroy_for_topic" do
- let!(:topic) { Fabricate(:topic) }
- 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.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, post: Fabricate(:post, topic: topic), user: user2)
- subject.destroy_for_topic(topic)
- 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
- TopicUser.create(user: user, topic: topic, bookmarked: true)
- subject.destroy_for_topic(topic)
- tu = TopicUser.find_by(user: user)
- expect(tu.bookmarked).to eq(false)
- end
- end
-
- describe ".destroy_for_topic (polymorphic)" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
let!(:topic) { Fabricate(:topic) }
let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
let!(:bookmark2) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
@@ -312,7 +132,7 @@ RSpec.describe BookmarkManager do
it "creates a notification for the reminder" do
described_class.send_reminder_notification(bookmark.id)
notif = notifications_for_user.last
- expect(notif.post_number).to eq(bookmark.post.post_number)
+ expect(notif.post_number).to eq(bookmark.bookmarkable.post_number)
end
context "when the bookmark does no longer exist" do
@@ -327,7 +147,7 @@ RSpec.describe BookmarkManager do
context "if the post has been deleted" do
before do
- bookmark.post.trash!
+ bookmark.bookmarkable.trash!
end
it "does not error and does not create a notification" do
described_class.send_reminder_notification(bookmark.id)
@@ -373,11 +193,7 @@ RSpec.describe BookmarkManager do
end
end
- describe "#create_for (use_polymorphic_bookmarks)" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
+ describe "#create_for" do
it "allows creating a bookmark for the topic and for the first post" do
subject.create_for(bookmarkable_id: post.topic_id, bookmarkable_type: "Topic", name: name)
bookmark = Bookmark.find_by(user: user, bookmarkable: post.topic)
diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb
index 58cdc1d7933..2855013d290 100644
--- a/spec/lib/bookmark_query_spec.rb
+++ b/spec/lib/bookmark_query_spec.rb
@@ -13,22 +13,30 @@ RSpec.describe BookmarkQuery do
end
describe "#list_all" do
- fab!(:bookmark1) { Fabricate(:bookmark, user: user) }
- fab!(:bookmark2) { Fabricate(:bookmark, user: user) }
- let!(:topic_user1) { Fabricate(:topic_user, topic: bookmark1.topic, user: user) }
- let!(:topic_user2) { Fabricate(:topic_user, topic: bookmark2.topic, user: user) }
+ before do
+ Bookmark.reset_bookmarkables
+ register_test_bookmarkable
+
+ Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
+ Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
+ user_bookmark
+ end
+
+ let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
+ let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
+ let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) }
+
+ after do
+ Bookmark.reset_bookmarkables
+ end
it "returns all the bookmarks for a user" do
- expect(bookmark_query.list_all.count).to eq(2)
+ expect(bookmark_query.list_all.count).to eq(3)
end
- it "does not return deleted posts" do
- bookmark1.post.trash!
- expect(bookmark_query.list_all.count).to eq(1)
- end
-
- it "does not return deleted topics" do
- bookmark1.topic.trash!
+ it "does not return deleted bookmarkables" do
+ post_bookmark.bookmarkable.trash!
+ topic_bookmark.bookmarkable.trash!
expect(bookmark_query.list_all.count).to eq(1)
end
@@ -41,149 +49,92 @@ RSpec.describe BookmarkQuery do
expect(preloaded_bookmarks.any?).to eq(true)
end
- it "does not query topic_users for the bookmark topic that are not the current user" do
- topic_user3 = Fabricate(:topic_user, topic: bookmark1.topic)
- bookmark = bookmark_query.list_all.find do |b|
- b.topic_id == bookmark1.topic_id
- end
-
- expect(bookmark.topic.topic_users.map(&:user_id)).to contain_exactly(user.id)
+ it "returns a mixture of post, topic, and custom bookmarkable type bookmarks" do
+ bookmarks = bookmark_query.list_all
+ expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
end
- context "for polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- Bookmark.reset_bookmarkables
- register_test_bookmarkable
+ it "handles the user not having permission for all of the bookmarks of a certain bookmarkable" do
+ UserTestBookmarkable.expects(:list_query).returns(nil)
+ bookmarks = bookmark_query.list_all
+ expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id])
+ end
- Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
- Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
- user_bookmark
- end
-
- let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
- let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
- let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) }
-
- after do
- Bookmark.reset_bookmarkables
- end
-
- it "returns a mixture of post, topic, and custom bookmarkable type bookmarks" do
- bookmarks = bookmark_query.list_all
- expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
- end
-
- it "handles the user not having permission for all of the bookmarks of a certain bookmarkable" do
- UserTestBookmarkable.expects(:list_query).returns(nil)
- bookmarks = bookmark_query.list_all
- expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id])
- end
-
- it "handles the user not having permission to see any of their bookmarks" do
- topic_bookmark.bookmarkable.update(category: Fabricate(:private_category, group: Fabricate(:group)))
- post_bookmark.bookmarkable.topic.update(category: topic_bookmark.bookmarkable.category)
- UserTestBookmarkable.expects(:list_query).returns(nil)
- bookmarks = bookmark_query.list_all
- expect(bookmarks.map(&:id)).to eq([])
- end
+ it "handles the user not having permission to see any of their bookmarks" do
+ topic_bookmark.bookmarkable.update(category: Fabricate(:private_category, group: Fabricate(:group)))
+ post_bookmark.bookmarkable.topic.update(category: topic_bookmark.bookmarkable.category)
+ UserTestBookmarkable.expects(:list_query).returns(nil)
+ bookmarks = bookmark_query.list_all
+ expect(bookmarks.map(&:id)).to eq([])
end
context "when q param is provided" do
let!(:post) { Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs")) }
- context "when not using polymorphic bookmarks" do
- let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later") }
- let(:bookmark4) { Fabricate(:bookmark, user: user, post: post) }
-
- before do
- Fabricate(:topic_user, user: user, topic: bookmark3.topic)
- Fabricate(:topic_user, user: user, topic: bookmark4.topic)
- end
-
- it "can search by bookmark name" do
- bookmarks = bookmark_query(params: { q: 'check' }).list_all
- expect(bookmarks.map(&:id)).to eq([bookmark3.id])
- end
-
- it "can search by post content" do
- bookmarks = bookmark_query(params: { q: 'content' }).list_all
- expect(bookmarks.map(&:id)).to eq([bookmark4.id])
- end
-
- it "can search by topic title" do
- bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
- expect(bookmarks.map(&:id)).to eq([bookmark4.id])
- end
+ before do
+ Bookmark.reset_bookmarkables
end
- context "when using polymorphic bookmarks" do
+ after do
+ Bookmark.reset_bookmarkables
+ end
+
+ let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later", bookmarkable: Fabricate(:post)) }
+ let(:bookmark4) { Fabricate(:bookmark, user: user, bookmarkable: post) }
+
+ before do
+ Fabricate(:topic_user, user: user, topic: bookmark3.bookmarkable.topic)
+ Fabricate(:topic_user, user: user, topic: bookmark4.bookmarkable.topic)
+ end
+
+ it "can search by bookmark name" do
+ bookmarks = bookmark_query(params: { q: 'check' }).list_all
+ expect(bookmarks.map(&:id)).to eq([bookmark3.id])
+ end
+
+ it "can search by post content" do
+ bookmarks = bookmark_query(params: { q: 'content' }).list_all
+ expect(bookmarks.map(&:id)).to eq([bookmark4.id])
+ end
+
+ it "can search by topic title" do
+ bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
+ expect(bookmarks.map(&:id)).to eq([bookmark4.id])
+ end
+
+ context "with custom bookmarkable fitering" do
before do
- SiteSetting.use_polymorphic_bookmarks = true
- Bookmark.reset_bookmarkables
+ register_test_bookmarkable
end
- after do
- Bookmark.reset_bookmarkables
- end
+ let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkking")) }
- let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later", bookmarkable: Fabricate(:post)) }
- let(:bookmark4) { Fabricate(:bookmark, user: user, bookmarkable: post) }
-
- before do
- Fabricate(:topic_user, user: user, topic: bookmark3.bookmarkable.topic)
- Fabricate(:topic_user, user: user, topic: bookmark4.bookmarkable.topic)
- end
-
- it "can search by bookmark name" do
- bookmarks = bookmark_query(params: { q: 'check' }).list_all
- expect(bookmarks.map(&:id)).to eq([bookmark3.id])
- end
-
- it "can search by post content" do
- bookmarks = bookmark_query(params: { q: 'content' }).list_all
- expect(bookmarks.map(&:id)).to eq([bookmark4.id])
- end
-
- it "can search by topic title" do
- bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
- expect(bookmarks.map(&:id)).to eq([bookmark4.id])
- end
-
- context "with custom bookmarkable fitering" do
- before do
- register_test_bookmarkable
- end
-
- let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) }
-
- it "allows searching bookmarkables by fields in other tables" do
- bookmarks = bookmark_query(params: { q: 'bookmarkq' }).list_all
- expect(bookmarks.map(&:id)).to eq([bookmark5.id])
- end
+ it "allows searching bookmarkables by fields in other tables" do
+ bookmarks = bookmark_query(params: { q: 'bookmarkk' }).list_all
+ expect(bookmarks.map(&:id)).to eq([bookmark5.id])
end
end
end
context "for a whispered post" do
before do
- bookmark1.post.update(post_type: Post.types[:whisper])
+ post_bookmark.bookmarkable.update(post_type: Post.types[:whisper])
end
context "when the user is moderator" do
it "does return the whispered post" do
user.update!(moderator: true)
- expect(bookmark_query.list_all.count).to eq(2)
+ expect(bookmark_query.list_all.count).to eq(3)
end
end
context "when the user is admin" do
it "does return the whispered post" do
user.update!(admin: true)
- expect(bookmark_query.list_all.count).to eq(2)
+ expect(bookmark_query.list_all.count).to eq(3)
end
end
context "when the user is not staff" do
it "does not return the whispered post" do
- expect(bookmark_query.list_all.count).to eq(1)
+ expect(bookmark_query.list_all.count).to eq(2)
end
end
end
@@ -191,7 +142,7 @@ RSpec.describe BookmarkQuery do
context "for a private message topic bookmark" do
let(:pm_topic) { Fabricate(:private_message_topic) }
before do
- bookmark1.update(post: Fabricate(:post, topic: pm_topic))
+ post_bookmark.update(bookmarkable: Fabricate(:post, topic: pm_topic))
TopicUser.change(user.id, pm_topic.id, total_msecs_viewed: 1)
end
@@ -200,7 +151,7 @@ RSpec.describe BookmarkQuery do
TopicAllowedUser.create(topic: pm_topic, user: user)
end
it "shows the user the bookmark in the PM" do
- expect(bookmark_query.list_all.map(&:id).count).to eq(2)
+ expect(bookmark_query.list_all.map(&:id).count).to eq(3)
end
end
@@ -211,19 +162,19 @@ RSpec.describe BookmarkQuery do
TopicAllowedGroup.create(topic: pm_topic, group: group)
end
it "shows the user the bookmark in the PM" do
- expect(bookmark_query.list_all.map(&:id).count).to eq(2)
+ expect(bookmark_query.list_all.map(&:id).count).to eq(3)
end
end
context "when the user is not a topic_allowed_user" do
it "does not show the user a bookmarked post in a PM where they are not an allowed user" do
- expect(bookmark_query.list_all.map(&:id).count).to eq(1)
+ expect(bookmark_query.list_all.map(&:id).count).to eq(2)
end
end
context "when the user is not in a topic_allowed_group" do
it "does not show the user a bookmarked post in a PM where they are not in an allowed group" do
- expect(bookmark_query.list_all.map(&:id).count).to eq(1)
+ expect(bookmark_query.list_all.map(&:id).count).to eq(2)
end
end
end
@@ -231,15 +182,15 @@ RSpec.describe BookmarkQuery do
context "when the topic category is private" do
let(:group) { Fabricate(:group) }
before do
- bookmark1.topic.update(category: Fabricate(:private_category, group: group))
- bookmark1.reload
+ post_bookmark.bookmarkable.topic.update(category: Fabricate(:private_category, group: group))
+ post_bookmark.reload
end
it "does not show the user a post/topic in a private category they cannot see" do
- expect(bookmark_query.list_all.map(&:id)).not_to include(bookmark1.id)
+ expect(bookmark_query.list_all.map(&:id)).not_to include(post_bookmark.id)
end
it "does show the user a post/topic in a private category they can see" do
GroupUser.create(user: user, group: group)
- expect(bookmark_query.list_all.map(&:id)).to include(bookmark1.id)
+ expect(bookmark_query.list_all.map(&:id)).to include(post_bookmark.id)
end
end
@@ -260,7 +211,7 @@ RSpec.describe BookmarkQuery do
before do
[bookmark1, bookmark2, bookmark3, bookmark4, bookmark5].each do |bm|
- Fabricate(:topic_user, topic: bm.topic, user: user)
+ Fabricate(:topic_user, topic: bm.bookmarkable.topic, user: user)
bm.reload
end
end
diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb
index 8665c6d934a..cf660394cee 100644
--- a/spec/lib/bookmark_reminder_notification_handler_spec.rb
+++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb
@@ -11,24 +11,25 @@ RSpec.describe BookmarkReminderNotificationHandler do
describe "#send_notification" do
let!(:bookmark) do
- Fabricate(:bookmark_next_business_day_reminder, user: user)
+ Fabricate(:bookmark_next_business_day_reminder, user: user, bookmarkable: Fabricate(:post))
end
it "creates a bookmark reminder notification with the correct details" do
subject.new(bookmark).send_notification
notif = bookmark.user.notifications.last
expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
- expect(notif.topic_id).to eq(bookmark.topic_id)
- expect(notif.post_number).to eq(bookmark.post.post_number)
+ expect(notif.topic_id).to eq(bookmark.bookmarkable.topic_id)
+ expect(notif.post_number).to eq(bookmark.bookmarkable.post_number)
data = JSON.parse(notif.data)
- expect(data["topic_title"]).to eq(bookmark.topic.title)
+ expect(data["title"]).to eq(bookmark.bookmarkable.topic.title)
expect(data["display_username"]).to eq(bookmark.user.username)
expect(data["bookmark_name"]).to eq(bookmark.name)
+ expect(data["bookmarkable_url"]).to eq(bookmark.bookmarkable.url)
end
- context "when the topic is deleted" do
+ context "when the bookmarkable is deleted" do
before do
- bookmark.topic.trash!
+ bookmark.bookmarkable.trash!
bookmark.reload
end
@@ -38,56 +39,9 @@ RSpec.describe BookmarkReminderNotificationHandler do
end
end
- context "when the post is deleted" do
- before do
- bookmark.post.trash!
- bookmark.reload
- end
-
- it "does not send a notification and updates last notification attempt time" do
- expect { subject.new(bookmark).send_notification }.not_to change { Notification.count }
- expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
- end
- end
-
- context "using polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
- let!(:bookmark) do
- Fabricate(:bookmark_next_business_day_reminder, user: user, bookmarkable: Fabricate(:post))
- end
-
- it "creates a bookmark reminder notification with the correct details" do
- subject.new(bookmark).send_notification
- notif = bookmark.user.notifications.last
- expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
- expect(notif.topic_id).to eq(bookmark.bookmarkable.topic_id)
- expect(notif.post_number).to eq(bookmark.bookmarkable.post_number)
- data = JSON.parse(notif.data)
- expect(data["title"]).to eq(bookmark.bookmarkable.topic.title)
- expect(data["display_username"]).to eq(bookmark.user.username)
- expect(data["bookmark_name"]).to eq(bookmark.name)
- expect(data["bookmarkable_url"]).to eq(bookmark.bookmarkable.url)
- end
-
- context "when the bookmarkable is deleted" do
- before do
- bookmark.bookmarkable.trash!
- bookmark.reload
- end
-
- it "does not send a notification and updates last notification attempt time" do
- expect { subject.new(bookmark).send_notification }.not_to change { Notification.count }
- expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
- end
- end
- end
-
context "when the auto_delete_preference is when_reminder_sent" do
before do
- TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true)
+ TopicUser.create!(topic: bookmark.bookmarkable.topic, user: user, bookmarked: true)
bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent])
end
@@ -98,24 +52,24 @@ RSpec.describe BookmarkReminderNotificationHandler do
it "changes the TopicUser bookmarked column to false" do
subject.new(bookmark).send_notification
- expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(false)
+ expect(TopicUser.find_by(topic: bookmark.bookmarkable.topic, user: user).bookmarked).to eq(false)
end
context "if there are still other bookmarks in the topic" do
before do
- Fabricate(:bookmark, post: Fabricate(:post, topic: bookmark.topic), user: user)
+ Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: bookmark.bookmarkable.topic), user: user)
end
it "does not change the TopicUser bookmarked column to false" do
subject.new(bookmark).send_notification
- expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(true)
+ expect(TopicUser.find_by(topic: bookmark.bookmarkable.topic, user: user).bookmarked).to eq(true)
end
end
end
context "when the auto_delete_preference is clear_reminder" do
before do
- TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true)
+ TopicUser.create!(topic: bookmark.bookmarkable.topic, user: user, bookmarked: true)
bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:clear_reminder])
end
@@ -124,14 +78,5 @@ RSpec.describe BookmarkReminderNotificationHandler do
expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil)
end
end
-
- context "when the post has been deleted" do
- it "does not send a notification" do
- bookmark.post.trash!
- bookmark.reload
- expect { subject.new(bookmark).send_notification }.not_to change { Notification.count }
- expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
- end
- end
end
end
diff --git a/spec/lib/post_creator_spec.rb b/spec/lib/post_creator_spec.rb
index 400076faba1..de5533e5317 100644
--- a/spec/lib/post_creator_spec.rb
+++ b/spec/lib/post_creator_spec.rb
@@ -807,13 +807,13 @@ describe PostCreator do
context "when the user has bookmarks with auto_delete_preference on_owner_reply" do
before do
- 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])
+ Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
+ Fabricate(:bookmark, user: user, bookmarkable: 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, post: Fabricate(:post, topic: topic), user: user)
+ Fabricate(:bookmark, bookmarkable: 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/lib/search_spec.rb b/spec/lib/search_spec.rb
index ddf15b55a02..ab700ea5771 100644
--- a/spec/lib/search_spec.rb
+++ b/spec/lib/search_spec.rb
@@ -1482,21 +1482,9 @@ describe Search do
it "can filter by posts in the user's bookmarks" do
expect(search_with_bookmarks.posts.map(&:id)).to eq([])
- Fabricate(:bookmark, user: user, post: bookmark_post1)
+ bm = Fabricate(:bookmark, user: user, bookmarkable: bookmark_post1)
expect(search_with_bookmarks.posts.map(&:id)).to match_array([bookmark_post1.id])
end
-
- context "using polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
- it "can filter by posts in the user's bookmarks" do
- expect(search_with_bookmarks.posts.map(&:id)).to eq([])
- bm = Fabricate(:bookmark, user: user, bookmarkable: bookmark_post1)
- expect(search_with_bookmarks.posts.map(&:id)).to match_array([bookmark_post1.id])
- end
- end
end
it 'supports pinned' do
diff --git a/spec/lib/topic_view_spec.rb b/spec/lib/topic_view_spec.rb
index 2e187bd72b4..b8151a4015d 100644
--- a/spec/lib/topic_view_spec.rb
+++ b/spec/lib/topic_view_spec.rb
@@ -396,9 +396,9 @@ RSpec.describe TopicView do
context "#bookmarks" do
let!(:user) { Fabricate(:user) }
- let!(:bookmark1) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) }
- let!(:bookmark2) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) }
- let!(:bookmark3) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic)) }
+ let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
+ let!(:bookmark2) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
+ let!(:bookmark3) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic)) }
it "returns all the bookmarks in the topic for a user" do
expect(TopicView.new(topic.id, user).bookmarks.pluck(:id)).to match_array(
@@ -413,25 +413,26 @@ RSpec.describe TopicView do
context "#bookmarks" do
let!(:user) { Fabricate(:user) }
- let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, post: topic.first_post, user: user) }
- let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, post: topic.posts[1], user: user) }
+ let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, bookmarkable: topic.first_post, user: user) }
+ let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, bookmarkable: topic.posts[1], user: user) }
it "gets the first post bookmark reminder at for the user" do
topic_view = TopicView.new(topic.id, user)
first, second = topic_view.bookmarks.sort_by(&:id)
- expect(first[:post_id]).to eq(bookmark1.post_id)
+ expect(first[:bookmarkable_id]).to eq(bookmark1.bookmarkable_id)
expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
- expect(second[:post_id]).to eq(bookmark2.post_id)
+ expect(second[:bookmarkable_id]).to eq(bookmark2.bookmarkable_id)
expect(second[:reminder_at]).to eq_time(bookmark2.reminder_at)
end
context "when the topic is deleted" do
it "returns []" do
topic_view = TopicView.new(topic, user)
+ expect(topic_view.bookmarks).to match_array([bookmark1, bookmark2])
PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
topic.reload
-
+ topic_view.instance_variable_set(:@bookmarks, nil)
expect(topic_view.bookmarks).to eq([])
end
end
@@ -444,7 +445,7 @@ RSpec.describe TopicView do
expect(topic_view.bookmarks.length).to eq(1)
first = topic_view.bookmarks.first
- expect(first[:post_id]).to eq(bookmark1.post_id)
+ expect(first[:bookmarkable_id]).to eq(bookmark1.bookmarkable_id)
expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
end
end
diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb
index a5eb5296993..162d2ddacfa 100644
--- a/spec/models/bookmark_spec.rb
+++ b/spec/models/bookmark_spec.rb
@@ -4,95 +4,36 @@ describe Bookmark do
fab!(:post) { Fabricate(:post) }
context "validations" do
- it "does not allow user to bookmark a post twice, enforces unique bookmark per post, user, and for_topic" do
- bookmark = Fabricate(:bookmark, post: post)
- user = bookmark.user
-
- bookmark_2 = Fabricate.build(
- :bookmark,
- post: post,
- user: user
- )
-
- expect(bookmark_2.valid?).to eq(false)
- end
-
- it "allows a user to bookmark a post twice if it is the first post and for_topic is different" do
- post.update!(post_number: 1)
- bookmark = Fabricate(:bookmark, post: post, for_topic: false)
- user = bookmark.user
-
- bookmark_2 = Fabricate(
- :bookmark,
- post: post,
- user: user,
- for_topic: true
- )
-
- expect(bookmark_2.valid?).to eq(true)
-
- bookmark_3 = Fabricate.build(
- :bookmark,
- post: post,
- user: user,
- for_topic: true
- )
-
- expect(bookmark_3.valid?).to eq(false)
- end
-
- describe "polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
- after do
- Bookmark.registered_bookmarkables = []
- end
-
- it "does not allow a user to create a bookmark with only one polymorphic column" do
- user = Fabricate(:user)
- bm = Bookmark.create(bookmarkable_id: post.id, user: user)
- expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
- bm = Bookmark.create(bookmarkable_type: "Post", user: user)
- expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
- bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
- expect(bm.errors.full_messages).to be_empty
- end
-
- it "does not allow a user to create a bookmark for the same record more than once" do
- user = Fabricate(:user)
- Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
- bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
- expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked", type: "Post"))
- end
-
- it "does not allow a user to create a bookmarkable for a type that has not been registered" do
- user = Fabricate(:user)
- bm = Bookmark.create(bookmarkable_type: "User", bookmarkable: Fabricate(:user), user: user)
- expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.invalid_bookmarkable", type: "User"))
- register_test_bookmarkable
- expect(bm.valid?).to eq(true)
- end
- end
- end
-
- describe "#find_for_topic_by_user" do
- it "gets the for_topic bookmark for a user for a specific topic" do
+ it "does not allow a user to create a bookmark with only one polymorphic column" do
user = Fabricate(:user)
- post.update!(post_number: 1)
- bookmark = Fabricate(:bookmark, user: user)
- bookmark_2 = Fabricate(:bookmark, user: user, post: post, for_topic: true)
- expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(bookmark_2)
- bookmark_2.update!(for_topic: false)
- expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(nil)
+ bm = Bookmark.create(bookmarkable_id: post.id, user: user)
+ expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
+ bm = Bookmark.create(bookmarkable_type: "Post", user: user)
+ expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
+ bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
+ expect(bm.errors.full_messages).to be_empty
+ end
+
+ it "does not allow a user to create a bookmark for the same record more than once" do
+ user = Fabricate(:user)
+ Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
+ bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
+ expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked", type: "Post"))
+ end
+
+ it "does not allow a user to create a bookmarkable for a type that has not been registered" do
+ user = Fabricate(:user)
+ bm = Bookmark.create(bookmarkable_type: "User", bookmarkable: Fabricate(:user), user: user)
+ expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.invalid_bookmarkable", type: "User"))
+ register_test_bookmarkable
+ expect(bm.valid?).to eq(true)
end
end
describe "#cleanup!" do
it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do
- bookmark = Fabricate(:bookmark, post: post)
- bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
+ bookmark = Fabricate(:bookmark, bookmarkable: post)
+ bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic))
post.trash!
post.update(deleted_at: 4.days.ago)
Bookmark.cleanup!
@@ -102,10 +43,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)
- bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
- bookmark3 = Fabricate(:bookmark, post: post2)
- bookmark4 = Fabricate(:bookmark, post: post2)
+ bookmark = Fabricate(:bookmark, bookmarkable: post)
+ bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic))
+ bookmark3 = Fabricate(:bookmark, bookmarkable: post2)
+ bookmark4 = Fabricate(:bookmark, bookmarkable: post2)
post.trash!
post.update(deleted_at: 4.days.ago)
post2.trash!
@@ -120,8 +61,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)
- bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
+ bookmark = Fabricate(:bookmark, bookmarkable: post)
+ bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic))
bookmark3 = Fabricate(:bookmark)
post.topic.trash!
post.topic.update(deleted_at: 4.days.ago)
@@ -132,7 +73,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)
+ bookmark = Fabricate(:bookmark, bookmarkable: post)
bookmark2 = Fabricate(:bookmark)
Bookmark.cleanup!
expect(Bookmark.find(bookmark.id)).to eq(bookmark)
@@ -146,106 +87,51 @@ describe Bookmark do
let(:category) { Fabricate(:category) }
let(:topic_in_category) { Fabricate(:topic, category: category) }
- context "for non-polymorphic bookmarks" do
- 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) }
+ 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, bookmarkable: 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
+ 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
- context "for polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- 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
- 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, bookmarkable: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) }
- let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) }
+ 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 "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 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 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 category_id option" do
+ expect(Bookmark.count_per_day(category_id: category.id)).to eq({
+ 3.days.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.bookmarkable.trash!
- expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
- bookmark4.bookmarkable.recover!
- bookmark4.bookmarkable.topic.trash!
- expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
- end
+ it "does not include deleted posts or topics" do
+ bookmark4.bookmarkable.trash!
+ expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+ bookmark4.bookmarkable.recover!
+ bookmark4.bookmarkable.topic.trash!
+ expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
end
end
diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb
index 2e5378e7bf6..bec5790145f 100644
--- a/spec/models/post_mover_spec.rb
+++ b/spec/models/post_mover_spec.rb
@@ -392,30 +392,16 @@ describe PostMover do
.to contain_exactly([1, 500], [2, 750])
end
- 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, 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)
- expect(bookmark3.reload.topic_id).to eq(new_topic.id)
- expect(bookmark4.reload.topic_id).to eq(new_post.topic_id)
- end
-
it "makes sure the topic_user.bookmarked value is reflected for users in the source and destination topic" do
Jobs.run_immediately!
user1 = Fabricate(:user)
user2 = Fabricate(:user)
- bookmark1 = Fabricate(:bookmark, post: p1, user: user1)
- bookmark2 = Fabricate(:bookmark, post: p4, user: user1)
+ bookmark1 = Fabricate(:bookmark, bookmarkable: p1, user: user1)
+ bookmark2 = Fabricate(:bookmark, bookmarkable: p4, user: user1)
- bookmark3 = Fabricate(:bookmark, post: p3, user: user2)
- bookmark4 = Fabricate(:bookmark, post: p4, user: user2)
+ bookmark3 = Fabricate(:bookmark, bookmarkable: p3, user: user2)
+ bookmark4 = Fabricate(:bookmark, bookmarkable: p4, user: user2)
tu1 = Fabricate(
:topic_user,
@@ -724,20 +710,6 @@ describe PostMover do
expect(Notification.exists?(admin_notification.id)).to eq(true)
end
- 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, 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)
- expect(bookmark3.reload.topic_id).to eq(destination_topic.id)
- expect(bookmark4.reload.topic_id).to eq(new_post.topic_id)
- end
-
context "post timings" do
fab!(:some_user) { Fabricate(:user) }
diff --git a/spec/models/user_bookmark_list_spec.rb b/spec/models/user_bookmark_list_spec.rb
index 1bd6c04c7bd..b03a7141d74 100644
--- a/spec/models/user_bookmark_list_spec.rb
+++ b/spec/models/user_bookmark_list_spec.rb
@@ -5,44 +5,36 @@ RSpec.describe UserBookmarkList do
fab!(:user) { Fabricate(:user) }
let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: params) }
- context "for non-polymorphic bookmarks" do
- before do
- 22.times do
- bookmark = Fabricate(:bookmark, user: user)
- Fabricate(:topic_user, topic: bookmark.topic, user: user)
- end
- end
+ before do
+ register_test_bookmarkable
- it "defaults to 20 per page" do
- expect(list.per_page).to eq(20)
- end
-
- context "when the per_page param is too high" do
- let(:params) { { per_page: 1000 } }
-
- it "does not allow more than X bookmarks to be requested per page" do
- expect(list.load.count).to eq(20)
- end
- end
+ Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
+ Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
+ user_bookmark
end
- context "for polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- register_test_bookmarkable
+ let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
+ let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
+ let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) }
- Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
- Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
- user_bookmark
- end
+ it "returns all types of bookmarks" do
+ list.load
+ expect(list.bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
+ end
- let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
- let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
- let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) }
+ it "defaults to 20 per page" do
+ expect(list.per_page).to eq(20)
+ end
- it "returns all types of bookmarks" do
- list.load
- expect(list.bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
+ context "when the per_page param is too high" do
+ let(:params) { { per_page: 1000 } }
+
+ it "does not allow more than X bookmarks to be requested per page" do
+ 22.times do
+ bookmark = Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post))
+ Fabricate(:topic_user, topic: bookmark.bookmarkable.topic, user: user)
+ end
+ expect(list.load.count).to eq(20)
end
end
end
diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb
index 2ad2eef4eed..d5f467d6915 100644
--- a/spec/requests/admin/users_controller_spec.rb
+++ b/spec/requests/admin/users_controller_spec.rb
@@ -308,7 +308,8 @@ RSpec.describe Admin::UsersController do
it "also prevents use of any api keys" do
api_key = Fabricate(:api_key, user: user)
post "/bookmarks.json", params: {
- post_id: Fabricate(:post).id
+ bookmarkable_id: Fabricate(:post).id,
+ bookmarkable_type: "Post"
}, headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb
index 64862fbeb3f..0d2ed96885d 100644
--- a/spec/requests/bookmarks_controller_spec.rb
+++ b/spec/requests/bookmarks_controller_spec.rb
@@ -17,45 +17,20 @@ describe BookmarksController do
RateLimiter.clear_all!
post "/bookmarks.json", params: {
- post_id: bookmark_post.id,
+ bookmarkable_id: bookmark_post.id,
+ bookmarkable_type: "Post",
reminder_at: (Time.zone.now + 1.day).iso8601
}
expect(response.status).to eq(200)
post "/bookmarks.json", params: {
- post_id: Fabricate(:post).id
+ bookmarkable_id: bookmark_post.id,
+ bookmarkable_type: "Post",
}
expect(response.status).to eq(429)
end
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- it "creates a for_topic bookmark" do
- post "/bookmarks.json", params: {
- post_id: bookmark_post.id,
- reminder_type: "tomorrow",
- reminder_at: (Time.zone.now + 1.day).iso8601,
- for_topic: true
- }
- expect(response.status).to eq(200)
- bookmark = Bookmark.find(response.parsed_body["id"])
- expect(bookmark.for_topic).to eq(true)
- end
-
- # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
- it "errors when trying to create a for_topic bookmark for post_number > 1" do
- post "/bookmarks.json", params: {
- post_id: Fabricate(:post, topic: bookmark_post.topic).id,
- reminder_type: "tomorrow",
- reminder_at: (Time.zone.now + 1.day).iso8601,
- for_topic: true
- }
- expect(response.status).to eq(400)
- expect(response.parsed_body['errors']).to include(
- I18n.t("bookmarks.errors.for_topic_must_use_first_post")
- )
- end
-
context "if the user reached the max bookmark limit" do
before do
SiteSetting.max_bookmarks_per_user = 1
@@ -63,11 +38,13 @@ describe BookmarksController do
it "returns failed JSON with a 400 error" do
post "/bookmarks.json", params: {
- post_id: bookmark_post.id,
+ bookmarkable_id: bookmark_post.id,
+ bookmarkable_type: "Post",
reminder_at: (Time.zone.now + 1.day).iso8601
}
post "/bookmarks.json", params: {
- post_id: Fabricate(:post).id
+ bookmarkable_id: bookmark_post.id,
+ bookmarkable_type: "Post",
}
expect(response.status).to eq(400)
@@ -78,27 +55,8 @@ describe BookmarksController do
end
end
- context "if the user already has bookmarked the post" do
+ context "if the user already has bookmarked the record" do
before do
- Fabricate(:bookmark, post: bookmark_post, user: bookmark_user)
- end
-
- it "returns failed JSON with a 400 error" do
- post "/bookmarks.json", params: {
- post_id: bookmark_post.id,
- reminder_at: (Time.zone.now + 1.day).iso8601
- }
-
- expect(response.status).to eq(400)
- expect(response.parsed_body['errors']).to include(
- I18n.t("bookmarks.errors.already_bookmarked_post")
- )
- end
- end
-
- context "if the user already has bookmarked the record (polymorphic)" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user)
Fabricate(:bookmark, bookmarkable: bookmark_topic, user: bookmark_user)
end
@@ -130,7 +88,7 @@ describe BookmarksController do
end
describe "#destroy" do
- let!(:bookmark) { Fabricate(:bookmark, post: bookmark_post, user: bookmark_user) }
+ let!(:bookmark) { Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user) }
it "destroys the bookmark" do
delete "/bookmarks/#{bookmark.id}.json"
@@ -141,33 +99,14 @@ describe BookmarksController do
delete "/bookmarks/#{bookmark.id}.json"
expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
expect(response.parsed_body["topic_bookmarked"]).to eq(false)
- bm2 = Fabricate(:bookmark, user: bookmark_user, post: Fabricate(:post, topic: bookmark_post.topic))
- Fabricate(:bookmark, user: bookmark_user, post: Fabricate(:post, topic: bookmark_post.topic))
+ bm2 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: Fabricate(:post, topic: bookmark_post.topic))
+ bm3 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: bookmark_post.topic)
delete "/bookmarks/#{bm2.id}.json"
expect(Bookmark.find_by(id: bm2.id)).to eq(nil)
expect(response.parsed_body["topic_bookmarked"]).to eq(true)
- end
-
- context "for polymorphic bookmarks" do
- let!(:bookmark) { Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user) }
-
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
- it "returns an indication of whether there are still bookmarks in the topic" do
- delete "/bookmarks/#{bookmark.id}.json"
- expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
- expect(response.parsed_body["topic_bookmarked"]).to eq(false)
- bm2 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: Fabricate(:post, topic: bookmark_post.topic))
- bm3 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: bookmark_post.topic)
- delete "/bookmarks/#{bm2.id}.json"
- expect(Bookmark.find_by(id: bm2.id)).to eq(nil)
- expect(response.parsed_body["topic_bookmarked"]).to eq(true)
- delete "/bookmarks/#{bm3.id}.json"
- expect(Bookmark.find_by(id: bm3.id)).to eq(nil)
- expect(response.parsed_body["topic_bookmarked"]).to eq(false)
- end
+ delete "/bookmarks/#{bm3.id}.json"
+ expect(Bookmark.find_by(id: bm3.id)).to eq(nil)
+ expect(response.parsed_body["topic_bookmarked"]).to eq(false)
end
context "if the bookmark has already been destroyed" do
diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb
index c5b50cbe0f8..c7c1eb0a3f1 100644
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@@ -649,7 +649,7 @@ describe PostsController do
describe "#destroy_bookmark" do
fab!(:post) { Fabricate(:post) }
- fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post) }
+ fab!(:bookmark) { Fabricate(:bookmark, user: user, bookmarkable: post) }
before do
sign_in(user)
@@ -663,7 +663,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))
+ Fabricate(:bookmark, user: user, bookmarkable: 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 3f14d1c8c96..8b13a2607fe 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -3273,8 +3273,8 @@ RSpec.describe TopicsController do
post = create_post
post2 = create_post(topic_id: post.topic_id)
- Fabricate(:bookmark, user: user, post: post)
- Fabricate(:bookmark, user: user, post: post2)
+ Fabricate(:bookmark, user: user, bookmarkable: post)
+ Fabricate(:bookmark, user: user, bookmarkable: post2)
put "/t/#{post.topic_id}/remove_bookmarks.json"
expect(Bookmark.where(user: user).count).to eq(0)
@@ -3292,7 +3292,7 @@ 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, user: user)
+ Fabricate(:bookmark, bookmarkable: post, user: user)
put "/t/#{post.topic_id}/remove_bookmarks.json"
expect(Bookmark.for_user_in_topic(user.id, post.topic_id).count).to eq(0)
end
@@ -3304,42 +3304,21 @@ RSpec.describe TopicsController do
sign_in(user)
end
- it "should create a new bookmark on the first post of the topic" do
+ it "should create a new bookmark for the topic" do
post = create_post
post2 = create_post(topic_id: post.topic_id)
put "/t/#{post.topic_id}/bookmark.json"
- expect(Bookmark.find_by(user_id: user.id).post_id).to eq(post.id)
+ expect(Bookmark.find_by(user_id: user.id).bookmarkable_id).to eq(post.topic_id)
end
it "errors if the topic is already bookmarked for the user" do
post = create_post
- Bookmark.create(post: post, user: user)
+ Bookmark.create(bookmarkable: post.topic, user: user)
put "/t/#{post.topic_id}/bookmark.json"
expect(response.status).to eq(400)
end
-
- context "bookmarks with reminders" do
- it "should create a new bookmark on the first post of the topic" do
- post = create_post
- post2 = create_post(topic_id: post.topic_id)
- put "/t/#{post.topic_id}/bookmark.json"
- expect(response.status).to eq(200)
-
- 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, user: user)
-
- put "/t/#{post.topic_id}/bookmark.json"
- expect(response.status).to eq(400)
- end
- end
end
describe '#reset_new' do
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 9e90df0a79a..fa8396c389a 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -5240,125 +5240,54 @@ describe UsersController do
end
describe "#bookmarks" do
- context "when polymorphic bookmarks are not used" do
- let(:bookmark1) { Fabricate(:bookmark, user: user1) }
- let(:bookmark2) { Fabricate(:bookmark, user: user1) }
- let(:bookmark3) { Fabricate(:bookmark) }
- before do
- TopicUser.change(user1.id, bookmark1.topic_id, total_msecs_viewed: 1)
- TopicUser.change(user1.id, bookmark2.topic_id, total_msecs_viewed: 1)
- bookmark3
- end
-
- it "returns a list of serialized bookmarks for the user" do
- sign_in(user1)
- get "/u/#{user1.username}/bookmarks.json"
- expect(response.status).to eq(200)
- 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(user1)
- get "/u/#{user1.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(user1)
- get "/u/#{bookmark3.user.username}/bookmarks.json"
- expect(response.status).to eq(403)
- end
-
- it "shows a helpful message if no bookmarks are found" do
- bookmark1.destroy
- bookmark2.destroy
- bookmark3.destroy
- sign_in(user1)
- get "/u/#{user1.username}/bookmarks.json"
- expect(response.status).to eq(200)
- expect(response.parsed_body['bookmarks']).to eq([])
- end
-
- it "shows a helpful message if no bookmarks are found for the search" do
- sign_in(user1)
- get "/u/#{user1.username}/bookmarks.json", params: {
- q: 'badsearch'
- }
- expect(response.status).to eq(200)
- expect(response.parsed_body['bookmarks']).to eq([])
- end
+ before do
+ register_test_bookmarkable
+ TopicUser.change(user1.id, bookmark1.bookmarkable.topic_id, total_msecs_viewed: 1)
+ TopicUser.change(user1.id, bookmark2.bookmarkable_id, total_msecs_viewed: 1)
+ Fabricate(:post, topic: bookmark2.bookmarkable)
+ bookmark3 && bookmark4
end
- context "for polymorphic bookmarks" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- register_test_bookmarkable
- TopicUser.change(user1.id, bookmark1.bookmarkable.topic_id, total_msecs_viewed: 1)
- TopicUser.change(user1.id, bookmark2.bookmarkable_id, total_msecs_viewed: 1)
- Fabricate(:post, topic: bookmark2.bookmarkable)
- bookmark3 && bookmark4
- end
+ after do
+ Bookmark.registered_bookmarkables = []
+ end
- after do
- Bookmark.registered_bookmarkables = []
- end
+ let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) }
+ let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) }
+ let(:bookmark3) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:user)) }
+ let(:bookmark4) { Fabricate(:bookmark) }
- let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) }
- let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) }
- let(:bookmark3) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:user)) }
- let(:bookmark4) { Fabricate(:bookmark) }
+ it "returns a list of serialized bookmarks for the user" do
+ sign_in(user1)
+ get "/u/#{user1.username}/bookmarks.json"
+ expect(response.status).to eq(200)
+ expect(response.parsed_body['user_bookmark_list']['bookmarks'].map { |b| b['id'] }).to match_array(
+ [bookmark1.id, bookmark2.id, bookmark3.id]
+ )
+ end
- it "returns a list of serialized bookmarks for the user including custom registered bookmarkables" do
- sign_in(user1)
- bookmark3.bookmarkable.user_profile.update!(bio_raw: "Something cooked
")
- bookmark3.bookmarkable.user_profile.rebake!
- get "/u/#{user1.username}/bookmarks.json"
- expect(response.status).to eq(200)
- response_bookmarks = response.parsed_body['user_bookmark_list']['bookmarks']
- expect(response_bookmarks.map { |b| b['id'] }).to match_array(
- [bookmark1.id, bookmark2.id, bookmark3.id]
- )
- expect(response_bookmarks.find { |b| b['id'] == bookmark3.id }['excerpt']).to eq('Something cooked')
- end
+ it "returns a list of serialized bookmarks for the user including custom registered bookmarkables" do
+ sign_in(user1)
+ bookmark3.bookmarkable.user_profile.update!(bio_raw: "Something cooked
")
+ bookmark3.bookmarkable.user_profile.rebake!
+ get "/u/#{user1.username}/bookmarks.json"
+ expect(response.status).to eq(200)
+ response_bookmarks = response.parsed_body['user_bookmark_list']['bookmarks']
+ expect(response_bookmarks.map { |b| b['id'] }).to match_array(
+ [bookmark1.id, bookmark2.id, bookmark3.id]
+ )
+ expect(response_bookmarks.find { |b| b['id'] == bookmark3.id }['excerpt']).to eq('Something cooked')
+ 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)
- bookmark3.update!(name: nil, reminder_at: 2.weeks.from_now)
+ 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)
+ bookmark3.update!(name: nil, reminder_at: 2.weeks.from_now)
- sign_in(user1)
- get "/u/#{user1.username}/bookmarks.ics"
- expect(response.status).to eq(200)
- expect(response.body.chomp).to eq(<<~ICS)
+ sign_in(user1)
+ get "/u/#{user1.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
@@ -5390,8 +5319,32 @@ describe UsersController do
URL:#{Discourse.base_url}/u/#{bookmark3.bookmarkable.username}
END:VEVENT
END:VCALENDAR
- ICS
- end
+ ICS
+ end
+
+ it "does not show another user's bookmarks" do
+ sign_in(Fabricate(:user))
+ get "/u/#{bookmark3.user.username}/bookmarks.json"
+ expect(response.status).to eq(403)
+ end
+
+ it "shows a helpful message if no bookmarks are found" do
+ bookmark1.destroy
+ bookmark2.destroy
+ bookmark3.destroy
+ sign_in(user1)
+ get "/u/#{user1.username}/bookmarks.json"
+ expect(response.status).to eq(200)
+ expect(response.parsed_body['bookmarks']).to eq([])
+ end
+
+ it "shows a helpful message if no bookmarks are found for the search" do
+ sign_in(user1)
+ get "/u/#{user1.username}/bookmarks.json", params: {
+ q: 'badsearch'
+ }
+ expect(response.status).to eq(200)
+ expect(response.parsed_body['bookmarks']).to eq([])
end
end
diff --git a/spec/script/import_scripts/base_spec.rb b/spec/script/import_scripts/base_spec.rb
index 4b55ecb9d99..030e2cd3359 100644
--- a/spec/script/import_scripts/base_spec.rb
+++ b/spec/script/import_scripts/base_spec.rb
@@ -49,26 +49,12 @@ describe ImportScripts::Base do
it "creates bookmarks, posts, and users" do
MockSpecImporter.new(import_data).perform
- expect(Bookmark.count).to eq(5)
+ expect(Bookmark.where(bookmarkable_type: "Post").count).to eq(5)
expect(Post.count).to eq(5)
expect(User.where('id > 0').count).to eq(1)
expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60)
end
- context "when polymorphic bookmarks are enabled" do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
- it "creates bookmarks, posts, and users" do
- MockSpecImporter.new(import_data).perform
- expect(Bookmark.where(bookmarkable_type: "Post").count).to eq(5)
- expect(Post.count).to eq(5)
- expect(User.where('id > 0').count).to eq(1)
- expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60)
- end
- end
-
it "does not change purge unactivated users setting if disabled" do
SiteSetting.purge_unactivated_users_grace_period_days = 0
MockSpecImporter.new(import_data).perform
diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb
index 57db8590f32..ac049c13d70 100644
--- a/spec/serializers/post_serializer_spec.rb
+++ b/spec/serializers/post_serializer_spec.rb
@@ -254,7 +254,7 @@ describe PostSerializer do
end
context "when a Bookmark record exists for the user on the post" do
- let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post) }
+ let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, bookmarkable: post) }
context "bookmarks with reminders" do
it "returns true" do
@@ -266,17 +266,6 @@ describe PostSerializer do
end
end
end
-
- context "when the post bookmark is for_topic" do
- let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post, for_topic: true) }
-
- context "bookmarks with reminders" do
- it "returns false, because we do not want to mark the post as bookmarked, because the bookmark is for the topic" do
- expect(serialized.as_json[:bookmarked]).to eq(false)
- expect(serialized.as_json[:bookmark_reminder_at]).to eq(nil)
- end
- end
- end
end
context "posts when group moderation is enabled" do
diff --git a/spec/serializers/user_bookmark_list_serializer_spec.rb b/spec/serializers/user_bookmark_list_serializer_spec.rb
index d7388786b07..df14e2f6c30 100644
--- a/spec/serializers/user_bookmark_list_serializer_spec.rb
+++ b/spec/serializers/user_bookmark_list_serializer_spec.rb
@@ -6,7 +6,6 @@ RSpec.describe UserBookmarkListSerializer do
context "for polymorphic bookmarks" do
before do
- SiteSetting.use_polymorphic_bookmarks = true
register_test_bookmarkable
Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
diff --git a/spec/serializers/user_bookmark_serializer_spec.rb b/spec/serializers/user_bookmark_serializer_spec.rb
deleted file mode 100644
index b88903d44a5..00000000000
--- a/spec/serializers/user_bookmark_serializer_spec.rb
+++ /dev/null
@@ -1,82 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe UserBookmarkSerializer do
- let(:user) { Fabricate(:user) }
- let(:post) { Fabricate(:post, user: user) }
- let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post) }
-
- it "serializes all properties correctly" do
- s = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
-
- expect(s.id).to eq(bookmark.id)
- expect(s.created_at).to eq_time(bookmark.created_at)
- expect(s.topic_id).to eq(bookmark.topic_id)
- expect(s.linked_post_number).to eq(bookmark.post.post_number)
- expect(s.post_id).to eq(bookmark.post_id)
- expect(s.name).to eq(bookmark.name)
- expect(s.reminder_at).to eq_time(bookmark.reminder_at)
- expect(s.title).to eq(bookmark.topic.title)
- expect(s.deleted).to eq(false)
- expect(s.hidden).to eq(false)
- expect(s.closed).to eq(false)
- expect(s.archived).to eq(false)
- expect(s.category_id).to eq(bookmark.topic.category_id)
- expect(s.archetype).to eq(bookmark.topic.archetype)
- expect(s.highest_post_number).to eq(1)
- expect(s.bumped_at).to eq_time(bookmark.topic.bumped_at)
- expect(s.slug).to eq(bookmark.topic.slug)
- expect(s.post_user_username).to eq(bookmark.post.user.username)
- expect(s.post_user_name).to eq(bookmark.post.user.name)
- expect(s.post_user_avatar_template).not_to eq(nil)
- expect(s.excerpt).to eq(PrettyText.excerpt(post.cooked, 300, keep_emoji_images: true))
- end
-
- it "uses the correct highest_post_number column based on whether the user is staff" do
- Fabricate(:post, topic: bookmark.topic)
- Fabricate(:post, topic: bookmark.topic)
- Fabricate(:whisper, topic: bookmark.topic)
- bookmark.reload
- serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
-
- expect(serializer.highest_post_number).to eq(3)
-
- user.update!(admin: true)
-
- expect(serializer.highest_post_number).to eq(4)
- end
-
- context "for_topic bookmarks" do
- before do
- bookmark.update!(for_topic: true)
- end
-
- it "uses the last_read_post_number + 1 for the for_topic bookmarks excerpt" do
- next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.topic)
- Fabricate(:post_with_external_links, topic: bookmark.topic)
- bookmark.reload
- TopicUser.change(user.id, bookmark.topic.id, { last_read_post_number: post.post_number })
- serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
- expect(serializer.excerpt).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true))
- end
-
- it "does not use a small post for the last unread cooked post" do
- small_action_post = Fabricate(:small_action, topic: bookmark.topic)
- next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.topic)
- Fabricate(:post_with_external_links, topic: bookmark.topic)
- bookmark.reload
- TopicUser.change(user.id, bookmark.topic.id, { last_read_post_number: post.post_number })
- serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
- expect(serializer.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.topic)
- small_action_post = Fabricate(:small_action, topic: bookmark.topic)
- bookmark.reload
- TopicUser.change(user.id, bookmark.topic.id, { last_read_post_number: small_action_post.post_number })
- serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
- expect(serializer.cooked).to eq(last_regular_post.cooked)
- expect(serializer.excerpt).to eq(PrettyText.excerpt(last_regular_post.cooked, 300, keep_emoji_images: true))
- end
- end
-end
diff --git a/spec/serializers/user_post_bookmark_serializer_spec.rb b/spec/serializers/user_post_bookmark_serializer_spec.rb
index 2e678c4e969..f0d55f868a0 100644
--- a/spec/serializers/user_post_bookmark_serializer_spec.rb
+++ b/spec/serializers/user_post_bookmark_serializer_spec.rb
@@ -1,10 +1,6 @@
# frozen_string_literal: true
RSpec.describe UserPostBookmarkSerializer do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
let(:user) { Fabricate(:user) }
let(:post) { Fabricate(:post, user: user, topic: topic) }
let(:topic) { Fabricate(:topic) }
diff --git a/spec/serializers/user_topic_bookmark_serializer_spec.rb b/spec/serializers/user_topic_bookmark_serializer_spec.rb
index 34f332e6edb..9f4534c5bee 100644
--- a/spec/serializers/user_topic_bookmark_serializer_spec.rb
+++ b/spec/serializers/user_topic_bookmark_serializer_spec.rb
@@ -1,10 +1,6 @@
# frozen_string_literal: true
RSpec.describe UserTopicBookmarkSerializer do
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
fab!(:user) { Fabricate(:user) }
let!(:topic) { Fabricate(:topic, user: user) }
let!(:post) { Fabricate(:post, topic: topic) }
diff --git a/spec/services/post_bookmarkable_spec.rb b/spec/services/post_bookmarkable_spec.rb
index 076f3667bb1..9f0d0397515 100644
--- a/spec/services/post_bookmarkable_spec.rb
+++ b/spec/services/post_bookmarkable_spec.rb
@@ -7,10 +7,6 @@ describe PostBookmarkable do
fab!(:guardian) { Guardian.new(user) }
fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
let!(:post1) { Fabricate(:post) }
let!(:post2) { Fabricate(:post) }
let!(:bookmark1) { Fabricate(:bookmark, user: user, bookmarkable: post1, name: "something i gotta do") }
diff --git a/spec/services/topic_bookmarkable_spec.rb b/spec/services/topic_bookmarkable_spec.rb
index 68128a5726b..46368592f8c 100644
--- a/spec/services/topic_bookmarkable_spec.rb
+++ b/spec/services/topic_bookmarkable_spec.rb
@@ -7,10 +7,6 @@ describe TopicBookmarkable do
fab!(:guardian) { Guardian.new(user) }
fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
- before do
- SiteSetting.use_polymorphic_bookmarks = true
- end
-
let!(:topic1) { Fabricate(:topic) }
let!(:topic2) { Fabricate(:topic) }
let!(:post) { Fabricate(:post, topic: topic1) }