From cd412056ad33732ba795d80f23de984c9268017f Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 26 Mar 2018 15:01:55 -0600 Subject: [PATCH 1/2] Notification fixes - When a comment reply is not visible, it will not trigger a notification email. - When a comment is approved/accepted, we'll check to see if the comment was visible before, if not, we'll delegate to the new reply notification handler to possibly send a notification email. --- .../index.js | 54 ++++++++++++++++--- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/plugins/talk-plugin-notifications-category-reply/index.js b/plugins/talk-plugin-notifications-category-reply/index.js index 0b6fe62a0..e214975ac 100644 --- a/plugins/talk-plugin-notifications-category-reply/index.js +++ b/plugins/talk-plugin-notifications-category-reply/index.js @@ -2,6 +2,12 @@ const { get } = require('lodash'); const path = require('path'); const handle = async (ctx, comment) => { + // Check to see if this reply is visible. + if (!comment.visible) { + ctx.log.info('comment was not visible, not sending notification'); + return; + } + // Check to see if this is a reply to an existing comment. const parentID = get(comment, 'parent_id', null); if (parentID === null) { @@ -90,12 +96,31 @@ const hydrate = async (ctx, category, context) => { return [headline, replier, permalink]; }; -const handler = { - handle, - category: 'reply', - event: 'commentAdded', - hydrate, - digestOrder: 30, +// commentAcceptedHandleAdapter will check to see if we need to send a +// notification for this comment if the comment has been recently approved but +// has not been approved before. +const commentAcceptedHandleAdapter = (ctx, comment) => { + // Don't send a notification for a non-visible comment. + if (!comment.visible) { + ctx.log.info('comment was not visible, not sending notification'); + return; + } + + // Don't send a notification if the comment was previously visible. + if ( + // TODO: (wyattjoh) this check is quite brittle, replace with a more concrete check. + comment.status_history + .slice(0, comment.status_history.length - 1) + .some(({ type }) => ['ACCEPTED', 'NONE'].includes(type)) + ) { + ctx.log.info( + 'comment was previously already visible, not sending another notification' + ); + return; + } + + // Delegate to the handle function. + return handle(ctx, comment); }; module.exports = { @@ -115,5 +140,20 @@ module.exports = { }, }, translations: path.join(__dirname, 'translations.yml'), - notifications: [handler], + notifications: [ + { + handle, + category: 'reply', + event: 'commentAdded', + hydrate, + digestOrder: 30, + }, + { + handle: commentAcceptedHandleAdapter, + category: 'reply', + event: 'commentAccepted', + hydrate, + digestOrder: 30, + }, + ], }; From 34e3266a6fda79459bb35668f70cc96927c505f7 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 26 Mar 2018 16:06:40 -0600 Subject: [PATCH 2/2] expanded fix to staff replies --- .../index.js | 56 ++++++++++++++++--- .../server/messages.js | 2 - 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/plugins/talk-plugin-notifications-category-staff/index.js b/plugins/talk-plugin-notifications-category-staff/index.js index a5d12c175..bf7da0776 100644 --- a/plugins/talk-plugin-notifications-category-staff/index.js +++ b/plugins/talk-plugin-notifications-category-staff/index.js @@ -2,6 +2,11 @@ const { get } = require('lodash'); const path = require('path'); const handle = async (ctx, comment) => { + if (!comment.visible) { + ctx.log.info('comment was not visible, not sending notification'); + return; + } + // Check to see if this is a reply to an existing comment. const parentID = get(comment, 'parent_id', null); if (parentID === null) { @@ -111,13 +116,31 @@ const hydrate = async (ctx, category, context) => { return [headline, replier, organizationName, permalink]; }; -const handler = { - handle, - category: 'staff', - event: 'commentAdded', - hydrate, - supersedesCategories: ['reply'], - digestOrder: 20, +// commentAcceptedHandleAdapter will check to see if we need to send a +// notification for this comment if the comment has been recently approved but +// has not been approved before. +const commentAcceptedHandleAdapter = (ctx, comment) => { + // Don't send a notification for a non-visible comment. + if (!comment.visible) { + ctx.log.info('comment was not visible, not sending notification'); + return; + } + + // Don't send a notification if the comment was previously visible. + if ( + // TODO: (wyattjoh) this check is quite brittle, replace with a more concrete check. + comment.status_history + .slice(0, comment.status_history.length - 1) + .some(({ type }) => ['ACCEPTED', 'NONE'].includes(type)) + ) { + ctx.log.info( + 'comment was previously already visible, not sending another notification' + ); + return; + } + + // Delegate to the handle function. + return handle(ctx, comment); }; module.exports = { @@ -137,5 +160,22 @@ module.exports = { }, }, translations: path.join(__dirname, 'translations.yml'), - notifications: [handler], + notifications: [ + { + handle, + category: 'staff', + event: 'commentAdded', + hydrate, + supersedesCategories: ['reply'], + digestOrder: 20, + }, + { + handle: commentAcceptedHandleAdapter, + category: 'staff', + event: 'commentAccepted', + hydrate, + supersedesCategories: ['reply'], + digestOrder: 20, + }, + ], }; diff --git a/plugins/talk-plugin-notifications/server/messages.js b/plugins/talk-plugin-notifications/server/messages.js index 67c4b9d6b..0ba9ee70b 100644 --- a/plugins/talk-plugin-notifications/server/messages.js +++ b/plugins/talk-plugin-notifications/server/messages.js @@ -63,8 +63,6 @@ const sendNotificationsBatch = async (ctx, notifications) => { return; } - console.log(notifications); - return Promise.all( map( notifications,