From 481c8314f0b79253578c0f7facbe91f792301411 Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Thu, 28 Nov 2019 04:02:35 +0530 Subject: [PATCH] FEATURE: consolidate group membership request notifications. --- .../discourse-common/lib/icon-library.js.es6 | 3 +- ...uest-consolidated-notification-item.js.es6 | 25 ++++++ app/models/notification.rb | 78 ++++++++++++++++++- config/locales/client.en.yml | 2 + spec/models/notification_spec.rb | 44 +++++++++++ 5 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 app/assets/javascripts/discourse/widgets/membership-request-consolidated-notification-item.js.es6 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 726b49cfdea..f0188fb2c5e 100644 --- a/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 +++ b/app/assets/javascripts/discourse-common/lib/icon-library.js.es6 @@ -36,7 +36,8 @@ const REPLACEMENTS = { "notification.watching_first_post": "far-dot-circle", "notification.group_message_summary": "users", "notification.post_approved": "check", - "notification.membership_request_accepted": "user-plus" + "notification.membership_request_accepted": "user-plus", + "notification.membership_request_consolidated": "users" }; // TODO: use lib/svg_sprite/fa4-renames.json here diff --git a/app/assets/javascripts/discourse/widgets/membership-request-consolidated-notification-item.js.es6 b/app/assets/javascripts/discourse/widgets/membership-request-consolidated-notification-item.js.es6 new file mode 100644 index 00000000000..ea5f54d1984 --- /dev/null +++ b/app/assets/javascripts/discourse/widgets/membership-request-consolidated-notification-item.js.es6 @@ -0,0 +1,25 @@ +import { createWidgetFrom } from "discourse/widgets/widget"; +import { DefaultNotificationItem } from "discourse/widgets/default-notification-item"; +import { userPath } from "discourse/lib/url"; + +createWidgetFrom( + DefaultNotificationItem, + "membership-request-consolidated-notification-item", + { + url() { + return userPath( + `${this.attrs.username || this.currentUser.username}/messages` + ); + }, + + text(notificationName, data) { + return I18n.t( + "notifications.membership_request_consolidated", + { + group_name: data.group_name, + count: parseInt(data.count, 10) + } + ); + } + } +); diff --git a/app/models/notification.rb b/app/models/notification.rb index fa6bb664909..a5c1e05389b 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -4,6 +4,9 @@ class Notification < ActiveRecord::Base belongs_to :user belongs_to :topic + MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS = 24 + MEMBERSHIP_REQUEST_CONSOLIDATION_THRESHOLD = 3 + validates_presence_of :data validates_presence_of :notification_type @@ -20,11 +23,15 @@ class Notification < ActiveRecord::Base attr_accessor :skip_send_email - after_commit :send_email, on: :create after_commit :refresh_notification_count, on: [:create, :update, :destroy] after_commit(on: :create) do - DiscourseEvent.trigger(:notification_created, self) + consolidated = consolidate_membership_requests + + unless consolidated + DiscourseEvent.trigger(:notification_created, self) + send_email + end end def self.ensure_consistency! @@ -66,7 +73,8 @@ class Notification < ActiveRecord::Base liked_consolidated: 19, post_approved: 20, code_review_commit_approved: 21, - membership_request_accepted: 22 + membership_request_accepted: 22, + membership_request_consolidated: 23 ) end @@ -226,6 +234,70 @@ class Notification < ActiveRecord::Base NotificationEmailer.process_notification(self) if !skip_send_email end + private + + def consolidate_membership_requests + return unless unread_pm? + + post_id = data_hash[:original_post_id] + return if post_id.blank? + + custom_field = PostCustomField.select(:value).find_by(post_id: post_id, name: "requested_group_id") + return if custom_field.blank? + + group_id = custom_field.value.to_i + group_name = Group.select(:name).find_by(id: group_id)&.name + return if group_name.blank? + + consolidation_window = MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours.ago + timestamp = Time.zone.now + unread = user.notifications.unread + + consolidated_notification = unread + .where("created_at > ? AND data::json ->> 'group_name' = ?", consolidation_window, group_name) + .find_by(notification_type: Notification.types[:membership_request_consolidated]) + + if consolidated_notification.present? + data = consolidated_notification.data_hash + data["count"] += 1 + + Notification.transaction do + consolidated_notification.update!( + data: data.to_json, + read: false, + updated_at: timestamp + ) + + destroy! + end + + return true + end + + notifications = unread + .where(notification_type: Notification.types[:private_message]) + .where("created_at > ? AND data::json ->> 'topic_title' = ?", consolidation_window, data_hash[:topic_title]) + + return if notifications.count < MEMBERSHIP_REQUEST_CONSOLIDATION_THRESHOLD + + Notification.transaction do + Notification.create!( + notification_type: Notification.types[:membership_request_consolidated], + user_id: user_id, + data: { + group_name: group_name, + count: notifications.count + }.to_json, + updated_at: timestamp, + created_at: timestamp + ) + + notifications.destroy_all + end + + true + end + end # == Schema Information diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 36fa5c10730..87879ab05cd 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1782,6 +1782,7 @@ en: topic_reminder: "{{username}} {{description}}" watching_first_post: "New Topic {{description}}" membership_request_accepted: "Membership accepted in '{{group_name}}'" + membership_request_consolidated: "{{count}} open membership requests for '{{group_name}}'" group_message_summary: one: "{{count}} message in your {{group_name}} inbox" @@ -1820,6 +1821,7 @@ en: topic_reminder: "topic reminder" liked_consolidated: "new likes" post_approved: "post approved" + membership_request_consolidated: "new membership requests" upload_selector: title: "Add an image" diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index eeb26d3f003..a78d6e8ed6c 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -355,5 +355,49 @@ describe Notification do expect(Notification.recent_report(user)).to contain_exactly(notification) end end + + describe '#consolidate_membership_requests' do + fab!(:group) { Fabricate(:group, name: "XXsssssddd") } + fab!(:user) { Fabricate(:user) } + fab!(:post) { Fabricate(:post) } + + def create_membership_request_notification + Notification.create( + notification_type: Notification.types[:private_message], + user_id: user.id, + data: { + topic_title: I18n.t('groups.request_membership_pm.title', group_name: group.name), + original_post_id: post.id + }.to_json, + updated_at: Time.zone.now, + created_at: Time.zone.now + ) + end + + before do + PostCustomField.create!(post_id: post.id, name: "requested_group_id", value: group.id) + create_membership_request_notification + end + + it 'should consolidate membership requests to a new notification' do + notification = create_membership_request_notification + notification.reload + + notification = create_membership_request_notification + expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + + notification = Notification.last + expect(notification.notification_type).to eq(Notification.types[:membership_request_consolidated]) + + data = notification.data_hash + expect(data[:group_name]).to eq(group.name) + expect(data[:count]).to eq(3) + + notification = create_membership_request_notification + expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + + expect(Notification.last.data_hash[:count]).to eq(4) + end + end end end