Skip to content

Commit

Permalink
Switched to using refresh and fixed tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@cmraible
cmraible committed Jun 18, 2024
1 parent 049f0d8 commit a13e0bb
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 50 deletions.
38 changes: 22 additions & 16 deletions ghost/members-events-service/lib/LastSeenAtUpdater.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class LastSeenAtUpdater {
subscribe(domainEvents) {
domainEvents.subscribe(MemberPageViewEvent, async (event) => {
try {
await this.updateLastSeenAt(event.data.memberId, event.timestamp);
await this.updateLastSeenAt(event.data.memberId, event.data.memberLastSeenAt, event.timestamp);
} catch (err) {
logging.error(`Error in LastSeenAtUpdater.MemberPageViewEvent listener for member ${event.data.memberId}`);
logging.error(err);
Expand All @@ -50,7 +50,7 @@ class LastSeenAtUpdater {

domainEvents.subscribe(MemberLinkClickEvent, async (event) => {
try {
await this.updateLastSeenAt(event.data.memberId, event.timestamp);
await this.updateLastSeenAt(event.data.memberId, event.data.memberLastSeenAt, event.timestamp);
} catch (err) {
logging.error(`Error in LastSeenAtUpdater.MemberLinkClickEvent listener for member ${event.data.memberId}`);
logging.error(err);
Expand Down Expand Up @@ -109,21 +109,27 @@ class LastSeenAtUpdater {
* @param {string} memberId The id of the member to be udpated
* @param {Date} timestamp The event timestamp
*/
async updateLastSeenAt(memberId, timestamp) {
async updateLastSeenAt(memberId, memberLastSeenAt, timestamp) {
const timezone = this._settingsCacheService.get('timezone');
const membersApi = this._getMembersApi();
await this._db.knex.transaction(async (trx) => {
const currentMember = await membersApi.members.get({id: memberId}, {require: true, transacting: trx, forUpdate: true});
const memberLastSeenAt = currentMember.get('last_seen_at');
if (memberLastSeenAt === null || moment(moment.utc(timestamp).tz(timezone).startOf('day')).isAfter(memberLastSeenAt)) {
const memberToUpdate = await membersApi.members.get({id: memberId}, {require: true, transacting: trx, forUpdate: false, withRelated: ['labels', 'newsletters']});
const updatedMember = await memberToUpdate.save({last_seen_at: moment.utc(timestamp).format('YYYY-MM-DD HH:mm:ss')}, {transacting: trx, patch: true, method: 'update', autoRefresh: true});
// The standard event doesn't get emitted inside the transaction, so we do it manually
this._events.emit('member.edited', updatedMember);
return Promise.resolve(updatedMember);
}
return Promise.resolve(undefined);
});
// First, check if memberLastSeenAt is null or before the beginning of the current day in the publication timezone
// This isn't strictly necessary since we will fetch the member row for update and double check this
// This is an optimization to avoid unnecessary database queries if last_seen_at is already after the beginning of the current day
if (memberLastSeenAt === null || moment(moment.utc(timestamp).tz(timezone).startOf('day')).isAfter(memberLastSeenAt)) {
const membersApi = this._getMembersApi();
await this._db.knex.transaction(async (trx) => {
// To avoid a race condition, we lock the member row for update, then the last_seen_at field again to prevent simultaneous updates
const currentMember = await membersApi.members.get({id: memberId}, {require: true, transacting: trx, forUpdate: true});
const currentMemberLastSeenAt = currentMember.get('last_seen_at');
if (currentMemberLastSeenAt === null || moment(moment.utc(timestamp).tz(timezone).startOf('day')).isAfter(currentMemberLastSeenAt)) {
const memberToUpdate = await currentMember.refresh({transacting: trx, forUpdate: false, withRelated: ['labels', 'newsletters']});
const updatedMember = await memberToUpdate.save({last_seen_at: moment.utc(timestamp).format('YYYY-MM-DD HH:mm:ss')}, {transacting: trx, patch: true, method: 'update'});
// The standard event doesn't get emitted inside the transaction, so we do it manually
this._events.emit('member.edited', updatedMember);
return Promise.resolve(updatedMember);
}
return Promise.resolve(undefined);
});
}
}

/**
Expand Down
65 changes: 31 additions & 34 deletions ghost/members-events-service/test/last-seen-at-updater.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('LastSeenAtUpdater', function () {
updater.subscribe(DomainEvents);
sinon.stub(updater, 'updateLastSeenAt');
DomainEvents.dispatch(MemberPageViewEvent.create({memberId: '1', memberLastSeenAt: previousLastSeen, url: '/'}, now.toDate()));
assert(updater.updateLastSeenAt.calledOnceWithExactly('1', now.toDate()));
assert(updater.updateLastSeenAt.calledOnceWithExactly('1', previousLastSeen, now.toDate()));
});

it('Calls updateLastSeenAt on MemberLinkClickEvents', async function () {
Expand All @@ -68,7 +68,7 @@ describe('LastSeenAtUpdater', function () {
updater.subscribe(DomainEvents);
sinon.stub(updater, 'updateLastSeenAt');
DomainEvents.dispatch(MemberLinkClickEvent.create({memberId: '1', memberLastSeenAt: previousLastSeen, url: '/'}, now.toDate()));
assert(updater.updateLastSeenAt.calledOnceWithExactly('1', now.toDate()));
assert(updater.updateLastSeenAt.calledOnceWithExactly('1', previousLastSeen, now.toDate()));
});

it('Calls updateLastSeenAt on email opened events', async function () {
Expand Down Expand Up @@ -138,8 +138,8 @@ describe('LastSeenAtUpdater', function () {
const transactionStub = sinon.stub().callsFake((callback) => {
return callback();
});
const updateStub = sinon.stub().resolves();
const getStub = sinon.stub().resolves({get: () => previousLastSeen});
const saveStub = sinon.stub().resolves();
const getStub = sinon.stub().resolves({get: () => previousLastSeen, save: saveStub});
const settingsCache = sinon.stub().returns('Asia/Bangkok');
const updater = new LastSeenAtUpdater({
services: {
Expand All @@ -150,8 +150,7 @@ describe('LastSeenAtUpdater', function () {
getMembersApi() {
return {
members: {
get: getStub,
update: updateStub
get: getStub
}
};
},
Expand All @@ -162,8 +161,8 @@ describe('LastSeenAtUpdater', function () {
},
events
});
await updater.updateLastSeenAt('1', now.toDate());
assert(updateStub.notCalled, 'The LastSeenAtUpdater should attempt a member update when the new timestamp is within the same day in the publication timezone.');
await updater.updateLastSeenAt('1', previousLastSeen, now.toDate());
assert(saveStub.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 () {
Expand Down Expand Up @@ -204,8 +203,9 @@ describe('LastSeenAtUpdater', function () {
const transactionStub = sinon.stub().callsFake((callback) => {
return callback();
});
const updateStub = sinon.stub().resolves();
const getStub = sinon.stub().resolves({get: () => previousLastSeen});
const saveStub = sinon.stub().resolves();
const refreshStub = sinon.stub().resolves({save: saveStub});
const getStub = sinon.stub().resolves({get: () => previousLastSeen, refresh: refreshStub});
const settingsCache = sinon.stub().returns('Europe/Paris');
const updater = new LastSeenAtUpdater({
services: {
Expand All @@ -216,8 +216,7 @@ describe('LastSeenAtUpdater', function () {
getMembersApi() {
return {
members: {
get: getStub,
update: updateStub
get: getStub
}
};
},
Expand All @@ -228,18 +227,18 @@ describe('LastSeenAtUpdater', function () {
},
events
});
await updater.updateLastSeenAt('1', now.toDate());
assert(updateStub.calledOnceWithExactly(
await updater.updateLastSeenAt('1', previousLastSeen, now.toDate());
assert(saveStub.calledOnceWithExactly(
sinon.match({last_seen_at: now.tz('utc').format('YYYY-MM-DD HH:mm:ss')}),
sinon.match({id: '1', transacting: sinon.match.any})
sinon.match({transacting: sinon.match.any, patch: true, method: 'update'})
), 'The LastSeenAtUpdater should attempt a member update with the current date.');
});

it('Doesn\'t update when last_seen_at is too recent', async function () {
const now = moment('2022-02-28T18:00:00Z');
const previousLastSeen = moment('2022-02-28T00:00:00Z').toISOString();
const updateStub = sinon.stub().resolves();
const getStub = sinon.stub().resolves({get: () => previousLastSeen});
const saveStub = sinon.stub().resolves();
const getStub = sinon.stub().resolves({get: () => previousLastSeen, save: saveStub});
const transactionStub = sinon.stub().callsFake((callback) => {
return callback();
});
Expand All @@ -253,8 +252,7 @@ describe('LastSeenAtUpdater', function () {
getMembersApi() {
return {
members: {
get: getStub,
update: updateStub
get: getStub
}
};
},
Expand All @@ -265,8 +263,8 @@ describe('LastSeenAtUpdater', function () {
},
events
});
await updater.updateLastSeenAt('1', now.toDate());
assert(updateStub.notCalled, 'The LastSeenAtUpdater should\'t update a member when the previous last_seen_at is close to the event timestamp.');
await updater.updateLastSeenAt('1', previousLastSeen, now.toDate());
assert(saveStub.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 () {
Expand Down Expand Up @@ -450,15 +448,16 @@ describe('LastSeenAtUpdater', function () {

it('avoids a race condition when updating last_seen_at', async function () {
const now = moment.utc('2022-02-28T18:00:00Z');
const updateStub = sinon.stub().resolves();
const saveStub = sinon.stub().resolves();
const refreshStub = sinon.stub().resolves({save: saveStub});
const settingsCache = sinon.stub().returns('Europe/Brussels');
const transactionStub = sinon.stub().callsFake((callback) => {
return callback();
});
const getStub = sinon.stub();
getStub.onFirstCall().resolves({get: () => null});
getStub.onSecondCall().resolves({get: () => now.toDate()});
getStub.resolves({get: () => now.toDate()});
getStub.onFirstCall().resolves({get: () => null, save: saveStub, refresh: refreshStub});
getStub.onSecondCall().resolves({get: () => now.toDate(), save: saveStub, refresh: refreshStub});
getStub.resolves({get: () => now.toDate(), save: saveStub, refresh: refreshStub});
const updater = new LastSeenAtUpdater({
services: {
settingsCache: {
Expand All @@ -468,7 +467,6 @@ describe('LastSeenAtUpdater', function () {
getMembersApi() {
return {
members: {
update: updateStub,
get: getStub
}
};
Expand All @@ -482,16 +480,15 @@ describe('LastSeenAtUpdater', function () {
});
sinon.stub(events, 'emit');
await Promise.all([
updater.updateLastSeenAt('1', now.toDate()),
updater.updateLastSeenAt('1', now.toDate()),
updater.updateLastSeenAt('1', now.toDate()),
updater.updateLastSeenAt('1', now.toDate())
updater.updateLastSeenAt('1', null, now.toDate()),
updater.updateLastSeenAt('1', null, now.toDate()),
updater.updateLastSeenAt('1', null, now.toDate()),
updater.updateLastSeenAt('1', null, now.toDate())
]);
assert(updateStub.calledOnce, `The LastSeenAtUpdater should attempt a member update only once, but was called ${updateStub.callCount} times`);

assert(updateStub.calledOnceWithExactly(
assert(saveStub.calledOnce, `The LastSeenAtUpdater should attempt a member update only once, but was called ${saveStub.callCount} times`);
assert(saveStub.calledOnceWithExactly(
sinon.match({last_seen_at: now.tz('utc').format('YYYY-MM-DD HH:mm:ss')}),
sinon.match({id: '1', transacting: sinon.match.any})
sinon.match({transacting: undefined, patch: true, method: 'update'})
), 'The LastSeenAtUpdater should attempt a member update with the current date.');

assert(events.emit.calledOnceWithExactly(
Expand Down

0 comments on commit a13e0bb

Please sign in to comment.