Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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`
This commit is contained in:
Martin Brennan 2020-03-12 10:16:00 +10:00 committed by GitHub
parent b9aaa9718d
commit 793f39139a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
31 changed files with 860 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/assets/javascripts/discourse-common/lib/icon-library.js.es6
View File

@ -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",

2
app/assets/javascripts/discourse/components/tap-tile.js.es6
View File

@ -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")

4
app/assets/javascripts/discourse/controllers/bookmark.js.es6
View File

@ -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);
}
});
},

1
app/assets/javascripts/discourse/lib/transform-post.js.es6
View File

@ -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,

10
app/assets/javascripts/discourse/models/post.js.es6
View File

@ -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);

2
app/assets/javascripts/discourse/templates/modal/bookmark.hbs
View File

@ -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")}} -->
{{tap-tile icon="desktop" text=(i18n "bookmarks.reminders.at_desktop") tileId=reminderTypes.AT_DESKTOP activeTile=grid.activeTile onChange=(action "selectReminderType")}}
{{/if}}
{{#if showLaterToday}}

19
app/assets/javascripts/discourse/widgets/bookmark-reminder-notification-item.js.es6 Normal file
View File

@ -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
});
}
}
);

5
app/assets/javascripts/discourse/widgets/post-menu.js.es6
View File

@ -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"
};
});

27
app/controllers/bookmarks_controller.rb
View File

@ -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

10
app/controllers/topics_controller.rb
View File

@ -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?

110
app/jobs/scheduled/bookmark_reminder_notifications.rb Normal file
View File

@ -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

56
app/models/bookmark.rb
View File

@ -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)

3
app/models/notification.rb
View File

@ -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

10
app/serializers/post_serializer.rb
View File

@ -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

1
app/services/user_destroyer.rb
View File

@ -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

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

@ -1807,6 +1807,7 @@ en:
mentioned: "<span>{{username}}</span> {{description}}"
group_mentioned: "<span>{{username}}</span> {{description}}"
quoted: "<span>{{username}}</span> {{description}}"
bookmark_reminder: "<span>{{username}}</span> {{description}}"
replied: "<span>{{username}}</span> {{description}}"
posted: "<span>{{username}}</span> {{description}}"
edited: "<span>{{username}}</span> {{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"

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

@ -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:

7
db/migrate/20200227073837_add_reminder_last_sent_at_to_bookmarks.rb Normal file
View File

@ -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

8
db/migrate/20200306060737_add_reminder_set_at_to_bookmarks.rb Normal file
View File

@ -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

4
lib/auth/default_current_user_provider.rb
View File

@ -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

73
lib/bookmark_manager.rb Normal file
View File

@ -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

74
lib/bookmark_reminder_notification_handler.rb Normal file
View File

@ -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

8
spec/components/auth/default_current_user_provider_spec.rb
View File

@ -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

14
spec/fabricators/bookmark_fabricator.rb
View File

@ -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

139
spec/jobs/bookmark_reminder_notifications_spec.rb Normal file
View File

@ -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

215
spec/lib/bookmark_manager_spec.rb Normal file
View File

@ -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

75
spec/lib/bookmark_reminder_notification_handler_spec.rb Normal file
View File

@ -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

10
spec/models/user_spec.rb
View File

@ -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

6
spec/requests/bookmarks_controller_spec.rb
View File

@ -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")
)

2
spec/requests/topics_controller_spec.rb
View File

@ -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

20
spec/services/user_destroyer_spec.rb
View File

@ -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) }