osr-plastic/osr-discourse-src
Archived
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

DEV: Change to use nil post ID instead of -1 post ID

Browse Source
This commit is contained in:
Martin Brennan 2021-09-08 15:00:54 +10:00
parent 7bd3f8cb0a
commit 45019bea92
No known key found for this signature in database
GPG Key ID: A08063EEF3EA26A4
22 changed files with 148 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -4,10 +4,7 @@ import { alias, and, not, or } from "@ember/object/computed";
import discourseComputed, { observes } from "discourse-common/utils/decorators";
import { isEmpty, isPresent } from "@ember/utils";
import { later, next, schedule } from "@ember/runloop";
import {
AUTO_DELETE_PREFERENCES,
FOR_TOPIC_POST_ID,
} from "discourse/models/bookmark";
import { AUTO_DELETE_PREFERENCES } from "discourse/models/bookmark";
import Composer from "discourse/models/composer";
import EmberObject, { action } from "@ember/object";
import I18n from "I18n";
@ -1196,7 +1193,7 @@ export default Controller.extend(bufferedProperty("model"), {
_toggleBookmark(target) {
return new Promise((resolve) => {
const bookmarkingTopic = target instanceof Topic;
const postId = bookmarkingTopic ? FOR_TOPIC_POST_ID : target.id;
const postId = bookmarkingTopic ? null : target.id;
const topicId = bookmarkingTopic ? target.id : target.topic_id;
let modalController = showModal("bookmark", {
@ -1233,9 +1230,9 @@ export default Controller.extend(bufferedProperty("model"), {
},
afterDelete: (topicBookmarked) => {
this.model.set(
"bookmarked_posts",
this.model.bookmarked_posts.filter(
(bookmarkedPost) => bookmarkedPost.post_id !== postId
"bookmarked_items",
this.model.bookmarked_items.filter(
(bookmarkedItem) => bookmarkedItem.post_id !== postId
)
);
target.deleteBookmark(topicBookmarked);
@ -1245,17 +1242,17 @@ export default Controller.extend(bufferedProperty("model"), {
},
_addOrUpdateBookmarkedPost(postId, reminderAt) {
if (!this.model.bookmarked_posts) {
this.model.set("bookmarked_posts", []);
if (!this.model.bookmarked_items) {
this.model.set("bookmarked_items", []);
}
let bookmarkedPost = this.model.bookmarked_posts.findBy("post_id", postId);
if (!bookmarkedPost) {
bookmarkedPost = { post_id: postId };
this.model.bookmarked_posts.pushObject(bookmarkedPost);
let bookmarkedItem = this.model.bookmarked_items.findBy("post_id", postId);
if (!bookmarkedItem) {
bookmarkedItem = { post_id: postId };
this.model.bookmarked_items.pushObject(bookmarkedItem);
}
bookmarkedPost.reminder_at = reminderAt;
bookmarkedItem.reminder_at = reminderAt;
},
_toggleTopicBookmark() {
@ -1263,8 +1260,8 @@ export default Controller.extend(bufferedProperty("model"), {
return Promise.resolve();
}
this.model.set("bookmarking", true);
const bookmarkedPostsCount = this.model.bookmarked_posts
? this.model.bookmarked_posts.length
const bookmarkedCount = this.model.bookmarked_items
? this.model.bookmarked_items.length
: 0;
const bookmarkTarget = async (target) => {
@ -1283,15 +1280,15 @@ export default Controller.extend(bufferedProperty("model"), {
const toggleBookmarkOnServer = async () => {
// bookmark the topic if there are no bookmarked posts
if (bookmarkedPostsCount === 0) {
if (bookmarkedCount === 0) {
return bookmarkTarget(this.model);
// if there is only one bookmarked post for the topic we want to
// get it in the stream and edit that one
} else if (bookmarkedPostsCount === 1) {
const postId = this.model.bookmarked_posts[0].post_id;
// if there is only one bookmarked item for the topic we want to
// get it in the stream and edit that one (could be a post or the topic-level bookmark)
} else if (bookmarkedCount === 1) {
const postId = this.model.bookmarked_items[0].post_id;
if (postId === FOR_TOPIC_POST_ID) {
if (!postId) {
return bookmarkTarget(this.model);
} else {
const post = await this.model.postById(postId);
@ -1299,7 +1296,7 @@ export default Controller.extend(bufferedProperty("model"), {
}
} else {
// otherwise we want to clear all the bookmarks out if there is more
// than one bookmarked post in the topic
// than one bookmarked item in the topic
return this.model
.deleteBookmarks()
.then(() => this.model.clearBookmarks())
@ -1309,7 +1306,7 @@ export default Controller.extend(bufferedProperty("model"), {
};
return new Promise((resolve) => {
if (bookmarkedPostsCount > 1) {
if (bookmarkedCount > 1) {
bootbox.confirm(
I18n.t("bookmarks.confirm_clear"),
I18n.t("no_value"),

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

@ -67,10 +67,9 @@ export default {
dependentKeys: ["topic.bookmarked", "topic.bookmarksWereChanged"],
id: "bookmark",
icon() {
const bookmarkedPosts = this.topic.bookmarked_posts;
const bookmarkedItems = this.topic.bookmarked_items || [];
if (
bookmarkedPosts &&
bookmarkedPosts.find((bookmarkedPost) => bookmarkedPost.reminder_at)
bookmarkedItems.find((bookmarkedItem) => bookmarkedItem.reminder_at)
) {
return "discourse-bookmark-clock";
}
@ -84,14 +83,14 @@ export default {
},
label() {
if (!this.topic.isPrivateMessage || this.site.mobileView) {
const bookmarkedPosts = this.topic.bookmarked_posts;
const bookmarkedPostsCount = bookmarkedPosts
? bookmarkedPosts.length
const bookmarkedItems = this.topic.bookmarked_items;
const bookmarkedItemsCount = bookmarkedItems
? bookmarkedItems.length
: 0;
if (bookmarkedPostsCount === 0) {
if (bookmarkedItemsCount === 0) {
return "bookmarked.title";
} else if (bookmarkedPostsCount === 1) {
} else if (bookmarkedItemsCount === 1) {
return "bookmarked.edit_bookmark";
} else {
return "bookmarked.clear_bookmarks";
@ -99,13 +98,13 @@ export default {
}
},
translatedTitle() {
const bookmarkedPosts = this.topic.bookmarked_posts;
if (!bookmarkedPosts || bookmarkedPosts.length === 0) {
const bookmarkedItems = this.topic.bookmarked_items;
if (!bookmarkedItems || bookmarkedItems.length === 0) {
return I18n.t("bookmarked.help.bookmark");
} else if (bookmarkedPosts.length === 1) {
} else if (bookmarkedItems.length === 1) {
return I18n.t("bookmarked.help.edit_bookmark");
} else if (
bookmarkedPosts.find((bookmarkedPost) => bookmarkedPost.reminder_at)
bookmarkedItems.find((bookmarkedItem) => bookmarkedItem.reminder_at)
) {
return I18n.t("bookmarked.help.unbookmark_with_reminder");
} else {

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

@ -18,8 +18,6 @@ export const AUTO_DELETE_PREFERENCES = {
ON_OWNER_REPLY: 2,
};
export const FOR_TOPIC_POST_ID = -1;
const Bookmark = RestModel.extend({
newBookmark: none("id"),

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

@ -1,5 +1,4 @@
import { alias, and, equal, notEmpty, or } from "@ember/object/computed";
import { FOR_TOPIC_POST_ID } from "discourse/models/bookmark";
import { fmt, propertyEqual } from "discourse/lib/computed";
import ActionSummary from "discourse/models/action-summary";
import Category from "discourse/models/category";
@ -412,18 +411,16 @@ const Topic = RestModel.extend({
clearBookmarks() {
this.toggleProperty("bookmarked");
const postIds = this.bookmarked_posts.mapBy("post_id");
const postIds = this.bookmarked_items.mapBy("post_id");
postIds.forEach((postId) => {
if (postId === FOR_TOPIC_POST_ID) {
return this.clearBookmark();
}
const loadedPost = this.postStream.findLoadedPost(postId);
if (loadedPost) {
loadedPost.clearBookmark();
}
});
this.set("bookmarked_posts", []);
this.clearBookmark();
this.set("bookmarked_items", []);
return postIds;
},

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

@ -266,7 +266,7 @@ acceptance("Topic Timeline", function (needs) {
],
chunk_size: 20,
bookmarked: false,
bookmarked_posts: null,
bookmarked_items: null,
topic_timer: null,
message_bus_last_id: 5,
participant_count: 1,

14
app/controllers/bookmarks_controller.rb
View File

@ -4,9 +4,13 @@ class BookmarksController < ApplicationController
requires_login
def create
params.require(:post_id)
if params[:post_id].to_i == Bookmark::FOR_TOPIC_POST_ID
params.require(:topic_id)
if params[:post_id].blank?
if params[:topic_id].blank?
raise Discourse::InvalidParameters.new("Either a post_id or a topic_id is required.")
end
topic_id = params[:topic_id].to_i
else
post_id = params[:post_id]&.to_i
end
RateLimiter.new(
@ -15,8 +19,8 @@ class BookmarksController < ApplicationController
bookmark_manager = BookmarkManager.new(current_user)
bookmark = bookmark_manager.create(
topic_id: params[:topic_id].to_i,
post_id: params[:post_id].to_i,
topic_id: topic_id,
post_id: post_id,
name: params[:name],
reminder_type: params[:reminder_type],
reminder_at: params[:reminder_at],

4
app/models/bookmark.rb
View File

@ -1,8 +1,6 @@
# frozen_string_literal: true
class Bookmark < ActiveRecord::Base
FOR_TOPIC_POST_ID = -1
belongs_to :user
belongs_to :post
belongs_to :topic
@ -80,7 +78,7 @@ class Bookmark < ActiveRecord::Base
end
def for_topic?
self.post_id == FOR_TOPIC_POST_ID
self.post_id.blank?
end
def for_post?

6
app/serializers/topic_view_serializer.rb
View File

@ -67,7 +67,7 @@ class TopicViewSerializer < ApplicationSerializer
:bookmark_reminder_type,
:bookmark_name,
:bookmark_auto_delete_preference,
:bookmarked_posts,
:bookmarked_items,
:message_archived,
:topic_timer,
:unicode_title,
@ -240,8 +240,8 @@ class TopicViewSerializer < ApplicationSerializer
object.bookmark&.auto_delete_preference
end
def bookmarked_posts
object.bookmarked_posts
def bookmarked_items
object.bookmarked_items
end
def topic_timer

10
app/serializers/user_bookmark_serializer.rb
View File

@ -35,7 +35,15 @@ class UserBookmarkSerializer < ApplicationSerializer
end
def post
@post ||= object.post || Post.unscoped.find(object.post_id)
if for_topic?
@post ||= topic.first_post
else
@post ||= object.post || Post.unscoped.find(object.post_id)
end
end
def for_topic?
object.for_topic?
end
def closed

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
bookmarked_items
}.each do |attr|
define_method("include_#{attr}?") do
false

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

@ -298,7 +298,7 @@ en:
edit_bookmark: "Edit Bookmark"
clear_bookmarks: "Clear Bookmarks"
help:
bookmark: "Click to bookmark the first post on this topic"
bookmark: "Click to bookmark this topic"
edit_bookmark: "Click to edit the bookmark on this topic"
unbookmark: "Click to remove all bookmarks in this topic"
unbookmark_with_reminder: "Click to remove all bookmarks and reminders in this topic."

3
db/migrate/20210907022247_make_post_id_optional_bookmarks.rb
View File

@ -3,12 +3,15 @@
class MakePostIdOptionalBookmarks < ActiveRecord::Migration[6.1]
def up
remove_index :bookmarks, [:user_id, :post_id], unique: true
change_column_null :bookmarks, :post_id, true
add_index :bookmarks, [:user_id, :post_id, :topic_id], unique: true
end
def down
Bookmark.where(post_id: -1).delete_all
Bookmark.where(post_id: nil).delete_all
remove_index :bookmarks, [:user_id, :post_id, :topic_id], unique: true
change_column_null :bookmarks, :post_id, false
add_index :bookmarks, [:user_id, :post_id], unique: true
end
end

2
lib/bookmark_manager.rb
View File

@ -8,7 +8,7 @@ class BookmarkManager
end
def create(post_id:, topic_id: nil, name: nil, reminder_type: nil, reminder_at: nil, options: {})
if post_id == Bookmark::FOR_TOPIC_POST_ID
if post_id.blank? && topic_id.present?
bookmark_for_topic = true
topic = Topic.find_by(id: topic_id)
else

13
lib/bookmark_query.rb
View File

@ -35,13 +35,22 @@ class BookmarkQuery
pms = Topic.private_messages_for_user(@user)
results = results.merge(topics.or(pms))
results = results.merge(Post.secured(@guardian))
results = results.where(
"posts.post_type IN (?) OR bookmarks.post_id IS NULL", Topic.visible_post_types(@guardian&.user)
)
if @params[:q].present?
term = @params[:q]
bookmark_ts_query = Search.ts_query(term: term)
results = results
.joins("LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.post_id")
.joins(<<~SQL)
LEFT JOIN post_search_data ON post_search_data.post_id = CASE
WHEN bookmarks.post_id IS NULL THEN (
SELECT posts.id FROM posts WHERE post_number = 1 AND topic_id = bookmarks.topic_id
)
WHEN bookmarks.post_id IS NOT NULL THEN bookmarks.post_id
END
SQL
.where(
"bookmarks.name ILIKE :q OR #{bookmark_ts_query} @@ post_search_data.search_data",
q: "%#{term}%"

4
lib/bookmark_reminder_notification_handler.rb
View File

@ -4,7 +4,7 @@ class BookmarkReminderNotificationHandler
def self.send_notification(bookmark)
return if bookmark.blank?
Bookmark.transaction do
if bookmark.post.blank? || bookmark.post.deleted_at.present?
if bookmark.post&.deleted_at.present?
clear_reminder(bookmark)
elsif bookmark.topic
create_notification(bookmark)
@ -31,7 +31,7 @@ class BookmarkReminderNotificationHandler
user.notifications.create!(
notification_type: Notification.types[:bookmark_reminder],
topic_id: bookmark.topic_id,
post_number: bookmark.post.post_number,
post_number: bookmark.for_topic? ? 1 : bookmark.post.post_number,
data: {
topic_title: bookmark.topic.title,
display_username: user.username,

7
lib/topic_view.rb
View File

@ -395,19 +395,20 @@ class TopicView
@topic.bookmarks.exists?(user_id: @user.id)
end
def bookmarked_posts
def bookmarked_items
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
reminder_at: reminder_at,
topic_level: post_id.blank?
}
end
end
def bookmark
return if !has_bookmarks?
@bookmark ||= @topic.bookmarks.where(user_id: @user.id, post_id: Bookmark::FOR_TOPIC_POST_ID).first
@bookmark ||= @topic.bookmarks.where(user_id: @user.id, post_id: nil).first
end
MAX_PARTICIPANTS = 24

18
spec/components/topic_view_spec.rb
View File

@ -408,19 +408,31 @@ describe TopicView do
end
end
context "#bookmarked_posts" do
context "#bookmarked_items" do
let!(:user) { Fabricate(:user) }
let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, post: topic.first_post, user: user) }
let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, post: topic.posts[1], user: user) }
let!(:bookmark3) { Fabricate(:bookmark_next_business_day_reminder, post: nil, user: user, topic: topic) }
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.bookmarked_items
expect(first[:post_id]).to eq(bookmark1.post_id)
expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
expect(first[:topic_level]).to eq(false)
expect(second[:post_id]).to eq(bookmark2.post_id)
expect(second[:reminder_at]).to eq_time(bookmark2.reminder_at)
expect(second[:topic_level]).to eq(false)
end
it "gets the topic level bookmark reminder at for the user" do
topic_view = TopicView.new(topic.id, user)
topic_level_item = topic_view.bookmarked_items.last
expect(topic_level_item[:post_id]).to eq(nil)
expect(topic_level_item[:reminder_at]).to eq(bookmark3.reminder_at)
expect(topic_level_item[:topic_level]).to eq(true)
end
context "when the topic is deleted" do
@ -429,7 +441,7 @@ describe TopicView do
PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
topic.reload
first, second = topic_view.bookmarked_posts
first, second = topic_view.bookmarked_items
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)

2
spec/fabricators/bookmark_fabricator.rb
View File

@ -3,7 +3,7 @@
Fabricator(:bookmark) do
user
post { Fabricate(:post) }
topic { |attrs| attrs[:post].topic }
topic { |attrs| attrs[:post].present? ? attrs[:post].topic : Fabricate(:topic) }
name "This looked interesting"
reminder_type { Bookmark.reminder_types[:tomorrow] }
reminder_at { 1.day.from_now.iso8601 }

10
spec/lib/bookmark_manager_spec.rb
View File

@ -42,22 +42,22 @@ RSpec.describe BookmarkManager do
end
context "when creating a topic-level bookmark" do
it "allows passing Bookmark::FOR_TOPIC_POST_ID as a post ID" do
subject.create(post_id: Bookmark::FOR_TOPIC_POST_ID, topic_id: post.topic.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
it "allows passing nil as a post ID" do
subject.create(post_id: nil, topic_id: post.topic.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
expect(
Bookmark.exists?(user: user, topic: post.topic, post_id: Bookmark::FOR_TOPIC_POST_ID)
Bookmark.exists?(user: user, topic: post.topic, post_id: nil)
).to eq(true)
end
it "when topic is deleted it raises invalid access from guardian check" do
post.topic.trash!
expect { subject.create(post_id: Bookmark::FOR_TOPIC_POST_ID, topic_id: post.topic.id, name: name) }.to raise_error(
expect { subject.create(post_id: nil, topic_id: post.topic.id, name: name) }.to raise_error(
Discourse::InvalidAccess
)
end
it "updates the topic user bookmarked column to true" do
subject.create(post_id: Bookmark::FOR_TOPIC_POST_ID, topic_id: post.topic.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
subject.create(post_id: nil, topic_id: post.topic.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
tu = TopicUser.find_by(user: user)
expect(tu.bookmarked).to eq(true)
tu.update(bookmarked: false)

32
spec/lib/bookmark_query_spec.rb
View File

@ -62,6 +62,28 @@ RSpec.describe BookmarkQuery do
bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
end
context "for bookmarks that are topic level" do
before do
@bookmark3.update(post_id: nil)
@bookmark4.update(post_id: nil)
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 "for a whispered post" do
@ -173,6 +195,7 @@ RSpec.describe BookmarkQuery do
let!(:bookmark3) { Fabricate(:bookmark, user: user, updated_at: 6.days.ago, reminder_type: nil, reminder_at: nil) }
let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago, reminder_type: nil, reminder_at: nil) }
let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago, reminder_type: nil, reminder_at: nil) }
let!(:bookmark6) { Fabricate(:bookmark, user: user, updated_at: 7.days.ago, post: nil, reminder_type: nil, reminder_at: nil) }
it "order defaults to updated_at DESC" do
expect(bookmark_query.list_all.map(&:id)).to eq([
@ -180,7 +203,8 @@ RSpec.describe BookmarkQuery do
bookmark2.id,
bookmark5.id,
bookmark4.id,
bookmark3.id
bookmark3.id,
bookmark6.id
])
end
@ -195,7 +219,8 @@ RSpec.describe BookmarkQuery do
bookmark5.id,
bookmark1.id,
bookmark2.id,
bookmark3.id
bookmark3.id,
bookmark6.id
])
end
@ -220,7 +245,8 @@ RSpec.describe BookmarkQuery do
bookmark4.id,
bookmark1.id,
bookmark2.id,
bookmark5.id
bookmark5.id,
bookmark6.id
])
end
end

13
spec/lib/bookmark_reminder_notification_handler_spec.rb
View File

@ -28,6 +28,19 @@ RSpec.describe BookmarkReminderNotificationHandler do
expect(data["bookmark_name"]).to eq(bookmark.name)
end
it "creates a bookmark reminder notification for a topic-level bookmark" do
bookmark.update(post_id: nil)
subject.send_notification(bookmark)
notif = bookmark.user.notifications.last
expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
expect(notif.topic_id).to eq(bookmark.topic_id)
expect(notif.post_number).to eq(1)
data = JSON.parse(notif.data)
expect(data["topic_title"]).to eq(bookmark.topic.title)
expect(data["display_username"]).to eq(bookmark.user.username)
expect(data["bookmark_name"]).to eq(bookmark.name)
end
it "clears the reminder" do
subject.send_notification(bookmark)
bookmark.reload

4
spec/requests/bookmarks_controller_spec.rb
View File

@ -25,7 +25,7 @@ describe BookmarksController do
it "creates a bookmark for the topic" do
post "/bookmarks.json", params: {
post_id: Bookmark::FOR_TOPIC_POST_ID,
post_id: nil,
topic_id: bookmark_post.topic.id,
reminder_type: "tomorrow",
reminder_at: (Time.zone.now + 1.day).iso8601
@ -34,7 +34,7 @@ describe BookmarksController do
expect(response.status).to eq(200)
expect(
Bookmark.exists?(
user: bookmark_user, post_id: Bookmark::FOR_TOPIC_POST_ID, topic_id: bookmark_post.topic.id
user: bookmark_user, post_id: nil, topic_id: bookmark_post.topic.id
)
).to eq(true)
end