Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FEATURE: Promote polymorphic bookmarks to default and migrate (#16729) 

This commit migrates all bookmarks to be polymorphic (using the
bookmarkable_id and bookmarkable_type) columns. It also deletes
all the old code guarded behind the use_polymorphic_bookmarks setting
and changes that setting to true for all sites and by default for
the sake of plugins.

No data is deleted in the migrations, the old post_id and for_topic
columns for bookmarks will be dropped later on.
This commit is contained in:
Martin Brennan 2022-05-23 10:07:15 +10:00 committed by GitHub
parent bf987af3ca
commit fcc2e7ebbf
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
63 changed files with 582 additions and 1833 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
app/assets/javascripts/discourse/app/components/bookmark-list.js
View File

@ -86,23 +86,19 @@ export default Component.extend(Scrolling, {
@action @action
editBookmark(bookmark) { editBookmark(bookmark) {
openBookmarkModal( openBookmarkModal(bookmark, {
bookmark, onAfterSave: (savedData) => {
{ this.appEvents.trigger(
onAfterSave: (savedData) => { "bookmarks:changed",
this.appEvents.trigger( savedData,
"bookmarks:changed", bookmark.attachedTo()
savedData, );
bookmark.attachedTo() this.reload();
);
this.reload();
},
onAfterDelete: () => {
this.reload();
},
}, },
{ use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } onAfterDelete: () => {
); this.reload();
},
});
}, },
@action @action

27
app/assets/javascripts/discourse/app/components/bookmark.js
View File

@ -113,8 +113,12 @@ export default Component.extend({
}, },
_loadPostLocalDates() { _loadPostLocalDates() {
if (this.model.bookmarkableType !== "Post") {
return;
}
let postEl = document.querySelector( let postEl = document.querySelector(
`[data-post-id="${this.model.postId}"]` `[data-post-id="${this.model.bookmarkableId}"]`
); );
let localDateEl; let localDateEl;
if (postEl) { if (postEl) {
@ -156,14 +160,8 @@ export default Component.extend({
auto_delete_preference: this.autoDeletePreference, auto_delete_preference: this.autoDeletePreference,
}; };
if (this.siteSettings.use_polymorphic_bookmarks) { data.bookmarkable_id = this.model.bookmarkableId;
data.bookmarkable_id = this.model.bookmarkableId; data.bookmarkable_type = this.model.bookmarkableType;
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;
}
if (this.editingExistingBookmark) { if (this.editingExistingBookmark) {
return ajax(`/bookmarks/${this.model.id}`, { return ajax(`/bookmarks/${this.model.id}`, {
@ -191,15 +189,8 @@ export default Component.extend({
name: this.model.name, name: this.model.name,
}; };
if (this.siteSettings.use_polymorphic_bookmarks) { data.bookmarkable_id = this.model.bookmarkableId;
data.bookmarkable_id = this.model.bookmarkableId; data.bookmarkable_type = this.model.bookmarkableType;
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;
}
this.afterSave(data); this.afterSave(data);
}, },

34
app/assets/javascripts/discourse/app/controllers/bookmark.js
View File

@ -11,28 +11,9 @@ export function openBookmarkModal(
onCloseWithoutSaving: null, onCloseWithoutSaving: null,
onAfterSave: null, onAfterSave: null,
onAfterDelete: null, onAfterDelete: null,
},
options = {
use_polymorphic_bookmarks: false,
} }
) { ) {
return new Promise((resolve) => { 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 = { const model = {
id: bookmark.id, id: bookmark.id,
reminderAt: bookmark.reminder_at, reminderAt: bookmark.reminder_at,
@ -40,19 +21,14 @@ export function openBookmarkModal(
name: bookmark.name, name: bookmark.name,
}; };
if (options.use_polymorphic_bookmarks) { model.bookmarkableId = bookmark.bookmarkable_id;
model.bookmarkableId = bookmark.bookmarkable_id; model.bookmarkableType = bookmark.bookmarkable_type;
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;
}
let modalController = showModal("bookmark", { let modalController = showModal("bookmark", {
model, model,
titleTranslated: modalTitle(), titleTranslated: I18n.t(
bookmark.id ? "bookmarks.edit" : "bookmarks.create"
),
modalClass: "bookmark-with-reminder", modalClass: "bookmark-with-reminder",
}); });
modalController.setProperties({ modalController.setProperties({

160
app/assets/javascripts/discourse/app/controllers/topic.js
View File

@ -767,38 +767,7 @@ export default Controller.extend(bufferedProperty("model"), {
} }
}, },
// TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
toggleBookmark(post) { 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) { if (!this.currentUser) {
return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); return bootbox.alert(I18n.t("bookmarks.not_bookmarked"));
} else if (post) { } else if (post) {
@ -818,16 +787,14 @@ export default Controller.extend(bufferedProperty("model"), {
post post
); );
} else { } else {
return this._toggleTopicLevelBookmarkPolymorphic().then( return this._toggleTopicLevelBookmark().then((changedIds) => {
(changedIds) => { if (!changedIds) {
if (!changedIds) { return;
return;
}
changedIds.forEach((id) =>
this.appEvents.trigger("post-stream:refresh", { id })
);
} }
); changedIds.forEach((id) =>
this.appEvents.trigger("post-stream:refresh", { id })
);
});
} }
}, },
@ -1285,56 +1252,46 @@ export default Controller.extend(bufferedProperty("model"), {
}, },
_modifyTopicBookmark(bookmark) { _modifyTopicBookmark(bookmark) {
return openBookmarkModal( return openBookmarkModal(bookmark, {
bookmark, onAfterSave: (savedData) => {
{ this._syncBookmarks(savedData);
onAfterSave: (savedData) => { this.model.set("bookmarking", false);
this._syncBookmarks(savedData); this.model.set("bookmarked", true);
this.model.set("bookmarking", false); this.model.incrementProperty("bookmarksWereChanged");
this.model.set("bookmarked", true); this.appEvents.trigger(
this.model.incrementProperty("bookmarksWereChanged"); "bookmarks:changed",
this.appEvents.trigger( savedData,
"bookmarks:changed", bookmark.attachedTo()
savedData, );
bookmark.attachedTo()
);
// TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed. // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed.
this.appEvents.trigger("topic:bookmark-toggled"); this.appEvents.trigger("topic:bookmark-toggled");
},
onAfterDelete: (topicBookmarked, bookmarkId) => {
this.model.removeBookmark(bookmarkId);
},
}, },
{ use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } onAfterDelete: (topicBookmarked, bookmarkId) => {
); this.model.removeBookmark(bookmarkId);
},
});
}, },
_modifyPostBookmark(bookmark, post) { _modifyPostBookmark(bookmark, post) {
return openBookmarkModal( return openBookmarkModal(bookmark, {
bookmark, onCloseWithoutSaving: () => {
{ post.appEvents.trigger("post-stream:refresh", {
onCloseWithoutSaving: () => { id: bookmark.bookmarkable_id,
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);
},
}, },
{ 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) { _syncBookmarks(data) {
@ -1352,7 +1309,6 @@ export default Controller.extend(bufferedProperty("model"), {
} }
}, },
// TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
async _toggleTopicLevelBookmark() { async _toggleTopicLevelBookmark() {
if (this.model.bookmarking) { if (this.model.bookmarking) {
return Promise.resolve(); return Promise.resolve();
@ -1362,40 +1318,6 @@ export default Controller.extend(bufferedProperty("model"), {
return this._maybeClearAllBookmarks(); 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) { if (this.model.bookmarkCount === 1) {
const topicBookmark = this.model.bookmarks.findBy( const topicBookmark = this.model.bookmarks.findBy(
"bookmarkable_type", "bookmarkable_type",

17
app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js
View File

@ -11,8 +11,7 @@ const DEFER_PRIORITY = 500;
export default { export default {
name: "topic-footer-buttons", name: "topic-footer-buttons",
initialize(container) { initialize() {
const siteSettings = container.lookup("site-settings:main");
registerTopicFooterButton({ registerTopicFooterButton({
id: "share-and-invite", id: "share-and-invite",
icon: "d-topic-share", icon: "d-topic-share",
@ -99,12 +98,9 @@ export default {
if (this.topic.bookmarkCount === 0) { if (this.topic.bookmarkCount === 0) {
return I18n.t("bookmarked.help.bookmark"); return I18n.t("bookmarked.help.bookmark");
} else if (this.topic.bookmarkCount === 1) { } else if (this.topic.bookmarkCount === 1) {
// TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. const anyTopicBookmarks = this.topic.bookmarks.some(
const anyTopicBookmarks = this.topic.bookmarks.some((bookmark) => { (bookmark) => bookmark.bookmarkable_type === "Topic"
return siteSettings.use_polymorphic_bookmarks );
? bookmark.for_topic
: bookmark.bookmarkable_type === "Topic";
});
if (anyTopicBookmarks) { if (anyTopicBookmarks) {
return I18n.t("bookmarked.help.edit_bookmark_for_topic"); return I18n.t("bookmarked.help.edit_bookmark_for_topic");
@ -119,10 +115,7 @@ export default {
return I18n.t("bookmarked.help.unbookmark"); return I18n.t("bookmarked.help.unbookmark");
} }
}, },
// TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. action: "toggleBookmark",
action: siteSettings.use_polymorphic_bookmarks
? "toggleBookmarkPolymorphic"
: "toggleBookmark",
dropdown() { dropdown() {
return this.site.mobileView; return this.site.mobileView;
}, },

19
app/assets/javascripts/discourse/app/models/bookmark.js
View File

@ -39,18 +39,10 @@ const Bookmark = RestModel.extend({
}, },
attachedTo() { attachedTo() {
if (this.siteSettings.use_polymorphic_bookmarks) { return {
return { target: this.bookmarkable_type.toLowerCase(),
target: this.bookmarkable_type.toLowerCase(), targetId: this.bookmarkable_id,
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 };
}, },
togglePin() { togglePin() {
@ -161,9 +153,6 @@ const Bookmark = RestModel.extend({
@discourseComputed("bookmarkable_type") @discourseComputed("bookmarkable_type")
bookmarkableTopicAlike(bookmarkable_type) { bookmarkableTopicAlike(bookmarkable_type) {
if (!this.siteSettings.use_polymorphic_bookmarks) {
return true;
}
return ["Topic", "Post"].includes(bookmarkable_type); return ["Topic", "Post"].includes(bookmarkable_type);
}, },
}); });

18
app/assets/javascripts/discourse/app/models/topic.js
View File

@ -397,15 +397,7 @@ const Topic = RestModel.extend({
this.set( this.set(
"bookmarks", "bookmarks",
this.bookmarks.filter((bookmark) => { this.bookmarks.filter((bookmark) => {
// TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. if (bookmark.id === id && bookmark.bookmarkable_type === "Topic") {
if (
(!this.siteSettings.use_polymorphic_bookmarks &&
bookmark.id === id &&
bookmark.for_topic) ||
(this.siteSettings.use_polymorphic_bookmarks &&
bookmark.id === id &&
bookmark.bookmarkable_type === "Topic")
) {
// TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed. // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed.
this.appEvents.trigger("topic:bookmark-toggled"); this.appEvents.trigger("topic:bookmark-toggled");
this.appEvents.trigger( this.appEvents.trigger(
@ -425,11 +417,9 @@ const Topic = RestModel.extend({
clearBookmarks() { clearBookmarks() {
this.toggleProperty("bookmarked"); this.toggleProperty("bookmarked");
const postIds = this.siteSettings.use_polymorphic_bookmarks const postIds = this.bookmarks
? this.bookmarks .filterBy("bookmarkable_type", "Post")
.filterBy("bookmarkable_type", "Post") .mapBy("bookmarkable_id");
.mapBy("bookmarkable_id")
: this.bookmarks.mapBy("post_id");
postIds.forEach((postId) => { postIds.forEach((postId) => {
const loadedPost = this.postStream.findLoadedPost(postId); const loadedPost = this.postStream.findLoadedPost(postId);
if (loadedPost) { if (loadedPost) {

2
app/assets/javascripts/discourse/app/templates/topic.hbs
View File

@ -223,7 +223,6 @@
recoverPost=(action "recoverPost") recoverPost=(action "recoverPost")
expandHidden=(action "expandHidden") expandHidden=(action "expandHidden")
toggleBookmark=(action "toggleBookmark") toggleBookmark=(action "toggleBookmark")
toggleBookmarkPolymorphic=(action "toggleBookmarkPolymorphic")
togglePostType=(action "togglePostType") togglePostType=(action "togglePostType")
rebakePost=(action "rebakePost") rebakePost=(action "rebakePost")
changePostOwner=(action "changePostOwner") changePostOwner=(action "changePostOwner")
@ -363,7 +362,6 @@
convertToPublicTopic=(action "convertToPublicTopic") convertToPublicTopic=(action "convertToPublicTopic")
convertToPrivateMessage=(action "convertToPrivateMessage") convertToPrivateMessage=(action "convertToPrivateMessage")
toggleBookmark=(action "toggleBookmark") toggleBookmark=(action "toggleBookmark")
toggleBookmarkPolymorphic=(action "toggleBookmarkPolymorphic")
showFlagTopic=(route-action "showFlagTopic") showFlagTopic=(route-action "showFlagTopic")
toggleArchiveMessage=(action "toggleArchiveMessage") toggleArchiveMessage=(action "toggleArchiveMessage")
editFirstPost=(action "editFirstPost") editFirstPost=(action "editFirstPost")

4
app/assets/javascripts/discourse/app/widgets/post-menu.js
View File

@ -355,9 +355,7 @@ registerButton(
return { return {
id: attrs.bookmarked ? "unbookmark" : "bookmark", id: attrs.bookmarked ? "unbookmark" : "bookmark",
action: siteSettings.use_polymorphic_bookmarks action: "toggleBookmark",
? "toggleBookmarkPolymorphic"
: "toggleBookmark",
title, title,
titleOptions, titleOptions,
className: classNames.join(" "), className: classNames.join(" "),

14
app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js
View File

@ -47,15 +47,25 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", {
// for topic level bookmarks we want to jump to the last unread post // for topic level bookmarks we want to jump to the last unread post
// instead of the OP // instead of the OP
let postNumber; let postNumber;
if (bookmark.for_topic) { if (bookmark.bookmarkable_type === "Topic") {
postNumber = bookmark.last_read_post_number + 1; postNumber = bookmark.last_read_post_number + 1;
} else { } else {
postNumber = bookmark.linked_post_number; 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", { return this.attach("quick-access-item", {
icon: this.icon(bookmark), icon: this.icon(bookmark),
href: postUrl(bookmark.slug, bookmark.topic_id, postNumber), href,
title: bookmark.name, title: bookmark.name,
content: bookmark.title, content: bookmark.title,
username: bookmark.post_user_username, username: bookmark.post_user_username,

31
app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js
View File

@ -83,16 +83,15 @@ acceptance("Bookmarking", function (needs) {
function handleRequest(request) { function handleRequest(request) {
const data = helper.parsePostData(request.requestBody); const data = helper.parsePostData(request.requestBody);
if (data.post_id === "398") { if (data.bookmarkable_id === "398" && data.bookmarkable_type === "Post") {
if (data.for_topic === "true") { return helper.response({ id: 1, success: "OK" });
return helper.response({ id: 3, success: "OK" }); } else if (data.bookmarkable_type === "Topic") {
} else { return helper.response({ id: 3, success: "OK" });
return helper.response({ id: 1, success: "OK" }); } else if (
} data.bookmarkable_id === "419" &&
} else if (data.post_id === "419") { data.bookmarkable_type === "Post"
) {
return helper.response({ id: 2, success: "OK" }); return helper.response({ id: 2, success: "OK" });
} else {
throw new Error("Pretender: unknown post_id");
} }
} }
server.post("/bookmarks", handleRequest); server.post("/bookmarks", handleRequest);
@ -368,13 +367,6 @@ acceptance("Bookmarking", function (needs) {
test("Creating and editing a topic level bookmark", async function (assert) { test("Creating and editing a topic level bookmark", async function (assert) {
await visit("/t/internationalization-localization/280"); await visit("/t/internationalization-localization/280");
await click("#topic-footer-button-bookmark"); 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"); await click("#save-bookmark");
assert.notOk( assert.notOk(
@ -389,13 +381,6 @@ acceptance("Bookmarking", function (needs) {
); );
await click("#topic-footer-button-bookmark"); 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 fillIn("input#bookmark-name", "Test name");
await click("#tap_tile_tomorrow"); await click("#tap_tile_tomorrow");

4
app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js vendored
View File

@ -427,7 +427,9 @@ export default {
created_at: "2020-04-07T05:30:40.446Z", created_at: "2020-04-07T05:30:40.446Z",
topic_id: 119, topic_id: 119,
linked_post_number: 1, 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", name: "test",
reminder_at: null, reminder_at: null,
title: "Yelling topic title :/", title: "Yelling topic title :/",

27
app/controllers/bookmarks_controller.rb
View File

@ -4,12 +4,8 @@ class BookmarksController < ApplicationController
requires_login requires_login
def create def create
if SiteSetting.use_polymorphic_bookmarks params.require(:bookmarkable_id)
params.require(:bookmarkable_id) params.require(:bookmarkable_type)
params.require(:bookmarkable_type)
else
params.require(:post_id)
end
RateLimiter.new( RateLimiter.new(
current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i 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(
bookmark = bookmark_manager.create_for( **create_params.merge(
**create_params.merge( bookmarkable_id: params[:bookmarkable_id],
bookmarkable_id: params[:bookmarkable_id], bookmarkable_type: params[:bookmarkable_type]
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? if bookmark_manager.errors.empty?
return render json: success_json.merge(id: bookmark.id) return render json: success_json.merge(id: bookmark.id)

6
app/controllers/posts_controller.rb
View File

@ -539,7 +539,11 @@ class PostsController < ApplicationController
def destroy_bookmark def destroy_bookmark
params.require(:post_id) 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) destroyed_bookmark = BookmarkManager.new(current_user).destroy(bookmark_id)
render json: success_json.merge(BookmarkManager.bookmark_metadata(destroyed_bookmark, current_user)) render json: success_json.merge(BookmarkManager.bookmark_metadata(destroyed_bookmark, current_user))

3
app/controllers/topics_controller.rb
View File

@ -608,10 +608,9 @@ class TopicsController < ApplicationController
def bookmark def bookmark
topic = Topic.find(params[:topic_id].to_i) topic = Topic.find(params[:topic_id].to_i)
first_post = topic.ordered_posts.first
bookmark_manager = BookmarkManager.new(current_user) 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? if bookmark_manager.errors.any?
return render_json_error(bookmark_manager, status: 400) return render_json_error(bookmark_manager, status: 400)

21
app/controllers/users_controller.rb
View File

@ -1725,20 +1725,13 @@ class UsersController < ApplicationController
end end
end end
format.ics do format.ics do
if SiteSetting.use_polymorphic_bookmarks @bookmark_reminders = Bookmark.with_reminders
@bookmark_reminders = Bookmark.with_reminders .where(user_id: user.id)
.where(user_id: user.id) .order(:reminder_at)
.order(:reminder_at) .map do |bookmark|
.map do |bookmark| bookmark.registered_bookmarkable.serializer.new(
bookmark.registered_bookmarkable.serializer.new( bookmark, scope: user_guardian, root: false
bookmark, scope: user_guardian, root: false )
)
end
else
@bookmark_reminders = Bookmark.with_reminders
.where(user_id: user.id)
.includes(:topic)
.order(:reminder_at)
end end
end end
end end

51
app/jobs/regular/export_user_archive.rb
View File

@ -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_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'], 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'], 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: ['bookmarkable_id', 'bookmarkable_type', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'],
bookmarks: ['post_id', 'topic_id', 'post_number', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'],
bookmarks_polymorphic: ['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'], 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'], 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'], likes: ['id', 'post_id', 'topic_id', 'post_number', 'created_at', 'updated_at', 'deleted_at', 'deleted_by'],
@ -50,16 +48,7 @@ module Jobs
components = [] components = []
COMPONENTS.each do |name| COMPONENTS.each do |name|
export_method = \ export_method = "#{name}_export"
if name == "bookmarks"
if SiteSetting.use_polymorphic_bookmarks
"bookmarks_polymorphic_export"
else
"bookmarks_export"
end
else
"#{name}_export"
end
h = { name: name, method: :"#{export_method}" } h = { name: name, method: :"#{export_method}" }
h[:filetype] = :csv h[:filetype] = :csv
filetype_method = :"#{name}_filetype" filetype_method = :"#{name}_filetype"
@ -240,8 +229,8 @@ module Jobs
end end
end end
def bookmarks_polymorphic_export def bookmarks_export
return enum_for(:bookmarks_polymorphic_export) unless block_given? return enum_for(:bookmarks_export) unless block_given?
@current_user.bookmarks.where.not(bookmarkable_type: nil).order(:id).each do |bookmark| @current_user.bookmarks.where.not(bookmarkable_type: nil).order(:id).each do |bookmark|
link = '' link = ''
@ -268,32 +257,6 @@ module Jobs
end end
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 def category_preferences_export
return enum_for(:category_preferences_export) unless block_given? return enum_for(:category_preferences_export) unless block_given?
@ -434,12 +397,6 @@ module Jobs
end end
end end
header_array.push("group_names") 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 else
header_array = HEADER_ATTRS_FOR[entity] header_array = HEADER_ATTRS_FOR[entity]
end end

33
app/jobs/regular/sync_topic_user_bookmarked.rb
View File

@ -5,8 +5,7 @@ module Jobs
def execute(args = {}) def execute(args = {})
topic_id = args[:topic_id] 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 UPDATE topic_users SET bookmarked = true
FROM bookmarks AS b FROM bookmarks AS b
INNER JOIN posts ON posts.id = b.bookmarkable_id AND b.bookmarkable_type = 'Post' 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 posts.deleted_at IS NULL AND
topic_users.topic_id = posts.topic_id AND topic_users.topic_id = posts.topic_id AND
topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} 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 UPDATE topic_users SET bookmarked = false
WHERE topic_users.bookmarked AND WHERE topic_users.bookmarked AND
( (
@ -27,31 +26,7 @@ module Jobs
AND bookmarks.user_id = topic_users.user_id AND bookmarks.user_id = topic_users.user_id
AND posts.deleted_at IS NULL AND posts.deleted_at IS NULL
) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
SQL 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
end end
end end
end end

99
app/models/bookmark.rb
View File

@ -34,16 +34,8 @@ class Bookmark < ActiveRecord::Base
end end
belongs_to :user belongs_to :user
belongs_to :post
has_one :topic, through: :post
belongs_to :bookmarkable, polymorphic: true 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 def self.auto_delete_preferences
@auto_delete_preferences ||= Enum.new( @auto_delete_preferences ||= Enum.new(
never: 0, never: 0,
@ -57,16 +49,6 @@ class Bookmark < ActiveRecord::Base
bookmarks_relation.select { |bm| bm.bookmarkable_type == type } bookmarks_relation.select { |bm| bm.bookmarkable_type == type }
end 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 :polymorphic_columns_present, on: [:create, :update]
validate :valid_bookmarkable_type, on: [:create, :update] validate :valid_bookmarkable_type, on: [:create, :update]
@ -85,41 +67,17 @@ class Bookmark < ActiveRecord::Base
end end
def polymorphic_columns_present def polymorphic_columns_present
return if !SiteSetting.use_polymorphic_bookmarks
return if self.bookmarkable_id.present? && self.bookmarkable_type.present? return if self.bookmarkable_id.present? && self.bookmarkable_type.present?
self.errors.add(:base, I18n.t("bookmarks.errors.bookmarkable_id_type_required")) self.errors.add(:base, I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
end end
def unique_per_bookmarkable 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) 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)) self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked", type: bookmarkable_type))
end 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 def ensure_sane_reminder_at_time
return if reminder_at.blank? return if reminder_at.blank?
if reminder_at < Time.zone.now if reminder_at < Time.zone.now
@ -145,27 +103,15 @@ class Bookmark < ActiveRecord::Base
end end
def valid_bookmarkable_type def valid_bookmarkable_type
return if !SiteSetting.use_polymorphic_bookmarks
return if Bookmark.valid_bookmarkable_types.include?(self.bookmarkable_type) return if Bookmark.valid_bookmarkable_types.include?(self.bookmarkable_type)
self.errors.add(:base, I18n.t("bookmarks.errors.invalid_bookmarkable", type: self.bookmarkable_type)) self.errors.add(:base, I18n.t("bookmarks.errors.invalid_bookmarkable", type: self.bookmarkable_type))
end 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? def auto_delete_when_reminder_sent?
self.auto_delete_preference == Bookmark.auto_delete_preferences[:when_reminder_sent] self.auto_delete_preference == Bookmark.auto_delete_preferences[:when_reminder_sent]
end 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? def auto_clear_reminder_when_reminder_sent?
self.auto_delete_preference == Bookmark.auto_delete_preferences[:clear_reminder] self.auto_delete_preference == Bookmark.auto_delete_preferences[:clear_reminder]
end end
@ -194,26 +140,16 @@ class Bookmark < ActiveRecord::Base
end end
scope :for_user_in_topic, ->(user_id, topic_id) { 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 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
.joins("LEFT JOIN topics ON topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic'") (topics.id = posts.topic_id)")
.where( .where(
"bookmarks.user_id = :user_id AND (topics.id = :topic_id OR posts.topic_id = :topic_id)", "bookmarks.user_id = :user_id AND (topics.id = :topic_id OR posts.topic_id = :topic_id)
user_id: user_id, topic_id: topic_id AND posts.deleted_at IS NULL AND topics.deleted_at IS NULL",
) user_id: user_id, topic_id: topic_id
else )
joins(:post).where(user_id: user_id, posts: { topic_id: topic_id })
end
} }
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) def self.count_per_day(opts = nil)
opts ||= {} opts ||= {}
result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago) result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago)
@ -223,15 +159,11 @@ class Bookmark < ActiveRecord::Base
end end
if opts[:category_id] if opts[:category_id]
if SiteSetting.use_polymorphic_bookmarks result = result
result = result .joins("LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
.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)")
.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")
.where("topics.deleted_at IS NULL AND posts.deleted_at IS NULL") .merge(Topic.in_category_and_subcategories(opts[:category_id]))
.merge(Topic.in_category_and_subcategories(opts[:category_id]))
else
result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id]))
end
end end
result.group('date(bookmarks.created_at)') result.group('date(bookmarks.created_at)')
@ -248,7 +180,10 @@ class Bookmark < ActiveRecord::Base
topics_deleted = DB.query(<<~SQL, grace_time: grace_time) topics_deleted = DB.query(<<~SQL, grace_time: grace_time)
DELETE FROM bookmarks b DELETE FROM bookmarks b
USING topics t, posts p 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) AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time)
RETURNING t.id AS topic_id RETURNING t.id AS topic_id
SQL SQL

5
app/models/post.rb
View File

@ -47,10 +47,7 @@ class Post < ActiveRecord::Base
has_one :post_stat has_one :post_stat
# TODO (martin) [POLYBOOK] has_many :bookmarks, as: :bookmarkable
# When we are ready we can add as: :bookmarkable here to use the
# polymorphic association.
has_many :bookmarks
has_one :incoming_email has_one :incoming_email

5
app/models/post_mover.rb
View File

@ -178,10 +178,7 @@ class PostMover
# we don't want to keep the old topic's OP bookmarked when we are # we don't want to keep the old topic's OP bookmarked when we are
# moving it into a new topic # moving it into a new topic
# Bookmark.where(bookmarkable: post).update_all(bookmarkable_id: new_post.id)
# 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)
new_post new_post
end end

14
app/serializers/post_serializer.rb
View File

@ -369,18 +369,10 @@ class PostSerializer < BasicPostSerializer
end end
def post_bookmark def post_bookmark
if SiteSetting.use_polymorphic_bookmarks if @topic_view.present?
if @topic_view.present? @post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.bookmarkable == object }
@post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.bookmarkable == object }
else
@post_bookmark ||= Bookmark.find_by(user: scope.user, bookmarkable: object)
end
else else
if @topic_view.present? @post_bookmark ||= Bookmark.find_by(user: scope.user, bookmarkable: object)
@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
end end
end end

8
app/serializers/user_bookmark_list_serializer.rb
View File

@ -4,12 +4,8 @@ class UserBookmarkListSerializer < ApplicationSerializer
attributes :more_bookmarks_url, :bookmarks attributes :more_bookmarks_url, :bookmarks
def bookmarks def bookmarks
if SiteSetting.use_polymorphic_bookmarks object.bookmarks.map do |bm|
object.bookmarks.map do |bm| bm.registered_bookmarkable.serializer.new(bm, scope: scope, root: false)
bm.registered_bookmarkable.serializer.new(bm, scope: scope, root: false)
end
else
object.bookmarks.map { |bm| UserBookmarkSerializer.new(bm, scope: scope, root: false) }
end end
end end

3
app/serializers/user_bookmark_serializer.rb
View File

@ -3,6 +3,9 @@
require_relative 'post_item_excerpt' require_relative 'post_item_excerpt'
# TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. # 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 class UserBookmarkSerializer < ApplicationSerializer
include PostItemExcerpt include PostItemExcerpt
include TopicTagsMixin include TopicTagsMixin

19
app/views/users/bookmarks.ics.erb
View File

@ -1,4 +1,3 @@
<% if SiteSetting.use_polymorphic_bookmarks %>
BEGIN:VCALENDAR BEGIN:VCALENDAR
VERSION:2.0 VERSION:2.0
PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_version %>//EN PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_version %>//EN
@ -14,21 +13,3 @@ URL:<%= bookmark.bookmarkable_url %>
END:VEVENT END:VEVENT
<% end %> <% end %>
END:VCALENDAR 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 %>

2
config/site_settings.yml
View File

@ -2403,7 +2403,7 @@ uncategorized:
use_polymorphic_bookmarks: use_polymorphic_bookmarks:
client: true client: true
default: false default: true
hidden: true hidden: true
suggest_weekends_in_date_pickers: suggest_weekends_in_date_pickers:

28
db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb Normal file
View File

@ -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

22
db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb Normal file
View File

@ -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

108
lib/bookmark_manager.rb
View File

@ -9,18 +9,37 @@ class BookmarkManager
end end
def self.bookmark_metadata(bookmark, user) def self.bookmark_metadata(bookmark, user)
if SiteSetting.use_polymorphic_bookmarks bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user)
bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user)
else
{ topic_bookmarked: Bookmark.for_user_in_topic(user.id, bookmark.topic.id).exists? }
end
end 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: {}) 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) bookmarkable = bookmarkable_type.constantize.find_by(id: bookmarkable_id)
registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type) registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type)
registered_bookmarkable.validate_before_create(@guardian, bookmarkable) registered_bookmarkable.validate_before_create(@guardian, bookmarkable)
@ -43,78 +62,12 @@ class BookmarkManager
bookmark bookmark
end 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) def destroy(bookmark_id)
bookmark = find_bookmark_and_check_access(bookmark_id) bookmark = find_bookmark_and_check_access(bookmark_id)
bookmark.destroy bookmark.destroy
if SiteSetting.use_polymorphic_bookmarks bookmark.registered_bookmarkable.after_destroy(@guardian, bookmark)
bookmark.registered_bookmarkable.after_destroy(@guardian, bookmark)
else
update_topic_user_bookmarked(bookmark.topic)
end
bookmark bookmark
end end
@ -134,8 +87,7 @@ class BookmarkManager
end end
def self.send_reminder_notification(id) def self.send_reminder_notification(id)
bookmark = Bookmark.find_by(id: id) BookmarkReminderNotificationHandler.new(Bookmark.find_by(id: id)).send_notification
BookmarkReminderNotificationHandler.new(bookmark).send_notification
end end
def update(bookmark_id:, name:, reminder_at:, options: {}) def update(bookmark_id:, name:, reminder_at:, options: {})

55
lib/bookmark_query.rb
View File

@ -10,9 +10,7 @@ class BookmarkQuery
end end
def self.preload(bookmarks, object) def self.preload(bookmarks, object)
if SiteSetting.use_polymorphic_bookmarks preload_polymorphic_associations(bookmarks)
preload_polymorphic_associations(bookmarks)
end
if @preload if @preload
@preload.each { |preload| preload.call(bookmarks, object) } @preload.each { |preload| preload.call(bookmarks, object) }
end end
@ -37,34 +35,6 @@ class BookmarkQuery
end end
def list_all 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] search_term = @params[:q]
ts_query = search_term.present? ? Search.ts_query(term: search_term) : nil ts_query = search_term.present? ? Search.ts_query(term: search_term) : nil
search_term_wildcard = search_term.present? ? "%#{search_term}%" : nil search_term_wildcard = search_term.present? ? "%#{search_term}%" : nil
@ -109,27 +79,4 @@ class BookmarkQuery
BookmarkQuery.preload(results, self) BookmarkQuery.preload(results, self)
results results
end 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 end

32
lib/bookmark_reminder_notification_handler.rb
View File

@ -10,13 +10,10 @@ class BookmarkReminderNotificationHandler
def send_notification def send_notification
return if bookmark.blank? return if bookmark.blank?
Bookmark.transaction do Bookmark.transaction do
# TODO (martin) [POLYBOOK] Can probably change this to call the if !bookmark.registered_bookmarkable.can_send_reminder?(bookmark)
# can_send_reminder? on the registered bookmarkable directly instead
# of having can_send_reminder?
if !can_send_reminder?
clear_reminder clear_reminder
else else
create_notification bookmark.registered_bookmarkable.send_reminder_notification(bookmark)
if bookmark.auto_delete_when_reminder_sent? if bookmark.auto_delete_when_reminder_sent?
BookmarkManager.new(bookmark.user).destroy(bookmark.id) BookmarkManager.new(bookmark.user).destroy(bookmark.id)
@ -40,29 +37,4 @@ class BookmarkReminderNotificationHandler
bookmark.clear_reminder! bookmark.clear_reminder!
end 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 end

6
lib/guardian/bookmark_guardian.rb
View File

@ -10,10 +10,6 @@ module BookmarkGuardian
end end
def can_see_bookmarkable?(bookmark) def can_see_bookmarkable?(bookmark)
if SiteSetting.use_polymorphic_bookmarks? bookmark.registered_bookmarkable.can_see?(self, bookmark)
return bookmark.registered_bookmarkable.can_see?(self, bookmark)
end
self.can_see_post?(bookmark.post)
end end
end end

16
lib/search.rb
View File

@ -459,16 +459,12 @@ class Search
# search based on a RegisteredBookmarkable's #search_query method. # search based on a RegisteredBookmarkable's #search_query method.
advanced_filter(/^in:(bookmarks)$/i) do |posts, match| advanced_filter(/^in:(bookmarks)$/i) do |posts, match|
if @guardian.user if @guardian.user
if SiteSetting.use_polymorphic_bookmarks posts.where(<<~SQL)
posts.where(<<~SQL) posts.id IN (
posts.id IN ( SELECT bookmarkable_id FROM bookmarks
SELECT bookmarkable_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id} AND bookmarks.bookmarkable_type = 'Post'
WHERE bookmarks.user_id = #{@guardian.user.id} AND bookmarks.bookmarkable_type = 'Post' )
) SQL
SQL
else
posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})")
end
end end
end end

12
lib/topic_view.rb
View File

@ -413,15 +413,9 @@ class TopicView
end end
def bookmarks def bookmarks
if SiteSetting.use_polymorphic_bookmarks @bookmarks ||= Bookmark.for_user_in_topic(@user, @topic.id).select(
@bookmarks ||= Bookmark.for_user_in_topic(@user, @topic.id).select( :id, :bookmarkable_id, :bookmarkable_type, :reminder_at, :name, :auto_delete_preference
: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
end end
MAX_PARTICIPANTS = 24 MAX_PARTICIPANTS = 24

4
plugins/discourse-narrative-bot/plugin.rb
View File

@ -281,10 +281,8 @@ after_initialize do
self.add_model_callback(Bookmark, :after_commit, on: :create) do self.add_model_callback(Bookmark, :after_commit, on: :create) do
if self.user.enqueue_narrative_bot_job? 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") 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 end
end end

7
plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb
View File

@ -249,13 +249,6 @@ describe DiscourseNarrativeBot::NewUserNarrative do
end end
it 'adds an after commit model callback to bookmark' do 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! Jobs.run_later!
bookmark = Fabricate(:bookmark, bookmarkable: Fabricate(:post)) 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" }) expect_job_enqueued(job: :bot_input, args: { user_id: bookmark.user_id, post_id: bookmark.bookmarkable_id, input: "bookmark" })

7
script/import_scripts/base.rb
View File

@ -624,12 +624,7 @@ class ImportScripts::Base
else else
begin begin
manager = BookmarkManager.new(user) manager = BookmarkManager.new(user)
bookmark = manager.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post")
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
created += 1 if manager.errors.none? created += 1 if manager.errors.none?
skipped += 1 if manager.errors.any? skipped += 1 if manager.errors.any?

20
spec/fabricators/bookmark_fabricator.rb
View File

@ -2,28 +2,10 @@
Fabricator(:bookmark) do Fabricator(:bookmark) do
user user
post {
if !SiteSetting.use_polymorphic_bookmarks
Fabricate(:post)
end
}
name "This looked interesting" name "This looked interesting"
reminder_at { 1.day.from_now.iso8601 } reminder_at { 1.day.from_now.iso8601 }
reminder_set_at { Time.zone.now } reminder_set_at { Time.zone.now }
bookmarkable { bookmarkable { Fabricate(:post) }
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
end end
Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do

6
spec/jobs/bookmark_reminder_notifications_spec.rb
View File

@ -67,9 +67,9 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
end end
it 'will not send notification when topic is not available' do it 'will not send notification when topic is not available' do
bookmark1.topic.destroy bookmark1.bookmarkable.topic.destroy
bookmark2.topic.destroy bookmark2.bookmarkable.topic.destroy
bookmark3.topic.destroy bookmark3.bookmarkable.topic.destroy
expect { subject.execute }.not_to change { Notification.where(notification_type: Notification.types[:bookmark_reminder]).count } expect { subject.execute }.not_to change { Notification.where(notification_type: Notification.types[:bookmark_reminder]).count }
end end
end end

65
spec/jobs/export_user_archive_spec.rb
View File

@ -280,20 +280,20 @@ describe Jobs::ExportUserArchive do
let(:post4) { Fabricate(:post, topic: private_message_topic) } let(:post4) { Fabricate(:post, topic: private_message_topic) }
let(:reminder_at) { 1.day.from_now } 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' 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 update1_at = now + 1.hours
bookmark1.update(name: 'great food recipe', updated_at: update1_at) 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 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 freeze_time now
tau_record = private_message_topic.topic_allowed_users.create!(user_id: user.id) 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! tau_record.destroy!
BookmarkReminderNotificationHandler.new(pending_reminder).send_notification BookmarkReminderNotificationHandler.new(pending_reminder).send_notification
@ -302,9 +302,8 @@ describe Jobs::ExportUserArchive do
expect(data.length).to eq(4) expect(data.length).to eq(4)
expect(data[0]['post_id']).to eq(post1.id.to_s) expect(data[0]['bookmarkable_id']).to eq(post1.id.to_s)
expect(data[0]['topic_id']).to eq(topic1.id.to_s) expect(data[0]['bookmarkable_type']).to eq("Post")
expect(data[0]['post_number']).to eq('5')
expect(data[0]['link']).to eq(post1.full_url) 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(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(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[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('') expect(data[3]['link']).to eq('')
end 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 end
context 'category_preferences' do context 'category_preferences' do

56
spec/jobs/sync_topic_user_bookmarked_spec.rb
View File

@ -13,8 +13,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
fab!(:tu5) { Fabricate(:topic_user, topic: topic, bookmarked: true) } fab!(:tu5) { Fabricate(:topic_user, topic: topic, bookmarked: true) }
it "corrects all topic_users.bookmarked records for the topic" do it "corrects all topic_users.bookmarked records for the topic" do
Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample) Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
subject.execute(topic_id: topic.id) subject.execute(topic_id: topic.id)
@ -26,8 +26,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
end end
it "does not consider topic as bookmarked if the bookmarked post is deleted" do it "does not consider topic as bookmarked if the bookmarked post is deleted" do
Fabricate(:bookmark, user: tu1.user, post: post1) Fabricate(:bookmark, user: tu1.user, bookmarkable: post1)
Fabricate(:bookmark, user: tu2.user, post: post1) Fabricate(:bookmark, user: tu2.user, bookmarkable: post1)
post1.trash! post1.trash!
@ -38,8 +38,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
end end
it "works when no topic id is provided (runs for all topics)" do 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: tu1.user, bookmarkable: topic.posts.sample)
Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
subject.execute subject.execute
@ -49,48 +49,4 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
expect(tu4.reload.bookmarked).to eq(true) expect(tu4.reload.bookmarked).to eq(true)
expect(tu5.reload.bookmarked).to eq(false) expect(tu5.reload.bookmarked).to eq(false)
end 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 end

200
spec/lib/bookmark_manager_spec.rb
View File

@ -9,175 +9,24 @@ RSpec.describe BookmarkManager do
subject { described_class.new(user) } 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 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 it "deletes the existing bookmark" do
subject.destroy(bookmark.id) subject.destroy(bookmark.id)
expect(Bookmark.exists?(id: bookmark.id)).to eq(false) expect(Bookmark.exists?(id: bookmark.id)).to eq(false)
end end
# TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
context "if the bookmark is the last one bookmarked in the topic" do context "if the bookmark is the last one bookmarked in the topic" do
it "marks the topic user bookmarked column as false" 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) subject.destroy(bookmark.id)
tu = TopicUser.find_by(user: user) tu = TopicUser.find_by(user: user)
expect(tu.bookmarked).to eq(false) expect(tu.bookmarked).to eq(false)
end end
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 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 it "raises an invalid access error" do
expect { subject.destroy(bookmark.id) }.to raise_error(Discourse::InvalidAccess) expect { subject.destroy(bookmark.id) }.to raise_error(Discourse::InvalidAccess)
end end
@ -191,7 +40,7 @@ RSpec.describe BookmarkManager do
end end
describe ".update" do 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_name) { "Some new name" }
let(:new_reminder_at) { 10.days.from_now } let(:new_reminder_at) { 10.days.from_now }
let(:options) { {} } let(:options) { {} }
@ -230,7 +79,7 @@ RSpec.describe BookmarkManager do
end end
context "if the bookmark is belonging to some other user" do 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 it "raises an invalid access error" do
expect { update_bookmark }.to raise_error(Discourse::InvalidAccess) expect { update_bookmark }.to raise_error(Discourse::InvalidAccess)
end end
@ -247,35 +96,6 @@ RSpec.describe BookmarkManager do
end end
describe ".destroy_for_topic" do 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!(:topic) { Fabricate(:topic) }
let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) } let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
let!(:bookmark2) { 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 it "creates a notification for the reminder" do
described_class.send_reminder_notification(bookmark.id) described_class.send_reminder_notification(bookmark.id)
notif = notifications_for_user.last 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 end
context "when the bookmark does no longer exist" do 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 context "if the post has been deleted" do
before do before do
bookmark.post.trash! bookmark.bookmarkable.trash!
end end
it "does not error and does not create a notification" do it "does not error and does not create a notification" do
described_class.send_reminder_notification(bookmark.id) described_class.send_reminder_notification(bookmark.id)
@ -373,11 +193,7 @@ RSpec.describe BookmarkManager do
end end
end end
describe "#create_for (use_polymorphic_bookmarks)" do describe "#create_for" do
before do
SiteSetting.use_polymorphic_bookmarks = true
end
it "allows creating a bookmark for the topic and for the first post" 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) subject.create_for(bookmarkable_id: post.topic_id, bookmarkable_type: "Topic", name: name)
bookmark = Bookmark.find_by(user: user, bookmarkable: post.topic) bookmark = Bookmark.find_by(user: user, bookmarkable: post.topic)

215
spec/lib/bookmark_query_spec.rb
View File

@ -13,22 +13,30 @@ RSpec.describe BookmarkQuery do
end end
describe "#list_all" do describe "#list_all" do
fab!(:bookmark1) { Fabricate(:bookmark, user: user) } before do
fab!(:bookmark2) { Fabricate(:bookmark, user: user) } Bookmark.reset_bookmarkables
let!(:topic_user1) { Fabricate(:topic_user, topic: bookmark1.topic, user: user) } register_test_bookmarkable
let!(:topic_user2) { Fabricate(:topic_user, topic: bookmark2.topic, user: user) }
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 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 end
it "does not return deleted posts" do it "does not return deleted bookmarkables" do
bookmark1.post.trash! post_bookmark.bookmarkable.trash!
expect(bookmark_query.list_all.count).to eq(1) topic_bookmark.bookmarkable.trash!
end
it "does not return deleted topics" do
bookmark1.topic.trash!
expect(bookmark_query.list_all.count).to eq(1) expect(bookmark_query.list_all.count).to eq(1)
end end
@ -41,149 +49,92 @@ RSpec.describe BookmarkQuery do
expect(preloaded_bookmarks.any?).to eq(true) expect(preloaded_bookmarks.any?).to eq(true)
end end
it "does not query topic_users for the bookmark topic that are not the current user" do it "returns a mixture of post, topic, and custom bookmarkable type bookmarks" do
topic_user3 = Fabricate(:topic_user, topic: bookmark1.topic) bookmarks = bookmark_query.list_all
bookmark = bookmark_query.list_all.find do |b| expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
b.topic_id == bookmark1.topic_id
end
expect(bookmark.topic.topic_users.map(&:user_id)).to contain_exactly(user.id)
end end
context "for polymorphic bookmarks" do it "handles the user not having permission for all of the bookmarks of a certain bookmarkable" do
before do UserTestBookmarkable.expects(:list_query).returns(nil)
SiteSetting.use_polymorphic_bookmarks = true bookmarks = bookmark_query.list_all
Bookmark.reset_bookmarkables expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id])
register_test_bookmarkable end
Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) it "handles the user not having permission to see any of their bookmarks" do
Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) topic_bookmark.bookmarkable.update(category: Fabricate(:private_category, group: Fabricate(:group)))
user_bookmark post_bookmark.bookmarkable.topic.update(category: topic_bookmark.bookmarkable.category)
end UserTestBookmarkable.expects(:list_query).returns(nil)
bookmarks = bookmark_query.list_all
let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) } expect(bookmarks.map(&:id)).to eq([])
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
end end
context "when q param is provided" do context "when q param is provided" do
let!(:post) { Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs")) } let!(:post) { Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs")) }
context "when not using polymorphic bookmarks" do before do
let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later") } Bookmark.reset_bookmarkables
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
end 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 before do
SiteSetting.use_polymorphic_bookmarks = true register_test_bookmarkable
Bookmark.reset_bookmarkables
end end
after do let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkking")) }
Bookmark.reset_bookmarkables
end
let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later", bookmarkable: Fabricate(:post)) } it "allows searching bookmarkables by fields in other tables" do
let(:bookmark4) { Fabricate(:bookmark, user: user, bookmarkable: post) } bookmarks = bookmark_query(params: { q: 'bookmarkk' }).list_all
expect(bookmarks.map(&:id)).to eq([bookmark5.id])
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
end end
end end
end end
context "for a whispered post" do context "for a whispered post" do
before do before do
bookmark1.post.update(post_type: Post.types[:whisper]) post_bookmark.bookmarkable.update(post_type: Post.types[:whisper])
end end
context "when the user is moderator" do context "when the user is moderator" do
it "does return the whispered post" do it "does return the whispered post" do
user.update!(moderator: true) user.update!(moderator: true)
expect(bookmark_query.list_all.count).to eq(2) expect(bookmark_query.list_all.count).to eq(3)
end end
end end
context "when the user is admin" do context "when the user is admin" do
it "does return the whispered post" do it "does return the whispered post" do
user.update!(admin: true) user.update!(admin: true)
expect(bookmark_query.list_all.count).to eq(2) expect(bookmark_query.list_all.count).to eq(3)
end end
end end
context "when the user is not staff" do context "when the user is not staff" do
it "does not return the whispered post" 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 end
end end
@ -191,7 +142,7 @@ RSpec.describe BookmarkQuery do
context "for a private message topic bookmark" do context "for a private message topic bookmark" do
let(:pm_topic) { Fabricate(:private_message_topic) } let(:pm_topic) { Fabricate(:private_message_topic) }
before do 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) TopicUser.change(user.id, pm_topic.id, total_msecs_viewed: 1)
end end
@ -200,7 +151,7 @@ RSpec.describe BookmarkQuery do
TopicAllowedUser.create(topic: pm_topic, user: user) TopicAllowedUser.create(topic: pm_topic, user: user)
end end
it "shows the user the bookmark in the PM" do 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
end end
@ -211,19 +162,19 @@ RSpec.describe BookmarkQuery do
TopicAllowedGroup.create(topic: pm_topic, group: group) TopicAllowedGroup.create(topic: pm_topic, group: group)
end end
it "shows the user the bookmark in the PM" do 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
end end
context "when the user is not a topic_allowed_user" do 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 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
end end
context "when the user is not in a topic_allowed_group" do 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 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 end
end end
@ -231,15 +182,15 @@ RSpec.describe BookmarkQuery do
context "when the topic category is private" do context "when the topic category is private" do
let(:group) { Fabricate(:group) } let(:group) { Fabricate(:group) }
before do before do
bookmark1.topic.update(category: Fabricate(:private_category, group: group)) post_bookmark.bookmarkable.topic.update(category: Fabricate(:private_category, group: group))
bookmark1.reload post_bookmark.reload
end end
it "does not show the user a post/topic in a private category they cannot see" do 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 end
it "does show the user a post/topic in a private category they can see" do it "does show the user a post/topic in a private category they can see" do
GroupUser.create(user: user, group: group) 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
end end
@ -260,7 +211,7 @@ RSpec.describe BookmarkQuery do
before do before do
[bookmark1, bookmark2, bookmark3, bookmark4, bookmark5].each do |bm| [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 bm.reload
end end
end end

79
spec/lib/bookmark_reminder_notification_handler_spec.rb
View File

@ -11,24 +11,25 @@ RSpec.describe BookmarkReminderNotificationHandler do
describe "#send_notification" do describe "#send_notification" do
let!(:bookmark) do let!(:bookmark) do
Fabricate(:bookmark_next_business_day_reminder, user: user) Fabricate(:bookmark_next_business_day_reminder, user: user, bookmarkable: Fabricate(:post))
end end
it "creates a bookmark reminder notification with the correct details" do it "creates a bookmark reminder notification with the correct details" do
subject.new(bookmark).send_notification subject.new(bookmark).send_notification
notif = bookmark.user.notifications.last notif = bookmark.user.notifications.last
expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder]) expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
expect(notif.topic_id).to eq(bookmark.topic_id) expect(notif.topic_id).to eq(bookmark.bookmarkable.topic_id)
expect(notif.post_number).to eq(bookmark.post.post_number) expect(notif.post_number).to eq(bookmark.bookmarkable.post_number)
data = JSON.parse(notif.data) 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["display_username"]).to eq(bookmark.user.username)
expect(data["bookmark_name"]).to eq(bookmark.name) expect(data["bookmark_name"]).to eq(bookmark.name)
expect(data["bookmarkable_url"]).to eq(bookmark.bookmarkable.url)
end end
context "when the topic is deleted" do context "when the bookmarkable is deleted" do
before do before do
bookmark.topic.trash! bookmark.bookmarkable.trash!
bookmark.reload bookmark.reload
end end
@ -38,56 +39,9 @@ RSpec.describe BookmarkReminderNotificationHandler do
end end
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 context "when the auto_delete_preference is when_reminder_sent" do
before 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]) bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent])
end end
@ -98,24 +52,24 @@ RSpec.describe BookmarkReminderNotificationHandler do
it "changes the TopicUser bookmarked column to false" do it "changes the TopicUser bookmarked column to false" do
subject.new(bookmark).send_notification 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 end
context "if there are still other bookmarks in the topic" do context "if there are still other bookmarks in the topic" do
before 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 end
it "does not change the TopicUser bookmarked column to false" do it "does not change the TopicUser bookmarked column to false" do
subject.new(bookmark).send_notification 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 end
end end
context "when the auto_delete_preference is clear_reminder" do context "when the auto_delete_preference is clear_reminder" do
before 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]) bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:clear_reminder])
end end
@ -124,14 +78,5 @@ RSpec.describe BookmarkReminderNotificationHandler do
expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil) expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil)
end end
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
end end

6
spec/lib/post_creator_spec.rb
View File

@ -807,13 +807,13 @@ describe PostCreator do
context "when the user has bookmarks with auto_delete_preference on_owner_reply" do context "when the user has bookmarks with auto_delete_preference on_owner_reply" do
before 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, bookmarkable: 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])
TopicUser.create!(topic: topic, user: user, bookmarked: true) TopicUser.create!(topic: topic, user: user, bookmarked: true)
end end
it "deletes the bookmarks, but not the ones without an auto_delete_preference" do 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) Fabricate(:bookmark, user: user)
creator.create creator.create
expect(Bookmark.where(user: user).count).to eq(2) expect(Bookmark.where(user: user).count).to eq(2)

14
spec/lib/search_spec.rb
View File

@ -1482,21 +1482,9 @@ describe Search do
it "can filter by posts in the user's bookmarks" do it "can filter by posts in the user's bookmarks" do
expect(search_with_bookmarks.posts.map(&:id)).to eq([]) 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]) expect(search_with_bookmarks.posts.map(&:id)).to match_array([bookmark_post1.id])
end 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 end
it 'supports pinned' do it 'supports pinned' do

19
spec/lib/topic_view_spec.rb
View File

@ -396,9 +396,9 @@ RSpec.describe TopicView do
context "#bookmarks" do context "#bookmarks" do
let!(:user) { Fabricate(:user) } let!(:user) { Fabricate(:user) }
let!(:bookmark1) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) } let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
let!(:bookmark2) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) } let!(:bookmark2) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
let!(:bookmark3) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic)) } let!(:bookmark3) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic)) }
it "returns all the bookmarks in the topic for a user" do it "returns all the bookmarks in the topic for a user" do
expect(TopicView.new(topic.id, user).bookmarks.pluck(:id)).to match_array( expect(TopicView.new(topic.id, user).bookmarks.pluck(:id)).to match_array(
@ -413,25 +413,26 @@ RSpec.describe TopicView do
context "#bookmarks" do context "#bookmarks" do
let!(:user) { Fabricate(:user) } let!(:user) { Fabricate(:user) }
let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, post: topic.first_post, user: user) } let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, bookmarkable: topic.first_post, user: user) }
let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, post: topic.posts[1], 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 it "gets the first post bookmark reminder at for the user" do
topic_view = TopicView.new(topic.id, user) topic_view = TopicView.new(topic.id, user)
first, second = topic_view.bookmarks.sort_by(&:id) 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(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) expect(second[:reminder_at]).to eq_time(bookmark2.reminder_at)
end end
context "when the topic is deleted" do context "when the topic is deleted" do
it "returns []" do it "returns []" do
topic_view = TopicView.new(topic, user) topic_view = TopicView.new(topic, user)
expect(topic_view.bookmarks).to match_array([bookmark1, bookmark2])
PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
topic.reload topic.reload
topic_view.instance_variable_set(:@bookmarks, nil)
expect(topic_view.bookmarks).to eq([]) expect(topic_view.bookmarks).to eq([])
end end
end end
@ -444,7 +445,7 @@ RSpec.describe TopicView do
expect(topic_view.bookmarks.length).to eq(1) expect(topic_view.bookmarks.length).to eq(1)
first = topic_view.bookmarks.first 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) expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
end end
end end

252
spec/models/bookmark_spec.rb
View File

@ -4,95 +4,36 @@ describe Bookmark do
fab!(:post) { Fabricate(:post) } fab!(:post) { Fabricate(:post) }
context "validations" do context "validations" do
it "does not allow user to bookmark a post twice, enforces unique bookmark per post, user, and for_topic" do it "does not allow a user to create a bookmark with only one polymorphic column" 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
user = Fabricate(:user) user = Fabricate(:user)
post.update!(post_number: 1) bm = Bookmark.create(bookmarkable_id: post.id, user: user)
bookmark = Fabricate(:bookmark, user: user) expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
bookmark_2 = Fabricate(:bookmark, user: user, post: post, for_topic: true) bm = Bookmark.create(bookmarkable_type: "Post", user: user)
expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(bookmark_2) expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
bookmark_2.update!(for_topic: false) bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(nil) 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 end
describe "#cleanup!" do describe "#cleanup!" do
it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do
bookmark = Fabricate(:bookmark, post: post) bookmark = Fabricate(:bookmark, bookmarkable: post)
bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic))
post.trash! post.trash!
post.update(deleted_at: 4.days.ago) post.update(deleted_at: 4.days.ago)
Bookmark.cleanup! 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 it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do
post2 = Fabricate(:post) post2 = Fabricate(:post)
bookmark = Fabricate(:bookmark, post: post) bookmark = Fabricate(:bookmark, bookmarkable: post)
bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic))
bookmark3 = Fabricate(:bookmark, post: post2) bookmark3 = Fabricate(:bookmark, bookmarkable: post2)
bookmark4 = Fabricate(:bookmark, post: post2) bookmark4 = Fabricate(:bookmark, bookmarkable: post2)
post.trash! post.trash!
post.update(deleted_at: 4.days.ago) post.update(deleted_at: 4.days.ago)
post2.trash! post2.trash!
@ -120,8 +61,8 @@ describe Bookmark do
end end
it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do
bookmark = Fabricate(:bookmark, post: post) bookmark = Fabricate(:bookmark, bookmarkable: post)
bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic))
bookmark3 = Fabricate(:bookmark) bookmark3 = Fabricate(:bookmark)
post.topic.trash! post.topic.trash!
post.topic.update(deleted_at: 4.days.ago) post.topic.update(deleted_at: 4.days.ago)
@ -132,7 +73,7 @@ describe Bookmark do
end end
it "does not delete bookmarks attached to posts that are not deleted or that have not met the 3 day grace period" do 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) bookmark2 = Fabricate(:bookmark)
Bookmark.cleanup! Bookmark.cleanup!
expect(Bookmark.find(bookmark.id)).to eq(bookmark) expect(Bookmark.find(bookmark.id)).to eq(bookmark)
@ -146,106 +87,51 @@ describe Bookmark do
let(:category) { Fabricate(:category) } let(:category) { Fabricate(:category) }
let(:topic_in_category) { Fabricate(:topic, category: 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!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) } let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) }
let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) } let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.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!(: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!(: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 it "gets the count of bookmarks grouped by date within the last 30 days by default" do
expect(Bookmark.count_per_day).to eq({ expect(Bookmark.count_per_day).to eq({
1.day.ago.to_date => 1, 1.day.ago.to_date => 1,
2.days.ago.to_date => 1, 2.days.ago.to_date => 1,
3.days.ago.to_date => 2 3.days.ago.to_date => 2
}) })
end
it "respects the start_date option" do
expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({
1.day.ago.to_date => 1,
})
end
it "respects the since_days_ago option" do
expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({
1.day.ago.to_date => 1,
})
end
it "respects the end_date option" do
expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({
2.days.ago.to_date => 1,
3.days.ago.to_date => 2,
})
end
it "respects the category_id option" do
expect(Bookmark.count_per_day(category_id: category.id)).to eq({
3.days.ago.to_date => 1,
})
end
it "does not include deleted posts or topics" do
bookmark4.post.trash!
expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
bookmark4.post.recover!
bookmark4.topic.trash!
expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
end
end end
context "for polymorphic bookmarks" do it "respects the start_date option" do
before do expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({
SiteSetting.use_polymorphic_bookmarks = true 1.day.ago.to_date => 1,
end })
end
let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) } it "respects the since_days_ago option" do
let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) } expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({
let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) } 1.day.ago.to_date => 1,
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) } end
it "gets the count of bookmarks grouped by date within the last 30 days by default" do it "respects the end_date option" do
expect(Bookmark.count_per_day).to eq({ expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({
1.day.ago.to_date => 1, 2.days.ago.to_date => 1,
2.days.ago.to_date => 1, 3.days.ago.to_date => 2,
3.days.ago.to_date => 2 })
}) end
end
it "respects the start_date option" do it "respects the category_id option" do
expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({ expect(Bookmark.count_per_day(category_id: category.id)).to eq({
1.day.ago.to_date => 1, 3.days.ago.to_date => 1,
}) })
end end
it "respects the since_days_ago option" do it "does not include deleted posts or topics" do
expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({ bookmark4.bookmarkable.trash!
1.day.ago.to_date => 1, expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
}) bookmark4.bookmarkable.recover!
end bookmark4.bookmarkable.topic.trash!
expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
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
end end
end end

36
spec/models/post_mover_spec.rb
View File

@ -392,30 +392,16 @@ describe PostMover do
.to contain_exactly([1, 500], [2, 750]) .to contain_exactly([1, 500], [2, 750])
end 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 it "makes sure the topic_user.bookmarked value is reflected for users in the source and destination topic" do
Jobs.run_immediately! Jobs.run_immediately!
user1 = Fabricate(:user) user1 = Fabricate(:user)
user2 = Fabricate(:user) user2 = Fabricate(:user)
bookmark1 = Fabricate(:bookmark, post: p1, user: user1) bookmark1 = Fabricate(:bookmark, bookmarkable: p1, user: user1)
bookmark2 = Fabricate(:bookmark, post: p4, user: user1) bookmark2 = Fabricate(:bookmark, bookmarkable: p4, user: user1)
bookmark3 = Fabricate(:bookmark, post: p3, user: user2) bookmark3 = Fabricate(:bookmark, bookmarkable: p3, user: user2)
bookmark4 = Fabricate(:bookmark, post: p4, user: user2) bookmark4 = Fabricate(:bookmark, bookmarkable: p4, user: user2)
tu1 = Fabricate( tu1 = Fabricate(
:topic_user, :topic_user,
@ -724,20 +710,6 @@ describe PostMover do
expect(Notification.exists?(admin_notification.id)).to eq(true) expect(Notification.exists?(admin_notification.id)).to eq(true)
end 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 context "post timings" do
fab!(:some_user) { Fabricate(:user) } fab!(:some_user) { Fabricate(:user) }

56
spec/models/user_bookmark_list_spec.rb
View File

@ -5,44 +5,36 @@ RSpec.describe UserBookmarkList do
fab!(:user) { Fabricate(:user) } fab!(:user) { Fabricate(:user) }
let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: params) } let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: params) }
context "for non-polymorphic bookmarks" do before do
before do register_test_bookmarkable
22.times do
bookmark = Fabricate(:bookmark, user: user)
Fabricate(:topic_user, topic: bookmark.topic, user: user)
end
end
it "defaults to 20 per page" do Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
expect(list.per_page).to eq(20) Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
end user_bookmark
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
end end
context "for polymorphic bookmarks" do let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
before do let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
SiteSetting.use_polymorphic_bookmarks = true let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) }
register_test_bookmarkable
Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) it "returns all types of bookmarks" do
Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) list.load
user_bookmark expect(list.bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
end end
let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) } it "defaults to 20 per page" do
let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) } expect(list.per_page).to eq(20)
let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) } end
it "returns all types of bookmarks" do context "when the per_page param is too high" do
list.load let(:params) { { per_page: 1000 } }
expect(list.bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
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 end
end end

3
spec/requests/admin/users_controller_spec.rb
View File

@ -308,7 +308,8 @@ RSpec.describe Admin::UsersController do
it "also prevents use of any api keys" do it "also prevents use of any api keys" do
api_key = Fabricate(:api_key, user: user) api_key = Fabricate(:api_key, user: user)
post "/bookmarks.json", params: { post "/bookmarks.json", params: {
post_id: Fabricate(:post).id bookmarkable_id: Fabricate(:post).id,
bookmarkable_type: "Post"
}, headers: { HTTP_API_KEY: api_key.key } }, headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200) expect(response.status).to eq(200)

