From 628ba9d1e2e6ee4b552aded1625c0ae84c81d78b Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 22 Apr 2020 13:44:19 +1000 Subject: [PATCH] FEATURE: Promote bookmarks with reminders to core functionality (#9369) The main thrust of this PR is to take all the conditional checks based on the `enable_bookmarks_with_reminders` away and only keep the code from the `true` path, making bookmarks with reminders the core bookmarks feature. There is also a migration to create `Bookmark` records out of `PostAction` bookmarks for a site. ### Summary * Remove logic based on whether enable_bookmarks_with_reminders is true. This site setting is now obsolete, the old bookmark functionality is being removed. Retain the setting and set the value to `true` in a migration. * Use the code from the rake task to create a database migration that creates bookmarks from post actions. * Change the bookmark report to read from the new table. * Get rid of old endpoints for bookmarks * Link to the new bookmarks list from the user summary page --- .../controllers/keyboard-shortcuts-help.js | 3 - .../discourse/controllers/topic.js | 5 +- .../discourse/controllers/user-activity.js | 2 - .../javascripts/discourse/models/topic.js | 33 +---- .../javascripts/discourse/routes/discovery.js | 5 +- .../modal/keyboard-shortcuts-help.hbs | 34 +++-- .../discourse/templates/user/activity.hbs | 12 +- .../discourse/templates/user/summary.hbs | 2 +- .../discourse/widgets/post-menu.js | 9 +- .../widgets/quick-access-bookmarks.js | 12 +- .../discourse/widgets/user-menu.js | 5 +- app/controllers/posts_controller.rb | 20 --- app/controllers/topics_controller.rb | 75 +++++------ .../bookmark_reminder_notifications.rb | 2 - app/models/bookmark.rb | 10 ++ app/models/reports/bookmarks.rb | 13 +- app/models/user_summary.rb | 9 +- app/serializers/post_serializer.rb | 2 +- app/serializers/topic_view_serializer.rb | 8 +- config/routes.rb | 1 - config/site_settings.yml | 2 +- ...te_bookmarks_from_post_action_bookmarks.rb | 58 ++++++++ lib/bookmark_reminder_notification_handler.rb | 2 - lib/search.rb | 6 +- lib/tasks/bookmarks.rake | 81 +++++------- .../new_user_narrative.rb | 6 +- plugins/discourse-narrative-bot/plugin.rb | 6 +- .../new_user_narrative_spec.rb | 19 --- .../bookmark_reminder_notifications_spec.rb | 9 -- ...mark_reminder_notification_handler_spec.rb | 12 -- spec/requests/admin/users_controller_spec.rb | 36 +++-- spec/requests/posts_controller_spec.rb | 125 ------------------ spec/requests/topics_controller_spec.rb | 28 ++-- spec/serializers/post_serializer_spec.rb | 16 +-- spec/tasks/bookmarks_spec.rb | 5 - test/javascripts/acceptance/topic-test.js | 5 +- .../controllers/preferences-account-test.js | 2 +- test/javascripts/fixtures/user_fixtures.js | 28 ++++ test/javascripts/helpers/site-settings.js | 1 - test/javascripts/widgets/post-test.js | 10 +- test/javascripts/widgets/user-menu-test.js | 6 +- 41 files changed, 255 insertions(+), 470 deletions(-) create mode 100644 db/migrate/20200409033412_create_bookmarks_from_post_action_bookmarks.rb diff --git a/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js b/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js index a0d42ac0618fa0..2b0df878828356 100644 --- a/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js +++ b/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js @@ -1,6 +1,5 @@ import Controller from "@ember/controller"; import ModalFunctionality from "discourse/mixins/modal-functionality"; -import { setting } from "discourse/lib/computed"; const KEY = "keyboard_shortcuts_help"; @@ -57,8 +56,6 @@ export default Controller.extend(ModalFunctionality, { this.set("shortcuts", null); }, - showBookmarkShortcuts: setting("enable_bookmarks_with_reminders"), - _defineShortcuts() { this.set("shortcuts", { jump_to: { diff --git a/app/assets/javascripts/discourse/controllers/topic.js b/app/assets/javascripts/discourse/controllers/topic.js index eea22c7d88bff8..b934c13543a427 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js +++ b/app/assets/javascripts/discourse/controllers/topic.js @@ -652,10 +652,7 @@ export default Controller.extend(bufferedProperty("model"), { if (!this.currentUser) { return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); } else if (post) { - if (this.siteSettings.enable_bookmarks_with_reminders) { - return post.toggleBookmarkWithReminder(); - } - return post.toggleBookmark().catch(popupAjaxError); + return post.toggleBookmarkWithReminder(); } else { return this.model.toggleBookmark().then(changedIds => { if (!changedIds) { diff --git a/app/assets/javascripts/discourse/controllers/user-activity.js b/app/assets/javascripts/discourse/controllers/user-activity.js index 03828c439b2d48..12333fcf074d7a 100644 --- a/app/assets/javascripts/discourse/controllers/user-activity.js +++ b/app/assets/javascripts/discourse/controllers/user-activity.js @@ -3,7 +3,6 @@ import { inject as service } from "@ember/service"; import Controller, { inject as controller } from "@ember/controller"; import { exportUserArchive } from "discourse/lib/export-csv"; import { observes } from "discourse-common/utils/decorators"; -import { setting } from "discourse/lib/computed"; export default Controller.extend({ application: controller(), @@ -12,7 +11,6 @@ export default Controller.extend({ userActionType: null, canDownloadPosts: alias("user.viewingSelf"), - bookmarksWithRemindersEnabled: setting("enable_bookmarks_with_reminders"), @observes("userActionType", "model.stream.itemsLoaded") _showFooter: function() { diff --git a/app/assets/javascripts/discourse/models/topic.js b/app/assets/javascripts/discourse/models/topic.js index 6ad8de27ec1ca4..1cbab38942ff14 100644 --- a/app/assets/javascripts/discourse/models/topic.js +++ b/app/assets/javascripts/discourse/models/topic.js @@ -400,10 +400,8 @@ const Topic = RestModel.extend({ afterTopicBookmarked(firstPost) { if (firstPost) { firstPost.set("bookmarked", true); - if (this.siteSettings.enable_bookmarks_with_reminders) { - this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at); - firstPost.set("bookmarked_with_reminder", true); - } + firstPost.set("bookmarked_with_reminder", true); + this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at); return [firstPost.id]; } }, @@ -438,24 +436,10 @@ const Topic = RestModel.extend({ return this.firstPost().then(firstPost => { const toggleBookmarkOnServer = () => { if (bookmark) { - if (this.siteSettings.enable_bookmarks_with_reminders) { - return firstPost.toggleBookmarkWithReminder().then(response => { - this.set("bookmarking", false); - if (response && response.closedWithoutSaving) { - this.set("bookmarked", false); - } else { - return this.afterTopicBookmarked(firstPost); - } - }); - } else { - return ajax(`/t/${this.id}/bookmark`, { type: "PUT" }) - .then(() => { - this.toggleProperty("bookmarked"); - return this.afterTopicBookmarked(firstPost); - }) - .catch(popupAjaxError) - .finally(() => this.set("bookmarking", false)); - } + return firstPost.toggleBookmarkWithReminder().then(() => { + this.set("bookmarking", false); + return this.afterTopicBookmarked(firstPost); + }); } else { return ajax(`/t/${this.id}/remove_bookmarks`, { type: "PUT" }) .then(() => { @@ -474,10 +458,7 @@ const Topic = RestModel.extend({ post.set("bookmarked", false); updated.push(post.id); } - if ( - this.siteSettings.enable_bookmarks_with_reminders && - post.bookmarked_with_reminder - ) { + if (post.bookmarked_with_reminder) { post.setProperties(clearedBookmarkProps); updated.push(post.id); } diff --git a/app/assets/javascripts/discourse/routes/discovery.js b/app/assets/javascripts/discourse/routes/discovery.js index 1dd2985c46e8d0..f7bd4a5a06fdd6 100644 --- a/app/assets/javascripts/discourse/routes/discovery.js +++ b/app/assets/javascripts/discourse/routes/discovery.js @@ -17,10 +17,7 @@ export default DiscourseRoute.extend(OpenComposer, { // including being able to show links to multiple posts to the same topic // and being based on a different model. better to just redirect const url = transition.intent.url; - if ( - this.siteSettings.enable_bookmarks_with_reminders && - url === "/bookmarks" - ) { + if (url === "/bookmarks") { this.transitionTo( "userActivity.bookmarksWithReminders", this.currentUser diff --git a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs index 5299b52f3702ea..813ab3a1f65568 100644 --- a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs +++ b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs @@ -55,24 +55,22 @@
  • {{html-safe shortcuts.actions.quote_post}}
    • - {{#if showBookmarkShortcuts}} -
    -

    {{i18n "keyboard_shortcuts_help.bookmarks.title"}}

    - -
    - {{/if}} +
    +

    {{i18n "keyboard_shortcuts_help.bookmarks.title"}}

    + +
    diff --git a/app/assets/javascripts/discourse/templates/user/activity.hbs b/app/assets/javascripts/discourse/templates/user/activity.hbs index 15ac59b652c73a..15b868def222d1 100644 --- a/app/assets/javascripts/discourse/templates/user/activity.hbs +++ b/app/assets/javascripts/discourse/templates/user/activity.hbs @@ -18,15 +18,9 @@ {{#link-to "userActivity.likesGiven"}}{{i18n "user_action_groups.1"}}{{/link-to}} {{#if user.showBookmarks}} - {{#if bookmarksWithRemindersEnabled}} -
  • - {{#link-to "userActivity.bookmarksWithReminders"}}{{i18n "user_action_groups.3"}}{{/link-to}} -
    • - {{else}} -
  • - {{#link-to "userActivity.bookmarks"}}{{i18n "user_action_groups.3"}}{{/link-to}} -
    • - {{/if}} +
  • + {{#link-to 'userActivity.bookmarksWithReminders'}}{{i18n 'user_action_groups.3'}}{{/link-to}} +
    • {{/if}} {{plugin-outlet name="user-activity-bottom" diff --git a/app/assets/javascripts/discourse/templates/user/summary.hbs b/app/assets/javascripts/discourse/templates/user/summary.hbs index ddd89595c52535..ff9c7520f907ee 100644 --- a/app/assets/javascripts/discourse/templates/user/summary.hbs +++ b/app/assets/javascripts/discourse/templates/user/summary.hbs @@ -27,7 +27,7 @@ {{#if model.bookmark_count}}
  • - {{#link-to "userActivity.bookmarks"}} + {{#link-to "userActivity.bookmarksWithReminders"}} {{user-stat value=model.bookmark_count label="user.summary.bookmark_count"}} {{/link-to}}
    • diff --git a/app/assets/javascripts/discourse/widgets/post-menu.js b/app/assets/javascripts/discourse/widgets/post-menu.js index c8374058ee4d4d..98046266bdda48 100644 --- a/app/assets/javascripts/discourse/widgets/post-menu.js +++ b/app/assets/javascripts/discourse/widgets/post-menu.js @@ -302,8 +302,8 @@ registerButton("bookmark", attrs => { }; }); -registerButton("bookmarkWithReminder", (attrs, state, siteSettings) => { - if (!attrs.canBookmark || !siteSettings.enable_bookmarks_with_reminders) { +registerButton("bookmarkWithReminder", attrs => { + if (!attrs.canBookmark) { return; } @@ -469,10 +469,7 @@ export default createWidget("post-menu", { // filter menu items based on site settings const orderedButtons = this.menuItems().filter(button => { - if ( - this.siteSettings.enable_bookmarks_with_reminders && - button === "bookmark" - ) { + if (button === "bookmark") { return false; } return true; diff --git a/app/assets/javascripts/discourse/widgets/quick-access-bookmarks.js b/app/assets/javascripts/discourse/widgets/quick-access-bookmarks.js index 7bbae631984160..787fce384d90c8 100644 --- a/app/assets/javascripts/discourse/widgets/quick-access-bookmarks.js +++ b/app/assets/javascripts/discourse/widgets/quick-access-bookmarks.js @@ -16,11 +16,7 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", { }, showAllHref() { - if (this.siteSettings.enable_bookmarks_with_reminders) { - return `${this.attrs.path}/activity/bookmarks-with-reminders`; - } else { - return `${this.attrs.path}/activity/bookmarks`; - } + return `${this.attrs.path}/activity/bookmarks-with-reminders`; }, emptyStatePlaceholderItem() { @@ -28,11 +24,7 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", { }, findNewItems() { - if (this.siteSettings.enable_bookmarks_with_reminders) { - return this.loadBookmarksWithReminders(); - } else { - return this.loadUserActivityBookmarks(); - } + return this.loadBookmarksWithReminders(); }, itemHtml(bookmark) { diff --git a/app/assets/javascripts/discourse/widgets/user-menu.js b/app/assets/javascripts/discourse/widgets/user-menu.js index 11987297266000..08792c4130c083 100644 --- a/app/assets/javascripts/discourse/widgets/user-menu.js +++ b/app/assets/javascripts/discourse/widgets/user-menu.js @@ -56,16 +56,13 @@ createWidget("user-menu-links", { }, bookmarksGlyph() { - let path = this.siteSettings.enable_bookmarks_with_reminders - ? "bookmarks-with-reminders" - : "bookmarks"; return { action: UserMenuAction.QUICK_ACCESS, actionParam: QuickAccess.BOOKMARKS, label: "user.bookmarks", className: "user-bookmarks-link", icon: "bookmark", - href: `${this.attrs.path}/activity/${path}` + href: `${this.attrs.path}/activity/bookmarks-with-reminders` }; }, diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 50c4ead17f29d9..c440d630f550b5 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -488,26 +488,6 @@ def notice render body: nil end - def bookmark - if params[:bookmarked] == "true" - post = find_post_from_params - result = PostActionCreator.create(current_user, post, :bookmark) - return render_json_error(result) if result.failed? - else - post_action = PostAction.find_by(post_id: params[:post_id], user_id: current_user.id) - raise Discourse::NotFound unless post_action - - post = Post.with_deleted.find_by(id: post_action&.post_id) - raise Discourse::NotFound unless post - - result = PostActionDestroyer.destroy(current_user, post, :bookmark) - return render_json_error(result) if result.failed? - end - - topic_user = TopicUser.get(post.topic, current_user) - render_json_dump(topic_bookmarked: topic_user.try(:bookmarked)) - end - def destroy_bookmark params.require(:post_id) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index fe399566d2cf85..e0ecde628f7a75 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -97,9 +97,9 @@ def show Notification.remove_for(current_user.id, params[:topic_id]) if current_user deleted = guardian.can_see_topic?(ex.obj, false) || - (!guardian.can_see_topic?(ex.obj) && - ex.obj&.access_topic_via_group && - ex.obj.deleted_at) + (!guardian.can_see_topic?(ex.obj) && + ex.obj&.access_topic_via_group && + ex.obj.deleted_at) if SiteSetting.detailed_404 if deleted @@ -231,11 +231,14 @@ def posts fetch_topic_view(options) - render_json_dump(TopicViewPostsSerializer.new(@topic_view, - scope: guardian, - root: false, - include_raw: !!params[:include_raw] - )) + render_json_dump( + TopicViewPostsSerializer.new( + @topic_view, + scope: guardian, + root: false, + include_raw: !!params[:include_raw] + ) + ) end def excerpts @@ -261,12 +264,12 @@ def excerpts .joins("LEFT JOIN users u on u.id = posts.user_id") .pluck(:id, :cooked, :username) .map do |post_id, cooked, username| - { - post_id: post_id, - username: username, - excerpt: PrettyText.excerpt(cooked, 800, keep_emoji_images: true) - } - end + { + post_id: post_id, + username: username, + excerpt: PrettyText.excerpt(cooked, 800, keep_emoji_images: true) + } + end render json: @posts.to_json end @@ -490,18 +493,7 @@ def remove_banner def remove_bookmarks topic = Topic.find(params[:topic_id].to_i) - - if SiteSetting.enable_bookmarks_with_reminders? - BookmarkManager.new(current_user).destroy_for_topic(topic) - 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) - end - end - + BookmarkManager.new(current_user).destroy_for_topic(topic) render body: nil end @@ -552,16 +544,11 @@ def bookmark topic = Topic.find(params[:topic_id].to_i) first_post = topic.ordered_posts.first - if SiteSetting.enable_bookmarks_with_reminders? - bookmark_manager = BookmarkManager.new(current_user) - bookmark_manager.create(post_id: first_post.id) + bookmark_manager = BookmarkManager.new(current_user) + bookmark_manager.create(post_id: first_post.id) - if bookmark_manager.errors.any? - return render_json_error(bookmark_manager, status: 400) - end - else - result = PostActionCreator.create(current_user, first_post, :bookmark) - return render_json_error(result) if result.failed? + if bookmark_manager.errors.any? + return render_json_error(bookmark_manager, status: 400) end render body: nil @@ -628,9 +615,9 @@ def invite_group topic = Topic.find_by(id: params[:topic_id]) unless pm_has_slots?(topic) - return render_json_error(I18n.t("pm_reached_recipients_limit", - recipients_limit: SiteSetting.max_allowed_message_recipients - )) + return render_json_error( + I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients) + ) end if topic.private_message? @@ -654,9 +641,9 @@ def invite ) unless pm_has_slots?(topic) - return render_json_error(I18n.t("pm_reached_recipients_limit", - recipients_limit: SiteSetting.max_allowed_message_recipients - )) + return render_json_error( + I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients) + ) end guardian.ensure_can_invite_to!(topic) @@ -683,7 +670,8 @@ def invite if group_names.present? json.merge!(errors: [ - I18n.t("topic_invite.failed_to_invite", + I18n.t( + "topic_invite.failed_to_invite", group_names: group_names ) ]) @@ -1011,7 +999,8 @@ def perform_show_response return end - topic_view_serializer = TopicViewSerializer.new(@topic_view, + topic_view_serializer = TopicViewSerializer.new( + @topic_view, scope: guardian, root: false, include_raw: !!params[:include_raw] diff --git a/app/jobs/scheduled/bookmark_reminder_notifications.rb b/app/jobs/scheduled/bookmark_reminder_notifications.rb index 20ce9cb8a8ed9d..991e0d23c1deb6 100644 --- a/app/jobs/scheduled/bookmark_reminder_notifications.rb +++ b/app/jobs/scheduled/bookmark_reminder_notifications.rb @@ -21,8 +21,6 @@ def self.max_reminder_notifications_per_run=(max) end def execute(args = nil) - return if !SiteSetting.enable_bookmarks_with_reminders? - bookmarks = Bookmark.pending_reminders .where.not(reminder_type: Bookmark.reminder_types[:at_desktop]) .includes(:user).order('reminder_at ASC') diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index f34e58c69585d1..2dc9733e6cb226 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -72,6 +72,16 @@ def self.reminder_types later_this_week: 8 ) end + + def self.count_per_day(opts = nil) + opts ||= {} + result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago) + result = result.where('bookmarks.created_at <= ?', opts[:end_date]) if opts[:end_date] + result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id])) if opts[:category_id] + result.group('date(bookmarks.created_at)') + .order('date(bookmarks.created_at)') + .count + end end # == Schema Information diff --git a/app/models/reports/bookmarks.rb b/app/models/reports/bookmarks.rb index f4728e1f58fac6..0844efb33d2008 100644 --- a/app/models/reports/bookmarks.rb +++ b/app/models/reports/bookmarks.rb @@ -3,5 +3,16 @@ Report.add_report('bookmarks') do |report| report.icon = 'bookmark' - post_action_report report, PostActionType.types[:bookmark] + category_filter = report.filters.dig(:category) + report.add_filter('category', default: category_filter) + + report.data = [] + Bookmark.count_per_day( + category_id: category_filter, + start_date: report.start_date, + end_date: report.end_date + ).each do |date, count| + report.data << { x: date, y: count } + end + add_counts report, Bookmark, 'bookmarks.created_at' end diff --git a/app/models/user_summary.rb b/app/models/user_summary.rb index 52c919957738a4..435702d67b339e 100644 --- a/app/models/user_summary.rb +++ b/app/models/user_summary.rb @@ -117,14 +117,7 @@ def user_stat end def bookmark_count - if SiteSetting.enable_bookmarks_with_reminders - Bookmark.where(user: @user).count - else - UserAction - .where(user: @user) - .where(action_type: UserAction::BOOKMARK) - .count - end + Bookmark.where(user: @user).count end def recent_time_read diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index ff880f14c385c8..7bf90b98ebdfca 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -345,7 +345,7 @@ def include_bookmark_id? end def post_bookmark - return nil if !SiteSetting.enable_bookmarks_with_reminders? || @topic_view.blank? + return nil if @topic_view.blank? @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id } end diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 4bace8d9edcfef..e40dca1d9fb3c7 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -186,15 +186,11 @@ def include_expandable_first_post? end def bookmarked - if SiteSetting.enable_bookmarks_with_reminders? - object.has_bookmarks? - else - object.topic_user&.bookmarked - end + object.has_bookmarks? end def include_bookmark_reminder_at? - SiteSetting.enable_bookmarks_with_reminders? && bookmarked + bookmarked end def bookmark_reminder_at diff --git a/config/routes.rb b/config/routes.rb index 2fc21b435ee807..9be2b3f44da1cc 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -585,7 +585,6 @@ put "admin/groups/:id/members" => "groups#add_members", constraints: AdminConstraint.new resources :posts do - put "bookmark" delete "bookmark", to: "posts#destroy_bookmark" put "wiki" put "post_type" diff --git a/config/site_settings.yml b/config/site_settings.yml index 0828616904e6b0..986fc3ba38c434 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -291,7 +291,7 @@ basic: default: false enable_bookmarks_with_reminders: client: true - default: false + default: true hidden: true enable_bookmark_at_desktop_reminders: client: true diff --git a/db/migrate/20200409033412_create_bookmarks_from_post_action_bookmarks.rb b/db/migrate/20200409033412_create_bookmarks_from_post_action_bookmarks.rb new file mode 100644 index 00000000000000..092a0e7b99246d --- /dev/null +++ b/db/migrate/20200409033412_create_bookmarks_from_post_action_bookmarks.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +class CreateBookmarksFromPostActionBookmarks < ActiveRecord::Migration[6.0] + def up + SiteSetting.enable_bookmarks_with_reminders = true + + bookmarks_to_create = [] + loop do + # post action type id 1 is :bookmark. we do not need to OFFSET here for + # paging because the WHERE bookmarks.id IS NULL clause handles this effectively, + # because we do not get bookmarks back that have already been inserted + post_action_bookmarks = DB.query( + <<~SQL, type_id: 1 + SELECT post_actions.id, post_actions.post_id, posts.topic_id, post_actions.user_id + FROM post_actions + INNER JOIN posts ON posts.id = post_actions.post_id + LEFT JOIN bookmarks ON bookmarks.post_id = post_actions.post_id AND bookmarks.user_id = post_actions.user_id + INNER JOIN topics ON topics.id = posts.topic_id + WHERE bookmarks.id IS NULL AND post_action_type_id = :type_id AND post_actions.deleted_at IS NULL AND posts.deleted_at IS NULL + LIMIT 2000 + SQL + ) + break if post_action_bookmarks.count.zero? + + post_action_bookmarks.each do |pab| + now = Time.zone.now + bookmarks_to_create << "(#{pab.topic_id}, #{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')" + end + + create_bookmarks(bookmarks_to_create) + bookmarks_to_create = [] + end # loop + end + + def down + raise ActiveRecord::IrreversibleMigration + end + + def create_bookmarks(bookmarks_to_create) + return if bookmarks_to_create.empty? + + # 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 + # + DB.exec( + <<~SQL + INSERT INTO bookmarks (topic_id, post_id, user_id, created_at, updated_at) + VALUES #{bookmarks_to_create.join(",\n")} + ON CONFLICT DO NOTHING + SQL + ) + end +end diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index 4591719fdc0fd2..3354be39494965 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -52,8 +52,6 @@ def self.cache_pending_at_desktop_reminder(user) end def self.send_at_desktop_reminder(user:, request_user_agent:) - return if !SiteSetting.enable_bookmarks_with_reminders - return if MobileDetection.mobile_device?(request_user_agent) return if !user_has_pending_at_desktop_reminders?(user) diff --git a/lib/search.rb b/lib/search.rb index bcbf5107e92552..ead773d097caa0 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -381,11 +381,7 @@ def post_action_type_filter(posts, post_action_type) advanced_filter(/^in:(bookmarks)$/) do |posts, match| if @guardian.user - if SiteSetting.enable_bookmarks_with_reminders - posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})") - else - post_action_type_filter(posts, PostActionType.types[:bookmark]) - end + posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})") end end diff --git a/lib/tasks/bookmarks.rake b/lib/tasks/bookmarks.rake index 833a5f80152643..d3cf545d2d08a5 100644 --- a/lib/tasks/bookmarks.rake +++ b/lib/tasks/bookmarks.rake @@ -9,58 +9,40 @@ require_dependency "rake_helpers" # 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') - .joins('INNER JOIN topics ON topics.id = posts.topic_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') - i = 0 - +task "bookmarks:sync_to_table" => :environment do |_t, args| bookmarks_to_create = [] - post_action_bookmarks.find_each(batch_size: 2000) 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 - - # once we get to 2000 records to create, insert them all and reset - # the array and counter to make sure we don't have too many in memory - if i >= 2000 - create_bookmarks(bookmarks_to_create) - i = 0 - bookmarks_to_create = [] + loop do + # post action type id 1 is :bookmark. we do not need to OFFSET here for + # paging because the WHERE bookmarks.id IS NULL clause handles this effectively, + # because we do not get bookmarks back that have already been inserted + post_action_bookmarks = DB.query( + <<~SQL, type_id: 1 + SELECT post_actions.id, post_actions.post_id, posts.topic_id, post_actions.user_id + FROM post_actions + INNER JOIN posts ON posts.id = post_actions.post_id + LEFT JOIN bookmarks ON bookmarks.post_id = post_actions.post_id AND bookmarks.user_id = post_actions.user_id + INNER JOIN topics ON topics.id = posts.topic_id + WHERE bookmarks.id IS NULL AND post_action_type_id = :type_id AND post_actions.deleted_at IS NULL AND posts.deleted_at IS NULL + LIMIT 2000 + SQL + ) + break if post_action_bookmarks.count.zero? + + post_action_bookmarks.each do |pab| + now = Time.zone.now + bookmarks_to_create << "(#{pab.topic_id}, #{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')" end - end - # if there was < 2000 bookmarks this finishes off the last ones - create_bookmarks(bookmarks_to_create) + create_bookmarks(bookmarks_to_create) + bookmarks_to_create = [] + end # loop - RakeHelpers.print_status_with_label("Bookmark creation complete! ", i, post_action_bookmark_count) - puts "" + puts "Bookmark creation complete!" end def create_bookmarks(bookmarks_to_create) + return if bookmarks_to_create.empty? + # 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' @@ -68,5 +50,12 @@ def create_bookmarks(bookmarks_to_create) # 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) + # + DB.exec( + <<~SQL + INSERT INTO bookmarks (topic_id, post_id, user_id, created_at, updated_at) + VALUES #{bookmarks_to_create.join(",\n")} + ON CONFLICT DO NOTHING + SQL + ) end diff --git a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb index 89afd4eb8702f0..072c54beafb1cb 100644 --- a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb +++ b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb @@ -238,11 +238,7 @@ def reply_to_bookmark return unless @post.user_id == self.discobot_user.id profile_page_url = url_helpers(:user_url, username: @user.username) - bookmark_url = if SiteSetting.enable_bookmarks_with_reminders? - "#{profile_page_url}/activity/bookmarks-with-reminders" - else - "#{profile_page_url}/activity/bookmarks" - end + bookmark_url = "#{profile_page_url}/activity/bookmarks-with-reminders" raw = <<~RAW #{I18n.t("#{I18N_KEY}.bookmark.reply", i18n_post_args(bookmark_url: bookmark_url))} diff --git a/plugins/discourse-narrative-bot/plugin.rb b/plugins/discourse-narrative-bot/plugin.rb index 3acbcc7a9204f1..29766f640e8de3 100644 --- a/plugins/discourse-narrative-bot/plugin.rb +++ b/plugins/discourse-narrative-bot/plugin.rb @@ -241,10 +241,8 @@ def fetch_avatar_url(user) end self.add_model_callback(Bookmark, :after_commit, on: :create) do - if SiteSetting.enable_bookmarks_with_reminders? - if self.post && self.user.enqueue_narrative_bot_job? - Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: :bookmark) - end + if self.post && self.user.enqueue_narrative_bot_job? + Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: :bookmark) end end diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb index 43ff1ac52a5886..d875de3b10094c 100644 --- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb +++ b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb @@ -247,26 +247,7 @@ end end - it 'should create the right reply' do - post.update!(user: discobot_user) - narrative.expects(:enqueue_timeout_job).with(user) - - narrative.input(:bookmark, user, post: post) - new_post = Post.last - profile_page_url = "#{Discourse.base_url}/u/#{user.username}" - - expected_raw = <<~RAW - #{I18n.t('discourse_narrative_bot.new_user_narrative.bookmark.reply', bookmark_url: "#{profile_page_url}/activity/bookmarks", base_uri: '')} - - #{I18n.t('discourse_narrative_bot.new_user_narrative.onebox.instructions', base_uri: '')} - RAW - - expect(new_post.raw).to eq(expected_raw.chomp) - expect(narrative.get_data(user)[:state].to_sym).to eq(:tutorial_onebox) - end - it 'should create the right reply when bookmarks with reminders are enabled' do - SiteSetting.enable_bookmarks_with_reminders = true post.update!(user: discobot_user) narrative.expects(:enqueue_timeout_job).with(user) diff --git a/spec/jobs/bookmark_reminder_notifications_spec.rb b/spec/jobs/bookmark_reminder_notifications_spec.rb index 92ae10002065ff..58f72625036f94 100644 --- a/spec/jobs/bookmark_reminder_notifications_spec.rb +++ b/spec/jobs/bookmark_reminder_notifications_spec.rb @@ -18,7 +18,6 @@ end before do - SiteSetting.enable_bookmarks_with_reminders = true # this is done to avoid model validations on Bookmark bookmark1.update_column(:reminder_at, five_minutes_ago - 10.minutes) bookmark2.update_column(:reminder_at, five_minutes_ago - 5.minutes) @@ -63,14 +62,6 @@ expect(Discourse.redis.get(described_class::JOB_RUN_NUMBER_KEY)).to eq('1') end - context "when the bookmark with reminder site setting is disabled" do - it "does nothing" do - Bookmark.expects(:where).never - SiteSetting.enable_bookmarks_with_reminders = false - subject.execute - end - end - context "when one of the bookmark reminder types is at_desktop" do let(:bookmark1) { Fabricate(:bookmark, reminder_type: :at_desktop) } it "is not included in the run, because it is not time-based" do diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb index 8cc1c4b63790d7..318a52429aad9e 100644 --- a/spec/lib/bookmark_reminder_notification_handler_spec.rb +++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb @@ -8,7 +8,6 @@ fab!(:user) { Fabricate(:user) } before do - SiteSetting.enable_bookmarks_with_reminders = true Discourse.redis.flushall end @@ -89,17 +88,6 @@ end end - context "when enable bookmarks with reminders is disabled" do - before do - SiteSetting.enable_bookmarks_with_reminders = false - end - - it "does nothing" do - BrowserDetection.expects(:device).never - send_reminder - end - end - def send_reminder subject.send_at_desktop_reminder(user: user, request_user_agent: user_agent) end diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index cb654a08c6cbdc..984297dadb88ff 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -124,11 +124,11 @@ end describe '#suspend' do - fab!(:post) { Fabricate(:post) } + fab!(:created_post) { Fabricate(:post) } let(:suspend_params) do { suspend_until: 5.hours.from_now, reason: "because of this post", - post_id: post.id } + post_id: created_post.id } end it "works properly" do @@ -156,13 +156,13 @@ expect(response.status).to eq(200) log = UserHistory.where(target_user_id: user.id).order('id desc').first - expect(log.post_id).to eq(post.id) + expect(log.post_id).to eq(created_post.id) end it "can delete an associated post" do put "/admin/users/#{user.id}/suspend.json", params: suspend_params.merge(post_action: 'delete') - post.reload - expect(post.deleted_at).to be_present + created_post.reload + expect(created_post.deleted_at).to be_present expect(response.status).to eq(200) end @@ -200,17 +200,17 @@ reply = PostCreator.create( Fabricate(:user), raw: 'this is the reply text', - reply_to_post_number: post.post_number, - topic_id: post.topic_id + reply_to_post_number: created_post.post_number, + topic_id: created_post.topic_id ) nested_reply = PostCreator.create( Fabricate(:user), raw: 'this is the reply text2', reply_to_post_number: reply.post_number, - topic_id: post.topic_id + topic_id: created_post.topic_id ) put "/admin/users/#{user.id}/suspend.json", params: suspend_params.merge(post_action: 'delete_replies') - expect(post.reload.deleted_at).to be_present + expect(created_post.reload.deleted_at).to be_present expect(reply.reload.deleted_at).to be_present expect(nested_reply.reload.deleted_at).to be_present expect(response.status).to eq(200) @@ -223,9 +223,9 @@ ) expect(response.status).to eq(200) - post.reload - expect(post.deleted_at).to be_blank - expect(post.raw).to eq("this is the edited content") + created_post.reload + expect(created_post.deleted_at).to be_blank + expect(created_post.raw).to eq("this is the edited content") expect(response.status).to eq(200) end end @@ -252,9 +252,8 @@ it "also prevents use of any api keys" do api_key = Fabricate(:api_key, user: user) - - put "/posts/#{Fabricate(:post).id}/bookmark.json", params: { - bookmarked: "true" + post "/bookmarks.json", params: { + post_id: Fabricate(:post).id }, headers: { HTTP_API_KEY: api_key.key } expect(response.status).to eq(200) @@ -264,10 +263,9 @@ user.reload expect(user).to be_suspended - put "/posts/#{Fabricate(:post).id}/bookmark.json", params: { - bookmarked: "true", - api_key: api_key.key - } + post "/bookmarks.json", params: { + post_id: Fabricate(:post).id + }, headers: { HTTP_API_KEY: api_key.key } expect(response.status).to eq(403) end end diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 09cefae4a0b560..af25d9c00066a5 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -482,131 +482,6 @@ end end - describe '#bookmark' do - include_examples 'action requires login', :put, "/posts/2/bookmark.json" - let!(:post) { post_by_user } - - describe 'when logged in' do - before do - sign_in(user) - end - - fab!(:private_message) { Fabricate(:private_message_post) } - - it "raises an error if the user doesn't have permission to see the post" do - put "/posts/#{private_message.id}/bookmark.json", params: { bookmarked: "true" } - expect(response).to be_forbidden - end - - it 'creates a bookmark' do - put "/posts/#{post.id}/bookmark.json", params: { bookmarked: "true" } - expect(response.status).to eq(200) - - post_action = PostAction.find_by(user: user, post: post) - expect(post_action.post_action_type_id).to eq(PostActionType.types[:bookmark]) - end - - context "removing a bookmark" do - let(:post_action) { PostActionCreator.create(user, post, :bookmark).post_action } - - it "returns the right response when post is not bookmarked" do - put "/posts/#{post_by_user.id}/bookmark.json" - expect(response.status).to eq(404) - end - - it "should be able to remove a bookmark" do - post_action - put "/posts/#{post.id}/bookmark.json" - - expect(PostAction.find_by(id: post_action.id)).to eq(nil) - end - - describe "when user doesn't have permission to see bookmarked post" do - it "should still be able to remove a bookmark" do - post_action - post = post_action.post - topic = post.topic - topic.convert_to_private_message(admin) - topic.remove_allowed_user(admin, user.username) - - expect(Guardian.new(user).can_see_post?(post.reload)).to eq(false) - - put "/posts/#{post.id}/bookmark.json" - - expect(PostAction.find_by(id: post_action.id)).to eq(nil) - end - end - - describe "when post has been deleted" do - it "should still be able to remove a bookmark" do - post = post_action.post - post.trash! - - put "/posts/#{post.id}/bookmark.json" - - expect(PostAction.find_by(id: post_action.id)).to eq(nil) - end - end - end - end - - context "api" do - let(:api_key) { Fabricate(:api_key, user: user) } - let(:master_key) { Fabricate(:api_key, user: nil) } - - # choosing an arbitrarily easy to mock trusted activity - it 'allows users with api key to bookmark posts' do - put "/posts/#{post.id}/bookmark.json", - params: { bookmarked: "true" }, - headers: { HTTP_API_KEY: api_key.key } - - expect(response.status).to eq(200) - expect(PostAction.where( - post: post, - user: user, - post_action_type_id: PostActionType.types[:bookmark] - ).count).to eq(1) - end - - it 'raises an error with a user key that does not match an optionally specified username' do - put "/posts/#{post.id}/bookmark.json", - params: { bookmarked: "true" }, - headers: { HTTP_API_KEY: api_key.key, HTTP_API_USERNAME: 'made_up' } - - expect(response.status).to eq(403) - end - - it 'allows users with a master api key to bookmark posts' do - put "/posts/#{post.id}/bookmark.json", - params: { bookmarked: "true" }, - headers: { HTTP_API_KEY: master_key.key, HTTP_API_USERNAME: user.username } - - expect(response.status).to eq(200) - expect(PostAction.where( - post: post, - user: user, - post_action_type_id: PostActionType.types[:bookmark] - ).count).to eq(1) - end - - it 'disallows phonies to bookmark posts' do - put "/posts/#{post.id}/bookmark.json", - params: { bookmarked: "true" }, - headers: { HTTP_API_KEY: SecureRandom.hex(32), HTTP_API_USERNAME: user.username } - - expect(response.status).to eq(403) - end - - it 'disallows blank api' do - put "/posts/#{post.id}/bookmark.json", - params: { bookmarked: "true" }, - headers: { HTTP_API_KEY: "", HTTP_API_USERNAME: user.username } - - expect(response.status).to eq(403) - end - end - end - describe '#wiki' do include_examples "action requires login", :put, "/posts/2/wiki.json" diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 751c8bf88b8f77..e48a2f94830421 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2346,19 +2346,15 @@ def headers(locale) describe '#remove_bookmarks' do it "should remove bookmarks properly from non first post" do - bookmark = PostActionType.types[:bookmark] sign_in(user) post = create_post post2 = create_post(topic_id: post.topic_id) - - PostActionCreator.new(user, post2, bookmark).perform - - put "/t/#{post.topic_id}/bookmark.json" - expect(PostAction.where(user_id: user.id, post_action_type: bookmark).count).to eq(2) + Fabricate(:bookmark, user: user, post: post) + Fabricate(:bookmark, user: user, post: post2) put "/t/#{post.topic_id}/remove_bookmarks.json" - expect(PostAction.where(user_id: user.id, post_action_type: bookmark).count).to eq(0) + expect(Bookmark.where(user: user).count).to eq(0) end it "should disallow bookmarks on posts you have no access to" do @@ -2369,10 +2365,7 @@ def headers(locale) 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 + context "bookmarks with reminders" do it "deletes all the bookmarks for the user in the topic" do sign_in(user) post = create_post @@ -2388,26 +2381,23 @@ def headers(locale) sign_in(user) end - it "should create a new post action for the bookmark on the first post of the topic" do + it "should create a new bookmark on the first post of the topic" do post = create_post post2 = create_post(topic_id: post.topic_id) put "/t/#{post.topic_id}/bookmark.json" - expect(PostAction.find_by(user_id: user.id, post_action_type: PostActionType.types[:bookmark]).post_id).to eq(post.id) + expect(Bookmark.find_by(user_id: user.id).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 + Bookmark.create(post: post, user: user, topic: post.topic) put "/t/#{post.topic_id}/bookmark.json" - expect(response).to be_forbidden + expect(response.status).to eq(400) end - context "when SiteSetting.enable_bookmarks_with_reminders is true" do - before do - SiteSetting.enable_bookmarks_with_reminders = true - end + context "bookmarks with reminders" do it "should create a new bookmark on the first post of the topic" do post = create_post post2 = create_post(topic_id: post.topic_id) diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index a438a06ea9df08..8791db56c6ef20 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -254,11 +254,7 @@ def json_for_user(user) context "when a Bookmark record exists for the user on the post" do let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post) } - context "when the site setting for bookmarks with reminders is enabled" do - before do - SiteSetting.enable_bookmarks_with_reminders = true - end - + context "bookmarks with reminders" do it "returns true" do expect(serialized.as_json[:bookmarked_with_reminder]).to eq(true) end @@ -275,16 +271,6 @@ def json_for_user(user) end end end - - context "when the site setting for bookmarks with reminders is disabled" do - it "does not return the bookmarked_with_reminder attribute" do - expect(serialized.as_json.key?(:bookmarked_with_reminder)).to eq(false) - end - - it "does not return the bookmark_reminder_at attribute" do - expect(serialized.as_json.key?(:bookmark_reminder_at)).to eq(false) - end - end end end diff --git a/spec/tasks/bookmarks_spec.rb b/spec/tasks/bookmarks_spec.rb index 91249c59f89626..c7b2c65bf6cc31 100644 --- a/spec/tasks/bookmarks_spec.rb +++ b/spec/tasks/bookmarks_spec.rb @@ -38,11 +38,6 @@ def invoke_task(args = nil) 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 - invoke_task(1) - expect(Bookmark.all.count).to eq(1) - end - it "skips post actions where the post topic no longer exists and does not error" do post1.topic.delete post1.reload diff --git a/test/javascripts/acceptance/topic-test.js b/test/javascripts/acceptance/topic-test.js index b5e2684e6f2237..b7edc5cc956794 100644 --- a/test/javascripts/acceptance/topic-test.js +++ b/test/javascripts/acceptance/topic-test.js @@ -397,10 +397,7 @@ QUnit.test( ); acceptance("Topic + Post Bookmarks with Reminders", { - loggedIn: true, - settings: { - enable_bookmarks_with_reminders: true - } + loggedIn: true }); QUnit.test("Bookmarks Modal", async assert => { diff --git a/test/javascripts/controllers/preferences-account-test.js b/test/javascripts/controllers/preferences-account-test.js index f953fb0332ba3d..f79642e2bd3064 100644 --- a/test/javascripts/controllers/preferences-account-test.js +++ b/test/javascripts/controllers/preferences-account-test.js @@ -12,7 +12,7 @@ QUnit.test("updating of associated accounts", function(assert) { is_anonymous: true }), currentUser: EmberObject.create({ - id: 1234, + id: 1234 }), site: EmberObject.create({ isMobileDevice: false diff --git a/test/javascripts/fixtures/user_fixtures.js b/test/javascripts/fixtures/user_fixtures.js index f6a2f870503aa3..b22180109da03d 100644 --- a/test/javascripts/fixtures/user_fixtures.js +++ b/test/javascripts/fixtures/user_fixtures.js @@ -397,6 +397,34 @@ export default { featured_user_badge_ids: [5870, 40673, 5868] } }, + "/u/eviltrout/bookmarks.json": { + user_bookmark_list: { + bookmarks: [ + { + excerpt: "Here this is my new topic where I yell.", + tags: [], + id: 576, + created_at: "2020-04-07T05:30:40.446Z", + topic_id: 119, + linked_post_number: 1, + post_id: 281, + name: "test", + reminder_at: null, + title: "Yelling topic title :/", + deleted: false, + hidden: false, + category_id: 1, + closed: false, + archived: false, + archetype: "regular", + highest_post_number: 5, + bumped_at: "2020-04-06T05:20:00.172Z", + slug: "yelling-topic-title", + username: "someguy" + } + ] + } + }, "/user_actions.json": { user_actions: [ { diff --git a/test/javascripts/helpers/site-settings.js b/test/javascripts/helpers/site-settings.js index c39424b0dfc079..bf55374eb85371 100644 --- a/test/javascripts/helpers/site-settings.js +++ b/test/javascripts/helpers/site-settings.js @@ -31,7 +31,6 @@ 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, diff --git a/test/javascripts/widgets/post-test.js b/test/javascripts/widgets/post-test.js index 7c19c715e68842..b7131fc2a5b801 100644 --- a/test/javascripts/widgets/post-test.js +++ b/test/javascripts/widgets/post-test.js @@ -532,19 +532,19 @@ widgetTest("can't bookmark", { widgetTest("bookmark", { template: - '{{mount-widget widget="post" args=args toggleBookmark=(action "toggleBookmark")}}', + '{{mount-widget widget="post" args=args toggleBookmarkWithReminder=(action "toggleBookmarkWithReminder")}}', beforeEach() { const args = { canBookmark: true }; this.set("args", args); - this.on("toggleBookmark", () => (args.bookmarked = true)); + this.on( + "toggleBookmarkWithReminder", + () => (args.bookmarked_with_reminder = true) + ); }, async test(assert) { assert.equal(find(".post-menu-area .bookmark").length, 1); assert.equal(find("button.bookmarked").length, 0); - - await click("button.bookmark"); - assert.equal(find("button.bookmarked").length, 1); } }); diff --git a/test/javascripts/widgets/user-menu-test.js b/test/javascripts/widgets/user-menu-test.js index 35d73a8542315e..39f2a337220021 100644 --- a/test/javascripts/widgets/user-menu-test.js +++ b/test/javascripts/widgets/user-menu-test.js @@ -145,11 +145,9 @@ widgetTest("bookmarks", { const bookmark = find(".quick-access-panel li a")[0]; assert.ok(bookmark); + assert.ok(bookmark.href.includes("/t/yelling-topic-title/119")); assert.ok( - bookmark.href.includes("/t/how-to-check-the-user-level-via-ajax/11993") - ); - assert.ok( - bookmark.innerHTML.includes("Abhishek_Gupta"), + bookmark.innerHTML.includes("someguy"), "should include the last poster's username" ); assert.ok(