{{d-button label="bookmarks.save" class="btn-primary" action=(action "saveAndClose")}}
diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index d342becadae..4cab4edf042 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -13,7 +13,7 @@ class BookmarksController < ApplicationController
bookmark = Bookmark.create(
user_id: current_user.id,
- topic_id: params[:topic_id],
+ topic_id: Post.select(:topic_id).find(params[:post_id]).topic_id,
post_id: params[:post_id],
name: params[:name],
reminder_type: Bookmark.reminder_types[params[:reminder_type].to_sym],
@@ -23,4 +23,16 @@ class BookmarksController < ApplicationController
return render json: success_json if bookmark.save
render json: failed_json.merge(errors: bookmark.errors.full_messages), status: 400
end
+
+ def destroy
+ params.require(:id)
+
+ bookmark = Bookmark.find_by(id: params[:id])
+ raise Discourse::NotFound if bookmark.blank?
+
+ raise Discourse::InvalidAccess.new if !guardian.can_delete?(bookmark)
+
+ bookmark.destroy
+ render json: success_json
+ end
end
diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
index caef113e502..5da0ded4228 100644
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@@ -514,7 +514,8 @@ class PostsController < ApplicationController
existing_bookmark = Bookmark.find_by(post_id: params[:post_id], user_id: current_user.id)
existing_bookmark.destroy if existing_bookmark.present?
- render json: success_json
+ topic_bookmarked = Bookmark.exists?(topic_id: existing_bookmark.topic_id, user_id: current_user.id)
+ render json: success_json.merge(topic_bookmarked: topic_bookmarked)
end
def wiki
diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index b8931c4dd9b..d26538b8055 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -486,11 +486,15 @@ class TopicsController < ApplicationController
def remove_bookmarks
topic = Topic.find(params[:topic_id].to_i)
- PostAction.joins(:post)
- .where(user_id: current_user.id)
- .where('topic_id = ?', topic.id).each do |pa|
+ if SiteSetting.enable_bookmarks_with_reminders?
+ Bookmark.where(user_id: current_user.id, topic_id: topic.id).destroy_all
+ else
+ PostAction.joins(:post)
+ .where(user_id: current_user.id)
+ .where('topic_id = ?', topic.id).each do |pa|
- PostActionDestroyer.destroy(current_user, pa.post, :bookmark)
+ PostActionDestroyer.destroy(current_user, pa.post, :bookmark)
+ end
end
render body: nil
@@ -543,8 +547,15 @@ class TopicsController < ApplicationController
topic = Topic.find(params[:topic_id].to_i)
first_post = topic.ordered_posts.first
- result = PostActionCreator.create(current_user, first_post, :bookmark)
- return render_json_error(result) if result.failed?
+ if SiteSetting.enable_bookmarks_with_reminders?
+ if Bookmark.exists?(user: current_user, post: first_post)
+ return render_json_error(I18n.t("bookmark.topic_already_bookmarked"), status: 403)
+ end
+ Bookmark.create(user: current_user, post: first_post, topic: topic)
+ else
+ result = PostActionCreator.create(current_user, first_post, :bookmark)
+ return render_json_error(result) if result.failed?
+ end
render body: nil
end
diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index 73a6bd723cf..6958064324b 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -29,7 +29,7 @@ end
#
# id :bigint not null, primary key
# user_id :bigint not null
-# topic_id :bigint
+# topic_id :bigint not null
# post_id :bigint not null
# name :string
# reminder_type :integer
diff --git a/app/models/post.rb b/app/models/post.rb
index 0e582145e35..0a1a70e6cdc 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -37,6 +37,7 @@ class Post < ActiveRecord::Base
has_many :uploads, through: :post_uploads
has_one :post_stat
+ has_many :bookmarks
has_one :incoming_email
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 61b047bfcd1..44d02260aea 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -96,6 +96,7 @@ class Topic < ActiveRecord::Base
belongs_to :category
has_many :category_users, through: :category
has_many :posts
+ has_many :bookmarks
has_many :ordered_posts, -> { order(post_number: :asc) }, class_name: "Post"
has_many :topic_allowed_users
has_many :topic_allowed_groups
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index d79c463d416..2cb628e519a 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -2649,6 +2649,10 @@ en:
name: "Name"
name_placeholder: "Name the bookmark to help jog your memory"
set_reminder: "Set a reminder"
+ actions:
+ delete_bookmark:
+ name: "Delete bookmark"
+ description: "Removes the bookmark from your profile and stops all reminders for the bookmark"
category:
can: "can… "
diff --git a/config/routes.rb b/config/routes.rb
index bb25d996b7a..7e98e510e4c 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -599,7 +599,7 @@ Discourse::Application.routes.draw do
end
end
- resources :bookmarks, only: %i[create]
+ resources :bookmarks, only: %i[create destroy]
resources :notifications, except: :show do
collection do
diff --git a/db/migrate/20191217035630_populate_topic_id_on_bookmarks.rb b/db/migrate/20191217035630_populate_topic_id_on_bookmarks.rb
new file mode 100644
index 00000000000..1918c185ac2
--- /dev/null
+++ b/db/migrate/20191217035630_populate_topic_id_on_bookmarks.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class PopulateTopicIdOnBookmarks < ActiveRecord::Migration[6.0]
+ def up
+ Bookmark.where(topic_id: nil).includes(:post).find_each do |bookmark|
+ bookmark.update_column(:topic_id, bookmark.post.topic_id)
+ end
+ end
+
+ def down
+ raise ActiveRecord::IrreversibleMigration
+ end
+end
diff --git a/db/migrate/20200203061927_mark_bookmarks_topic_id_not_null.rb b/db/migrate/20200203061927_mark_bookmarks_topic_id_not_null.rb
new file mode 100644
index 00000000000..5eacd99216e
--- /dev/null
+++ b/db/migrate/20200203061927_mark_bookmarks_topic_id_not_null.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class MarkBookmarksTopicIdNotNull < ActiveRecord::Migration[6.0]
+ def change
+ change_column_null :bookmarks, :topic_id, false
+ end
+end
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 3ceaf6d7777..080faf78a9c 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -3,6 +3,7 @@
require 'guardian/category_guardian'
require 'guardian/ensure_magic'
require 'guardian/post_guardian'
+require 'guardian/bookmark_guardian'
require 'guardian/topic_guardian'
require 'guardian/user_guardian'
require 'guardian/post_revision_guardian'
@@ -14,6 +15,7 @@ class Guardian
include EnsureMagic
include CategoryGuardian
include PostGuardian
+ include BookmarkGuardian
include TopicGuardian
include UserGuardian
include PostRevisionGuardian
diff --git a/lib/guardian/bookmark_guardian.rb b/lib/guardian/bookmark_guardian.rb
new file mode 100644
index 00000000000..9f449757777
--- /dev/null
+++ b/lib/guardian/bookmark_guardian.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+module BookmarkGuardian
+ def can_delete_bookmark?(bookmark)
+ @user == bookmark.user
+ end
+end
diff --git a/lib/tasks/bookmarks.rake b/lib/tasks/bookmarks.rake
new file mode 100644
index 00000000000..d8f99281cc0
--- /dev/null
+++ b/lib/tasks/bookmarks.rake
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require_dependency "rake_helpers"
+
+##
+# This will create records in the new bookmarks table from PostAction
+# records. The task is idempotent, it will not create additional bookmark
+# records for PostActions that have already been created in the new table.
+# You can provide a sync_limit for a smaller batch run.
+#
+desc "migrates old PostAction bookmarks to the new Bookmark model & table"
+task "bookmarks:sync_to_table", [:sync_limit] => :environment do |_t, args|
+ sync_limit = args[:sync_limit] || 0
+ post_action_bookmarks = PostAction
+ .select('post_actions.id', 'post_actions.post_id', 'posts.topic_id', 'post_actions.user_id')
+ .where(post_action_type_id: PostActionType.types[:bookmark])
+ .joins(:post)
+ .where(deleted_at: nil)
+ .joins('LEFT JOIN bookmarks ON bookmarks.post_id = post_actions.post_id AND bookmarks.user_id = post_actions.user_id')
+ .where('bookmarks.id IS NULL')
+
+ # if sync_limit is provided as an argument this will cap
+ # the number of bookmarks that will be created in a run of
+ # this task (for huge bookmark count communities)
+ if sync_limit > 0
+ post_action_bookmarks = post_action_bookmarks.limit(sync_limit)
+ end
+
+ post_action_bookmark_count = post_action_bookmarks.count('post_actions.id')
+ bookmarks_to_create = []
+ i = 0
+
+ Bookmark.transaction do
+ post_action_bookmarks.find_each(batch_size: 1000) do |pab|
+ RakeHelpers.print_status_with_label("Creating post new bookmarks.......", i, post_action_bookmark_count)
+ now = Time.zone.now
+ bookmarks_to_create << {
+ topic_id: pab.topic_id,
+ post_id: pab.post_id,
+ user_id: pab.user_id,
+ created_at: now,
+ updated_at: now
+ }
+
+ i += 1
+ end
+
+ # this will ignore conflicts in the bookmarks table so
+ # if the user already has a post bookmarked in the new way,
+ # then we don't error and keep on truckin'
+ #
+ # we shouldn't have duplicates here at any rate because of
+ # the above LEFT JOIN but best to be safe knowing this
+ # won't blow up
+ Bookmark.insert_all(bookmarks_to_create)
+ end
+
+ RakeHelpers.print_status_with_label("Bookmark creation complete! ", i, post_action_bookmark_count)
+ puts ""
+end
diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb
index 3234b622808..a92f272e52a 100644
--- a/spec/fabricators/bookmark_fabricator.rb
+++ b/spec/fabricators/bookmark_fabricator.rb
@@ -3,7 +3,7 @@
Fabricator(:bookmark) do
user
post { Fabricate(:post) }
- topic nil
+ topic { |attrs| attrs[:post].topic }
name "This looked interesting"
reminder_type { Bookmark.reminder_types[:tomorrow] }
reminder_at { (Time.now.utc + 1.day).iso8601 }
diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb
index 64e23960f5d..cfe7f6c9836 100644
--- a/spec/requests/bookmarks_controller_spec.rb
+++ b/spec/requests/bookmarks_controller_spec.rb
@@ -5,6 +5,7 @@ require 'rails_helper'
describe BookmarksController do
let(:current_user) { Fabricate(:user) }
let(:bookmark_post) { Fabricate(:post) }
+ let(:bookmark_user) { current_user }
before do
sign_in(current_user)
@@ -13,7 +14,7 @@ describe BookmarksController do
describe "#create" do
context "if the user already has bookmarked the post" do
before do
- Fabricate(:bookmark, post: bookmark_post, user: current_user)
+ Fabricate(:bookmark, post: bookmark_post, user: bookmark_user)
end
it "returns failed JSON with a 422 error" do
@@ -44,4 +45,38 @@ describe BookmarksController do
end
end
end
+
+ describe "#destroy" do
+ let!(:bookmark) { Fabricate(:bookmark, post: bookmark_post, user: bookmark_user) }
+
+ it "destroys the bookmark" do
+ delete "/bookmarks/#{bookmark.id}.json"
+ expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
+ end
+
+ context "if the bookmark has already been destroyed" do
+ it "returns failed JSON with a 403 error" do
+ bookmark.destroy!
+ delete "/bookmarks/#{bookmark.id}.json"
+
+ expect(response.status).to eq(404)
+ expect(JSON.parse(response.body)['errors'].first).to include(
+ I18n.t("not_found")
+ )
+ end
+ end
+
+ context "if the bookmark does not belong to the user" do
+ let(:bookmark_user) { Fabricate(:user) }
+
+ it "returns failed JSON with a 403 error" do
+ delete "/bookmarks/#{bookmark.id}.json"
+
+ expect(response.status).to eq(403)
+ expect(JSON.parse(response.body)['errors'].first).to include(
+ I18n.t("invalid_access")
+ )
+ end
+ end
+ end
end
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index 9ac9edea019..3394894737a 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -2327,6 +2327,64 @@ RSpec.describe TopicsController do
put "/t/#{pm.topic_id}/bookmark.json"
expect(response).to be_forbidden
end
+
+ context "when SiteSetting.enable_bookmarks_with_reminders is true" do
+ before do
+ SiteSetting.enable_bookmarks_with_reminders = true
+ end
+ it "deletes all the bookmarks for the user in the topic" do
+ sign_in(user)
+ post = create_post
+ Fabricate(:bookmark, post: post, topic: post.topic, user: user)
+ put "/t/#{post.topic_id}/remove_bookmarks.json"
+ expect(Bookmark.where(user: user, topic: topic).count).to eq(0)
+ end
+ end
+ end
+
+ describe "#bookmark" do
+ before do
+ sign_in(user)
+ end
+
+ it "should create a new post action for the 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(PostAction.find_by(user_id: user.id, post_action_type: PostActionType.types[:bookmark]).post_id).to eq(post.id)
+ end
+
+ it "errors if the topic is already bookmarked for the user" do
+ post = create_post
+ PostActionCreator.new(user, post, PostActionType.types[:bookmark]).perform
+
+ put "/t/#{post.topic_id}/bookmark.json"
+ expect(response).to be_forbidden
+ end
+
+ context "when SiteSetting.enable_bookmarks_with_reminders is true" do
+ before do
+ SiteSetting.enable_bookmarks_with_reminders = true
+ end
+ 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"
+
+ bookmarks_for_topic = Bookmark.where(topic: post.topic, user: user)
+ 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, topic: post.topic, user: user)
+
+ put "/t/#{post.topic_id}/bookmark.json"
+ expect(response).to be_forbidden
+ end
+ end
end
describe '#reset_new' do
diff --git a/spec/serializers/topic_list_item_serializer_spec.rb b/spec/serializers/topic_list_item_serializer_spec.rb
index f798ec3498c..1ca54ef3117 100644
--- a/spec/serializers/topic_list_item_serializer_spec.rb
+++ b/spec/serializers/topic_list_item_serializer_spec.rb
@@ -6,11 +6,10 @@ describe TopicListItemSerializer do
let(:topic) do
date = Time.zone.now
- Fabricate.build(:topic,
- title: 'test',
+ Fabricate(:topic,
+ title: 'This is a test topic title',
created_at: date - 2.minutes,
- bumped_at: date,
- posters: [],
+ bumped_at: date
)
end
@@ -18,7 +17,7 @@ describe TopicListItemSerializer do
SiteSetting.topic_featured_link_enabled = true
serialized = TopicListItemSerializer.new(topic, scope: Guardian.new, root: false).as_json
- expect(serialized[:title]).to eq("test")
+ expect(serialized[:title]).to eq("This is a test topic title")
expect(serialized[:bumped]).to eq(true)
expect(serialized[:featured_link]).to eq(nil)
expect(serialized[:featured_link_root_domain]).to eq(nil)
diff --git a/spec/tasks/bookmarks_spec.rb b/spec/tasks/bookmarks_spec.rb
new file mode 100644
index 00000000000..16fea02f982
--- /dev/null
+++ b/spec/tasks/bookmarks_spec.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe "bookmarks tasks" do
+ let(:user1) { Fabricate(:user) }
+ let(:user2) { Fabricate(:user) }
+ let(:user3) { Fabricate(:user) }
+ let(:post1) { Fabricate(:post) }
+ let(:post2) { Fabricate(:post) }
+ let(:post3) { Fabricate(:post) }
+
+ before do
+ Rake::Task.clear
+ Discourse::Application.load_tasks
+
+ create_post_actions_and_existing_bookmarks
+ end
+
+ it "migrates all PostActions" do
+ Rake::Task['bookmarks:sync_to_table'].invoke
+
+ expect(Bookmark.all.count).to eq(3)
+ end
+
+ it "does not create bookmarks that already exist in the bookmarks table for a user" do
+ Fabricate(:bookmark, user: user1, post: post1)
+
+ Rake::Task['bookmarks:sync_to_table'].invoke
+
+ expect(Bookmark.all.count).to eq(3)
+ expect(Bookmark.where(post: post1, user: user1).count).to eq(1)
+ end
+
+ it "respects the sync_limit if provided and stops creating bookmarks at the limit (so this can be run progrssively" do
+ Rake::Task['bookmarks:sync_to_table'].invoke(1)
+ expect(Bookmark.all.count).to eq(1)
+ end
+
+ def create_post_actions_and_existing_bookmarks
+ Fabricate(:post_action, user: user1, post: post1, post_action_type_id: PostActionType.types[:bookmark])
+ Fabricate(:post_action, user: user2, post: post2, post_action_type_id: PostActionType.types[:bookmark])
+ Fabricate(:post_action, user: user3, post: post3, post_action_type_id: PostActionType.types[:bookmark])
+ end
+end
diff --git a/test/javascripts/acceptance/topic-test.js.es6 b/test/javascripts/acceptance/topic-test.js.es6
index 2e63209b94c..cf61e32e22d 100644
--- a/test/javascripts/acceptance/topic-test.js.es6
+++ b/test/javascripts/acceptance/topic-test.js.es6
@@ -333,3 +333,17 @@ QUnit.test("Quoting a quote keeps the original poster name", async assert => {
.indexOf('quote="codinghorror said, post:3, topic:280"') !== -1
);
});
+
+acceptance("Topic + Post Bookmarks with Reminders", {
+ loggedIn: true,
+ settings: {
+ enable_bookmarks_with_reminders: true
+ }
+});
+
+QUnit.test("Bookmarks Modal", async assert => {
+ await visit("/t/internationalization-localization/280");
+ await click(".topic-post:first-child button.show-more-actions");
+ await click(".topic-post:first-child button.bookmark");
+ assert.ok(exists("#bookmark-reminder-modal"), "it shows the bookmark modal");
+});
diff --git a/test/javascripts/helpers/site-settings.js b/test/javascripts/helpers/site-settings.js
index 25568eda89d..c4eb5f7f74c 100644
--- a/test/javascripts/helpers/site-settings.js
+++ b/test/javascripts/helpers/site-settings.js
@@ -12,8 +12,8 @@ Discourse.SiteSettingsOriginal = {
ga_universal_tracking_code: "",
ga_universal_domain_name: "auto",
top_menu: "latest|new|unread|categories|top",
- post_menu: "like|share|flag|edit|bookmark|delete|admin|reply",
- post_menu_hidden_items: "flag|edit|delete|admin",
+ post_menu: "like|share|flag|edit|bookmark|bookmarkWithReminder|delete|admin|reply",
+ post_menu_hidden_items: "flag|bookmark|bookmarkWithReminder|edit|delete|admin",
share_links: "twitter|facebook|email",
category_colors:
"BF1E2E|F1592A|F7941D|9EB83B|3AB54A|12A89D|25AAE2|0E76BD|652D90|92278F|ED207B|8C6238|231F20|27AA5B|B3B5B4|E45735",
@@ -28,6 +28,7 @@ Discourse.SiteSettingsOriginal = {
allow_new_registrations: true,
enable_google_logins: true,
enable_google_oauth2_logins: false,
+ enable_bookmarks_with_reminders: false,
enable_twitter_logins: true,
enable_facebook_logins: true,
enable_github_logins: true,