91
spec/requests/bookmarks_controller_spec.rb
View File

@ -17,45 +17,20 @@ describe BookmarksController do
RateLimiter.clear_all! RateLimiter.clear_all!
post "/bookmarks.json", params: { 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 reminder_at: (Time.zone.now + 1.day).iso8601
} }
expect(response.status).to eq(200) expect(response.status).to eq(200)
post "/bookmarks.json", params: { post "/bookmarks.json", params: {
post_id: Fabricate(:post).id bookmarkable_id: bookmark_post.id,
bookmarkable_type: "Post",
} }
expect(response.status).to eq(429) expect(response.status).to eq(429)
end 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 context "if the user reached the max bookmark limit" do
before do before do
SiteSetting.max_bookmarks_per_user = 1 SiteSetting.max_bookmarks_per_user = 1
@ -63,11 +38,13 @@ describe BookmarksController do
it "returns failed JSON with a 400 error" do it "returns failed JSON with a 400 error" do
post "/bookmarks.json", params: { 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 reminder_at: (Time.zone.now + 1.day).iso8601
} }
post "/bookmarks.json", params: { post "/bookmarks.json", params: {
post_id: Fabricate(:post).id bookmarkable_id: bookmark_post.id,
bookmarkable_type: "Post",
} }
expect(response.status).to eq(400) expect(response.status).to eq(400)
@ -78,27 +55,8 @@ describe BookmarksController do
end end
end end
context "if the user already has bookmarked the post" do context "if the user already has bookmarked the record" do
before 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_post, user: bookmark_user)
Fabricate(:bookmark, bookmarkable: bookmark_topic, user: bookmark_user) Fabricate(:bookmark, bookmarkable: bookmark_topic, user: bookmark_user)
end end
@ -130,7 +88,7 @@ describe BookmarksController do
end end
describe "#destroy" do 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 it "destroys the bookmark" do
delete "/bookmarks/#{bookmark.id}.json" delete "/bookmarks/#{bookmark.id}.json"
@ -141,33 +99,14 @@ describe BookmarksController do
delete "/bookmarks/#{bookmark.id}.json" delete "/bookmarks/#{bookmark.id}.json"
expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
expect(response.parsed_body["topic_bookmarked"]).to eq(false) expect(response.parsed_body["topic_bookmarked"]).to eq(false)
bm2 = 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))
Fabricate(:bookmark, user: bookmark_user, post: Fabricate(:post, topic: bookmark_post.topic)) bm3 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: bookmark_post.topic)
delete "/bookmarks/#{bm2.id}.json" delete "/bookmarks/#{bm2.id}.json"
expect(Bookmark.find_by(id: bm2.id)).to eq(nil) expect(Bookmark.find_by(id: bm2.id)).to eq(nil)
expect(response.parsed_body["topic_bookmarked"]).to eq(true) expect(response.parsed_body["topic_bookmarked"]).to eq(true)
end delete "/bookmarks/#{bm3.id}.json"
expect(Bookmark.find_by(id: bm3.id)).to eq(nil)
context "for polymorphic bookmarks" do expect(response.parsed_body["topic_bookmarked"]).to eq(false)
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
end end
context "if the bookmark has already been destroyed" do context "if the bookmark has already been destroyed" do

