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'); + }); });