From 72ac675e4ed498d4a961470358b7f807fa7f4a36 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Tue, 9 Apr 2024 11:53:37 -0600 Subject: [PATCH] FEATURE: Consolidate link notifications (#26567) Just like we have for consolidating likes this adds similar functionality for consolidating links. --- .../addon/lib/icon-library.js | 1 + .../app/lib/notification-types-manager.js | 2 + .../notification-types/linked-consolidated.js | 16 +++++ .../fixtures/concerns/notification-types.js | 2 + .../linked-consolidated-test.js | 64 +++++++++++++++++++ app/models/notification.rb | 1 + .../notifications/consolidation_planner.rb | 21 ++++++ config/locales/client.en.yml | 5 ++ config/locales/server.en.yml | 2 + config/site_settings.yml | 4 ++ .../api/schemas/json/site_response.json | 4 ++ .../consolidation_planner_spec.rb | 51 ++++++++++++++- 12 files changed, 170 insertions(+), 3 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/lib/notification-types/linked-consolidated.js create mode 100644 app/assets/javascripts/discourse/tests/unit/lib/notification-types/linked-consolidated-test.js diff --git a/app/assets/javascripts/discourse-common/addon/lib/icon-library.js b/app/assets/javascripts/discourse-common/addon/lib/icon-library.js index e86d77f861e..8a0a43f47fd 100644 --- a/app/assets/javascripts/discourse-common/addon/lib/icon-library.js +++ b/app/assets/javascripts/discourse-common/addon/lib/icon-library.js @@ -41,6 +41,7 @@ export const REPLACEMENTS = { "notification.invitee_accepted": "user", "notification.moved_post": "sign-out-alt", "notification.linked": "link", + "notification.linked_consolidated": "link", "notification.granted_badge": "certificate", "notification.topic_reminder": "far-clock", "notification.watching_first_post": "discourse-bell-one", diff --git a/app/assets/javascripts/discourse/app/lib/notification-types-manager.js b/app/assets/javascripts/discourse/app/lib/notification-types-manager.js index ffcfc6bcf72..665de7915c2 100644 --- a/app/assets/javascripts/discourse/app/lib/notification-types-manager.js +++ b/app/assets/javascripts/discourse/app/lib/notification-types-manager.js @@ -9,6 +9,7 @@ import GroupMessageSummary from "discourse/lib/notification-types/group-message- import InviteeAccepted from "discourse/lib/notification-types/invitee-accepted"; import Liked from "discourse/lib/notification-types/liked"; import LikedConsolidated from "discourse/lib/notification-types/liked-consolidated"; +import LinkedConsolidated from "discourse/lib/notification-types/linked-consolidated"; import MembershipRequestAccepted from "discourse/lib/notification-types/membership-request-accepted"; import MembershipRequestConsolidated from "discourse/lib/notification-types/membership-request-consolidated"; import MovedPost from "discourse/lib/notification-types/moved-post"; @@ -25,6 +26,7 @@ const CLASS_FOR_TYPE = { invitee_accepted: InviteeAccepted, liked: Liked, liked_consolidated: LikedConsolidated, + linked_consolidated: LinkedConsolidated, membership_request_accepted: MembershipRequestAccepted, membership_request_consolidated: MembershipRequestConsolidated, moved_post: MovedPost, diff --git a/app/assets/javascripts/discourse/app/lib/notification-types/linked-consolidated.js b/app/assets/javascripts/discourse/app/lib/notification-types/linked-consolidated.js new file mode 100644 index 00000000000..bfe4a896108 --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/notification-types/linked-consolidated.js @@ -0,0 +1,16 @@ +import NotificationTypeBase from "discourse/lib/notification-types/base"; +import { userPath } from "discourse/lib/url"; +import I18n from "discourse-i18n"; + +export default class extends NotificationTypeBase { + get linkHref() { + // Linking here for now until we have a proper new page for "linked" in the profile + return userPath(`${this.currentUser.username}/notifications`); + } + + get description() { + return I18n.t("notifications.linked_consolidated_description", { + count: this.notification.data.count, + }); + } +} diff --git a/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js b/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js index 512f63e00c8..83f912464ae 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js +++ b/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js @@ -40,6 +40,8 @@ export const NOTIFICATION_TYPES = { question_answer_user_commented: 35, watching_category_or_tag: 36, new_features: 37, + admin_problems: 38, + linked_consolidated: 39, following: 800, following_created_topic: 801, following_replied: 802, diff --git a/app/assets/javascripts/discourse/tests/unit/lib/notification-types/linked-consolidated-test.js b/app/assets/javascripts/discourse/tests/unit/lib/notification-types/linked-consolidated-test.js new file mode 100644 index 00000000000..26cf97209af --- /dev/null +++ b/app/assets/javascripts/discourse/tests/unit/lib/notification-types/linked-consolidated-test.js @@ -0,0 +1,64 @@ +import { setupTest } from "ember-qunit"; +import { module, test } from "qunit"; +import Notification from "discourse/models/notification"; +import { NOTIFICATION_TYPES } from "discourse/tests/fixtures/concerns/notification-types"; +import { createRenderDirector } from "discourse/tests/helpers/notification-types-helper"; +import { deepMerge } from "discourse-common/lib/object"; +import I18n from "discourse-i18n"; + +function getNotification(overrides = {}) { + return Notification.create( + deepMerge( + { + id: 11, + user_id: 1, + notification_type: NOTIFICATION_TYPES.linked_consolidated, + read: false, + high_priority: false, + created_at: "2022-07-01T06:00:32.173Z", + data: { + topic_title: "this is some topic and it's irrelevant", + original_post_id: 3294, + original_post_type: 1, + original_username: "liker439", + display_username: "liker439", + username: "liker439", + count: 44, + }, + }, + overrides + ) + ); +} + +module("Unit | Notification Types | linked-consolidated", function (hooks) { + setupTest(hooks); + + test("linkHref", function (assert) { + const notification = getNotification(); + const director = createRenderDirector( + notification, + "linked_consolidated", + this.siteSettings + ); + assert.strictEqual( + director.linkHref, + "/u/eviltrout/notifications", + "links to the notifications page of the user" + ); + }); + + test("description", function (assert) { + const notification = getNotification(); + const director = createRenderDirector( + notification, + "linked_consolidated", + this.siteSettings + ); + assert.strictEqual( + director.description, + I18n.t("notifications.linked_consolidated_description", { count: 44 }), + "displays the right content" + ); + }); +}); diff --git a/app/models/notification.rb b/app/models/notification.rb index 7ee7e3ed907..32ad9bed680 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -159,6 +159,7 @@ class Notification < ActiveRecord::Base watching_category_or_tag: 36, new_features: 37, admin_problems: 38, + linked_consolidated: 39, following: 800, # Used by https://github.com/discourse/discourse-follow following_created_topic: 801, # Used by https://github.com/discourse/discourse-follow following_replied: 802, # Used by https://github.com/discourse/discourse-follow diff --git a/app/services/notifications/consolidation_planner.rb b/app/services/notifications/consolidation_planner.rb index 87704b010e8..4e8b458d794 100644 --- a/app/services/notifications/consolidation_planner.rb +++ b/app/services/notifications/consolidation_planner.rb @@ -15,6 +15,7 @@ module Notifications consolidation_plans = [ liked_by_two_users, liked, + linked, group_message_summary, group_membership, new_features_notification, @@ -98,6 +99,26 @@ module Notifications ) end + def linked + ConsolidateNotifications + .new( + from: Notification.types[:linked], + to: Notification.types[:linked_consolidated], + threshold: -> { SiteSetting.notification_consolidation_threshold }, + consolidation_window: SiteSetting.linked_notification_consolidation_window_mins.minutes, + unconsolidated_query_blk: filtered_by_data_attribute("display_username"), + consolidated_query_blk: filtered_by_data_attribute("display_username"), + ) + .set_mutations( + set_data_blk: + Proc.new do |notification| + data = notification.data_hash + data.merge(username: data[:display_username]) + end, + ) + .set_precondition(precondition_blk: Proc.new { |data| data[:username2].blank? }) + end + def group_membership ConsolidateNotifications .new( diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index c7abeca864c..a0b7be3bbeb 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2667,6 +2667,10 @@ en: one: "liked %{count} of your posts" other: "liked %{count} of your posts" liked_consolidated: "%{username} %{description}" + linked_consolidated_description: + one: "linked %{count} of your posts" + other: "linked %{count} of your posts" + linked_consolidated: "%{username} %{description}" private_message: "%{username} %{description}" invited_to_private_message: "

%{username} %{description}" invited_to_topic: "%{username} %{description}" @@ -2740,6 +2744,7 @@ en: watching_first_post: "new topic" topic_reminder: "topic reminder" liked_consolidated: "new likes" + linked_consolidated: "new links" post_approved: "post approved" membership_request_consolidated: "new membership requests" reaction: "new reaction" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 8967166d20f..80b97f4467a 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2384,6 +2384,8 @@ en: likes_notification_consolidation_window_mins: "Duration in minutes where liked notifications are consolidated into a single notification once the threshold has been reached. The threshold can be configured via `SiteSetting.notification_consolidation_threshold`." + linked_notification_consolidation_window_mins: "Duration in minutes where linked notifications are consolidated into a single notification once the threshold has been reached. The threshold can be configured via `SiteSetting.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 519879096d6..a8919e49980 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2749,6 +2749,10 @@ uncategorized: default: 120 min: 1 + linked_notification_consolidation_window_mins: + default: 120 + min: 1 + delete_drafts_older_than_n_days: default: 180 max: 36500 diff --git a/spec/requests/api/schemas/json/site_response.json b/spec/requests/api/schemas/json/site_response.json index 2004f7e0153..0b77cd938cf 100644 --- a/spec/requests/api/schemas/json/site_response.json +++ b/spec/requests/api/schemas/json/site_response.json @@ -74,6 +74,9 @@ "liked_consolidated": { "type": "integer" }, + "linked_consolidated": { + "type": "integer" + }, "post_approved": { "type": "integer" }, @@ -156,6 +159,7 @@ "watching_first_post", "topic_reminder", "liked_consolidated", + "linked_consolidated", "post_approved", "code_review_commit_approved", "membership_request_accepted", diff --git a/spec/services/notifications/consolidation_planner_spec.rb b/spec/services/notifications/consolidation_planner_spec.rb index 0ccf5674fdf..ae126eaaee1 100644 --- a/spec/services/notifications/consolidation_planner_spec.rb +++ b/spec/services/notifications/consolidation_planner_spec.rb @@ -7,10 +7,11 @@ RSpec.describe Notifications::ConsolidationPlanner do let(:threshold) { 1 } fab!(:user) let(:like_user) { "user1" } + let(:link_user) { "user2" } before { SiteSetting.notification_consolidation_threshold = threshold } - it "does nothing it haven't passed the consolidation threshold yet" do + it "does nothing when it hasn't passed the consolidation threshold yet for likes" do notification = build_notification(:liked, { display_username: like_user }) saved_like = planner.consolidate_or_save!(notification) @@ -19,7 +20,16 @@ RSpec.describe Notifications::ConsolidationPlanner do expect(saved_like.notification_type).to eq(Notification.types[:liked]) end - it "consolidates multiple notifications into a new one" do + it "does nothing when it hasn't passed the consolidation threshold yet for links" do + notification = build_notification(:linked, { display_username: link_user }) + + saved_link = planner.consolidate_or_save!(notification) + + expect(saved_link.id).to be_present + expect(saved_link.notification_type).to eq(Notification.types[:linked]) + end + + it "consolidates multiple like notifications into a new one" do first_notification = Fabricate( :notification, @@ -37,7 +47,25 @@ RSpec.describe Notifications::ConsolidationPlanner do expect(data["count"]).to eq(threshold + 1) end - it "updates the notification if we already consolidated it" do + it "consolidates multiple link notifications into a new one" do + first_notification = + Fabricate( + :notification, + user: user, + notification_type: Notification.types[:linked], + data: { display_username: link_user }.to_json, + ) + notification = build_notification(:linked, { display_username: link_user }) + + consolidated_link = planner.consolidate_or_save!(notification) + + expect(consolidated_link.id).not_to eq(first_notification.id) + expect(consolidated_link.notification_type).to eq(Notification.types[:linked_consolidated]) + data = JSON.parse(consolidated_link.data) + expect(data["count"]).to eq(threshold + 1) + end + + it "updates the like notification if we already consolidated it" do count = 5 Fabricate( :notification, @@ -53,6 +81,23 @@ RSpec.describe Notifications::ConsolidationPlanner do data = JSON.parse(updated.data) expect(data["count"]).to eq(count + 1) end + + it "updates the link notification if we already consolidated it" do + count = 5 + Fabricate( + :notification, + user: user, + notification_type: Notification.types[:linked_consolidated], + data: { count: count, display_username: link_user }.to_json, + ) + notification = build_notification(:linked, { display_username: link_user }) + + updated = planner.consolidate_or_save!(notification) + + expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + data = JSON.parse(updated.data) + expect(data["count"]).to eq(count + 1) + end end def build_notification(type_sym, data)