4
spec/requests/posts_controller_spec.rb
View File

@ -649,7 +649,7 @@ describe PostsController do
describe "#destroy_bookmark" do describe "#destroy_bookmark" do
fab!(:post) { Fabricate(:post) } fab!(:post) { Fabricate(:post) }
fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post) } fab!(:bookmark) { Fabricate(:bookmark, user: user, bookmarkable: post) }
before do before do
sign_in(user) sign_in(user)
@ -663,7 +663,7 @@ describe PostsController do
context "when the user still has bookmarks in the topic" do context "when the user still has bookmarks in the topic" do
before do before do
Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic)) Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post, topic: post.topic))
end end
it "marks topic_bookmarked as true" do it "marks topic_bookmarked as true" do
delete "/posts/#{post.id}/bookmark.json" delete "/posts/#{post.id}/bookmark.json"

33
spec/requests/topics_controller_spec.rb
View File

@ -3273,8 +3273,8 @@ RSpec.describe TopicsController do
post = create_post post = create_post
post2 = create_post(topic_id: post.topic_id) post2 = create_post(topic_id: post.topic_id)
Fabricate(:bookmark, user: user, post: post) Fabricate(:bookmark, user: user, bookmarkable: post)
Fabricate(:bookmark, user: user, post: post2) Fabricate(:bookmark, user: user, bookmarkable: post2)
put "/t/#{post.topic_id}/remove_bookmarks.json" put "/t/#{post.topic_id}/remove_bookmarks.json"
expect(Bookmark.where(user: user).count).to eq(0) 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 it "deletes all the bookmarks for the user in the topic" do
sign_in(user) sign_in(user)
post = create_post post = create_post
Fabricate(:bookmark, post: post, user: user) Fabricate(:bookmark, bookmarkable: post, user: user)
put "/t/#{post.topic_id}/remove_bookmarks.json" put "/t/#{post.topic_id}/remove_bookmarks.json"
expect(Bookmark.for_user_in_topic(user.id, post.topic_id).count).to eq(0) expect(Bookmark.for_user_in_topic(user.id, post.topic_id).count).to eq(0)
end end
@ -3304,42 +3304,21 @@ RSpec.describe TopicsController do
sign_in(user) sign_in(user)
end 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 post = create_post
post2 = create_post(topic_id: post.topic_id) post2 = create_post(topic_id: post.topic_id)
put "/t/#{post.topic_id}/bookmark.json" 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 end
it "errors if the topic is already bookmarked for the user" do it "errors if the topic is already bookmarked for the user" do
post = create_post post = create_post
Bookmark.create(post: post, user: user) Bookmark.create(bookmarkable: post.topic, user: user)
put "/t/#{post.topic_id}/bookmark.json" put "/t/#{post.topic_id}/bookmark.json"
expect(response.status).to eq(400) expect(response.status).to eq(400)
end 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 end
describe '#reset_new' do describe '#reset_new' do

