From 6261339da9def45e435c7d6bdc0ee1090cc2bb48 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 11 Dec 2019 14:04:02 +1000 Subject: [PATCH] Improving bookmarks part 1 (#8466) Note: All of this functionality is hidden behind a hidden, default false, site setting called `enable_bookmarks_with_reminders`. Also, any feedback on Ember code would be greatly appreciated! This is part 1 of the bookmark improvements. The next PR will address the backend logic to send reminder notifications for bookmarked posts to users. This PR adds the following functionality: * We are adding a new `bookmarks` table and `Bookmark` model to make the bookmarks a first-class citizen and to allow attaching reminders to them. * Posts now have a new button in their actions menu that has the icon of an actual book * Clicking the button opens the new bookmark modal. * Both name and the reminder type are optional. * If you close the modal without doing anything, the bookmark is saved with no reminder. * If you click the Cancel button, no bookmark is saved at all. * All of the reminder type tiles are dynamic and the times they show will be based on your user timezone set in your profile (this should already be set for you). * If for some reason a user does not have their timezone set they will not be able to set a reminder, but they will still be able to create a bookmark. * A bookmark can be deleted by clicking on the book icon again which will be red if the post is bookmarked. This PR does NOT do anything to migrate or change existing bookmarks in the form of `PostActions`, the two features live side-by-side here. Also this does nothing to the topic bookmarking. --- .../templates/components/tap-tile-grid.hbs | 1 + .../admin/templates/components/tap-tile.hbs | 2 + .../discourse/components/tap-tile-grid.js.es6 | 6 + .../discourse/components/tap-tile.js.es6 | 12 + .../discourse/controllers/bookmark.js.es6 | 217 ++++++++++++++++++ .../discourse/controllers/topic.js.es6 | 10 + .../discourse/lib/transform-post.js.es6 | 2 + .../javascripts/discourse/models/post.js.es6 | 33 +++ .../discourse/templates/modal/bookmark.hbs | 49 ++++ .../javascripts/discourse/templates/topic.hbs | 1 + .../discourse/widgets/post-menu.js.es6 | 40 +++- .../common/components/bookmark-modal.scss | 14 ++ .../common/components/tap-tile.scss | 41 ++++ .../stylesheets/desktop/topic-post.scss | 3 + app/controllers/bookmarks_controller.rb | 26 +++ app/controllers/posts_controller.rb | 9 + app/models/bookmark.rb | 54 +++++ app/serializers/post_serializer.rb | 35 ++- config/locales/client.en.yml | 21 ++ config/locales/server.en.yml | 15 ++ config/routes.rb | 3 + config/site_settings.yml | 10 +- ...00434_create_standalone_bookmarks_table.rb | 22 ++ lib/topic_view.rb | 4 + spec/fabricators/bookmark_fabricator.rb | 24 ++ spec/requests/bookmarks_controller_spec.rb | 47 ++++ spec/serializers/post_serializer_spec.rb | 70 +++++- .../controllers/bookmark-test.js.es6 | 144 ++++++++++++ .../fixtures/session-fixtures.js.es6 | 3 +- 29 files changed, 903 insertions(+), 15 deletions(-) create mode 100644 app/assets/javascripts/admin/templates/components/tap-tile-grid.hbs create mode 100644 app/assets/javascripts/admin/templates/components/tap-tile.hbs create mode 100644 app/assets/javascripts/discourse/components/tap-tile-grid.js.es6 create mode 100644 app/assets/javascripts/discourse/components/tap-tile.js.es6 create mode 100644 app/assets/javascripts/discourse/controllers/bookmark.js.es6 create mode 100644 app/assets/javascripts/discourse/templates/modal/bookmark.hbs create mode 100644 app/assets/stylesheets/common/components/bookmark-modal.scss create mode 100644 app/assets/stylesheets/common/components/tap-tile.scss create mode 100644 app/controllers/bookmarks_controller.rb create mode 100644 app/models/bookmark.rb create mode 100644 db/migrate/20191205100434_create_standalone_bookmarks_table.rb create mode 100644 spec/fabricators/bookmark_fabricator.rb create mode 100644 spec/requests/bookmarks_controller_spec.rb create mode 100644 test/javascripts/controllers/bookmark-test.js.es6 diff --git a/app/assets/javascripts/admin/templates/components/tap-tile-grid.hbs b/app/assets/javascripts/admin/templates/components/tap-tile-grid.hbs new file mode 100644 index 00000000000..7807cc0567d --- /dev/null +++ b/app/assets/javascripts/admin/templates/components/tap-tile-grid.hbs @@ -0,0 +1 @@ +{{ yield (hash activeTile=this.activeTile) }} diff --git a/app/assets/javascripts/admin/templates/components/tap-tile.hbs b/app/assets/javascripts/admin/templates/components/tap-tile.hbs new file mode 100644 index 00000000000..4e042009450 --- /dev/null +++ b/app/assets/javascripts/admin/templates/components/tap-tile.hbs @@ -0,0 +1,2 @@ +{{d-icon icon}} +{{text}} diff --git a/app/assets/javascripts/discourse/components/tap-tile-grid.js.es6 b/app/assets/javascripts/discourse/components/tap-tile-grid.js.es6 new file mode 100644 index 00000000000..37d5fda33a1 --- /dev/null +++ b/app/assets/javascripts/discourse/components/tap-tile-grid.js.es6 @@ -0,0 +1,6 @@ +import Component from "@ember/component"; + +export default Component.extend({ + classNames: ["tap-tile-grid"], + activeTile: null +}); diff --git a/app/assets/javascripts/discourse/components/tap-tile.js.es6 b/app/assets/javascripts/discourse/components/tap-tile.js.es6 new file mode 100644 index 00000000000..a7dfbe267cc --- /dev/null +++ b/app/assets/javascripts/discourse/components/tap-tile.js.es6 @@ -0,0 +1,12 @@ +import Component from "@ember/component"; +import { propertyEqual } from "discourse/lib/computed"; + +export default Component.extend({ + classNames: ["tap-tile"], + classNameBindings: ["active"], + click() { + this.onSelect(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 new file mode 100644 index 00000000000..b02a2d9ac6e --- /dev/null +++ b/app/assets/javascripts/discourse/controllers/bookmark.js.es6 @@ -0,0 +1,217 @@ +import Controller from "@ember/controller"; +import ModalFunctionality from "discourse/mixins/modal-functionality"; +import discourseComputed from "discourse-common/utils/decorators"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import { htmlSafe } from "@ember/template"; +import { ajax } from "discourse/lib/ajax"; +import { reads } from "@ember/object/computed"; + +const START_OF_DAY_HOUR = 8; +const REMINDER_TYPES = { + AT_DESKTOP: "at_desktop", + LATER_TODAY: "later_today", + NEXT_BUSINESS_DAY: "next_business_day", + TOMORROW: "tomorrow", + NEXT_WEEK: "next_week", + NEXT_MONTH: "next_month", + CUSTOM: "custom" +}; + +export default Controller.extend(ModalFunctionality, { + loading: false, + errorMessage: null, + name: null, + selectedReminderType: null, + closeWithoutSaving: false, + isSavingBookmarkManually: false, + onCloseWithoutSaving: null, + + onShow() { + this.setProperties({ + errorMessage: null, + name: null, + selectedReminderType: null, + closeWithoutSaving: false, + isSavingBookmarkManually: false + }); + }, + + // we always want to save the bookmark unless the user specifically + // clicks the save or cancel button to mimic browser behaviour + onClose() { + if (!this.closeWithoutSaving && !this.isSavingBookmarkManually) { + this.saveBookmark(); + } + if (this.onCloseWithoutSaving && this.closeWithoutSaving) { + this.onCloseWithoutSaving(); + } + }, + + usingMobileDevice: reads("site.mobileView"), + + @discourseComputed() + reminderTypes: () => { + return REMINDER_TYPES; + }, + + @discourseComputed() + showLaterToday() { + return !this.laterToday().isSame(this.tomorrow(), "date"); + }, + + @discourseComputed() + laterTodayFormatted() { + return htmlSafe( + I18n.t("bookmarks.reminders.later_today", { + date: this.laterToday().format(I18n.t("dates.time")) + }) + ); + }, + + @discourseComputed() + tomorrowFormatted() { + return htmlSafe( + I18n.t("bookmarks.reminders.tomorrow", { + date: this.tomorrow().format(I18n.t("dates.time_short_day")) + }) + ); + }, + + @discourseComputed() + nextBusinessDayFormatted() { + return htmlSafe( + I18n.t("bookmarks.reminders.next_business_day", { + date: this.nextBusinessDay().format(I18n.t("dates.time_short_day")) + }) + ); + }, + + @discourseComputed() + nextWeekFormatted() { + return htmlSafe( + I18n.t("bookmarks.reminders.next_week", { + date: this.nextWeek().format(I18n.t("dates.month_day_time")) + }) + ); + }, + + @discourseComputed() + nextMonthFormatted() { + return htmlSafe( + I18n.t("bookmarks.reminders.next_month", { + date: this.nextMonth().format(I18n.t("dates.month_day_time")) + }) + ); + }, + + @discourseComputed() + userHasTimezoneSet() { + return !_.isEmpty(this.userTimezone()); + }, + + saveBookmark() { + const reminderAt = this.reminderAt(); + const data = { + reminder_type: this.selectedReminderType, + reminder_at: reminderAt ? reminderAt.toISOString() : null, + name: this.name, + post_id: this.model.postId + }; + + return ajax("/bookmarks", { type: "POST", data }); + }, + + reminderAt() { + if (!this.selectedReminderType) { + return; + } + + switch (this.selectedReminderType) { + case REMINDER_TYPES.AT_DESKTOP: + // TODO: Implement at desktop bookmark reminder functionality + return ""; + case REMINDER_TYPES.LATER_TODAY: + return this.laterToday(); + case REMINDER_TYPES.NEXT_BUSINESS_DAY: + return this.nextBusinessDay(); + case REMINDER_TYPES.TOMORROW: + return this.tomorrow(); + case REMINDER_TYPES.NEXT_WEEK: + return this.nextWeek(); + case REMINDER_TYPES.NEXT_MONTH: + return this.nextMonth(); + case REMINDER_TYPES.CUSTOM: + // TODO: Implement custom bookmark reminder times + return ""; + } + }, + + nextWeek() { + return this.startOfDay(this.now().add(7, "days")); + }, + + nextMonth() { + return this.startOfDay(this.now().add(1, "month")); + }, + + nextBusinessDay() { + const currentDay = this.now().isoWeekday(); // 1=Mon, 7=Sun + let next = null; + + // friday + if (currentDay === 5) { + next = this.now().add(3, "days"); + // saturday + } else if (currentDay === 6) { + next = this.now().add(2, "days"); + } else { + next = this.now().add(1, "day"); + } + + return this.startOfDay(next); + }, + + tomorrow() { + return this.startOfDay(this.now().add(1, "day")); + }, + + startOfDay(momentDate) { + return momentDate.hour(START_OF_DAY_HOUR).startOf("hour"); + }, + + userTimezone() { + return this.currentUser.timezone; + }, + + now() { + return moment.tz(this.userTimezone()); + }, + + laterToday() { + let later = this.now().add(3, "hours"); + return later.minutes() < 30 + ? later.minutes(30) + : later.add(30, "minutes").startOf("hour"); + }, + + actions: { + saveAndClose() { + this.isSavingBookmarkManually = true; + this.saveBookmark() + .then(() => this.send("closeModal")) + .catch(e => { + this.isSavingBookmarkManually = false; + popupAjaxError(e); + }); + }, + + closeWithoutSavingBookmark() { + this.closeWithoutSaving = true; + this.send("closeModal"); + }, + + selectReminderType(type) { + this.set("selectedReminderType", type); + } + } +}); diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index ed1f8d8add4..66bb4acebfb 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -679,6 +679,16 @@ export default Controller.extend(bufferedProperty("model"), { } }, + toggleBookmarkWithReminder(post) { + if (!this.currentUser) { + return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); + } else if (post) { + return post.toggleBookmarkWithReminder(); + } else { + return this.model.toggleBookmarkWithReminder(); + } + }, + toggleFeaturedOnProfile() { if (!this.currentUser) return; diff --git a/app/assets/javascripts/discourse/lib/transform-post.js.es6 b/app/assets/javascripts/discourse/lib/transform-post.js.es6 index a0189345210..f6d19c6b46b 100644 --- a/app/assets/javascripts/discourse/lib/transform-post.js.es6 +++ b/app/assets/javascripts/discourse/lib/transform-post.js.es6 @@ -35,6 +35,8 @@ export function transformBasicPost(post) { username: post.username, avatar_template: post.avatar_template, bookmarked: post.bookmarked, + bookmarkedWithReminder: post.bookmarked_with_reminder, + bookmarkReminderAt: post.bookmark_reminder_at, 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 5bb020e4442..3fbf7af8771 100644 --- a/app/assets/javascripts/discourse/models/post.js.es6 +++ b/app/assets/javascripts/discourse/models/post.js.es6 @@ -16,6 +16,7 @@ import Composer from "discourse/models/composer"; import { Promise } from "rsvp"; import Site from "discourse/models/site"; import User from "discourse/models/user"; +import showModal from "discourse/lib/show-modal"; const Post = RestModel.extend({ // TODO: Remove this once one instantiate all `Discourse.Post` models via the store. @@ -336,6 +337,32 @@ const Post = RestModel.extend({ }); }, + toggleBookmarkWithReminder() { + this.toggleProperty("bookmarkedWithReminder"); + if (this.bookmarkedWithReminder) { + let controller = showModal("bookmark", { + model: { + postId: this.id + }, + title: "post.bookmarks.create", + modalClass: "bookmark-with-reminder" + }); + controller.setProperties({ + onCloseWithoutSaving: () => { + this.toggleProperty("bookmarkedWithReminder"); + this.appEvents.trigger("post-stream:refresh", { id: this.id }); + } + }); + } else { + return Post.destroyBookmark(this.id) + .then(() => this.appEvents.trigger("page:bookmark-post-toggled", this)) + .catch(error => { + this.toggleProperty("bookmarkedWithReminder"); + throw new Error(error); + }); + } + }, + updateActionsSummary(json) { if (json && json.id === this.id) { json = Post.munge(json); @@ -385,6 +412,12 @@ Post.reopenClass({ }); }, + destroyBookmark(postId) { + return ajax(`/posts/${postId}/bookmark`, { + type: "DELETE" + }); + }, + deleteMany(post_ids, { agreeWithFirstReplyFlag = true } = {}) { return ajax("/posts/destroy_many", { type: "DELETE", diff --git a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs new file mode 100644 index 00000000000..ff1800cdcd3 --- /dev/null +++ b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs @@ -0,0 +1,49 @@ +{{#d-modal-body}} + {{#conditional-loading-spinner condition=loading}} + {{#if errorMessage}} +
+
+
{{errorMessage}}
+
+
+ {{/if}} + +
+ + + {{input value=name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}} +
+ +
+ + + {{#if userHasTimezoneSet}} + {{#tap-tile-grid activeTile=selectedReminderType as |grid|}} + {{#if usingMobileDevice}} + + {{/if}} + + {{#if showLaterToday}} + {{tap-tile icon="far-moon" text=laterTodayFormatted tileId=reminderTypes.LATER_TODAY activeTile=grid.activeTile onSelect=(action "selectReminderType")}} + {{/if}} + {{tap-tile icon="briefcase" text=nextBusinessDayFormatted tileId=reminderTypes.NEXT_BUSINESS_DAY activeTile=grid.activeTile onSelect=(action "selectReminderType")}} + {{tap-tile icon="far-sun" text=tomorrowFormatted tileId=reminderTypes.TOMORROW activeTile=grid.activeTile onSelect=(action "selectReminderType")}} + {{tap-tile icon="far-clock" text=nextWeekFormatted tileId=reminderTypes.NEXT_WEEK activeTile=grid.activeTile onSelect=(action "selectReminderType")}} + {{tap-tile icon="far-calendar-plus" text=nextMonthFormatted tileId=reminderTypes.NEXT_MONTH activeTile=grid.activeTile onSelect=(action "selectReminderType")}} + + {{/tap-tile-grid}} + {{else}} +
{{{i18n "bookmarks.no_timezone" basePath=basePath }}}
+ {{/if}} +
+ +
+ {{d-button label="bookmarks.save" class="btn-primary" action=(action "saveAndClose")}} + {{d-modal-cancel close=(action "closeWithoutSavingBookmark")}} +
+ {{/conditional-loading-spinner}} +{{/d-modal-body}} diff --git a/app/assets/javascripts/discourse/templates/topic.hbs b/app/assets/javascripts/discourse/templates/topic.hbs index ad6a1557840..f1cdd677658 100644 --- a/app/assets/javascripts/discourse/templates/topic.hbs +++ b/app/assets/javascripts/discourse/templates/topic.hbs @@ -193,6 +193,7 @@ expandHidden=(action "expandHidden") newTopicAction=(action "replyAsNewTopic") toggleBookmark=(action "toggleBookmark") + toggleBookmarkWithReminder=(action "toggleBookmarkWithReminder") togglePostType=(action "togglePostType") rebakePost=(action "rebakePost") changePostOwner=(action "changePostOwner") diff --git a/app/assets/javascripts/discourse/widgets/post-menu.js.es6 b/app/assets/javascripts/discourse/widgets/post-menu.js.es6 index 2039ed446f5..a21c921420b 100644 --- a/app/assets/javascripts/discourse/widgets/post-menu.js.es6 +++ b/app/assets/javascripts/discourse/widgets/post-menu.js.es6 @@ -301,6 +301,41 @@ registerButton("bookmark", attrs => { }; }); +registerButton("bookmarkWithReminder", (attrs, state, siteSettings) => { + if (!attrs.canBookmark || !siteSettings.enable_bookmarks_with_reminders) { + return; + } + + let classNames = ["bookmark", "with-reminder"]; + let title = "bookmarks.not_bookmarked"; + let titleOptions = {}; + + if (attrs.bookmarkedWithReminder) { + classNames.push("bookmarked"); + + if (attrs.bookmarkReminderAt) { + let reminderAtDate = moment(attrs.bookmarkReminderAt).tz( + Discourse.currentUser.timezone + ); + title = "bookmarks.created_with_reminder"; + titleOptions = { + date: reminderAtDate.format(I18n.t("dates.long_with_year")) + }; + } else { + title = "bookmarks.created"; + } + } + + return { + id: attrs.bookmarkedWithReminder ? "unbookmark" : "bookmark", + action: "toggleBookmarkWithReminder", + title, + titleOptions, + className: classNames.join(" "), + icon: "book" + }; +}); + registerButton("admin", attrs => { if (!attrs.canManage && !attrs.canWiki) { return; @@ -409,7 +444,10 @@ export default createWidget("post-menu", { const hiddenSetting = siteSettings.post_menu_hidden_items || ""; const hiddenButtons = hiddenSetting .split("|") - .filter(s => !attrs.bookmarked || s !== "bookmark"); + .filter(s => !attrs.bookmarked || s !== "bookmark") + .filter( + s => !attrs.bookmarkedWithReminder || s !== "bookmarkWithReminder" + ); if (currentUser && keyValueStore) { const likedPostId = keyValueStore.getInt("likedPostId"); diff --git a/app/assets/stylesheets/common/components/bookmark-modal.scss b/app/assets/stylesheets/common/components/bookmark-modal.scss new file mode 100644 index 00000000000..8df4d572035 --- /dev/null +++ b/app/assets/stylesheets/common/components/bookmark-modal.scss @@ -0,0 +1,14 @@ +.bookmark-with-reminder.modal { + .modal-body { + max-width: 410px; + min-width: 380px; + + .control-label { + font-weight: 700; + } + + .ember-text-field.bookmark-name { + width: 100%; + } + } +} diff --git a/app/assets/stylesheets/common/components/tap-tile.scss b/app/assets/stylesheets/common/components/tap-tile.scss new file mode 100644 index 00000000000..7e8149650ab --- /dev/null +++ b/app/assets/stylesheets/common/components/tap-tile.scss @@ -0,0 +1,41 @@ +.tap-tile-grid { + display: flex; + flex-direction: row; + flex-wrap: wrap; + justify-content: space-between; + $tile-width: 100px; + $horizontal-tile-padding: 5px; + + .tap-tile { + padding: 10px $horizontal-tile-padding; + display: flex; + flex-direction: column; + text-align: center; + align-items: center; + border: 1px solid $primary-medium; + margin: 0 0 20px; + width: $tile-width; + font-size: $font-down-2; + cursor: pointer; + + &:hover { + background-color: $tertiary-low; + } + + &.active { + background-color: $highlight-medium; + } + + .svg-icon, + .svg-icon-title { + width: 2.5em; + height: 2.5em; + margin-bottom: 10px; + } + } + + &::after { + content: ""; + width: $tile-width + ($horizontal-tile-padding * 3); + } +} diff --git a/app/assets/stylesheets/desktop/topic-post.scss b/app/assets/stylesheets/desktop/topic-post.scss index ee169e499f9..d6e0bfd9c87 100644 --- a/app/assets/stylesheets/desktop/topic-post.scss +++ b/app/assets/stylesheets/desktop/topic-post.scss @@ -206,6 +206,9 @@ nav.post-controls { &.bookmarked .d-icon { color: $tertiary; } + &.with-reminder.bookmarked .d-icon { + color: $danger; + } } } .post-admin-menu { diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb new file mode 100644 index 00000000000..d342becadae --- /dev/null +++ b/app/controllers/bookmarks_controller.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class BookmarksController < ApplicationController + requires_login + + 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: params[:topic_id], + post_id: params[:post_id], + name: params[:name], + reminder_type: Bookmark.reminder_types[params[:reminder_type].to_sym], + 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 + end +end diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index c28102530cd..44dd116c19f 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -508,6 +508,15 @@ class PostsController < ApplicationController render_json_dump(topic_bookmarked: topic_user.try(:bookmarked)) end + def destroy_bookmark + params.require(:post_id) + + existing_bookmark = Bookmark.find_by(post_id: params[:post_id], user_id: current_user.id) + existing_bookmark.destroy if existing_bookmark.present? + + render json: success_json + end + def wiki post = find_post_from_params guardian.ensure_can_wiki!(post) diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb new file mode 100644 index 00000000000..73a6bd723cf --- /dev/null +++ b/app/models/bookmark.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +class Bookmark < ActiveRecord::Base + belongs_to :user + belongs_to :post + belongs_to :topic + + validates :reminder_at, presence: { + message: I18n.t("bookmarks.errors.time_must_be_provided", reminder_type: I18n.t("bookmarks.reminders.at_desktop")), + if: -> { reminder_type.present? && reminder_type != Bookmark.reminder_types[:at_desktop] } + } + + def self.reminder_types + @reminder_type = Enum.new( + at_desktop: 0, + later_today: 1, + next_business_day: 2, + tomorrow: 3, + next_week: 4, + next_month: 5, + custom: 6 + ) + end +end + +# == Schema Information +# +# Table name: bookmarks +# +# id :bigint not null, primary key +# user_id :bigint not null +# topic_id :bigint +# post_id :bigint not null +# name :string +# reminder_type :integer +# reminder_at :datetime +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_bookmarks_on_post_id (post_id) +# index_bookmarks_on_reminder_at (reminder_at) +# index_bookmarks_on_reminder_type (reminder_type) +# index_bookmarks_on_topic_id (topic_id) +# index_bookmarks_on_user_id (user_id) +# index_bookmarks_on_user_id_and_post_id (user_id,post_id) UNIQUE +# +# Foreign Keys +# +# fk_rails_... (post_id => posts.id) +# fk_rails_... (topic_id => topics.id) +# fk_rails_... (user_id => users.id) +# diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 8651252e845..b148045041d 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -49,6 +49,8 @@ class PostSerializer < BasicPostSerializer :user_title, :reply_to_user, :bookmarked, + :bookmarked_with_reminder, + :bookmark_reminder_at, :raw, :actions_summary, :moderator?, @@ -218,10 +220,6 @@ class PostSerializer < BasicPostSerializer } end - def bookmarked - true - end - def deleted_by BasicUserSerializer.new(object.deleted_by, root: false).as_json end @@ -309,8 +307,35 @@ class PostSerializer < BasicPostSerializer !(SiteSetting.suppress_reply_when_quoting && object.reply_quoted?) && object.reply_to_user end + # this atrtribute is not even included unless include_bookmarked? is true, + # which is why it is always true if included + def bookmarked + true + end + + def bookmarked_with_reminder + true + end + def include_bookmarked? - actions.present? && actions.keys.include?(PostActionType.types[:bookmark]) + (actions.present? && actions.keys.include?(PostActionType.types[:bookmark])) + end + + def include_bookmarked_with_reminder? + post_bookmark.present? + end + + def include_bookmark_reminder_at? + include_bookmarked_with_reminder? + end + + def post_bookmark + return nil if !SiteSetting.enable_bookmarks_with_reminders? + @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id } + end + + def bookmark_reminder_at + post_bookmark&.reminder_at end def include_display_username? diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 1db21152703..5f9055905fa 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -40,6 +40,10 @@ en: # Use Moment.js format string: https://momentjs.com/docs/#/displaying/format/ time: "HH:mm" # Use Moment.js format string: https://momentjs.com/docs/#/displaying/format/ + time_short_day: "ddd HH:mm a" + # Use Moment.js format string: https://momentjs.com/docs/#/displaying/format/ + month_day_time: "MMM D, HH:mm a" + # Use Moment.js format string: https://momentjs.com/docs/#/displaying/format/ timeline_date: "MMM YYYY" # Use Moment.js format string: https://momentjs.com/docs/#/displaying/format/ long_no_year: "D MMM HH:mm" @@ -303,8 +307,19 @@ en: bookmarks: created: "you've bookmarked this post" not_bookmarked: "bookmark this post" + created_with_reminder: "you've bookmarked this post with a reminder at %{date}" remove: "Remove Bookmark" confirm_clear: "Are you sure you want to clear all your bookmarks from this topic?" + save: "Save" + no_timezone: "You have not set a timezone yet. You will not be able to set reminders. Set one up in your profile." + reminders: + at_desktop: "Next time I'm at my desktop" + later_today: "Later today
{{date}}" + next_business_day: "Next business day
{{date}}" + tomorrow: "Tomorrow
{{date}}" + next_week: "Next week
{{date}}" + next_month: "Next month
{{date}}" + custom: "Custom date and time" drafts: resume: "Resume" @@ -2606,6 +2621,12 @@ en: html_part: title: "Show the html part of the email" button: "HTML" + + bookmarks: + create: "Create bookmark" + name: "Name" + name_placeholder: "Name the bookmark to help jog your memory" + set_reminder: "Set a reminder" category: can: "can… " diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 74d947869d1..2f1cf679582 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -379,6 +379,20 @@ en: excerpt_image: "image" + bookmarks: + errors: + already_bookmarked_post: "You cannot bookmark the same post twice." + time_must_be_provided: "time must be provided for all reminders except '%{reminder_type}'" + + reminders: + at_desktop: "Next time I'm at my desktop" + later_today: "Later today
{{date}}" + next_business_day: "Next business day
{{date}}" + tomorrow: "Tomorrow
{{date}}" + next_week: "Next week
{{date}}" + next_month: "Next month
{{date}}" + custom: "Custom date and time" + groups: success: bulk_add: @@ -561,6 +575,7 @@ en: attributes: word: too_many: "Too many words for that action" + uncategorized_category_name: "Uncategorized" diff --git a/config/routes.rb b/config/routes.rb index 0edafdffa75..60a82ae4760 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -573,6 +573,7 @@ Discourse::Application.routes.draw do resources :posts do put "bookmark" + delete "bookmark", to: "posts#destroy_bookmark" put "wiki" put "post_type" put "rebake" @@ -592,6 +593,8 @@ Discourse::Application.routes.draw do end end + resources :bookmarks, only: %i[create] + resources :notifications, except: :show do collection do put 'mark-read' => 'notifications#mark_read' diff --git a/config/site_settings.yml b/config/site_settings.yml index 53a864d27a1..a39d2bbd651 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -187,7 +187,7 @@ basic: post_menu: client: true type: list - default: "read|like|share|flag|edit|bookmark|delete|admin|reply" + default: "read|like|share|flag|edit|bookmark|bookmarkWithReminder|delete|admin|reply" allow_any: false choices: - read @@ -199,10 +199,11 @@ basic: - bookmark - admin - reply + - bookmarkWithReminder post_menu_hidden_items: client: true type: list - default: "flag|bookmark|edit|delete|admin" + default: "flag|bookmark|bookmarkWithReminder|edit|delete|admin" allow_any: false choices: - like @@ -213,6 +214,7 @@ basic: - bookmark - admin - reply + - bookmarkWithReminder share_links: client: true type: list @@ -288,6 +290,10 @@ basic: enable_whispers: client: true default: false + enable_bookmarks_with_reminders: + client: true + default: false + hidden: true push_notifications_prompt: default: true client: true diff --git a/db/migrate/20191205100434_create_standalone_bookmarks_table.rb b/db/migrate/20191205100434_create_standalone_bookmarks_table.rb new file mode 100644 index 00000000000..74c636a8815 --- /dev/null +++ b/db/migrate/20191205100434_create_standalone_bookmarks_table.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class CreateStandaloneBookmarksTable < ActiveRecord::Migration[6.0] + def up + create_table :bookmarks do |t| + t.references :user, index: true, foreign_key: true, null: false + t.references :topic, index: true, foreign_key: true, null: true + t.references :post, index: true, foreign_key: true, null: false + t.string :name, null: true + t.integer :reminder_type, null: true, index: true + t.datetime :reminder_at, null: true, index: true + + t.timestamps + end + + add_index :bookmarks, [:user_id, :post_id], unique: true + end + + def down + drop_table(:bookmarks) if table_exists?(:bookmarks) + end +end diff --git a/lib/topic_view.rb b/lib/topic_view.rb index b95254d14ae..4e6a8f597d2 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -428,6 +428,10 @@ class TopicView @links ||= TopicLink.topic_map(@guardian, @topic.id) end + def user_post_bookmarks + @user_post_bookmarks ||= Bookmark.where(user: @user, post_id: unfiltered_post_ids) + end + def reviewable_counts if @reviewable_counts.nil? diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb new file mode 100644 index 00000000000..3234b622808 --- /dev/null +++ b/spec/fabricators/bookmark_fabricator.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +Fabricator(:bookmark) do + user + post { Fabricate(:post) } + topic nil + name "This looked interesting" + reminder_type { Bookmark.reminder_types[:tomorrow] } + reminder_at { (Time.now.utc + 1.day).iso8601 } +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 + else + Time.now.utc + 1.day + end + date.iso8601 + end +end diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb new file mode 100644 index 00000000000..64e23960f5d --- /dev/null +++ b/spec/requests/bookmarks_controller_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe BookmarksController do + let(:current_user) { Fabricate(:user) } + let(:bookmark_post) { Fabricate(:post) } + + before do + sign_in(current_user) + end + + describe "#create" do + context "if the user already has bookmarked the post" do + before do + Fabricate(:bookmark, post: bookmark_post, user: current_user) + end + + it "returns failed JSON with a 422 error" do + post "/bookmarks.json", params: { + post_id: bookmark_post.id, + reminder_type: "tomorrow", + reminder_at: (Time.now.utc + 1.day).iso8601 + } + + expect(response.status).to eq(422) + expect(JSON.parse(response.body)['errors']).to include( + I18n.t("bookmarks.errors.already_bookmarked_post") + ) + end + end + + context "if the user provides a reminder type that needs a reminder_at that is missing" do + it "returns failed JSON with a 400 error" do + post "/bookmarks.json", params: { + post_id: bookmark_post.id, + reminder_type: "tomorrow" + } + + expect(response.status).to eq(400) + expect(JSON.parse(response.body)['errors'].first).to include( + I18n.t("bookmarks.errors.time_must_be_provided", reminder_type: I18n.t("bookmarks.reminders.at_desktop")) + ) + end + end + end +end diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 1032909aad3..fe30193d30f 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -109,12 +109,6 @@ describe PostSerializer do let(:user) { Fabricate.build(:user, id: 101) } let(:raw) { "Raw contents of the post." } - def serialized_post_for_user(u) - s = PostSerializer.new(post, scope: Guardian.new(u), root: false) - s.add_raw = true - s.as_json - end - context "a public post" do let(:post) { Fabricate.build(:post, raw: raw, user: user) } @@ -231,4 +225,68 @@ describe PostSerializer do end end + context "post with bookmarks" do + let(:current_user) { Fabricate(:user) } + let(:serialized) do + s = serialized_post(current_user) + s.post_actions = PostAction.counts_for([post], current_user)[post.id] + s.topic_view = TopicView.new(post.topic, current_user) + s + end + + context "when a user post action for the bookmark exists" do + before do + PostActionCreator.create(current_user, post, :bookmark) + end + + it "returns true" do + expect(serialized.as_json[:bookmarked]).to eq(true) + end + end + + context "when a user post action for the bookmark does not exist" do + it "does not return the bookmarked attribute" do + expect(serialized.as_json.key?(:bookmarked)).to eq(false) + end + end + + 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 + + it "returns true" do + expect(serialized.as_json[:bookmarked_with_reminder]).to eq(true) + end + + it "returns the reminder_at for the bookmark" do + expect(serialized.as_json[:bookmark_reminder_at]).to eq(bookmark.reminder_at.iso8601) + end + end + + context "when the site setting for bookmarks with reminders is disabled" do + it "does not return the bookmarked 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 + + def serialized_post(u) + s = PostSerializer.new(post, scope: Guardian.new(u), root: false) + s.add_raw = true + s + end + + def serialized_post_for_user(u) + s = serialized_post(u) + s.as_json + end end diff --git a/test/javascripts/controllers/bookmark-test.js.es6 b/test/javascripts/controllers/bookmark-test.js.es6 new file mode 100644 index 00000000000..acb9085f69f --- /dev/null +++ b/test/javascripts/controllers/bookmark-test.js.es6 @@ -0,0 +1,144 @@ +import { currentUser } from "helpers/qunit-helpers"; +let BookmarkController; + +moduleFor("controller:bookmark", { + beforeEach() { + BookmarkController = this.subject({ currentUser: currentUser() }); + } +}); + +function mockMomentTz(dateString) { + let now = moment.tz(dateString, BookmarkController.currentUser.timezone); + sandbox.useFakeTimers(now.valueOf()); +} + +QUnit.test("showLaterToday when later today is tomorrow do not show", function( + assert +) { + mockMomentTz("2019-12-11T13:00:00Z"); + + assert.equal(BookmarkController.get("showLaterToday"), false); +}); + +QUnit.test( + "showLaterToday when later today is before the end of the day, show", + function(assert) { + mockMomentTz("2019-12-11T08:00:00Z"); + + assert.equal(BookmarkController.get("showLaterToday"), true); + } +); + +QUnit.test("nextWeek gets next week correctly", function(assert) { + mockMomentTz("2019-12-11T08:00:00Z"); + + assert.equal( + BookmarkController.nextWeek().format("YYYY-MM-DD"), + "2019-12-18" + ); +}); + +QUnit.test("nextMonth gets next month correctly", function(assert) { + mockMomentTz("2019-12-11T08:00:00Z"); + + assert.equal( + BookmarkController.nextMonth().format("YYYY-MM-DD"), + "2020-01-11" + ); +}); + +QUnit.test( + "nextBusinessDay gets next business day of monday correctly if today is friday", + function(assert) { + mockMomentTz("2019-12-13T08:00:00Z"); + + assert.equal( + BookmarkController.nextBusinessDay().format("YYYY-MM-DD"), + "2019-12-16" + ); + } +); + +QUnit.test( + "nextBusinessDay gets next business day of monday correctly if today is saturday", + function(assert) { + mockMomentTz("2019-12-14T08:00:00Z"); + + assert.equal( + BookmarkController.nextBusinessDay().format("YYYY-MM-DD"), + "2019-12-16" + ); + } +); + +QUnit.test( + "nextBusinessDay gets next business day of monday correctly if today is sunday", + function(assert) { + mockMomentTz("2019-12-15T08:00:00Z"); + + assert.equal( + BookmarkController.nextBusinessDay().format("YYYY-MM-DD"), + "2019-12-16" + ); + } +); + +QUnit.test( + "nextBusinessDay gets next business day of thursday correctly if today is wednesday", + function(assert) { + mockMomentTz("2019-12-11T08:00:00Z"); + + assert.equal( + BookmarkController.nextBusinessDay().format("YYYY-MM-DD"), + "2019-12-12" + ); + } +); + +QUnit.test("tomorrow gets tomorrow correctly", function(assert) { + mockMomentTz("2019-12-11T08:00:00Z"); + + assert.equal( + BookmarkController.tomorrow().format("YYYY-MM-DD"), + "2019-12-12" + ); +}); + +QUnit.test( + "startOfDay changes the time of the provided date to 8:00am correctly", + function(assert) { + let dt = moment.tz( + "2019-12-11T11:37:16Z", + BookmarkController.currentUser.timezone + ); + + assert.equal( + BookmarkController.startOfDay(dt).format("YYYY-MM-DD HH:mm:ss"), + "2019-12-11 08:00:00" + ); + } +); + +QUnit.test( + "laterToday gets 3 hours from now and if before half-past, it sets the time to half-past", + function(assert) { + mockMomentTz("2019-12-11T08:13:00Z"); + + assert.equal( + BookmarkController.laterToday().format("YYYY-MM-DD HH:mm:ss"), + "2019-12-11 21:30:00" + ); + } +); + +QUnit.test( + "laterToday gets 3 hours from now and if after half-past, it rounds up to the next hour", + function(assert) { + mockMomentTz("2019-12-11T08:43:00Z"); + + assert.equal( + BookmarkController.laterToday().format("YYYY-MM-DD HH:mm:ss"), + "2019-12-11 22:00:00" + ); + } +); diff --git a/test/javascripts/fixtures/session-fixtures.js.es6 b/test/javascripts/fixtures/session-fixtures.js.es6 index c4184c13fea..090bf099366 100644 --- a/test/javascripts/fixtures/session-fixtures.js.es6 +++ b/test/javascripts/fixtures/session-fixtures.js.es6 @@ -27,7 +27,8 @@ export default { muted_category_ids: [], dismissed_banner_key: null, akismet_review_count: 0, - title_count_mode: "notifications" + title_count_mode: "notifications", + timezone: "Australia/Brisbane" } } };