From ebe65577ed33f0d44c82380b194dbd13398e5237 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 16 Jan 2019 10:40:16 +0800 Subject: [PATCH] FEATURE: Consolidate likes notifications. (#6879) --- .../discourse-common/lib/icon-library.js.es6 | 1 + .../discourse/models/user-stream.js.es6 | 12 ++- .../routes/user-activity-stream.js.es6 | 17 ++-- .../widgets/notification-item.js.es6 | 22 ++++- app/controllers/user_actions_controller.rb | 19 +++-- app/models/notification.rb | 9 +- app/models/user_action.rb | 7 ++ app/services/post_action_notifier.rb | 85 +++++++++++++++++-- config/locales/client.en.yml | 4 + config/locales/server.en.yml | 4 + config/site_settings.yml | 8 ++ spec/fabricators/post_fabricator.rb | 4 +- spec/models/notification_spec.rb | 26 ++++++ spec/models/post_action_spec.rb | 68 +++++++++++++++ spec/requests/user_actions_controller_spec.rb | 23 +++++ .../fixtures/notification_fixtures.js.es6 | 10 ++- .../javascripts/fixtures/site-fixtures.js.es6 | 5 +- .../javascripts/widgets/user-menu-test.js.es6 | 24 ++++++ 18 files changed, 317 insertions(+), 31 deletions(-) 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 1fe28e89eec..5a1eb190366 100644 --- a/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 +++ b/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 @@ -24,6 +24,7 @@ const REPLACEMENTS = { "notification.liked": "heart", "notification.liked_2": "heart", "notification.liked_many": "heart", + "notification.liked_consolidated": "heart", "notification.private_message": "far-envelope", "notification.invited_to_private_message": "far-envelope", "notification.invited_to_topic": "hand-point-right", diff --git a/app/assets/javascripts/discourse/models/user-stream.js.es6 b/app/assets/javascripts/discourse/models/user-stream.js.es6 index 3f1650c3002..891b7f937fa 100644 --- a/app/assets/javascripts/discourse/models/user-stream.js.es6 +++ b/app/assets/javascripts/discourse/models/user-stream.js.es6 @@ -30,14 +30,16 @@ export default RestModel.extend({ "/user_actions.json?offset=%@&username=%@" ), - filterBy(filter, noContentHelpKey) { + filterBy(filter, noContentHelpKey, actingUsername) { this.setProperties({ filter, itemsLoaded: 0, content: [], - noContentHelpKey: noContentHelpKey, - lastLoadedUrl: null + noContentHelpKey, + lastLoadedUrl: null, + actingUsername }); + return this.findItems(); }, @@ -77,6 +79,10 @@ export default RestModel.extend({ findUrl += "&no_results_help_key=" + this.get("noContentHelpKey"); } + if (this.get("actingUsername")) { + findUrl += `&acting_username=${this.get("actingUsername")}`; + } + // Don't load the same stream twice. We're probably at the end. const lastLoadedUrl = this.get("lastLoadedUrl"); if (lastLoadedUrl === findUrl) { diff --git a/app/assets/javascripts/discourse/routes/user-activity-stream.js.es6 b/app/assets/javascripts/discourse/routes/user-activity-stream.js.es6 index bd927e36742..4ba462efd7a 100644 --- a/app/assets/javascripts/discourse/routes/user-activity-stream.js.es6 +++ b/app/assets/javascripts/discourse/routes/user-activity-stream.js.es6 @@ -1,17 +1,20 @@ import ViewingActionType from "discourse/mixins/viewing-action-type"; export default Discourse.Route.extend(ViewingActionType, { + queryParams: { + acting_username: { refreshModel: true } + }, + model() { return this.modelFor("user").get("stream"); }, - afterModel() { - return this.modelFor("user") - .get("stream") - .filterBy( - this.get("userActionType"), - this.get("noContentHelpKey") || "user_activity.no_default" - ); + afterModel(model, transition) { + return model.filterBy( + this.get("userActionType"), + this.get("noContentHelpKey") || "user_activity.no_default", + transition.queryParams.acting_username + ); }, renderTemplate() { diff --git a/app/assets/javascripts/discourse/widgets/notification-item.js.es6 b/app/assets/javascripts/discourse/widgets/notification-item.js.es6 index 3de11d5cafd..bd43d2d8560 100644 --- a/app/assets/javascripts/discourse/widgets/notification-item.js.es6 +++ b/app/assets/javascripts/discourse/widgets/notification-item.js.es6 @@ -16,6 +16,7 @@ import { iconNode } from "discourse-common/lib/icon-library"; const LIKED_TYPE = 5; const INVITED_TYPE = 8; const GROUP_SUMMARY_TYPE = 16; +export const LIKED_CONSOLIDATED_TYPE = 19; createWidget("notification-item", { tagName: "li", @@ -61,6 +62,14 @@ createWidget("notification-item", { return userPath(data.display_username); } + if (attrs.notification_type === LIKED_CONSOLIDATED_TYPE) { + return userPath( + `${ + this.currentUser.username + }/notifications/likes-received?acting_username=${data.display_username}` + ); + } + if (data.group_id) { return userPath(data.username + "/messages/group/" + data.group_name); } @@ -77,7 +86,16 @@ createWidget("notification-item", { return this.attrs.fancy_title; } - const title = data.topic_title; + let title; + + if (this.attrs.notification_type === LIKED_CONSOLIDATED_TYPE) { + title = I18n.t("notifications.liked_consolidated_description", { + count: parseInt(data.count) + }); + } else { + title = data.topic_title; + } + return Ember.isEmpty(title) ? "" : escapeExpression(title); }, @@ -95,9 +113,11 @@ createWidget("notification-item", { const username = formatUsername(data.display_username); const description = this.description(); + if (notificationType === LIKED_TYPE && data.count > 1) { const count = data.count - 2; const username2 = formatUsername(data.username2); + if (count === 0) { return I18n.t("notifications.liked_2", { description, diff --git a/app/controllers/user_actions_controller.rb b/app/controllers/user_actions_controller.rb index d72b28d49f4..2d26bbbf34d 100644 --- a/app/controllers/user_actions_controller.rb +++ b/app/controllers/user_actions_controller.rb @@ -2,7 +2,7 @@ class UserActionsController < ApplicationController def index params.require(:username) - params.permit(:filter, :offset) + params.permit(:filter, :offset, :acting_username) per_chunk = 30 @@ -11,13 +11,16 @@ class UserActionsController < ApplicationController action_types = (params[:filter] || "").split(",").map(&:to_i) - opts = { user_id: user.id, - user: user, - offset: params[:offset].to_i, - limit: per_chunk, - action_types: action_types, - guardian: guardian, - ignore_private_messages: params[:filter] ? false : true } + opts = { + user_id: user.id, + user: user, + offset: params[:offset].to_i, + limit: per_chunk, + action_types: action_types, + guardian: guardian, + ignore_private_messages: params[:filter] ? false : true, + acting_username: params[:acting_username] + } # Pending is restricted stream = if opts[:action_types].include?(UserAction::PENDING) diff --git a/app/models/notification.rb b/app/models/notification.rb index 27ecc9a4c30..40663702986 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -13,6 +13,12 @@ class Notification < ActiveRecord::Base scope :visible , lambda { joins('LEFT JOIN topics ON notifications.topic_id = topics.id') .where('topics.id IS NULL OR topics.deleted_at IS NULL') } + scope :get_liked_by, ->(user) { + where("data::json ->> 'original_username' = ?", user.username_lower) + .where(notification_type: Notification.types[:liked]) + .order(created_at: :desc) + } + attr_accessor :skip_send_email after_commit :send_email, on: :create @@ -53,7 +59,8 @@ class Notification < ActiveRecord::Base group_mentioned: 15, group_message_summary: 16, watching_first_post: 17, - topic_reminder: 18 + topic_reminder: 18, + liked_consolidated: 19, ) end diff --git a/app/models/user_action.rb b/app/models/user_action.rb index 4d9e86da4a0..8a7d8d06020 100644 --- a/app/models/user_action.rb +++ b/app/models/user_action.rb @@ -201,6 +201,7 @@ class UserAction < ActiveRecord::Base ignore_private_messages = opts[:ignore_private_messages] offset = opts[:offset] || 0 limit = opts[:limit] || 60 + acting_username = opts[:acting_username] # Acting user columns. Can be extended by plugins to include custom avatar # columns @@ -258,6 +259,12 @@ class UserAction < ActiveRecord::Base builder.where("a.user_id = :user_id", user_id: user_id.to_i) builder.where("a.action_type in (:action_types)", action_types: action_types) if action_types && action_types.length > 0 + if acting_username + builder.where("u.username_lower = :acting_username", + acting_username: acting_username.downcase + ) + end + unless SiteSetting.enable_mentions? builder.where("a.action_type <> :mention_type", mention_type: UserAction::MENTION) end diff --git a/app/services/post_action_notifier.rb b/app/services/post_action_notifier.rb index c4d988f25dc..39dd205db00 100644 --- a/app/services/post_action_notifier.rb +++ b/app/services/post_action_notifier.rb @@ -76,16 +76,87 @@ class PostActionNotifier post = post_action.post return if post_action.user.blank? - alerter.create_notification( - post.user, - Notification.types[:liked], - post, - display_username: post_action.user.username, - post_action_id: post_action.id, - user_id: post_action.user_id + user_notifications = post.user.notifications + + consolidation_window = + SiteSetting.likes_notification_consolidation_window_mins.minutes.ago + + liked_by_user_notifications = + user_notifications + .get_liked_by(post_action.user) + .where("created_at > ?", consolidation_window) + + user_liked_consolidated_notification = + user_notifications + .where( + "created_at > ? AND notification_type = ?", + consolidation_window, + Notification.types[:liked_consolidated] + ) + .first + + if user_liked_consolidated_notification + update_consolidated_liked_notification_count!( + user_liked_consolidated_notification + ) + elsif ( + liked_by_user_notifications.count >= + SiteSetting.likes_notification_consolidation_threshold ) + create_consolidated_liked_notification!( + liked_by_user_notifications, + post, + post_action + ) + else + alerter.create_notification( + post.user, + Notification.types[:liked], + post, + display_username: post_action.user.username, + post_action_id: post_action.id, + user_id: post_action.user_id + ) + end end + def self.update_consolidated_liked_notification_count!(notification) + Notification.transaction do + data = JSON.parse(notification.data) + data["count"] += 1 + + notification.update!( + data: data.to_json, + read: false + ) + end + end + private_class_method :update_consolidated_liked_notification_count! + + def self.create_consolidated_liked_notification!(notifications, + post, + post_action) + + Notification.transaction do + timestamp = notifications.last.created_at + + Notification.create!( + notification_type: Notification.types[:liked_consolidated], + user_id: post.user_id, + data: { + username: post_action.user.username, + display_username: post_action.user.username, + count: notifications.count + 1 + }.to_json, + updated_at: timestamp, + created_at: timestamp + ) + + notifications.delete_all + end + end + private_class_method :create_consolidated_liked_notification! + def self.after_create_post_revision(post_revision) return if @disabled diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 12d29ad8515..15bdffa7dd4 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1522,6 +1522,10 @@ en: liked_many: one: "{{username}}, {{username2}} and 1 other {{description}}" other: "{{username}}, {{username2}} and {{count}} others {{description}}" + liked_consolidated_description: + one: "liked {{count}} of your posts" + other: "liked {{count}} of your posts" + liked_consolidated: "{{username}} {{description}}" private_message: "{{username}} {{description}}" invited_to_private_message: "

{{username}} {{description}}" invited_to_topic: "{{username}} {{description}}" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 996f501c7e4..8939c16d631 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1789,6 +1789,10 @@ en: disable_edit_notifications: "Disables edit notifications by the system user when 'download_remote_images_to_local' is active." + likes_notification_consolidation_threshold: "Number of liked notifications received before the notifications are consolidated into a single one. The window can be configured via `SiteSetting.likes_notification_consolidation_window_mins`." + + likes_notification_consolidation_window_mins: "Duration in minutes where liked notifications are consolidated into a single notification once the threshold has been reaced. The threshold can be configured via `SiteSetting.likes_notification_consolidation_threshold`." + automatically_unpin_topics: "Automatically unpin topics when the user reaches the bottom." read_time_word_count: "Word count per minute for calculating estimated reading time." diff --git a/config/site_settings.yml b/config/site_settings.yml index 8e669595c75..d788ca9e8d0 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1700,6 +1700,14 @@ uncategorized: disable_edit_notifications: false + likes_notification_consolidation_threshold: + default: 5 + min: 3 + + likes_notification_consolidation_window_mins: + default: 120 + min: 1 + delete_drafts_older_than_n_days: default: 180 diff --git a/spec/fabricators/post_fabricator.rb b/spec/fabricators/post_fabricator.rb index b01f7e17a45..41de111122a 100644 --- a/spec/fabricators/post_fabricator.rb +++ b/spec/fabricators/post_fabricator.rb @@ -29,14 +29,14 @@ Fabricator(:moderator_post, from: :post) do end Fabricator(:basic_reply, from: :post) do - user(:coding_horror) + user(fabricator: :coding_horror) reply_to_post_number 1 topic raw 'this reply has no quotes' end Fabricator(:reply, from: :post) do - user(:coding_horror) + user(fabricator: :coding_horror) topic raw ' [quote="Evil Trout, post:1"]hello[/quote] diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 34af1be3431..39b20554f06 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -250,6 +250,32 @@ describe Notification do end end + describe '.get_liked_by' do + let(:post) { Fabricate(:post) } + let(:user) { Fabricate(:user) } + + before do + PostActionNotifier.enable + end + + it 'should return the right notifications' do + expect(Notification.get_liked_by(user)).to eq([]) + + expect do + PostAlerter.post_created(Fabricate(:basic_reply, + user: user, + topic: post.topic + )) + + PostAction.act(user, post, PostActionType.types[:like]) + end.to change { Notification.count }.by(2) + + expect(Notification.get_liked_by(user)).to contain_exactly( + Notification.find_by(notification_type: Notification.types[:liked]) + ) + end + end + end # pulling this out cause I don't want an observer diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 8cf531d4033..b415c3e602f 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -296,6 +296,74 @@ describe PostAction do expect(Notification.exists?(id: notification.id)).to eq(false) end + describe 'likes consolidation' do + let(:liker) { Fabricate(:user) } + let(:likee) { Fabricate(:user) } + + before do + SiteSetting.likes_notification_consolidation_threshold = 3 + end + + it 'should consolidate likes notification when the threshold is reached' do + freeze_time + + expect do + 4.times do + PostAction.act( + liker, + Fabricate(:post, user: likee), + PostActionType.types[:like] + ) + end + end.to change { likee.reload.notifications.count }.by(1) + + notification = likee.notifications.last + + expect(notification.notification_type).to eq( + Notification.types[:liked_consolidated] + ) + + data = JSON.parse(notification.data) + + expect(data["username"]).to eq(liker.username) + expect(data["display_username"]).to eq(liker.username) + expect(data["count"]).to eq(4) + + notification.update!(read: true) + + expect do + 2.times do + PostAction.act( + liker, + Fabricate(:post, user: likee), + PostActionType.types[:like] + ) + end + end.to_not change { likee.reload.notifications.count } + + data = JSON.parse(notification.reload.data) + + expect(notification.read).to eq(false) + expect(data["count"]).to eq(6) + + freeze_time( + SiteSetting.likes_notification_consolidation_window_mins.minutes.since + ) + + expect do + PostAction.act( + liker, + Fabricate(:post, user: likee), + PostActionType.types[:like] + ) + end.to change { likee.reload.notifications.count }.by(1) + + notification = likee.notifications.last + + expect(notification.notification_type).to eq(Notification.types[:liked]) + end + end + it "should not generate a notification if liker has been muted" do mutee = Fabricate(:user) MutedUser.create!(user_id: post.user.id, muted_user_id: mutee.id) diff --git a/spec/requests/user_actions_controller_spec.rb b/spec/requests/user_actions_controller_spec.rb index 471a66271ed..2e68d54da0f 100644 --- a/spec/requests/user_actions_controller_spec.rb +++ b/spec/requests/user_actions_controller_spec.rb @@ -34,6 +34,29 @@ describe UserActionsController do expect(action["post_number"]).to eq(1) end + it 'can be filtered by acting_username' do + UserActionCreator.enable + PostActionNotifier.enable + + post = Fabricate(:post) + user = Fabricate(:user) + PostAction.act(user, post, PostActionType.types[:like]) + + get "/user_actions.json", params: { + username: post.user.username, + acting_username: user.username + } + + expect(response.status).to eq(200) + + response_body = JSON.parse(response.body) + + expect(response_body["user_actions"].count).to eq(1) + + expect(response_body["user_actions"].first["acting_username"]) + .to eq(user.username) + end + it 'renders help text if provided for self' do logged_in = sign_in(Fabricate(:user)) diff --git a/test/javascripts/fixtures/notification_fixtures.js.es6 b/test/javascripts/fixtures/notification_fixtures.js.es6 index d9669d9d9b2..2178823ffc9 100644 --- a/test/javascripts/fixtures/notification_fixtures.js.es6 +++ b/test/javascripts/fixtures/notification_fixtures.js.es6 @@ -1,4 +1,6 @@ /*jshint maxlen:10000000 */ +import { LIKED_CONSOLIDATED_TYPE } from "discourse/widgets/notification-item"; + export default { "/notifications": { notifications: [ @@ -10,7 +12,13 @@ export default { topic_id: 1234, slug: "a-slug", data: { topic_title: "some title", display_username: "velesin" } - } + }, + { + id: 456, + notification_type: LIKED_CONSOLIDATED_TYPE, + read: false, + data: { display_username: "aquaman", count: "5" } + }, ] } }; diff --git a/test/javascripts/fixtures/site-fixtures.js.es6 b/test/javascripts/fixtures/site-fixtures.js.es6 index 647b908d51d..ebad5e4f619 100644 --- a/test/javascripts/fixtures/site-fixtures.js.es6 +++ b/test/javascripts/fixtures/site-fixtures.js.es6 @@ -1,3 +1,5 @@ +import { LIKED_CONSOLIDATED_TYPE } from "discourse/widgets/notification-item"; + export default { "site.json": { site: { @@ -16,7 +18,8 @@ export default { posted: 9, moved_post: 10, linked: 11, - granted_badge: 12 + granted_badge: 12, + liked_consolidated: LIKED_CONSOLIDATED_TYPE, }, post_types: { regular: 1, diff --git a/test/javascripts/widgets/user-menu-test.js.es6 b/test/javascripts/widgets/user-menu-test.js.es6 index c6e75902723..25d59f5a629 100644 --- a/test/javascripts/widgets/user-menu-test.js.es6 +++ b/test/javascripts/widgets/user-menu-test.js.es6 @@ -15,6 +15,30 @@ widgetTest("basics", { } }); +widgetTest("notifications", { + template: '{{mount-widget widget="user-menu"}}', + + test(assert) { + const $links = find(".notifications li a"); + + assert.equal($links.length, 2); + assert.ok($links[0].href.includes("/t/a-slug/123")); + + assert.ok( + $links[1].href.includes( + "/u/eviltrout/notifications/likes-received?acting_username=aquaman" + ) + ); + + assert.equal( + $links[1].text, + `aquaman ${I18n.t("notifications.liked_consolidated_description", { + count: 5 + })}` + ); + } +}); + widgetTest("log out", { template: '{{mount-widget widget="user-menu" logout=(action "logout")}}',