181
spec/requests/users_controller_spec.rb
View File

@ -5240,125 +5240,54 @@ describe UsersController do
end end
describe "#bookmarks" do describe "#bookmarks" do
context "when polymorphic bookmarks are not used" do before do
let(:bookmark1) { Fabricate(:bookmark, user: user1) } register_test_bookmarkable
let(:bookmark2) { Fabricate(:bookmark, user: user1) } TopicUser.change(user1.id, bookmark1.bookmarkable.topic_id, total_msecs_viewed: 1)
let(:bookmark3) { Fabricate(:bookmark) } TopicUser.change(user1.id, bookmark2.bookmarkable_id, total_msecs_viewed: 1)
before do Fabricate(:post, topic: bookmark2.bookmarkable)
TopicUser.change(user1.id, bookmark1.topic_id, total_msecs_viewed: 1) bookmark3 && bookmark4
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
end end
context "for polymorphic bookmarks" do after do
before do Bookmark.registered_bookmarkables = []
SiteSetting.use_polymorphic_bookmarks = true end
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 let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) }
Bookmark.registered_bookmarkables = [] let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) }
end let(:bookmark3) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:user)) }
let(:bookmark4) { Fabricate(:bookmark) }
let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) } it "returns a list of serialized bookmarks for the user" do
let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) } sign_in(user1)
let(:bookmark3) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:user)) } get "/u/#{user1.username}/bookmarks.json"
let(:bookmark4) { Fabricate(:bookmark) } 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 it "returns a list of serialized bookmarks for the user including custom registered bookmarkables" do
sign_in(user1) sign_in(user1)
bookmark3.bookmarkable.user_profile.update!(bio_raw: "<p>Something cooked</p>") bookmark3.bookmarkable.user_profile.update!(bio_raw: "<p>Something cooked</p>")
bookmark3.bookmarkable.user_profile.rebake! bookmark3.bookmarkable.user_profile.rebake!
get "/u/#{user1.username}/bookmarks.json" get "/u/#{user1.username}/bookmarks.json"
expect(response.status).to eq(200) expect(response.status).to eq(200)
response_bookmarks = response.parsed_body['user_bookmark_list']['bookmarks'] response_bookmarks = response.parsed_body['user_bookmark_list']['bookmarks']
expect(response_bookmarks.map { |b| b['id'] }).to match_array( expect(response_bookmarks.map { |b| b['id'] }).to match_array(
[bookmark1.id, bookmark2.id, bookmark3.id] [bookmark1.id, bookmark2.id, bookmark3.id]
) )
expect(response_bookmarks.find { |b| b['id'] == bookmark3.id }['excerpt']).to eq('Something cooked') expect(response_bookmarks.find { |b| b['id'] == bookmark3.id }['excerpt']).to eq('Something cooked')
end end
it "returns an .ics file of bookmark reminders for the user in date order" do 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) bookmark1.update!(name: nil, reminder_at: 1.day.from_now)
bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now) bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now)
bookmark3.update!(name: nil, reminder_at: 2.weeks.from_now) bookmark3.update!(name: nil, reminder_at: 2.weeks.from_now)
sign_in(user1) sign_in(user1)
get "/u/#{user1.username}/bookmarks.ics" get "/u/#{user1.username}/bookmarks.ics"
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(response.body.chomp).to eq(<<~ICS) expect(response.body).to eq(<<~ICS)
BEGIN:VCALENDAR BEGIN:VCALENDAR
VERSION:2.0 VERSION:2.0
PRODID:-//Discourse//#{Discourse.current_hostname}//#{Discourse.full_version}//EN PRODID:-//Discourse//#{Discourse.current_hostname}//#{Discourse.full_version}//EN
@ -5390,8 +5319,32 @@ describe UsersController do
URL:#{Discourse.base_url}/u/#{bookmark3.bookmarkable.username} URL:#{Discourse.base_url}/u/#{bookmark3.bookmarkable.username}
END:VEVENT END:VEVENT
END:VCALENDAR END:VCALENDAR
ICS ICS
end 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
end end

