From 31a4135fec6b511a1438cf04144d5a68ad1a448c Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Mon, 25 Jul 2022 17:35:46 +0200 Subject: [PATCH] Added members.last_commented_at and last_seen_at update when commenting (#15088) refs https://github.com/TryGhost/Team/issues/1717 - Updates last_commented_at and last_seen_at (only once a day) - Used the LastSeenAtUpdater, so we can combine updating last_commented_at and last_seen_at in one query + used same pattern - Updated comments service to await emails in order to make E2E tests more stable (as we don't have any method to await emails and test emails otherwise). This removed the email sending logic from the `onCreated` hook of the model. --- ghost/core/core/server/models/comment.js | 5 - .../core/server/services/comments/emails.js | 4 +- .../core/server/services/comments/service.js | 26 ++++- .../e2e-api/members-comments/comments.test.js | 50 +++++++-- ghost/member-events/index.js | 3 +- ghost/member-events/lib/MemberCommentEvent.js | 28 +++++ ghost/members-api/lib/repositories/member.js | 3 +- .../lib/last-seen-at-updater.js | 32 +++++- .../test/last-seen-at-updater.test.js | 105 +++++++++++++++++- 9 files changed, 235 insertions(+), 21 deletions(-) create mode 100644 ghost/member-events/lib/MemberCommentEvent.js diff --git a/ghost/core/core/server/models/comment.js b/ghost/core/core/server/models/comment.js index 0fe94040c5..5562fab0df 100644 --- a/ghost/core/core/server/models/comment.js +++ b/ghost/core/core/server/models/comment.js @@ -2,7 +2,6 @@ const ghostBookshelf = require('./base'); const _ = require('lodash'); const errors = require('@tryghost/errors'); const tpl = require('@tryghost/tpl'); -const commentsService = require('../services/comments'); const messages = { commentNotFound: 'Comment could not be found', @@ -70,10 +69,6 @@ const Comment = ghostBookshelf.Model.extend({ onCreated: function onCreated(model, options) { ghostBookshelf.Model.prototype.onCreated.apply(this, arguments); - if (!options.context.internal) { - commentsService.api.sendNewCommentNotifications(model); - } - model.emitChange('added', options); }, diff --git a/ghost/core/core/server/services/comments/emails.js b/ghost/core/core/server/services/comments/emails.js index 7d13eeb927..4e6a236e8f 100644 --- a/ghost/core/core/server/services/comments/emails.js +++ b/ghost/core/core/server/services/comments/emails.js @@ -49,7 +49,7 @@ class CommentsServiceEmails { const {html, text} = await this.renderEmailTemplate('new-comment', templateData); - this.sendMail({ + await this.sendMail({ to, subject, html, @@ -148,7 +148,7 @@ class CommentsServiceEmails { const {html, text} = await this.renderEmailTemplate('report', templateData); - this.sendMail({ + await this.sendMail({ to, subject, html, diff --git a/ghost/core/core/server/services/comments/service.js b/ghost/core/core/server/services/comments/service.js index 097dfdb10a..0cb07accd4 100644 --- a/ghost/core/core/server/services/comments/service.js +++ b/ghost/core/core/server/services/comments/service.js @@ -1,5 +1,7 @@ const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); +const {MemberCommentEvent} = require('@tryghost/member-events'); +const DomainEvents = require('@tryghost/domain-events'); const messages = { commentNotFound: 'Comment could not be found', @@ -24,10 +26,10 @@ class CommentsService { } async sendNewCommentNotifications(comment) { - this.emails.notifyPostAuthors(comment); + await this.emails.notifyPostAuthors(comment); if (comment.get('parent_id')) { - this.emails.notifyParentCommentAuthor(comment); + await this.emails.notifyParentCommentAuthor(comment); } } @@ -92,6 +94,16 @@ class CommentsService { status: 'published' }, options); + if (!options.context.internal) { + await this.sendNewCommentNotifications(model); + } + + DomainEvents.dispatch(MemberCommentEvent.create({ + memberId: member, + postId: post, + commentId: model.id + })); + return model; } @@ -130,6 +142,16 @@ class CommentsService { status: 'published' }, options); + if (!options.context.internal) { + await this.sendNewCommentNotifications(model); + } + + DomainEvents.dispatch(MemberCommentEvent.create({ + memberId: member, + postId: parentComment.get('post_id'), + commentId: model.id + })); + return model; } diff --git a/ghost/core/test/e2e-api/members-comments/comments.test.js b/ghost/core/test/e2e-api/members-comments/comments.test.js index e81734a926..cb42a70628 100644 --- a/ghost/core/test/e2e-api/members-comments/comments.test.js +++ b/ghost/core/test/e2e-api/members-comments/comments.test.js @@ -3,6 +3,8 @@ const {agentProvider, mockManager, fixtureManager, matchers} = require('../../ut const {anyEtag, anyObjectId, anyLocationFor, anyISODateTime, anyUuid, anyNumber, anyBoolean} = matchers; const should = require('should'); const models = require('../../../core/server/models'); +const moment = require('moment-timezone'); +const settingsCache = require('../../../core/shared/settings-cache'); let membersAgent, membersAgent2, member, postId, commentId; @@ -101,6 +103,8 @@ describe('Comments API', function () { }); it('Can comment on a post', async function () { + await models.Member.edit({last_seen_at: null, last_commented_at: null}, {id: member.get('id')}); + const {body} = await membersAgent .post(`/api/comments/`) .body({comments: [{ @@ -118,9 +122,6 @@ describe('Comments API', function () { // Save for other tests commentId = body.comments[0].id; - // Wait for the emails (because this happens async) - await sleep(100); - // Check if author got an email mockManager.assert.sentEmailCount(1); mockManager.assert.sentEmail({ @@ -129,6 +130,16 @@ describe('Comments API', function () { // Note that the tag is removed by the sanitizer html: new RegExp(escapeRegExp('

This is a message

New line

')) }); + + // Wait for the dispatched events (because this happens async) + await sleep(200); + + // Check last_updated_at changed? + member = await models.Member.findOne({id: member.id}); + should.notEqual(member.get('last_seen_at'), null, 'The member should have a `last_seen_at` property after posting a comment.'); + + // Check last_commented_at changed? + should.notEqual(member.get('last_commented_at'), null, 'The member should have a `last_commented_at` property after posting a comment.'); }); it('Can browse all comments of a post', async function () { @@ -144,6 +155,11 @@ describe('Comments API', function () { }); it('Can reply to your own comment', async function () { + // Should not update last_seen_at or last_commented_at when both are already set to a value on the same day + const timezone = settingsCache.get('timezone'); + const date = moment.utc(new Date()).tz(timezone).startOf('day').toDate(); + await models.Member.edit({last_seen_at: date, last_commented_at: date}, {id: member.get('id')}); + const {body} = await membersAgent .post(`/api/comments/`) .body({comments: [{ @@ -160,18 +176,28 @@ describe('Comments API', function () { comments: [commentMatcherNoMember] }); - // Wait for the emails (because this happens async) - await sleep(100); - // Check only the author got an email (because we are the author of this parent comment) mockManager.assert.sentEmailCount(1); mockManager.assert.sentEmail({ subject: '💬 You have a new comment on one of your posts', to: fixtureManager.get('users', 0).email }); + + // Wait for the dispatched events (because this happens async) + await sleep(200); + + // Check last updated_at is not changed? + member = await models.Member.findOne({id: member.id}); + should.equal(member.get('last_seen_at').getTime(), date.getTime(), 'The member should not update `last_seen_at` if last seen at is same day'); + + // Check last_commented_at changed? + should.equal(member.get('last_commented_at').getTime(), date.getTime(), 'The member should not update `last_commented_at` f last seen at is same day'); }); it('Can reply to a comment', async function () { + const date = new Date(0); + await models.Member.edit({last_seen_at: date, last_commented_at: date}, {id: member.get('id')}); + const {body} = await membersAgent .post(`/api/comments/`) .body({comments: [{ @@ -188,8 +214,6 @@ describe('Comments API', function () { comments: [commentMatcherNoMember] }); - // Wait for the emails (because this happens async) - await sleep(100); mockManager.assert.sentEmailCount(2); mockManager.assert.sentEmail({ subject: '💬 You have a new comment on one of your posts', @@ -200,6 +224,16 @@ describe('Comments API', function () { subject: '💬 You have a new reply on one of your comments', to: fixtureManager.get('members', 0).email }); + + // Wait for the dispatched events (because this happens async) + await sleep(250); + + // Check last_updated_at changed? + member = await models.Member.findOne({id: member.id}); + should.notEqual(member.get('last_seen_at').getTime(), date.getTime(), 'Should update `last_seen_at` property after posting a comment.'); + + // Check last_commented_at changed? + should.notEqual(member.get('last_commented_at').getTime(), date.getTime(), 'Should update `last_commented_at` property after posting a comment.'); }); it('Can like a comment', async function () { diff --git a/ghost/member-events/index.js b/ghost/member-events/index.js index c43a7533a7..904828b217 100644 --- a/ghost/member-events/index.js +++ b/ghost/member-events/index.js @@ -6,5 +6,6 @@ module.exports = { MemberPaidConverstionEvent: require('./lib/MemberPaidConversionEvent'), MemberPaidCancellationEvent: require('./lib/MemberPaidCancellationEvent'), MemberPageViewEvent: require('./lib/MemberPageViewEvent'), - SubscriptionCreatedEvent: require('./lib/SubscriptionCreatedEvent') + SubscriptionCreatedEvent: require('./lib/SubscriptionCreatedEvent'), + MemberCommentEvent: require('./lib/MemberCommentEvent') }; diff --git a/ghost/member-events/lib/MemberCommentEvent.js b/ghost/member-events/lib/MemberCommentEvent.js new file mode 100644 index 0000000000..08798491b8 --- /dev/null +++ b/ghost/member-events/lib/MemberCommentEvent.js @@ -0,0 +1,28 @@ +/** + * @typedef {object} MemberCommentEventData + * @prop {string} memberId + * @prop {string} commentId + * @prop {string} postId + */ + +/** + * Server-side event firing on page views (page, post, tags...) + */ +module.exports = class MemberCommentEvent { + /** + * @param {MemberCommentEventData} data + * @param {Date} timestamp + */ + constructor(data, timestamp) { + this.data = data; + this.timestamp = timestamp; + } + + /** + * @param {MemberCommentEventData} data + * @param {Date} [timestamp] + */ + static create(data, timestamp) { + return new MemberCommentEvent(data, timestamp || new Date); + } +}; diff --git a/ghost/members-api/lib/repositories/member.js b/ghost/members-api/lib/repositories/member.js index 3015f8d516..0cb91402a1 100644 --- a/ghost/members-api/lib/repositories/member.js +++ b/ghost/members-api/lib/repositories/member.js @@ -318,7 +318,8 @@ module.exports = class MemberRepository { 'products', 'newsletters', 'enable_comment_notifications', - 'last_seen_at' + 'last_seen_at', + 'last_commented_at' ]); // Determine if we need to fetch the initial member with relations diff --git a/ghost/members-events-service/lib/last-seen-at-updater.js b/ghost/members-events-service/lib/last-seen-at-updater.js index 70cc129ecc..db17a871f4 100644 --- a/ghost/members-events-service/lib/last-seen-at-updater.js +++ b/ghost/members-events-service/lib/last-seen-at-updater.js @@ -1,4 +1,4 @@ -const {MemberPageViewEvent} = require('@tryghost/member-events'); +const {MemberPageViewEvent, MemberCommentEvent} = require('@tryghost/member-events'); const moment = require('moment-timezone'); const {IncorrectUsageError} = require('@tryghost/errors'); @@ -32,6 +32,10 @@ class LastSeenAtUpdater { this._domainEventsService.subscribe(MemberPageViewEvent, async (event) => { await this.updateLastSeenAt(event.data.memberId, event.data.memberLastSeenAt, event.timestamp); }); + + this._domainEventsService.subscribe(MemberCommentEvent, async (event) => { + await this.updateLastCommentedAt(event.data.memberId, event.timestamp); + }); } /** @@ -54,6 +58,32 @@ class LastSeenAtUpdater { }); } } + + /** + * Updates the member.last_seen_at field if it wasn't updated in the current day yet (in the publication timezone) + * Example: current time is 2022-02-28 18:00:00 + * - memberLastSeenAt is 2022-02-27 23:00:00, timestamp is current time, then `last_seen_at` is set to the current time + * - memberLastSeenAt is 2022-02-28 01:00:00, timestamp is current time, then `last_seen_at` isn't changed + * @param {string} memberId The id of the member to be udpated + * @param {Date} timestamp The event timestamp + */ + async updateLastCommentedAt(memberId, timestamp) { + const membersApi = await this._getMembersApi(); + const member = await membersApi.members.get({id: memberId}, {require: true}); + const timezone = this._settingsCacheService.get('timezone'); + + const memberLastSeenAt = member.get('last_seen_at'); + const memberLastCommentedAt = member.get('last_commented_at'); + + if (memberLastSeenAt === null || moment(moment.utc(timestamp).tz(timezone).startOf('day')).isAfter(memberLastSeenAt) || memberLastCommentedAt === null || moment(moment.utc(timestamp).tz(timezone).startOf('day')).isAfter(memberLastCommentedAt)) { + await membersApi.members.update({ + last_seen_at: moment.utc(timestamp).format('YYYY-MM-DD HH:mm:ss'), + last_commented_at: moment.utc(timestamp).format('YYYY-MM-DD HH:mm:ss') + }, { + id: memberId + }); + } + } } module.exports = LastSeenAtUpdater; diff --git a/ghost/members-events-service/test/last-seen-at-updater.test.js b/ghost/members-events-service/test/last-seen-at-updater.test.js index 88a8416bc8..b3ee4a5fc3 100644 --- a/ghost/members-events-service/test/last-seen-at-updater.test.js +++ b/ghost/members-events-service/test/last-seen-at-updater.test.js @@ -6,7 +6,7 @@ const assert = require('assert'); const sinon = require('sinon'); const {LastSeenAtUpdater} = require('../'); const DomainEvents = require('@tryghost/domain-events'); -const {MemberPageViewEvent} = require('@tryghost/member-events'); +const {MemberPageViewEvent, MemberCommentEvent} = require('@tryghost/member-events'); const moment = require('moment'); describe('LastSeenAtUpdater', function () { @@ -35,6 +35,30 @@ describe('LastSeenAtUpdater', function () { assert(updater.updateLastSeenAt.calledOnceWithExactly('1', previousLastSeen, now.toDate())); }); + it('Calls updateLastCommentedAt on MemberCommentEvents', async function () { + const now = moment('2022-02-28T18:00:00Z').utc(); + const stub = sinon.stub().resolves(); + const settingsCache = sinon.stub().returns('Etc/UTC'); + const updater = new LastSeenAtUpdater({ + services: { + settingsCache: { + get: settingsCache + }, + domainEvents: DomainEvents + }, + async getMembersApi() { + return { + members: { + update: stub + } + }; + } + }); + sinon.stub(updater, 'updateLastCommentedAt'); + DomainEvents.dispatch(MemberCommentEvent.create({memberId: '1'}, now.toDate())); + assert(updater.updateLastCommentedAt.calledOnceWithExactly('1', now.toDate())); + }); + it('works correctly on another timezone (not updating last_seen_at)', async function () { const now = moment('2022-02-28T04:00:00Z').utc(); const previousLastSeen = moment('2022-02-27T20:00:00Z').toISOString(); @@ -59,6 +83,38 @@ describe('LastSeenAtUpdater', function () { assert(stub.notCalled, 'The LastSeenAtUpdater should attempt a member update when the new timestamp is within the same day in the publication timezone.'); }); + it('works correctly on another timezone (not updating last_commented_at)', async function () { + const now = moment('2022-02-28T04:00:00Z').utc(); + const previousLastSeen = moment('2022-02-27T20:00:00Z').toISOString(); + const stub = sinon.stub().resolves(); + const settingsCache = sinon.stub().returns('Asia/Bangkok'); + const updater = new LastSeenAtUpdater({ + services: { + settingsCache: { + get: settingsCache + }, + domainEvents: DomainEvents + }, + async getMembersApi() { + return { + members: { + update: stub, + get: () => { + return { + id: '1', + get: () => { + return previousLastSeen; + } + }; + } + } + }; + } + }); + await updater.updateLastCommentedAt('1', now.toDate()); + assert(stub.notCalled, 'The LastSeenAtUpdater should attempt a member update when the new timestamp is within the same day in the publication timezone.'); + }); + it('works correctly on another timezone (updating last_seen_at)', async function () { const now = moment('2022-02-28T04:00:00Z').utc(); const previousLastSeen = moment('2022-02-27T20:00:00Z').toISOString(); @@ -111,6 +167,38 @@ describe('LastSeenAtUpdater', function () { assert(stub.notCalled, 'The LastSeenAtUpdater should\'t update a member when the previous last_seen_at is close to the event timestamp.'); }); + it('Doesn\'t update when last_commented_at is too recent', async function () { + const now = moment('2022-02-28T18:00:00Z'); + const previousLastSeen = moment('2022-02-28T00:00:00Z').toISOString(); + const stub = sinon.stub().resolves(); + const settingsCache = sinon.stub().returns('Etc/UTC'); + const updater = new LastSeenAtUpdater({ + services: { + settingsCache: { + get: settingsCache + }, + domainEvents: DomainEvents + }, + async getMembersApi() { + return { + members: { + update: stub, + get: () => { + return { + id: '1', + get: () => { + return previousLastSeen; + } + }; + } + } + }; + } + }); + await updater.updateLastCommentedAt('1', now.toDate()); + assert(stub.notCalled, 'The LastSeenAtUpdater should\'t update a member when the previous last_seen_at is close to the event timestamp.'); + }); + it('Doesn\'t fire on other events', async function () { const now = moment('2022-02-28T18:00:00Z'); const stub = sinon.stub().resolves(); @@ -133,4 +221,19 @@ describe('LastSeenAtUpdater', function () { await updater.updateLastSeenAt('1', undefined, now.toDate()); assert(stub.notCalled, 'The LastSeenAtUpdater should never fire on MemberPageViewEvent events.'); }); + + it('throws if getMembersApi is not passed to LastSeenAtUpdater', async function () { + const settingsCache = sinon.stub().returns('Asia/Bangkok'); + + should.throws(() => { + new LastSeenAtUpdater({ + services: { + settingsCache: { + get: settingsCache + }, + domainEvents: DomainEvents + } + }); + }, 'Missing option getMembersApi'); + }); });