From e1e74abd4ffbc030c836a055c2bfb7b794eb0fa8 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 13 Feb 2020 16:26:02 +1000 Subject: [PATCH] FEATURE: Improving bookmarks part 2 -- Topic Bookmarking (#8954) ### UI Changes If `SiteSetting.enable_bookmarks_with_reminders` is enabled: * Clicking "Bookmark" on a topic will create a new Bookmark record instead of a post + user action * Clicking "Clear Bookmarks" on a topic will delete all the new Bookmark records on a topic * The topic bookmark buttons control the post bookmark flags correctly and vice-versa Disabled selecting the "reminder type" for bookmarks in the UI because the backend functionality is not done yet (of sending users notifications etc.) ### Other Changes * Added delete bookmark route (but no UI yet) * Added a rake task to sync the old PostAction bookmarks to the new Bookmark table, which can be run as many times as we want for a site (it will not create duplicates). --- .../discourse/controllers/bookmark.js.es6 | 1 + .../discourse/models/bookmark.js.es6 | 24 ++++++++ .../javascripts/discourse/models/post.js.es6 | 10 +++- .../javascripts/discourse/models/topic.js.es6 | 16 ++++- .../discourse/routes/app-route-map.js.es6 | 2 +- .../discourse/templates/modal/bookmark.hbs | 6 +- app/controllers/bookmarks_controller.rb | 14 ++++- app/controllers/posts_controller.rb | 3 +- app/controllers/topics_controller.rb | 23 +++++-- app/models/bookmark.rb | 2 +- app/models/post.rb | 1 + app/models/topic.rb | 1 + config/locales/client.en.yml | 4 ++ config/routes.rb | 2 +- ...17035630_populate_topic_id_on_bookmarks.rb | 13 ++++ ...061927_mark_bookmarks_topic_id_not_null.rb | 7 +++ lib/guardian.rb | 2 + lib/guardian/bookmark_guardian.rb | 7 +++ lib/tasks/bookmarks.rake | 60 +++++++++++++++++++ spec/fabricators/bookmark_fabricator.rb | 2 +- spec/requests/bookmarks_controller_spec.rb | 37 +++++++++++- spec/requests/topics_controller_spec.rb | 58 ++++++++++++++++++ .../topic_list_item_serializer_spec.rb | 9 ++- spec/tasks/bookmarks_spec.rb | 45 ++++++++++++++ test/javascripts/acceptance/topic-test.js.es6 | 14 +++++ test/javascripts/helpers/site-settings.js | 5 +- 26 files changed, 342 insertions(+), 26 deletions(-) create mode 100644 app/assets/javascripts/discourse/models/bookmark.js.es6 create mode 100644 db/migrate/20191217035630_populate_topic_id_on_bookmarks.rb create mode 100644 db/migrate/20200203061927_mark_bookmarks_topic_id_not_null.rb create mode 100644 lib/guardian/bookmark_guardian.rb create mode 100644 lib/tasks/bookmarks.rake create mode 100644 spec/tasks/bookmarks_spec.rb diff --git a/app/assets/javascripts/discourse/controllers/bookmark.js.es6 b/app/assets/javascripts/discourse/controllers/bookmark.js.es6 index fd61ec759dd..3be0e107afe 100644 --- a/app/assets/javascripts/discourse/controllers/bookmark.js.es6 +++ b/app/assets/javascripts/discourse/controllers/bookmark.js.es6 @@ -48,6 +48,7 @@ export default Controller.extend(ModalFunctionality, { }, usingMobileDevice: reads("site.mobileView"), + showBookmarkReminderControls: false, @discourseComputed() reminderTypes: () => { diff --git a/app/assets/javascripts/discourse/models/bookmark.js.es6 b/app/assets/javascripts/discourse/models/bookmark.js.es6 new file mode 100644 index 00000000000..2ae4e63e4cd --- /dev/null +++ b/app/assets/javascripts/discourse/models/bookmark.js.es6 @@ -0,0 +1,24 @@ +import { none } from "@ember/object/computed"; +import { computed } from "@ember/object"; +import { ajax } from "discourse/lib/ajax"; +import { Promise } from "rsvp"; +import RestModel from "discourse/models/rest"; + +const Bookmark = RestModel.extend({ + newBookmark: none("id"), + + @computed + get url() { + return Discourse.getURL(`/bookmarks/${this.id}`); + }, + + destroy() { + if (this.newBookmark) return Promise.resolve(); + + return ajax(this.url, { + type: "DELETE" + }); + } +}); + +export default Bookmark; diff --git a/app/assets/javascripts/discourse/models/post.js.es6 b/app/assets/javascripts/discourse/models/post.js.es6 index 6a100d519d4..a08f960ef09 100644 --- a/app/assets/javascripts/discourse/models/post.js.es6 +++ b/app/assets/javascripts/discourse/models/post.js.es6 @@ -353,14 +353,20 @@ const Post = RestModel.extend({ this.appEvents.trigger("post-stream:refresh", { id: this.id }); }, afterSave: reminderAtISO => { - this.set("bookmark_reminder_at", reminderAtISO); + this.setProperties({ + "topic.bookmarked": true, + bookmark_reminder_at: reminderAtISO + }); this.appEvents.trigger("post-stream:refresh", { id: this.id }); } }); } else { this.set("bookmark_reminder_at", null); return Post.destroyBookmark(this.id) - .then(() => this.appEvents.trigger("page:bookmark-post-toggled", this)) + .then(result => { + this.set("topic.bookmarked", result.topic_bookmarked); + this.appEvents.trigger("page:bookmark-post-toggled", this); + }) .catch(error => { this.toggleProperty("bookmarked_with_reminder"); throw new Error(error); diff --git a/app/assets/javascripts/discourse/models/topic.js.es6 b/app/assets/javascripts/discourse/models/topic.js.es6 index 514c3564e6f..fcf1892ec89 100644 --- a/app/assets/javascripts/discourse/models/topic.js.es6 +++ b/app/assets/javascripts/discourse/models/topic.js.es6 @@ -402,6 +402,9 @@ const Topic = RestModel.extend({ this.toggleProperty("bookmarked"); if (bookmark && firstPost) { firstPost.set("bookmarked", true); + if (this.siteSettings.enable_bookmarks_with_reminders) { + firstPost.set("bookmarked_with_reminder", true); + } return [firstPost.id]; } if (!bookmark && posts) { @@ -409,7 +412,14 @@ const Topic = RestModel.extend({ posts.forEach(post => { if (post.get("bookmarked")) { post.set("bookmarked", false); - updated.push(post.get("id")); + updated.push(post.id); + } + if ( + this.siteSettings.enable_bookmarks_with_reminders && + post.get("bookmarked_with_reminder") + ) { + post.set("bookmarked_with_reminder", false); + updated.push(post.id); } }); return updated; @@ -424,7 +434,9 @@ const Topic = RestModel.extend({ const unbookmarkedPosts = []; if (!bookmark && posts) { posts.forEach( - post => post.get("bookmarked") && unbookmarkedPosts.push(post) + post => + (post.get("bookmarked") || post.get("bookmarked_with_reminder")) && + unbookmarkedPosts.push(post) ); } diff --git a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 index 42d8f57f5c7..16720a568ef 100644 --- a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 +++ b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 @@ -48,7 +48,7 @@ export default function() { }); }); - // filters + // filters (e.g. bookmarks, posted, read, unread, latest) Site.currentProp("filters").forEach(filter => { // legacy route this.route(filter + "ParentCategory", { path: "/c/:slug/l/" + filter }); diff --git a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs index c975f699c84..254f1bfbee7 100644 --- a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs +++ b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs @@ -1,4 +1,4 @@ -{{#d-modal-body}} +{{#d-modal-body id="bookmark-reminder-modal"}} {{#conditional-loading-spinner condition=loading}} {{#if errorMessage}}
@@ -13,9 +13,10 @@ {{i18n 'post.bookmarks.name'}} - {{input value=name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}} + {{input value=name name="name" class="bookmark-name" enter=(action "saveAndClose") placeholder=(i18n "post.bookmarks.name_placeholder")}}
+ {{#if showBookmarkReminderControls}}
+ {{/if}}
{{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,