16
spec/script/import_scripts/base_spec.rb
View File

@ -49,26 +49,12 @@ describe ImportScripts::Base do
it "creates bookmarks, posts, and users" do it "creates bookmarks, posts, and users" do
MockSpecImporter.new(import_data).perform 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(Post.count).to eq(5)
expect(User.where('id > 0').count).to eq(1) expect(User.where('id > 0').count).to eq(1)
expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60) expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60)
end 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 it "does not change purge unactivated users setting if disabled" do
SiteSetting.purge_unactivated_users_grace_period_days = 0 SiteSetting.purge_unactivated_users_grace_period_days = 0
MockSpecImporter.new(import_data).perform MockSpecImporter.new(import_data).perform

13
spec/serializers/post_serializer_spec.rb
View File

@ -254,7 +254,7 @@ describe PostSerializer do
end end
context "when a Bookmark record exists for the user on the post" do 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 context "bookmarks with reminders" do
it "returns true" do it "returns true" do
@ -266,17 +266,6 @@ describe PostSerializer do
end end
end 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 end
context "posts when group moderation is enabled" do context "posts when group moderation is enabled" do

1
spec/serializers/user_bookmark_list_serializer_spec.rb
View File

@ -6,7 +6,6 @@ RSpec.describe UserBookmarkListSerializer do
context "for polymorphic bookmarks" do context "for polymorphic bookmarks" do
before do before do
SiteSetting.use_polymorphic_bookmarks = true
register_test_bookmarkable register_test_bookmarkable
Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)

