From 793f39139acee67e15bdcce0bc39acef5b351b54 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 12 Mar 2020 10:16:00 +1000 Subject: [PATCH] FEATURE: Send notifications for time-based and At Desktop bookmark reminders (#9071) * This PR implements the scheduling and notification system for bookmark reminders. Every 5 minutes a schedule runs to check any reminders that need to be sent before now, limited to **300** reminders at a time. Any leftover reminders will be sent in the next run. This is to avoid having to deal with fickle sidekiq and reminders in the far-flung future, which would necessitate having a background job anyway to clean up any missing `enqueue_at` reminders. * If a reminder is sent its `reminder_at` time is cleared and the `reminder_last_sent_at` time is filled in. Notifications are only user-level notifications for now. * All JavaScript and frontend code related to displaying the bookmark reminder notification is contained here. The reminder functionality is now re-enabled in the bookmark modal as well. * This PR also implements the "Remind me next time I am at my desktop" bookmark reminder functionality. When the user is on a mobile device they are able to select this option. When they choose this option we set a key in Redis saying they have a pending at desktop reminder. The next time they change devices we check if the new device is desktop, and if it is we send reminders using a DistributedMutex. There is also a job to ensure consistency of these reminders in Redis (in case Redis drops the ball) and the at desktop reminders expire after 20 days. * Also in this PR is a fix to delete all Bookmarks for a user via `UserDestroyer` --- .../discourse-common/lib/icon-library.js.es6 | 1 + .../discourse/components/tap-tile.js.es6 | 2 +- .../discourse/controllers/bookmark.js.es6 | 4 +- .../discourse/lib/transform-post.js.es6 | 1 + .../javascripts/discourse/models/post.js.es6 | 10 +- .../discourse/templates/modal/bookmark.hbs | 2 +- ...bookmark-reminder-notification-item.js.es6 | 19 ++ .../discourse/widgets/post-menu.js.es6 | 5 +- app/controllers/bookmarks_controller.rb | 27 +-- app/controllers/topics_controller.rb | 10 +- .../bookmark_reminder_notifications.rb | 110 +++++++++ app/models/bookmark.rb | 56 ++++- app/models/notification.rb | 3 +- app/serializers/post_serializer.rb | 10 + app/services/user_destroyer.rb | 1 + config/locales/client.en.yml | 2 + config/locales/server.en.yml | 2 + ..._add_reminder_last_sent_at_to_bookmarks.rb | 7 + ...060737_add_reminder_set_at_to_bookmarks.rb | 8 + lib/auth/default_current_user_provider.rb | 4 + lib/bookmark_manager.rb | 73 ++++++ lib/bookmark_reminder_notification_handler.rb | 74 ++++++ .../default_current_user_provider_spec.rb | 8 + spec/fabricators/bookmark_fabricator.rb | 14 +- .../bookmark_reminder_notifications_spec.rb | 139 +++++++++++ spec/lib/bookmark_manager_spec.rb | 215 ++++++++++++++++++ ...mark_reminder_notification_handler_spec.rb | 75 ++++++ spec/models/user_spec.rb | 10 - spec/requests/bookmarks_controller_spec.rb | 6 +- spec/requests/topics_controller_spec.rb | 2 +- spec/services/user_destroyer_spec.rb | 20 ++ 31 files changed, 860 insertions(+), 60 deletions(-) create mode 100644 app/assets/javascripts/discourse/widgets/bookmark-reminder-notification-item.js.es6 create mode 100644 app/jobs/scheduled/bookmark_reminder_notifications.rb create mode 100644 db/migrate/20200227073837_add_reminder_last_sent_at_to_bookmarks.rb create mode 100644 db/migrate/20200306060737_add_reminder_set_at_to_bookmarks.rb create mode 100644 lib/bookmark_manager.rb create mode 100644 lib/bookmark_reminder_notification_handler.rb create mode 100644 spec/jobs/bookmark_reminder_notifications_spec.rb create mode 100644 spec/lib/bookmark_manager_spec.rb create mode 100644 spec/lib/bookmark_reminder_notification_handler_spec.rb diff --git a/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 b/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 index b1f1d48829a..dab500bbdfe 100644 --- a/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 +++ b/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 @@ -21,6 +21,7 @@ const REPLACEMENTS = { "notification.replied": "reply", "notification.posted": "reply", "notification.edited": "pencil-alt", + "notification.bookmark_reminder": "discourse-bookmark-clock", "notification.liked": "heart", "notification.liked_2": "heart", "notification.liked_many": "heart", diff --git a/app/assets/javascripts/discourse/components/tap-tile.js.es6 b/app/assets/javascripts/discourse/components/tap-tile.js.es6 index a7dfbe267cc..3a5203573d0 100644 --- a/app/assets/javascripts/discourse/components/tap-tile.js.es6 +++ b/app/assets/javascripts/discourse/components/tap-tile.js.es6 @@ -5,7 +5,7 @@ export default Component.extend({ classNames: ["tap-tile"], classNameBindings: ["active"], click() { - this.onSelect(this.tileId); + this.onChange(this.tileId); }, active: propertyEqual("activeTile", "tileId") diff --git a/app/assets/javascripts/discourse/controllers/bookmark.js.es6 b/app/assets/javascripts/discourse/controllers/bookmark.js.es6 index 3be0e107afe..27e3e8418bd 100644 --- a/app/assets/javascripts/discourse/controllers/bookmark.js.es6 +++ b/app/assets/javascripts/discourse/controllers/bookmark.js.es6 @@ -48,7 +48,7 @@ export default Controller.extend(ModalFunctionality, { }, usingMobileDevice: reads("site.mobileView"), - showBookmarkReminderControls: false, + showBookmarkReminderControls: true, @discourseComputed() reminderTypes: () => { @@ -122,7 +122,7 @@ export default Controller.extend(ModalFunctionality, { return ajax("/bookmarks", { type: "POST", data }).then(() => { if (this.afterSave) { - this.afterSave(reminderAtISO); + this.afterSave(reminderAtISO, this.selectedReminderType); } }); }, diff --git a/app/assets/javascripts/discourse/lib/transform-post.js.es6 b/app/assets/javascripts/discourse/lib/transform-post.js.es6 index f6d19c6b46b..09cb6d967c3 100644 --- a/app/assets/javascripts/discourse/lib/transform-post.js.es6 +++ b/app/assets/javascripts/discourse/lib/transform-post.js.es6 @@ -37,6 +37,7 @@ export function transformBasicPost(post) { bookmarked: post.bookmarked, bookmarkedWithReminder: post.bookmarked_with_reminder, bookmarkReminderAt: post.bookmark_reminder_at, + bookmarkReminderType: post.bookmark_reminder_type, yours: post.yours, shareUrl: post.get("shareUrl"), staff: post.staff, diff --git a/app/assets/javascripts/discourse/models/post.js.es6 b/app/assets/javascripts/discourse/models/post.js.es6 index 4ba35b31ac9..2629afcb338 100644 --- a/app/assets/javascripts/discourse/models/post.js.es6 +++ b/app/assets/javascripts/discourse/models/post.js.es6 @@ -351,16 +351,20 @@ const Post = RestModel.extend({ this.toggleProperty("bookmarked_with_reminder"); this.appEvents.trigger("post-stream:refresh", { id: this.id }); }, - afterSave: reminderAtISO => { + afterSave: (reminderAtISO, reminderType) => { this.setProperties({ "topic.bookmarked": true, - bookmark_reminder_at: reminderAtISO + bookmark_reminder_at: reminderAtISO, + bookmark_reminder_type: reminderType }); this.appEvents.trigger("post-stream:refresh", { id: this.id }); } }); } else { - this.set("bookmark_reminder_at", null); + this.setProperties({ + bookmark_reminder_at: null, + bookmark_reminder_type: null + }); return Post.destroyBookmark(this.id) .then(result => { this.set("topic.bookmarked", result.topic_bookmarked); diff --git a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs index 0098c5b237b..e32f2467ebf 100644 --- a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs +++ b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs @@ -25,7 +25,7 @@ {{#if userHasTimezoneSet}} {{#tap-tile-grid activeTile=selectedReminderType as |grid|}} {{#if usingMobileDevice}} - + {{tap-tile icon="desktop" text=(i18n "bookmarks.reminders.at_desktop") tileId=reminderTypes.AT_DESKTOP activeTile=grid.activeTile onChange=(action "selectReminderType")}} {{/if}} {{#if showLaterToday}} diff --git a/app/assets/javascripts/discourse/widgets/bookmark-reminder-notification-item.js.es6 b/app/assets/javascripts/discourse/widgets/bookmark-reminder-notification-item.js.es6 new file mode 100644 index 00000000000..f02bc7a223e --- /dev/null +++ b/app/assets/javascripts/discourse/widgets/bookmark-reminder-notification-item.js.es6 @@ -0,0 +1,19 @@ +import { createWidgetFrom } from "discourse/widgets/widget"; +import { DefaultNotificationItem } from "discourse/widgets/default-notification-item"; +import { formatUsername } from "discourse/lib/utilities"; + +createWidgetFrom( + DefaultNotificationItem, + "bookmark-reminder-notification-item", + { + text(notificationName, data) { + const username = formatUsername(data.display_username); + const description = this.description(data); + + return I18n.t("notifications.bookmark_reminder", { + description, + username + }); + } + } +); diff --git a/app/assets/javascripts/discourse/widgets/post-menu.js.es6 b/app/assets/javascripts/discourse/widgets/post-menu.js.es6 index b5562aa2f1a..6ae6460fac4 100644 --- a/app/assets/javascripts/discourse/widgets/post-menu.js.es6 +++ b/app/assets/javascripts/discourse/widgets/post-menu.js.es6 @@ -332,7 +332,10 @@ registerButton("bookmarkWithReminder", (attrs, state, siteSettings) => { title, titleOptions, className: classNames.join(" "), - icon: attrs.bookmarkReminderAt ? "discourse-bookmark-clock" : "bookmark" + icon: + attrs.bookmarkReminderAt || attrs.bookmarkReminderType === "at_desktop" + ? "discourse-bookmark-clock" + : "bookmark" }; }); diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index 4cab4edf042..c47dfdc0254 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/app/controllers/bookmarks_controller.rb @@ -6,33 +6,24 @@ class BookmarksController < ApplicationController def create params.require(:post_id) - existing_bookmark = Bookmark.find_by(post_id: params[:post_id], user_id: current_user.id) - if existing_bookmark.present? - return render json: failed_json.merge(errors: [I18n.t("bookmarks.errors.already_bookmarked_post")]), status: 422 - end - - bookmark = Bookmark.create( - user_id: current_user.id, - topic_id: Post.select(:topic_id).find(params[:post_id]).topic_id, + bookmark_manager = BookmarkManager.new(current_user) + bookmark = bookmark_manager.create( post_id: params[:post_id], name: params[:name], - reminder_type: Bookmark.reminder_types[params[:reminder_type].to_sym], + reminder_type: params[:reminder_type], reminder_at: params[:reminder_at] ) - return render json: success_json if bookmark.save - render json: failed_json.merge(errors: bookmark.errors.full_messages), status: 400 + if bookmark_manager.errors.empty? + return render json: success_json + end + + render json: failed_json.merge(errors: bookmark_manager.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 + BookmarkManager.new(current_user).destroy(params[:id]) render json: success_json end end diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index d26538b8055..61b6d4bd688 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -487,7 +487,7 @@ class TopicsController < ApplicationController topic = Topic.find(params[:topic_id].to_i) if SiteSetting.enable_bookmarks_with_reminders? - Bookmark.where(user_id: current_user.id, topic_id: topic.id).destroy_all + BookmarkManager.new(current_user).destroy_for_topic(topic) else PostAction.joins(:post) .where(user_id: current_user.id) @@ -548,10 +548,12 @@ class TopicsController < ApplicationController first_post = topic.ordered_posts.first 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) + bookmark_manager = BookmarkManager.new(current_user) + bookmark_manager.create(post_id: first_post.id) + + if bookmark_manager.errors + return render_json_error(bookmark_manager, status: 400) 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? diff --git a/app/jobs/scheduled/bookmark_reminder_notifications.rb b/app/jobs/scheduled/bookmark_reminder_notifications.rb new file mode 100644 index 00000000000..20ce9cb8a8e --- /dev/null +++ b/app/jobs/scheduled/bookmark_reminder_notifications.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +module Jobs + + # Runs periodically to send out bookmark reminders, capped at 300 at a time. + # Any leftovers will be caught in the next run, because the reminder_at column + # is set to NULL once a reminder has been sent. + class BookmarkReminderNotifications < ::Jobs::Scheduled + JOB_RUN_NUMBER_KEY ||= 'jobs_bookmark_reminder_notifications_job_run_num'.freeze + AT_DESKTOP_CONSISTENCY_RUN_NUMBER ||= 6 + + every 5.minutes + + def self.max_reminder_notifications_per_run + @@max_reminder_notifications_per_run ||= 3 + @@max_reminder_notifications_per_run + end + + def self.max_reminder_notifications_per_run=(max) + @@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') + + bookmarks.limit(BookmarkReminderNotifications.max_reminder_notifications_per_run).each do |bookmark| + BookmarkReminderNotificationHandler.send_notification(bookmark) + end + + # we only want to ensure the desktop consistency every X runs of this job + # (every 30 mins in this case) so we don't bother redis too much, and the + # at desktop consistency problem should not really happen unless people + # are setting the "at desktop" reminder, going out for milk, and never coming + # back + current_job_run_number = Discourse.redis.get(JOB_RUN_NUMBER_KEY).to_i + if current_job_run_number == AT_DESKTOP_CONSISTENCY_RUN_NUMBER + ensure_at_desktop_consistency + end + + increment_job_run_number(current_job_run_number) + end + + def increment_job_run_number(current_job_run_number) + if current_job_run_number.zero? || current_job_run_number == AT_DESKTOP_CONSISTENCY_RUN_NUMBER + new_job_run_number = 1 + else + new_job_run_number = current_job_run_number + 1 + end + Discourse.redis.set(JOB_RUN_NUMBER_KEY, new_job_run_number) + end + + def ensure_at_desktop_consistency + pending_at_desktop_bookmark_reminders = \ + Bookmark.includes(:user) + .references(:user) + .pending_at_desktop_reminders + .where('users.last_seen_at >= :one_day_ago', one_day_ago: 1.day.ago.utc) + + return if pending_at_desktop_bookmark_reminders.count.zero? + + unique_users = pending_at_desktop_bookmark_reminders.map(&:user).uniq.map { |u| [u.id, u] }.flatten + unique_users = Hash[*unique_users] + pending_reminders_for_redis_check = unique_users.keys.map do |user_id| + "#{BookmarkReminderNotificationHandler::PENDING_AT_DESKTOP_KEY_PREFIX}#{user_id}" + end + + Discourse.redis.mget(pending_reminders_for_redis_check).each.with_index do |value, idx| + next if value.present? + user_id = pending_reminders_for_redis_check[idx][/\d+/].to_i + user = unique_users[user_id] + + user_pending_bookmark_reminders = pending_at_desktop_bookmark_reminders.select do |bookmark| + bookmark.user == user + end + + user_expired_bookmark_reminders = user_pending_bookmark_reminders.select do |bookmark| + bookmark.reminder_set_at <= expiry_limit_datetime + end + + next if user_pending_bookmark_reminders.length == user_expired_bookmark_reminders.length + + # only tell the cache-gods that this user has pending "at desktop" reminders + # if they haven't let them all expire before coming back to their desktop + # + # the next time they visit the desktop the reminders will be cleared out once + # the notifications are sent + BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(user) + end + + clear_expired_at_desktop_reminders + end + + def clear_expired_at_desktop_reminders + Bookmark + .pending_at_desktop_reminders + .where('reminder_set_at <= :expiry_limit_datetime', expiry_limit_datetime: expiry_limit_datetime) + .update_all( + reminder_set_at: nil, reminder_type: nil + ) + end + + def expiry_limit_datetime + BookmarkReminderNotificationHandler::PENDING_AT_DESKTOP_EXPIRY_DAYS.days.ago.utc + end + end +end diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 6958064324b..d25a30b8650 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -10,6 +10,41 @@ class Bookmark < ActiveRecord::Base if: -> { reminder_type.present? && reminder_type != Bookmark.reminder_types[:at_desktop] } } + validate :unique_per_post_for_user + validate :ensure_sane_reminder_at_time + + def unique_per_post_for_user + existing_bookmark = Bookmark.find_by(user_id: user_id, post_id: post_id) + return if existing_bookmark.blank? || existing_bookmark.id == id + self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post")) + end + + def ensure_sane_reminder_at_time + return if reminder_at.blank? + if reminder_at < Time.zone.now + self.errors.add(:base, I18n.t("bookmarks.errors.cannot_set_past_reminder")) + end + if reminder_at > 10.years.from_now.utc + self.errors.add(:base, I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future")) + end + end + + scope :pending_reminders, ->(before_time = Time.now.utc) do + where("reminder_at IS NOT NULL AND reminder_at <= :before_time", before_time: before_time) + end + + scope :pending_at_desktop_reminders, ->(before_time = Time.now.utc) do + where("reminder_at IS NULL AND reminder_type = :at_desktop", at_desktop: reminder_types[:at_desktop]) + end + + scope :pending_reminders_for_user, ->(user) do + pending_reminders.where(user: user) + end + + scope :at_desktop_reminders_for_user, ->(user) do + where("reminder_type = :at_desktop AND user_id = :user_id", at_desktop: reminder_types[:at_desktop], user_id: user.id) + end + def self.reminder_types @reminder_type = Enum.new( at_desktop: 0, @@ -27,20 +62,23 @@ end # # Table name: bookmarks # -# id :bigint not null, primary key -# user_id :bigint not null -# topic_id :bigint not null -# post_id :bigint not null -# name :string -# reminder_type :integer -# reminder_at :datetime -# created_at :datetime not null -# updated_at :datetime not null +# id :bigint not null, primary key +# user_id :bigint not null +# topic_id :bigint not null +# post_id :bigint not null +# name :string +# reminder_type :integer +# reminder_at :datetime +# created_at :datetime not null +# updated_at :datetime not null +# reminder_last_sent_at :datetime +# reminder_set_at :datetime # # Indexes # # index_bookmarks_on_post_id (post_id) # index_bookmarks_on_reminder_at (reminder_at) +# index_bookmarks_on_reminder_set_at (reminder_set_at) # index_bookmarks_on_reminder_type (reminder_type) # index_bookmarks_on_topic_id (topic_id) # index_bookmarks_on_user_id (user_id) diff --git a/app/models/notification.rb b/app/models/notification.rb index b9443d89fb5..a44086f6529 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -103,7 +103,8 @@ class Notification < ActiveRecord::Base post_approved: 20, code_review_commit_approved: 21, membership_request_accepted: 22, - membership_request_consolidated: 23 + membership_request_consolidated: 23, + bookmark_reminder: 24 ) end diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index ade1417dc66..b78c90f4fb9 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -51,6 +51,7 @@ class PostSerializer < BasicPostSerializer :bookmarked, :bookmarked_with_reminder, :bookmark_reminder_at, + :bookmark_reminder_type, :raw, :actions_summary, :moderator?, @@ -329,6 +330,10 @@ class PostSerializer < BasicPostSerializer include_bookmarked_with_reminder? end + def include_bookmark_reminder_type? + include_bookmarked_with_reminder? + end + def post_bookmark return nil if !SiteSetting.enable_bookmarks_with_reminders? || @topic_view.blank? @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id } @@ -338,6 +343,11 @@ class PostSerializer < BasicPostSerializer post_bookmark&.reminder_at end + def bookmark_reminder_type + return if post_bookmark.blank? + Bookmark.reminder_types[post_bookmark.reminder_type].to_s + end + def include_display_username? SiteSetting.enable_names? end diff --git a/app/services/user_destroyer.rb b/app/services/user_destroyer.rb index 7844496362f..e0f7543ac66 100644 --- a/app/services/user_destroyer.rb +++ b/app/services/user_destroyer.rb @@ -28,6 +28,7 @@ class UserDestroyer optional_transaction(open_transaction: opts[:transaction]) do UserSecurityKey.where(user_id: user.id).delete_all + Bookmark.where(user_id: user.id).delete_all Draft.where(user_id: user.id).delete_all Reviewable.where(created_by_id: user.id).delete_all diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index ea18a5b17a8..c699eef32f5 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1807,6 +1807,7 @@ en: mentioned: "{{username}} {{description}}" group_mentioned: "{{username}} {{description}}" quoted: "{{username}} {{description}}" + bookmark_reminder: "{{username}} {{description}}" replied: "{{username}} {{description}}" posted: "{{username}} {{description}}" edited: "{{username}} {{description}}" @@ -1860,6 +1861,7 @@ en: posted: "new post" moved_post: "post moved" linked: "linked" + bookmark_reminder: "bookmark reminder" granted_badge: "badge granted" invited_to_topic: "invited to topic" group_mentioned: "group mentioned" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 0b1011a46cd..f2608550873 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -385,6 +385,8 @@ en: bookmarks: errors: already_bookmarked_post: "You cannot bookmark the same post twice." + cannot_set_past_reminder: "You cannot set a bookmark reminder in the past." + cannot_set_reminder_in_distant_future: "You cannot set a bookmark reminder more than 10 years in the future." time_must_be_provided: "time must be provided for all reminders except '%{reminder_type}'" reminders: diff --git a/db/migrate/20200227073837_add_reminder_last_sent_at_to_bookmarks.rb b/db/migrate/20200227073837_add_reminder_last_sent_at_to_bookmarks.rb new file mode 100644 index 00000000000..3a1aa86bff7 --- /dev/null +++ b/db/migrate/20200227073837_add_reminder_last_sent_at_to_bookmarks.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddReminderLastSentAtToBookmarks < ActiveRecord::Migration[6.0] + def change + add_column :bookmarks, :reminder_last_sent_at, :datetime, null: true + end +end diff --git a/db/migrate/20200306060737_add_reminder_set_at_to_bookmarks.rb b/db/migrate/20200306060737_add_reminder_set_at_to_bookmarks.rb new file mode 100644 index 00000000000..93ddeff2f13 --- /dev/null +++ b/db/migrate/20200306060737_add_reminder_set_at_to_bookmarks.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddReminderSetAtToBookmarks < ActiveRecord::Migration[6.0] + def change + add_column :bookmarks, :reminder_set_at, :datetime, null: true + add_index :bookmarks, :reminder_set_at + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 5fe22b4adce..caee9ac1cf1 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -124,6 +124,10 @@ class Auth::DefaultCurrentUserProvider u.update_last_seen! u.update_ip_address!(ip) end + + BookmarkReminderNotificationHandler.defer_at_desktop_reminder( + user: u, request_user_agent: @request.user_agent + ) end @env[CURRENT_USER_KEY] = current_user diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb new file mode 100644 index 00000000000..0dcfa2b79f0 --- /dev/null +++ b/lib/bookmark_manager.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +class BookmarkManager + include HasErrors + + def initialize(user) + @user = user + end + + def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil) + reminder_type = Bookmark.reminder_types[reminder_type.to_sym] if reminder_type.present? + + bookmark = Bookmark.create( + user_id: @user.id, + topic_id: topic_id_for_post(post_id), + post_id: post_id, + name: name, + reminder_type: reminder_type, + reminder_at: reminder_at, + reminder_set_at: Time.zone.now + ) + + if bookmark.errors.any? + return add_errors_from(bookmark) + end + + BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(@user) + bookmark + end + + def destroy(bookmark_id) + bookmark = Bookmark.find_by(id: bookmark_id) + + raise Discourse::NotFound if bookmark.blank? + raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_delete?(bookmark) + + bookmark.destroy + clear_at_desktop_cache_if_required + end + + def destroy_for_topic(topic) + topic_bookmarks = Bookmark.where(user_id: @user.id, topic_id: topic.id) + + Bookmark.transaction do + topic_bookmarks.each do |bookmark| + raise Discourse::InvalidAccess.new if !Guardian.new(user).can_delete?(bookmark) + bookmark.destroy + end + end + + clear_at_desktop_cache_if_required + end + + def self.send_reminder_notification(id) + bookmark = Bookmark.find_by(id: id) + BookmarkReminderNotificationHandler.send_notification(bookmark) + end + + private + + def topic_id_for_post(post_id) + Post.where(id: post_id).pluck_first(:topic_id) + end + + def clear_at_desktop_cache_if_required + return if user_has_any_pending_at_desktop_reminders? + Discourse.redis.del(BookmarkReminderNotificationHandler::PENDING_AT_DESKTOP_KEY_PREFIX + @user.id.to_s) + end + + def user_has_any_pending_at_desktop_reminders? + Bookmark.at_desktop_reminders_for_user(@user).any? + end +end diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb new file mode 100644 index 00000000000..166c46274ea --- /dev/null +++ b/lib/bookmark_reminder_notification_handler.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +class BookmarkReminderNotificationHandler + PENDING_AT_DESKTOP_KEY_PREFIX ||= 'pending_at_desktop_bookmark_reminder_user_'.freeze + PENDING_AT_DESKTOP_EXPIRY_DAYS ||= 20 + + def self.send_notification(bookmark) + return if bookmark.blank? + Bookmark.transaction do + if bookmark.post.blank? + return clear_reminder(bookmark) + end + + create_notification(bookmark) + clear_reminder(bookmark) + end + end + + def self.clear_reminder(bookmark) + Rails.logger.debug( + "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder info: #{bookmark.reminder_at} | #{Bookmark.reminder_types[bookmark.reminder_type]}" + ) + + bookmark.update( + reminder_at: nil, + reminder_type: nil, + reminder_last_sent_at: Time.zone.now, + reminder_set_at: nil + ) + end + + def self.create_notification(bookmark) + user = bookmark.user + user.notifications.create!( + notification_type: Notification.types[:bookmark_reminder], + topic_id: bookmark.topic_id, + post_number: bookmark.post.post_number, + data: { + topic_title: bookmark.topic.title, + display_username: user.username, + bookmark_name: bookmark.name + }.to_json + ) + end + + def self.user_has_pending_at_desktop_reminders?(user) + Discourse.redis.exists("#{PENDING_AT_DESKTOP_KEY_PREFIX}#{user.id}") + end + + def self.cache_pending_at_desktop_reminder(user) + Discourse.redis.setex("#{PENDING_AT_DESKTOP_KEY_PREFIX}#{user.id}", PENDING_AT_DESKTOP_EXPIRY_DAYS.days, true) + end + + def self.send_at_desktop_reminder(user:, request_user_agent:) + return if !SiteSetting.enable_bookmarks_with_reminders + + return if MobileDetection.mobile_device?(BrowserDetection.device(request_user_agent).to_s) + + return if !user_has_pending_at_desktop_reminders?(user) + + DistributedMutex.synchronize("sending_at_desktop_bookmark_reminders_user_#{user.id}") do + Bookmark.at_desktop_reminders_for_user(user).each do |bookmark| + BookmarkReminderNotificationHandler.send_notification(bookmark) + end + Discourse.redis.del("#{PENDING_AT_DESKTOP_KEY_PREFIX}#{user.id}") + end + end + + def self.defer_at_desktop_reminder(user:, request_user_agent:) + Scheduler::Defer.later "Sending Desktop Bookmark Reminders" do + send_at_desktop_reminder(user: user, request_user_agent: request_user_agent) + end + end +end diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 2e0650cf30f..2fd165c848e 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -443,6 +443,14 @@ describe Auth::DefaultCurrentUserProvider do expect(u.last_seen_at).to eq(nil) end end + + it "defers any at_desktop bookmark reminders" do + BookmarkReminderNotificationHandler.expects(:defer_at_desktop_reminder).with( + user: user, request_user_agent: 'test' + ) + provider2 = provider("/", "HTTP_COOKIE" => "_t=#{unhashed_token}", "HTTP_USER_AGENT" => 'test') + provider2.current_user + end end it "should update last seen for non ajax" do diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb index a92f272e52a..ede44dfecae 100644 --- a/spec/fabricators/bookmark_fabricator.rb +++ b/spec/fabricators/bookmark_fabricator.rb @@ -6,19 +6,21 @@ Fabricator(:bookmark) do topic { |attrs| attrs[:post].topic } name "This looked interesting" reminder_type { Bookmark.reminder_types[:tomorrow] } - reminder_at { (Time.now.utc + 1.day).iso8601 } + reminder_at { 1.day.from_now.iso8601 } + reminder_set_at { Time.zone.now } end Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do reminder_type { Bookmark.reminder_types[:next_business_day] } reminder_at do - date = if Time.now.utc.friday? - Time.now.utc + 3.days - elsif Time.now.utc.saturday? - Time.now.utc + 2.days + date = if Time.zone.now.friday? + Time.zone.now + 3.days + elsif Time.zone.now.saturday? + Time.zone.now + 2.days else - Time.now.utc + 1.day + Time.zone.now + 1.day end date.iso8601 end + reminder_set_at { Time.zone.now } end diff --git a/spec/jobs/bookmark_reminder_notifications_spec.rb b/spec/jobs/bookmark_reminder_notifications_spec.rb new file mode 100644 index 00000000000..92ae1000206 --- /dev/null +++ b/spec/jobs/bookmark_reminder_notifications_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Jobs::BookmarkReminderNotifications do + subject { described_class.new } + + let(:five_minutes_ago) { Time.zone.now - 5.minutes } + let(:bookmark1) { Fabricate(:bookmark) } + let(:bookmark2) { Fabricate(:bookmark) } + let(:bookmark3) { Fabricate(:bookmark) } + let!(:bookmarks) do + [ + bookmark1, + bookmark2, + bookmark3 + ] + 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) + bookmark3.update_column(:reminder_at, five_minutes_ago) + Discourse.redis.flushall + end + + it "sends every reminder and marks the reminder_at to nil for all bookmarks, as well as last sent date" do + subject.execute + bookmark1.reload + bookmark2.reload + bookmark3.reload + expect(bookmark1.reminder_at).to eq(nil) + expect(bookmark1.reminder_last_sent_at).not_to eq(nil) + expect(bookmark2.reminder_at).to eq(nil) + expect(bookmark2.reminder_last_sent_at).not_to eq(nil) + expect(bookmark3.reminder_at).to eq(nil) + expect(bookmark3.reminder_last_sent_at).not_to eq(nil) + end + + it "will not send a reminder for a bookmark in the future" do + bookmark4 = Fabricate(:bookmark, reminder_at: Time.zone.now + 1.day) + BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark1) + BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark2) + BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark3) + BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark4).never + subject.execute + expect(bookmark4.reload.reminder_at).not_to eq(nil) + end + + it "increments the job run number correctly and resets to 1 when it reaches 6" do + expect(Discourse.redis.get(described_class::JOB_RUN_NUMBER_KEY)).to eq(nil) + subject.execute + expect(Discourse.redis.get(described_class::JOB_RUN_NUMBER_KEY)).to eq('1') + subject.execute + subject.execute + subject.execute + subject.execute + subject.execute + expect(Discourse.redis.get(described_class::JOB_RUN_NUMBER_KEY)).to eq('6') + subject.execute + 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 + BookmarkManager.any_instance.expects(:send_reminder_notification).with(bookmark1.id).never + subject.execute + end + end + + context "when the number of notifications exceed max_reminder_notifications_per_run" do + it "does not send them in the current run, but will send them in the next" do + begin + Jobs::BookmarkReminderNotifications.max_reminder_notifications_per_run = 2 + subject.execute + expect(bookmark1.reload.reminder_at).to eq(nil) + expect(bookmark2.reload.reminder_at).to eq(nil) + expect(bookmark3.reload.reminder_at).not_to eq(nil) + end + end + end + + context "when this is the 6th run (so every half hour) of this job we need to ensure consistency of at_desktop reminders" do + let(:set_at) { Time.zone.now } + let!(:bookmark) do + Fabricate( + :bookmark, + reminder_type: Bookmark.reminder_types[:at_desktop], + reminder_at: nil, + reminder_set_at: set_at + ) + end + before do + Discourse.redis.set(Jobs::BookmarkReminderNotifications::JOB_RUN_NUMBER_KEY, 6) + bookmark.user.update(last_seen_at: Time.zone.now - 1.minute) + end + context "when an at_desktop reminder is not pending in redis for a user who should have one" do + it "puts the pending reminder into redis" do + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(bookmark.user)).to eq(false) + subject.execute + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(bookmark.user)).to eq(true) + end + + context "if the user has not been seen in the past 24 hours" do + before do + bookmark.user.update(last_seen_at: Time.zone.now - 25.hours) + end + it "does not put the pending reminder into redis" do + subject.execute + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(bookmark.user)).to eq(false) + end + end + + context "if the at_desktop reminder is expired (set over PENDING_AT_DESKTOP_EXPIRY_DAYS days ago)" do + let(:set_at) { Time.zone.now - (BookmarkReminderNotificationHandler::PENDING_AT_DESKTOP_EXPIRY_DAYS + 1).days } + it "does not put the pending reminder into redis, and clears the reminder type/time" do + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(bookmark.user)).to eq(false) + subject.execute + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(bookmark.user)).to eq(false) + bookmark.reload + expect(bookmark.reminder_set_at).to eq(nil) + expect(bookmark.reminder_last_sent_at).to eq(nil) + expect(bookmark.reminder_type).to eq(nil) + end + end + end + end +end diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb new file mode 100644 index 00000000000..92b7c2df213 --- /dev/null +++ b/spec/lib/bookmark_manager_spec.rb @@ -0,0 +1,215 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe BookmarkManager do + let(:user) { Fabricate(:user) } + + let(:reminder_type) { 'tomorrow' } + let(:reminder_at) { (Time.zone.now + 1.day).iso8601 } + fab!(:post) { Fabricate(:post) } + let(:name) { 'Check this out!' } + + subject { described_class.new(user) } + + describe ".create" do + it "creates the bookmark for the user" do + subject.create(post_id: post.id, name: name) + bookmark = Bookmark.find_by(user: user) + + expect(bookmark.post_id).to eq(post.id) + expect(bookmark.topic_id).to eq(post.topic_id) + end + + context "when a reminder time + type is provided" do + it "saves the values correctly" do + subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + bookmark = Bookmark.find_by(user: user) + + expect(bookmark.reminder_at).to eq(reminder_at) + expect(bookmark.reminder_set_at).not_to eq(nil) + expect(bookmark.reminder_type).to eq(Bookmark.reminder_types[:tomorrow]) + end + end + + context "when the reminder type is at_desktop" do + let(:reminder_type) { 'at_desktop' } + let(:reminder_at) { nil } + + def create_bookmark + subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + end + + it "this is a special case which needs client-side logic and has no reminder_at datetime" do + create_bookmark + bookmark = Bookmark.find_by(user: user) + + expect(bookmark.reminder_at).to eq(nil) + expect(bookmark.reminder_type).to eq(Bookmark.reminder_types[:at_desktop]) + end + + it "sets a redis key for the user so we know they have a pending at_desktop reminder" do + create_bookmark + expect(Discourse.redis.get("pending_at_desktop_bookmark_reminder_user_#{user.id}")).not_to eq(nil) + end + end + + context "when the bookmark already exists for the user & post" do + before do + Bookmark.create(post: post, user: user, topic: post.topic) + end + + it "adds an error to the manager" do + subject.create(post_id: post.id) + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked_post")) + end + end + + context "when the reminder time is not provided when it needs to be" do + let(:reminder_at) { nil } + it "adds an error to the manager" do + subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + expect(subject.errors.full_messages).to include( + "Reminder at " + I18n.t("bookmarks.errors.time_must_be_provided", reminder_type: I18n.t("bookmarks.reminders.at_desktop")) + ) + end + end + + context "when the reminder time is in the past" do + let(:reminder_at) { (Time.zone.now - 10.days).iso8601 } + it "adds an error to the manager" do + subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_past_reminder")) + end + end + + context "when the reminder time is far-flung (> 10 years from now)" do + let(:reminder_at) { (Time.zone.now + 11.years).iso8601 } + it "adds an error to the manager" do + subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at) + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future")) + end + end + end + + describe ".destroy" do + let!(:bookmark) { Fabricate(:bookmark, user: user, post: post) } + it "deletes the existing bookmark" do + subject.destroy(bookmark.id) + expect(Bookmark.exists?(id: bookmark.id)).to eq(false) + end + + context "if the bookmark is belonging to some other user" do + let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) } + it "raises an invalid access error" do + expect { subject.destroy(bookmark.id) }.to raise_error(Discourse::InvalidAccess) + end + end + + context "if the bookmark no longer exists" do + it "raises an invalid access error" do + expect { subject.destroy(9999) }.to raise_error(Discourse::NotFound) + end + end + + context "if the user has pending at desktop reminders for another bookmark" do + before do + Fabricate(:bookmark, user: user, post: Fabricate(:post), reminder_type: Bookmark.reminder_types[:at_desktop]) + BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(user) + end + it "does not clear the at bookmark redis key" do + subject.destroy(bookmark.id) + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(user)).to eq(true) + end + end + + context "if the user has pending at desktop reminders for another bookmark" do + it "does clear the at bookmark redis key" do + BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(user) + subject.destroy(bookmark.id) + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(user)).to eq(false) + end + end + end + + describe ".destroy_for_topic" do + let!(:topic) { Fabricate(:topic) } + let(:bookmark1) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) } + let(:bookmark2) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) } + + it "destroys all bookmarks for the topic for the specified user" do + subject.destroy_for_topic(topic) + expect(Bookmark.where(user: user, topic: topic).length).to eq(0) + end + + it "does not destroy any other user's topic bookmarks" do + user2 = Fabricate(:user) + Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user2) + subject.destroy_for_topic(topic) + expect(Bookmark.where(user: user2, topic: topic).length).to eq(1) + end + + context "if the user has pending at desktop reminders for another bookmark" do + before do + Fabricate(:bookmark, user: user, post: Fabricate(:post), reminder_type: Bookmark.reminder_types[:at_desktop]) + BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(user) + end + it "does not clear the at bookmark redis key" do + subject.destroy_for_topic(topic) + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(user)).to eq(true) + end + end + + context "if the user has pending at desktop reminders for another bookmark" do + it "does clear the at bookmark redis key" do + BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(user) + subject.destroy_for_topic(topic) + expect(BookmarkReminderNotificationHandler.user_has_pending_at_desktop_reminders?(user)).to eq(false) + end + end + + end + + describe ".send_reminder_notification" do + let(:bookmark) { Fabricate(:bookmark, user: user) } + it "clears the reminder_at and sets the reminder_last_sent_at" do + expect(bookmark.reminder_last_sent_at).to eq(nil) + described_class.send_reminder_notification(bookmark.id) + bookmark.reload + expect(bookmark.reminder_at).to eq(nil) + expect(bookmark.reminder_last_sent_at).not_to eq(nil) + end + + it "creates a notification for the reminder" do + described_class.send_reminder_notification(bookmark.id) + notif = notifications_for_user.last + expect(notif.post_number).to eq(bookmark.post.post_number) + end + + context "when the bookmark does no longer exist" do + before do + bookmark.destroy + end + it "does not error, and does not create a notification" do + described_class.send_reminder_notification(bookmark.id) + expect(notifications_for_user.any?).to eq(false) + end + end + + context "if the post has been deleted" do + before do + bookmark.post.trash! + end + it "does not error, and does not create a notification, and clears the reminder" do + described_class.send_reminder_notification(bookmark.id) + bookmark.reload + expect(bookmark.reminder_at).to eq(nil) + expect(notifications_for_user.any?).to eq(false) + end + end + + def notifications_for_user + Notification.where(notification_type: Notification.types[:bookmark_reminder], user_id: bookmark.user.id) + end + end +end diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb new file mode 100644 index 00000000000..e64c9b1c704 --- /dev/null +++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe BookmarkReminderNotificationHandler do + subject { described_class } + + fab!(:user) { Fabricate(:user) } + + before do + SiteSetting.enable_bookmarks_with_reminders = true + end + + context "when the user agent is for mobile" do + let(:user_agent) { "Mozilla/5.0 (iPhone; CPU iPhone OS 9_1 like Mac OS X) AppleWebKit/601.1.46 (KHTML, like Gecko) Version/9.0 Mobile/13B143 Safari/601.1" } + it "does not attempt to send any reminders" do + DistributedMutex.expects(:synchronize).never + send_reminder + end + end + + context "when the user agent is for desktop" do + let(:user_agent) { "Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36" } + fab!(:reminder) do + Fabricate( + :bookmark, + user: user, + reminder_type: Bookmark.reminder_types[:at_desktop], + reminder_at: nil, + reminder_set_at: Time.zone.now + ) + end + + context "when there are pending bookmark at desktop reminders" do + before do + described_class.cache_pending_at_desktop_reminder(user) + end + + it "deletes the key in redis" do + send_reminder + expect(described_class.user_has_pending_at_desktop_reminders?(user)).to eq(false) + end + + it "sends a notification to the user and clears the reminder_at" do + send_reminder + expect(Notification.where(user: user, notification_type: Notification.types[:bookmark_reminder]).count).to eq(1) + expect(reminder.reload.reminder_type).to eq(nil) + expect(reminder.reload.reminder_last_sent_at).not_to eq(nil) + expect(reminder.reload.reminder_set_at).to eq(nil) + end + end + + context "when there are no pending bookmark at desktop reminders" do + it "does nothing" do + DistributedMutex.expects(:synchronize).never + send_reminder + 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 + end + + def send_reminder + subject.send_at_desktop_reminder(user: user, request_user_agent: user_agent) + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ef6b64d511a..d94d8d47c5f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2212,16 +2212,6 @@ describe User do end end - describe "Destroying a user with security key" do - let!(:security_key) { Fabricate(:user_security_key_with_random_credential, user: user) } - fab!(:admin) { Fabricate(:admin) } - - it "removes the security key" do - UserDestroyer.new(admin).destroy(user) - expect(UserSecurityKey.where(user_id: user.id).count).to eq(0) - end - end - describe 'Secure identifier for a user which is a string other than the ID used to identify the user in some cases e.g. security keys' do describe '#create_or_fetch_secure_identifier' do context 'if the user already has a secure identifier' do diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb index cfe7f6c9836..8c29af87073 100644 --- a/spec/requests/bookmarks_controller_spec.rb +++ b/spec/requests/bookmarks_controller_spec.rb @@ -17,14 +17,14 @@ describe BookmarksController do Fabricate(:bookmark, post: bookmark_post, user: bookmark_user) end - it "returns failed JSON with a 422 error" do + it "returns failed JSON with a 400 error" do post "/bookmarks.json", params: { post_id: bookmark_post.id, reminder_type: "tomorrow", - reminder_at: (Time.now.utc + 1.day).iso8601 + reminder_at: (Time.zone.now + 1.day).iso8601 } - expect(response.status).to eq(422) + expect(response.status).to eq(400) expect(JSON.parse(response.body)['errors']).to include( I18n.t("bookmarks.errors.already_bookmarked_post") ) diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 481de344e4b..42f002f4816 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2387,7 +2387,7 @@ RSpec.describe TopicsController do Bookmark.create(post: post, topic: post.topic, user: user) put "/t/#{post.topic_id}/bookmark.json" - expect(response).to be_forbidden + expect(response.status).to eq(400) end end end diff --git a/spec/services/user_destroyer_spec.rb b/spec/services/user_destroyer_spec.rb index 02f5e506f7d..a26da77d4ea 100644 --- a/spec/services/user_destroyer_spec.rb +++ b/spec/services/user_destroyer_spec.rb @@ -332,6 +332,26 @@ describe UserDestroyer do end end + describe "Destroying a user with security key" do + let!(:security_key) { Fabricate(:user_security_key_with_random_credential, user: user) } + fab!(:admin) { Fabricate(:admin) } + + it "removes the security key" do + UserDestroyer.new(admin).destroy(user) + expect(UserSecurityKey.where(user_id: user.id).count).to eq(0) + end + end + + describe "Destroying a user with a bookmark" do + let!(:bookmark) { Fabricate(:bookmark, user: user) } + fab!(:admin) { Fabricate(:admin) } + + it "removes the bookmark" do + UserDestroyer.new(admin).destroy(user) + expect(Bookmark.where(user_id: user.id).count).to eq(0) + end + end + context 'user got an email' do let!(:email_log) { Fabricate(:email_log, user: user) }