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

FEATURE: Topic-level bookmarks (#14353) 

Allows creating a bookmark with the `for_topic` flag introduced in d1d2298a4c set to true. This happens when clicking on the Bookmark button in the topic footer when no other posts are bookmarked. In a later PR, when clicking on these topic-level bookmarks the user will be taken to the last unread post in the topic, not the OP. Only the OP can have a topic level bookmark, and users can also make a post-level bookmark on the OP of the topic.

I had to do some pretty heavy refactors because most of the bookmark code in the JS topics controller was centred around instances of Post JS models, but the topic level bookmark is not centred around a post. Some refactors were just for readability as well.

Also removes some missed reminderType code from the purge in 41e19adb0d
This commit is contained in:
Martin Brennan 2021-09-21 08:45:47 +10:00 committed by GitHub
parent 02f7035cbe
commit 0c42a1e5f3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
28 changed files with 446 additions and 172 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -167,25 +167,13 @@ export default Component.extend({
localStorage.bookmarkDeleteOption = this.autoDeletePreference;
let reminderType;
if (this.selectedReminderType === TIME_SHORTCUT_TYPES.NONE) {
reminderType = null;
} else if (
this.selectedReminderType === TIME_SHORTCUT_TYPES.LAST_CUSTOM ||
this.selectedReminderType === TIME_SHORTCUT_TYPES.POST_LOCAL_DATE
) {
reminderType = TIME_SHORTCUT_TYPES.CUSTOM;
} else {
reminderType = this.selectedReminderType;
}
const data = {
reminder_type: reminderType,
reminder_at: reminderAtISO,
name: this.model.name,
post_id: this.model.postId,
id: this.model.id,
auto_delete_preference: this.autoDeletePreference,
for_topic: this.model.forTopic,
};
if (this.editingExistingBookmark) {
@ -207,9 +195,10 @@ export default Component.extend({
return;
}
this.afterSave({
reminderAt: reminderAtISO,
reminderType: this.selectedReminderType,
autoDeletePreference: this.autoDeletePreference,
reminder_at: reminderAtISO,
for_topic: this.model.forTopic,
auto_delete_preference: this.autoDeletePreference,
post_id: this.model.postId,
id: this.model.id || response.id,
name: this.model.name,
});
@ -220,7 +209,7 @@ export default Component.extend({
type: "DELETE",
}).then((response) => {
if (this.afterDelete) {
this.afterDelete(response.topic_bookmarked);
this.afterDelete(response.topic_bookmarked, this.model.id);
}
});
},

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

@ -215,15 +215,23 @@ export default Controller.extend(bufferedProperty("model"), {
if (posts) {
posts
.filter(
(p) =>
p.bookmarked &&
p.bookmark_auto_delete_preference ===
(post) =>
post.bookmarked &&
post.bookmark_auto_delete_preference ===
AUTO_DELETE_PREFERENCES.ON_OWNER_REPLY
)
.forEach((p) => {
p.clearBookmark();
.forEach((post) => {
post.clearBookmark();
this.model.removeBookmark(post.bookmark_id);
});
}
const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true);
if (
forTopicBookmark?.auto_delete_preference ===
AUTO_DELETE_PREFERENCES.ON_OWNER_REPLY
) {
this.model.removeBookmark(forTopicBookmark.id);
}
},
_forceRefreshPostStream() {
@ -723,9 +731,15 @@ export default Controller.extend(bufferedProperty("model"), {
if (!this.currentUser) {
return bootbox.alert(I18n.t("bookmarks.not_bookmarked"));
} else if (post) {
return this._togglePostBookmark(post);
const bookmarkForPost = this.model.bookmarks.find(
(bookmark) => bookmark.post_id === post.id && !bookmark.for_topic
);
return this._modifyPostBookmark(
bookmarkForPost || { post_id: post.id, for_topic: false },
post
);
} else {
return this._toggleTopicBookmark(this.model).then((changedIds) => {
return this._toggleTopicLevelBookmark().then((changedIds) => {
if (!changedIds) {
return;
}
@ -1189,110 +1203,151 @@ export default Controller.extend(bufferedProperty("model"), {
}
},
_togglePostBookmark(post) {
_modifyTopicBookmark(bookmark) {
const title = bookmark.id
? "post.bookmarks.edit_for_topic"
: "post.bookmarks.create_for_topic";
return this._openBookmarkModal(bookmark, title, {
onAfterSave: () => {
this.model.set("bookmarked", true);
this.model.incrementProperty("bookmarksWereChanged");
},
});
},
_modifyPostBookmark(bookmark, post) {
const title = bookmark.id ? "post.bookmarks.edit" : "post.bookmarks.create";
return this._openBookmarkModal(bookmark, title, {
onCloseWithoutSaving: () => {
post.appEvents.trigger("post-stream:refresh", {
id: bookmark.post_id,
});
},
onAfterSave: (savedData) => {
post.createBookmark(savedData);
this.model.afterPostBookmarked(post, savedData);
return [post.id];
},
onAfterDelete: (topicBookmarked) => {
post.deleteBookmark(topicBookmarked);
},
});
},
_openBookmarkModal(
bookmark,
title,
callbacks = {
onCloseWithoutSaving: null,
onAfterSave: null,
onAfterDelete: null,
}
) {
return new Promise((resolve) => {
let modalController = showModal("bookmark", {
model: {
postId: post.id,
id: post.bookmark_id,
reminderAt: post.bookmark_reminder_at,
autoDeletePreference: post.bookmark_auto_delete_preference,
name: post.bookmark_name,
postId: bookmark.post_id,
id: bookmark.id,
reminderAt: bookmark.reminder_at,
autoDeletePreference: bookmark.auto_delete_preference,
name: bookmark.name,
forTopic: bookmark.for_topic,
},
title: post.bookmark_id
? "post.bookmarks.edit"
: "post.bookmarks.create",
title,
modalClass: "bookmark-with-reminder",
});
modalController.setProperties({
onCloseWithoutSaving: () => {
resolve({ closedWithoutSaving: true });
post.appEvents.trigger("post-stream:refresh", { id: post.id });
if (callbacks.onCloseWithoutSaving) {
callbacks.onCloseWithoutSaving();
}
resolve();
},
afterSave: (savedData) => {
this._addOrUpdateBookmarkedPost(post.id, savedData.reminderAt);
post.createBookmark(savedData);
resolve({ closedWithoutSaving: false });
this._syncBookmarks(savedData);
this.model.set("bookmarking", false);
let resolveData;
if (callbacks.onAfterSave) {
resolveData = callbacks.onAfterSave(savedData);
}
resolve(resolveData);
},
afterDelete: (topicBookmarked) => {
this.model.set(
"bookmarked_posts",
this.model.bookmarked_posts.filter((x) => x.post_id !== post.id)
);
post.deleteBookmark(topicBookmarked);
afterDelete: (topicBookmarked, bookmarkId) => {
this.model.removeBookmark(bookmarkId);
if (callbacks.onAfterDelete) {
callbacks.onAfterDelete(topicBookmarked);
}
resolve();
},
});
});
},
_addOrUpdateBookmarkedPost(postId, reminderAt) {
if (!this.model.bookmarked_posts) {
this.model.set("bookmarked_posts", []);
_syncBookmarks(data) {
if (!this.model.bookmarks) {
this.model.set("bookmarks", []);
}
let bookmarkedPost = this.model.bookmarked_posts.findBy("post_id", postId);
if (!bookmarkedPost) {
bookmarkedPost = { post_id: postId };
this.model.bookmarked_posts.pushObject(bookmarkedPost);
const bookmark = this.model.bookmarks.findBy("id", data.id);
if (!bookmark) {
this.model.bookmarks.pushObject(data);
} else {
bookmark.reminder_at = data.reminder_at;
bookmark.name = data.name;
bookmark.auto_delete_preference = data.auto_delete_preference;
}
bookmarkedPost.reminder_at = reminderAt;
},
_toggleTopicBookmark() {
async _toggleTopicLevelBookmark() {
if (this.model.bookmarking) {
return Promise.resolve();
}
this.model.set("bookmarking", true);
const bookmarkedPostsCount = this.model.bookmarked_posts
? this.model.bookmarked_posts.length
: 0;
const bookmarkPost = async (post) => {
const opts = await this._togglePostBookmark(post);
this.model.set("bookmarking", false);
if (opts.closedWithoutSaving) {
return;
}
this.model.afterPostBookmarked(post);
return [post.id];
};
if (this.model.bookmarkCount > 1) {
return this._maybeClearAllBookmarks();
}
const toggleBookmarkOnServer = async () => {
if (bookmarkedPostsCount === 0) {
const firstPost = await this.model.firstPost();
return bookmarkPost(firstPost);
} else if (bookmarkedPostsCount === 1) {
const postId = this.model.bookmarked_posts[0].post_id;
const post = await this.model.postById(postId);
return bookmarkPost(post);
if (this.model.bookmarkCount === 1) {
const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true);
if (forTopicBookmark) {
return this._modifyTopicBookmark(forTopicBookmark);
} else {
return this.model
.deleteBookmarks()
.then(() => this.model.clearBookmarks())
.catch(popupAjaxError)
.finally(() => this.model.set("bookmarking", false));
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({
post_id: firstPost.id,
for_topic: true,
});
}
},
_maybeClearAllBookmarks() {
return new Promise((resolve) => {
if (bookmarkedPostsCount > 1) {
bootbox.confirm(
I18n.t("bookmarks.confirm_clear"),
I18n.t("no_value"),
I18n.t("yes_value"),
(confirmed) => {
if (confirmed) {
toggleBookmarkOnServer().then(resolve);
} else {
this.model.set("bookmarking", false);
resolve();
}
bootbox.confirm(
I18n.t("bookmarks.confirm_clear"),
I18n.t("no_value"),
I18n.t("yes_value"),
(confirmed) => {
if (confirmed) {
return this.model
.deleteBookmarks()
.then(() => resolve(this.model.clearBookmarks()))
.catch(popupAjaxError)
.finally(() => {
this.model.set("bookmarking", false);
});
} else {
this.model.set("bookmarking", false);
resolve();
}
);
} else {
toggleBookmarkOnServer().then(resolve);
}
}
);
});
},

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

@ -67,8 +67,7 @@ export default {
dependentKeys: ["topic.bookmarked", "topic.bookmarksWereChanged"],
id: "bookmark",
icon() {
const bookmarkedPosts = this.topic.bookmarked_posts;
if (bookmarkedPosts && bookmarkedPosts.find((x) => x.reminder_at)) {
if (this.topic.bookmarks.some((bookmark) => bookmark.reminder_at)) {
return "discourse-bookmark-clock";
}
return "bookmark";
@ -81,14 +80,9 @@ export default {
},
label() {
if (!this.topic.isPrivateMessage || this.site.mobileView) {
const bookmarkedPosts = this.topic.bookmarked_posts;
const bookmarkedPostsCount = bookmarkedPosts
? bookmarkedPosts.length
: 0;
if (bookmarkedPostsCount === 0) {
if (this.topic.bookmarkCount === 0) {
return "bookmarked.title";
} else if (bookmarkedPostsCount === 1) {
} else if (this.topic.bookmarkCount === 1) {
return "bookmarked.edit_bookmark";
} else {
return "bookmarked.clear_bookmarks";
@ -96,12 +90,19 @@ export default {
}
},
translatedTitle() {
const bookmarkedPosts = this.topic.bookmarked_posts;
if (!bookmarkedPosts || bookmarkedPosts.length === 0) {
if (this.topic.bookmarkCount === 0) {
return I18n.t("bookmarked.help.bookmark");
} else if (bookmarkedPosts.length === 1) {
return I18n.t("bookmarked.help.edit_bookmark");
} else if (bookmarkedPosts.find((x) => x.reminder_at)) {
} else if (this.topic.bookmarkCount === 1) {
if (
this.topic.bookmarks.filter((bookmark) => bookmark.for_topic).length
) {
return I18n.t("bookmarked.help.edit_bookmark_for_topic");
} else {
return I18n.t("bookmarked.help.edit_bookmark");
}
} else if (
this.topic.bookmarks.some((bookmark) => bookmark.reminder_at)
) {
return I18n.t("bookmarked.help.unbookmark_with_reminder");
} else {
return I18n.t("bookmarked.help.unbookmark");

6
app/assets/javascripts/discourse/app/models/post.js
View File

@ -308,9 +308,8 @@ const Post = RestModel.extend({
this.setProperties({
"topic.bookmarked": true,
bookmarked: true,
bookmark_reminder_at: data.reminderAt,
bookmark_reminder_type: data.reminderType,
bookmark_auto_delete_preference: data.autoDeletePreference,
bookmark_reminder_at: data.reminder_at,
bookmark_auto_delete_preference: data.auto_delete_preference,
bookmark_name: data.name,
bookmark_id: data.id,
});
@ -329,7 +328,6 @@ const Post = RestModel.extend({
clearBookmark() {
this.setProperties({
bookmark_reminder_at: null,
bookmark_reminder_type: null,
bookmark_name: null,
bookmark_id: null,
bookmarked: false,

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

@ -376,17 +376,31 @@ const Topic = RestModel.extend({
return ajax(`/t/${this.id}/remove_bookmarks`, { type: "PUT" });
},
bookmarkCount: alias("bookmarks.length"),
removeBookmark(id) {
if (!this.bookmarks) {
this.set("bookmarks", []);
}
this.set(
"bookmarks",
this.bookmarks.filter((bookmark) => bookmark.id !== id)
);
this.set("bookmarked", this.bookmarks.length);
this.incrementProperty("bookmarksWereChanged");
},
clearBookmarks() {
this.toggleProperty("bookmarked");
const postIds = this.bookmarked_posts.mapBy("post_id");
const postIds = this.bookmarks.mapBy("post_id");
postIds.forEach((postId) => {
const loadedPost = this.postStream.findLoadedPost(postId);
if (loadedPost) {
loadedPost.clearBookmark();
}
});
this.set("bookmarked_posts", []);
this.set("bookmarks", []);
return postIds;
},
@ -609,6 +623,7 @@ Topic.reopenClass({
munge(json) {
// ensure we are not overriding category computed property
delete json.category;
json.bookmarks = json.bookmarks || [];
return json;
},

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

@ -57,11 +57,6 @@ async function testTopicLevelBookmarkButtonIcon(assert, postNumber) {
acceptance("Bookmarking", function (needs) {
needs.user();
let steps = [];
needs.hooks.beforeEach(function () {
steps = [];
});
const topicResponse = topicFixtures["/t/280/1.json"];
topicResponse.post_stream.posts[0].cooked += `<span data-date="2036-01-15" data-time="00:35:00" class="discourse-local-date cooked-date past" data-timezone="Europe/London">
@ -85,10 +80,13 @@ acceptance("Bookmarking", function (needs) {
needs.pretender((server, helper) => {
function handleRequest(request) {
const data = helper.parsePostData(request.requestBody);
steps.push(data.reminder_type || "none");
if (data.post_id === "398") {
return helper.response({ id: 1, success: "OK" });
if (data.for_topic === "true") {
return helper.response({ id: 3, success: "OK" });
} else {
return helper.response({ id: 1, success: "OK" });
}
} else if (data.post_id === "419") {
return helper.response({ id: 2, success: "OK" });
} else {
@ -98,6 +96,7 @@ acceptance("Bookmarking", function (needs) {
server.post("/bookmarks", handleRequest);
server.put("/bookmarks/1", handleRequest);
server.put("/bookmarks/2", handleRequest);
server.put("/bookmarks/3", handleRequest);
server.delete("/bookmarks/1", () =>
helper.response({ success: "OK", topic_bookmarked: false })
);
@ -131,13 +130,6 @@ acceptance("Bookmarking", function (needs) {
assert.ok(exists(".tap-tile-date-input"), "it shows the custom date input");
assert.ok(exists(".tap-tile-time-input"), "it shows the custom time input");
await click("#save-bookmark");
assert.deepEqual(steps, [
"tomorrow",
"start_of_next_business_week",
"next_month",
"custom",
]);
});
test("Saving a bookmark with a reminder", async function (assert) {
@ -156,7 +148,6 @@ acceptance("Bookmarking", function (needs) {
),
"it shows the bookmark clock icon because of the reminder"
);
assert.deepEqual(steps, ["tomorrow"]);
});
test("Opening the options panel and remembering the option", async function (assert) {
@ -177,7 +168,6 @@ acceptance("Bookmarking", function (needs) {
"it should reopen the options panel"
);
assert.equal(selectKit(".bookmark-option-selector").header().value(), 1);
assert.deepEqual(steps, ["none"]);
});
test("Saving a bookmark with no reminder or name", async function (assert) {
@ -195,7 +185,6 @@ acceptance("Bookmarking", function (needs) {
),
"it shows the regular bookmark active icon"
);
assert.deepEqual(steps, ["none"]);
});
test("Deleting a bookmark with a reminder", async function (assert) {
@ -203,8 +192,6 @@ acceptance("Bookmarking", function (needs) {
await openBookmarkModal();
await click("#tap_tile_tomorrow");
assert.deepEqual(steps, ["tomorrow"]);
await openEditBookmarkModal();
assert.ok(
@ -264,7 +251,6 @@ acceptance("Bookmarking", function (needs) {
"08:00",
"it should prefill the bookmark time"
);
assert.deepEqual(steps, ["tomorrow"]);
});
test("Using a post date for the reminder date", async function (assert) {
@ -370,6 +356,78 @@ acceptance("Bookmarking", function (needs) {
);
});
test("Creating and editing a topic level bookmark", async function (assert) {
await visit("/t/internationalization-localization/280");
await click("#topic-footer-button-bookmark");
assert.equal(
query("#discourse-modal-title").innerText,
I18n.t("post.bookmarks.create_for_topic"),
"The create modal says creating a topic bookmark"
);
await click("#save-bookmark");
assert.notOk(
exists(".topic-post:first-child button.bookmark.bookmarked"),
"the first post is not marked as being bookmarked"
);
assert.equal(
query("#topic-footer-button-bookmark").innerText,
I18n.t("bookmarked.edit_bookmark"),
"A topic level bookmark button has a label 'Edit Bookmark'"
);
await click("#topic-footer-button-bookmark");
assert.equal(
query("#discourse-modal-title").innerText,
I18n.t("post.bookmarks.edit_for_topic"),
"The edit modal says editing a topic bookmark"
);
await fillIn("input#bookmark-name", "Test name");
await click("#tap_tile_tomorrow");
await click("#topic-footer-button-bookmark");
assert.equal(
query("input#bookmark-name").value,
"Test name",
"The topic level bookmark editing preserves the values entered"
);
await click(".d-modal-cancel");
await openBookmarkModal(1);
await click("#save-bookmark");
assert.ok(
exists(".topic-post:first-child button.bookmark.bookmarked"),
"the first post is bookmarked independently of the topic level bookmark"
);
// deleting all bookmarks in the topic
assert.equal(
query("#topic-footer-button-bookmark").innerText,
I18n.t("bookmarked.clear_bookmarks"),
"the footer button says Clear Bookmarks because there is more than one"
);
await click("#topic-footer-button-bookmark");
await click("a.btn-primary");
assert.ok(
!exists(".topic-post:first-child button.bookmark.bookmarked"),
"the first post bookmark is deleted"
);
assert.equal(
query("#topic-footer-button-bookmark").innerText,
I18n.t("bookmarked.title"),
"the topic level bookmark is deleted"
);
});
test("The topic level bookmark button opens the edit modal if only one post in the post stream is bookmarked", async function (assert) {
await visit("/t/internationalization-localization/280");
await openBookmarkModal(2);

5
app/assets/javascripts/discourse/tests/acceptance/topic-timeline-test.js
View File

@ -45,6 +45,7 @@ acceptance("Topic Timeline", function (needs) {
read: true,
user_title: null,
bookmarked: false,
bookmarks: [],
actions_summary: [
{
id: 3,
@ -120,6 +121,7 @@ acceptance("Topic Timeline", function (needs) {
read: true,
user_title: null,
bookmarked: false,
bookmarks: [],
actions_summary: [
{
id: 3,
@ -236,6 +238,7 @@ acceptance("Topic Timeline", function (needs) {
current_post_number: 1,
highest_post_number: 2,
last_read_post_number: 0,
bookmarks: [],
last_read_post_id: null,
deleted_by: {
id: 7,
@ -266,7 +269,7 @@ acceptance("Topic Timeline", function (needs) {
],
chunk_size: 20,
bookmarked: false,
bookmarked_posts: null,
bookmarks: [],
topic_timer: null,
message_bus_last_id: 5,
participant_count: 1,

10
app/assets/javascripts/discourse/tests/fixtures/poll.js vendored
View File

@ -234,7 +234,8 @@ export default {
{ id: 8, count: 0, hidden: false, can_act: true }
],
chunk_size: 20,
bookmarked: false
bookmarked: false,
bookmarks: [],
},
"/t/14.json": {
post_stream: {
@ -390,7 +391,8 @@ export default {
{ id: 8, count: 0, hidden: false, can_act: true }
],
chunk_size: 20,
bookmarked: false
bookmarked: false,
bookmarks: [],
},
"/t/15.json": {
post_stream: {
@ -543,7 +545,8 @@ export default {
{ id: 8, count: 0, hidden: false, can_act: true }
],
chunk_size: 20,
bookmarked: false
bookmarked: false,
bookmarks: [],
},
"/t/topic_with_pie_chart_poll.json": {
post_stream: {
@ -691,6 +694,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
topic_timer: null,
message_bus_last_id: 1,
participant_count: 1,

1
app/assets/javascripts/discourse/tests/fixtures/private-messages-fixtures.js vendored
View File

@ -46,6 +46,7 @@ export default {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
views: 5,
like_count: 0,

26
app/assets/javascripts/discourse/tests/fixtures/topic.js vendored
View File

@ -136,6 +136,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
message_archived: false,
topic_timer: null,
message_bus_last_id: 5,
@ -3005,6 +3006,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
suggested_topics: [
{
id: 27331,
@ -3029,6 +3031,7 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 11,
@ -3072,6 +3075,7 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 3,
@ -3152,6 +3156,7 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 4,
@ -3195,6 +3200,8 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 62,
@ -3273,6 +3280,7 @@ export default {
archived: false,
notification_level: 1,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 9,
@ -3862,6 +3870,7 @@ export default {
],
chunk_size: 20,
bookmarked: null,
bookmarks: [],
tags: null,
},
"/t/9/1.json": {
@ -4093,6 +4102,7 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 0,
@ -4122,6 +4132,7 @@ export default {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 0,
@ -4152,6 +4163,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
destination_category_id: 3,
},
"/t/12/1.json": {
@ -4179,6 +4191,7 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 0,
@ -4420,6 +4433,7 @@ export default {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "private_message",
like_count: 0,
@ -4463,6 +4477,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
message_archived: false,
featured_link: null,
},
@ -4696,6 +4711,7 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 0,
@ -4725,6 +4741,7 @@ export default {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 0,
@ -4755,6 +4772,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
},
"/t/301/1.json": {
post_stream: {
@ -4986,6 +5004,7 @@ export default {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 0,
@ -5015,6 +5034,7 @@ export default {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
archetype: "regular",
like_count: 0,
@ -5045,6 +5065,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
},
"/t/34/1.json": {
post_stream: {
@ -5296,6 +5317,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
message_archived: false,
topic_timer: null,
message_bus_last_id: 7,
@ -5341,6 +5363,7 @@ export default {
user_title: "a title",
title_is_group: false,
bookmarked: false,
bookmarks: [],
actions_summary: [
{
id: 3,
@ -5409,6 +5432,7 @@ export default {
read: true,
user_title: null,
bookmarked: false,
bookmarks: [],
actions_summary: [
{
id: 2,
@ -5484,6 +5508,7 @@ export default {
user_title: "a title",
title_is_group: false,
bookmarked: false,
bookmarks: [],
actions_summary: [
{
id: 3,
@ -5579,6 +5604,7 @@ export default {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
topic_timer: null,
message_bus_last_id: 4,
participant_count: 2,

1
app/controllers/bookmarks_controller.rb
View File

@ -15,6 +15,7 @@ class BookmarksController < ApplicationController
post_id: params[:post_id],
name: params[:name],
reminder_at: params[:reminder_at],
for_topic: params[:for_topic] == "true",
options: {
auto_delete_preference: params[:auto_delete_preference] || 0
}

17
app/models/bookmark.rb
View File

@ -38,13 +38,16 @@ class Bookmark < ActiveRecord::Base
on: [:create, :update],
if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_user_id? }
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 :ensure_sane_reminder_at_time
validate :bookmark_limit_not_reached
validates :name, length: { maximum: 100 }
def unique_per_post_for_user
is_first_post = Post.where(id: post_id).pluck_first(:post_number) == 1
exists = if is_first_post
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)
@ -55,6 +58,12 @@ class Bookmark < ActiveRecord::Base
end
end
def for_topic_must_use_first_post
if !is_for_first_post? && self.for_topic
self.errors.add(:base, I18n.t("bookmarks.errors.for_topic_must_use_first_post"))
end
end
def ensure_sane_reminder_at_time
return if reminder_at.blank?
if reminder_at < Time.zone.now
@ -79,6 +88,10 @@ class Bookmark < ActiveRecord::Base
)
end
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 no_reminder?
self.reminder_at.blank?
end

4
app/serializers/post_serializer.rb
View File

@ -359,9 +359,9 @@ class PostSerializer < BasicPostSerializer
def post_bookmark
if @topic_view.present?
@post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id }
@post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id && !bookmark.for_topic }
else
@post_bookmark ||= object.bookmarks.find_by(user: scope.user)
@post_bookmark ||= object.bookmarks.find_by(user: scope.user, for_topic: false)
end
end

6
app/serializers/topic_view_serializer.rb
View File

@ -62,7 +62,7 @@ class TopicViewSerializer < ApplicationSerializer
:is_warning,
:chunk_size,
:bookmarked,
:bookmarked_posts,
:bookmarks,
:message_archived,
:topic_timer,
:unicode_title,
@ -193,8 +193,8 @@ class TopicViewSerializer < ApplicationSerializer
object.has_bookmarks?
end
def bookmarked_posts
object.bookmarked_posts
def bookmarks
object.bookmarks
end
def topic_timer

3
app/serializers/user_bookmark_serializer.rb
View File

@ -28,7 +28,8 @@ class UserBookmarkSerializer < ApplicationSerializer
:slug,
:post_user_username,
:post_user_avatar_template,
:post_user_name
:post_user_name,
:for_topic
def topic_id
post.topic_id

2
app/serializers/web_hook_topic_view_serializer.rb
View File

@ -22,7 +22,7 @@ class WebHookTopicViewSerializer < TopicViewSerializer
image_url
slow_mode_seconds
slow_mode_enabled_until
bookmarked_posts
bookmarks
}.each do |attr|
define_method("include_#{attr}?") do
false

3
config/locales/client.en.yml
View File

@ -303,6 +303,7 @@ en:
help:
bookmark: "Click to bookmark the first post on this topic"
edit_bookmark: "Click to edit the bookmark on this topic"
edit_bookmark_for_topic: "Click to edit the bookmark for this topic"
unbookmark: "Click to remove all bookmarks in this topic"
unbookmark_with_reminder: "Click to remove all bookmarks and reminders in this topic."
@ -3187,7 +3188,9 @@ en:
bookmarks:
create: "Create bookmark"
create_for_topic: "Create bookmark for topic"
edit: "Edit bookmark"
edit_for_topic: "Edit bookmark for topic"
created: "Created"
updated: "Updated"
name: "Name"

1
config/locales/server.en.yml
View File

@ -431,6 +431,7 @@ en:
cannot_set_past_reminder: "You cannot set a bookmark reminder in the past."
cannot_set_reminder_in_distant_future: "You cannot set a bookmark reminder more than 10 years in the future."
time_must_be_provided: "time must be provided for all reminders"
for_topic_must_use_first_post: "You can only use the first post to bookmark the topic."
reminders:
at_desktop: "Next time I'm at my desktop"

38
lib/bookmark_manager.rb
View File

@ -7,8 +7,41 @@ class BookmarkManager
@user = user
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.
# TODO (martin) (2021-12-01) Remove reminder_type keyword argument once plugins are not using it.
def create(post_id:, name: nil, reminder_at: nil, reminder_type: nil, options: {})
def create(
post_id:,
name: nil,
reminder_type: nil,
reminder_at: nil,
for_topic: false,
options: {}
)
post = Post.find_by(id: post_id)
# no bookmarking deleted posts or topics
@ -24,7 +57,8 @@ class BookmarkManager
post: post,
name: name,
reminder_at: reminder_at,
reminder_set_at: Time.zone.now
reminder_set_at: Time.zone.now,
for_topic: for_topic
}.merge(options)
)

14
lib/topic_view.rb
View File

@ -393,17 +393,13 @@ class TopicView
def has_bookmarks?
return false if @user.blank?
return false if @topic.trashed?
@topic.bookmarks.exists?(user_id: @user.id)
bookmarks.any?
end
def bookmarked_posts
return nil unless has_bookmarks?
@topic.bookmarks.where(user: @user).pluck(:post_id, :reminder_at).map do |post_id, reminder_at|
{
post_id: post_id,
reminder_at: reminder_at
}
end
def bookmarks
@bookmarks ||= @topic.bookmarks.where(user: @user).joins(:topic).select(
:id, :post_id, :for_topic, :reminder_at, :name, :auto_delete_preference
)
end
MAX_PARTICIPANTS = 24

6
plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6
View File

@ -19,12 +19,12 @@ function initialize(api) {
api.modifyClass("controller:topic", {
pluginId: PLUGIN_ID,
_togglePostBookmark(post) {
_modifyBookmark(bookmark, post) {
// if we are talking to discobot then any bookmarks should just
// be created without reminder options, to streamline the new user
// narrative.
const discobotUserId = -2;
if (post.user_id === discobotUserId && !post.bookmarked) {
if (post && post.user_id === discobotUserId && !post.bookmarked) {
return ajax("/bookmarks", {
type: "POST",
data: { post_id: post.id },
@ -37,7 +37,7 @@ function initialize(api) {
post.appEvents.trigger("post-stream:refresh", { id: this.id });
});
}
return this._super(post);
return this._super(bookmark, post);
},
});

5
plugins/poll/test/javascripts/acceptance/poll-in-reply-history-test.js.es6
View File

@ -54,6 +54,7 @@ acceptance("Poll in a post reply history", function (needs) {
"/letter_avatar_proxy/v4/letter/a/bbce88/{size}.png",
},
bookmarked: false,
bookmarks: [],
actions_summary: [
{
id: 3,
@ -158,6 +159,7 @@ acceptance("Poll in a post reply history", function (needs) {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
like_count: 0,
views: 3,
@ -212,6 +214,7 @@ acceptance("Poll in a post reply history", function (needs) {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
like_count: 0,
views: 4,
@ -302,6 +305,7 @@ acceptance("Poll in a post reply history", function (needs) {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
topic_timer: null,
message_bus_last_id: 4,
participant_count: 1,
@ -395,6 +399,7 @@ acceptance("Poll in a post reply history", function (needs) {
can_wiki: false,
user_title: null,
bookmarked: false,
bookmarks: [],
actions_summary: [],
moderator: false,
admin: true,

5
plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6
View File

@ -46,6 +46,7 @@ acceptance("Poll quote", function (needs) {
user_title: "Tester",
title_is_group: false,
bookmarked: false,
bookmarks: [],
raw:
"[poll name=poll1 type=regular results=always chartType=bar]\n* Alpha\n* Beta\n[/poll]\n\n[poll name=poll2 type=regular results=always chartType=bar]\n* First\n* Second\n[/poll]",
actions_summary: [
@ -175,6 +176,7 @@ acceptance("Poll quote", function (needs) {
user_title: "Tester",
title_is_group: false,
bookmarked: false,
bookmarks: [],
actions_summary: [
{
id: 3,
@ -238,6 +240,7 @@ acceptance("Poll quote", function (needs) {
archived: false,
notification_level: 1,
bookmarked: false,
bookmarks: [],
liked: false,
tags: [],
like_count: 0,
@ -282,6 +285,7 @@ acceptance("Poll quote", function (needs) {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
tags: [],
like_count: 0,
@ -362,6 +366,7 @@ acceptance("Poll quote", function (needs) {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
topic_timer: null,
message_bus_last_id: 2,
participant_count: 1,

7
plugins/poll/test/javascripts/acceptance/poll-results-test.js.es6
View File

@ -48,6 +48,7 @@ acceptance("Poll results", function (needs) {
can_wiki: true,
title_is_group: false,
bookmarked: false,
bookmarks: [],
raw:
"[poll type=regular results=always public=true chartType=bar]\n* Option #1\n* Option #2\n[/poll]",
actions_summary: [
@ -154,6 +155,7 @@ acceptance("Poll results", function (needs) {
read: true,
title_is_group: false,
bookmarked: false,
bookmarks: [],
actions_summary: [
{ id: 3, can_act: true },
{ id: 4, can_act: true },
@ -247,6 +249,7 @@ acceptance("Poll results", function (needs) {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
tags: [],
like_count: 0,
@ -302,6 +305,7 @@ acceptance("Poll results", function (needs) {
archived: false,
notification_level: 2,
bookmarked: false,
bookmarks: [],
liked: false,
tags: [],
like_count: 0,
@ -349,6 +353,7 @@ acceptance("Poll results", function (needs) {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
tags: ["abc", "e", "b"],
like_count: 0,
@ -394,6 +399,7 @@ acceptance("Poll results", function (needs) {
archived: false,
notification_level: 3,
bookmarked: false,
bookmarks: [],
liked: false,
tags: [],
like_count: 0,
@ -461,6 +467,7 @@ acceptance("Poll results", function (needs) {
],
chunk_size: 20,
bookmarked: false,
bookmarks: [],
topic_timer: null,
message_bus_last_id: 5,
participant_count: 1,

12
spec/components/topic_view_spec.rb
View File

@ -408,7 +408,7 @@ describe TopicView do
end
end
context "#bookmarked_posts" do
context "#bookmarks" do
let!(:user) { Fabricate(:user) }
let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, post: topic.first_post, user: user) }
let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, post: topic.posts[1], user: user) }
@ -416,7 +416,7 @@ describe TopicView do
it "gets the first post bookmark reminder at for the user" do
topic_view = TopicView.new(topic.id, user)
first, second = topic_view.bookmarked_posts
first, second = topic_view.bookmarks
expect(first[:post_id]).to eq(bookmark1.post_id)
expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
expect(second[:post_id]).to eq(bookmark2.post_id)
@ -424,12 +424,12 @@ describe TopicView do
end
context "when the topic is deleted" do
it "returns nil" do
it "returns []" do
topic_view = TopicView.new(topic, user)
PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
topic.reload
expect(topic_view.bookmarked_posts).to eq(nil)
expect(topic_view.bookmarks).to eq([])
end
end
@ -439,8 +439,8 @@ describe TopicView do
PostDestroyer.new(Fabricate(:admin), topic.posts.second).destroy
topic.reload
expect(topic_view.bookmarked_posts.length).to eq(1)
first = topic_view.bookmarked_posts.first
expect(topic_view.bookmarks.length).to eq(1)
first = topic_view.bookmarks.first
expect(first[:post_id]).to eq(bookmark1.post_id)
expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
end

23
spec/lib/bookmark_manager_spec.rb
View File

@ -17,7 +17,28 @@ RSpec.describe BookmarkManager do
bookmark = Bookmark.find_by(user: user)
expect(bookmark.post_id).to eq(post.id)
expect(bookmark.topic.id).to eq(post.topic_id)
expect(bookmark.topic_id).to eq(post.topic_id)
end
it "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
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 from guardian check" do

25
spec/requests/bookmarks_controller_spec.rb
View File

@ -30,6 +30,31 @@ describe BookmarksController do
expect(response.status).to eq(429)
end
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
it "errors when trying to create a for_topic bookmark for post_number > 1" do
post "/bookmarks.json", params: {
post_id: Fabricate(:post, topic: bookmark_post.topic).id,
reminder_type: "tomorrow",
reminder_at: (Time.zone.now + 1.day).iso8601,
for_topic: true
}
expect(response.status).to eq(400)
expect(response.parsed_body['errors']).to include(
I18n.t("bookmarks.errors.for_topic_must_use_first_post")
)
end
context "if the user reached the max bookmark limit" do
before do
SiteSetting.max_bookmarks_per_user = 1

14
spec/serializers/post_serializer_spec.rb
View File

@ -45,7 +45,8 @@ describe PostSerializer do
it "should not allow user to flag post and notify non human user" do
post.update!(user: Discourse.system_user)
serializer = PostSerializer.new(post,
serializer = PostSerializer.new(
post,
scope: Guardian.new(actor),
root: false
)
@ -247,6 +248,17 @@ describe PostSerializer do
end
end
end
context "when the post bookmark is for_topic" do
let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post, for_topic: true) }
context "bookmarks with reminders" do
it "returns false, because we do not want to mark the post as bookmarked, because the bookmark is for the topic" do
expect(serialized.as_json[:bookmarked]).to eq(false)
expect(serialized.as_json[:bookmark_reminder_at]).to eq(nil)
end
end
end
end
context "posts when group moderation is enabled" do