82
spec/serializers/user_bookmark_serializer_spec.rb
View File

@ -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

4
spec/serializers/user_post_bookmark_serializer_spec.rb
View File

@ -1,10 +1,6 @@
# frozen_string_literal: true # frozen_string_literal: true
RSpec.describe UserPostBookmarkSerializer do RSpec.describe UserPostBookmarkSerializer do
before do
SiteSetting.use_polymorphic_bookmarks = true
end
let(:user) { Fabricate(:user) } let(:user) { Fabricate(:user) }
let(:post) { Fabricate(:post, user: user, topic: topic) } let(:post) { Fabricate(:post, user: user, topic: topic) }
let(:topic) { Fabricate(:topic) } let(:topic) { Fabricate(:topic) }

4
spec/serializers/user_topic_bookmark_serializer_spec.rb
View File

@ -1,10 +1,6 @@
# frozen_string_literal: true # frozen_string_literal: true
RSpec.describe UserTopicBookmarkSerializer do RSpec.describe UserTopicBookmarkSerializer do
before do
SiteSetting.use_polymorphic_bookmarks = true
end
fab!(:user) { Fabricate(:user) } fab!(:user) { Fabricate(:user) }
let!(:topic) { Fabricate(:topic, user: user) } let!(:topic) { Fabricate(:topic, user: user) }
let!(:post) { Fabricate(:post, topic: topic) } let!(:post) { Fabricate(:post, topic: topic) }

4
spec/services/post_bookmarkable_spec.rb
View File

@ -7,10 +7,6 @@ describe PostBookmarkable do
fab!(:guardian) { Guardian.new(user) } fab!(:guardian) { Guardian.new(user) }
fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) } fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
before do
SiteSetting.use_polymorphic_bookmarks = true
end
let!(:post1) { Fabricate(:post) } let!(:post1) { Fabricate(:post) }
let!(:post2) { Fabricate(:post) } let!(:post2) { Fabricate(:post) }
let!(:bookmark1) { Fabricate(:bookmark, user: user, bookmarkable: post1, name: "something i gotta do") } let!(:bookmark1) { Fabricate(:bookmark, user: user, bookmarkable: post1, name: "something i gotta do") }

4
spec/services/topic_bookmarkable_spec.rb
View File

@ -7,10 +7,6 @@ describe TopicBookmarkable do
fab!(:guardian) { Guardian.new(user) } fab!(:guardian) { Guardian.new(user) }
fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) } fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
before do
SiteSetting.use_polymorphic_bookmarks = true
end
let!(:topic1) { Fabricate(:topic) } let!(:topic1) { Fabricate(:topic) }
let!(:topic2) { Fabricate(:topic) } let!(:topic2) { Fabricate(:topic) }
let!(:post) { Fabricate(:post, topic: topic1) } let!(:post) { Fabricate(:post, topic: topic1) }