From 8662252ae1f62c14bf573ca7d5b9aced4936b3ec Mon Sep 17 00:00:00 2001 From: Matt Hanley <3798302+matthanley@users.noreply.github.com> Date: Mon, 25 Apr 2022 17:18:16 +0100 Subject: [PATCH] Updated default newsletter model sort order (#14571) refs https://github.com/TryGhost/Team/issues/1552 - The API doesn't enforce a unique sort order but we infer the "default newsletter" based on ordering so we need to ensure a consistent and deterministic ordering behaviour --- core/server/models/newsletter.js | 23 ++++++++++++++++++- core/server/services/newsletters/service.js | 4 ++++ .../__snapshots__/newsletters.test.js.snap | 4 ++-- .../services/newsletters/service.test.js | 21 +++++++++++++---- 4 files changed, 45 insertions(+), 7 deletions(-) diff --git a/core/server/models/newsletter.js b/core/server/models/newsletter.js index 74f3bb4094..7ccad05d02 100644 --- a/core/server/models/newsletter.js +++ b/core/server/models/newsletter.js @@ -41,8 +41,29 @@ const Newsletter = ghostBookshelf.Model.extend({ }, { orderDefaultOptions: function orderDefaultOptions() { return { - sort_order: 'ASC' + sort_order: 'ASC', + created_at: 'ASC', + id: 'ASC' }; + }, + + getNextAvailableSortOrder: async function getNextAvailableSortOrder(unfilteredOptions = {}) { + const options = {}; + if (unfilteredOptions.transacting) { + options.transacting = unfilteredOptions.transacting; + } + + const lastNewsletter = await this.findPage({ + filter: 'status:active', + order: 'sort_order DESC', // there's no NQL syntax available here + limit: 1, + columns: ['sort_order'] + }, options); + + if (lastNewsletter.data.length > 0) { + return lastNewsletter.data[0].get('sort_order') + 1; + } + return 0; } }); diff --git a/core/server/services/newsletters/service.js b/core/server/services/newsletters/service.js index 4179a34462..564b8e0685 100644 --- a/core/server/services/newsletters/service.js +++ b/core/server/services/newsletters/service.js @@ -82,6 +82,10 @@ class NewslettersService { // remove any email properties that are not allowed to be set without verification const {cleanedAttrs, emailsToVerify} = await this.prepAttrsForEmailVerification(attrs); + // add this newsletter last + const sortOrder = await this.NewsletterModel.getNextAvailableSortOrder(options); + cleanedAttrs.sort_order = sortOrder; + // add the model now because we need the ID for sending verification emails const newsletter = await this.NewsletterModel.add(cleanedAttrs, options); diff --git a/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap b/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap index 17473dfd64..afbe8053cd 100644 --- a/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap +++ b/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap @@ -25,7 +25,7 @@ Object { "show_header_name": true, "show_header_title": true, "slug": "my-test-newsletter-with-custom-sender_email", - "sort_order": 0, + "sort_order": 4, "status": "active", "subscribe_on_signup": true, "title_alignment": "center", @@ -70,7 +70,7 @@ Object { "show_header_name": true, "show_header_title": true, "slug": "my-test-newsletter", - "sort_order": 0, + "sort_order": 3, "status": "active", "subscribe_on_signup": true, "title_alignment": "center", diff --git a/test/unit/server/services/newsletters/service.test.js b/test/unit/server/services/newsletters/service.test.js index 1a67cae426..636fd3e910 100644 --- a/test/unit/server/services/newsletters/service.test.js +++ b/test/unit/server/services/newsletters/service.test.js @@ -62,26 +62,39 @@ describe('NewslettersService', function () { }); describe('add', function () { - let addStub; + let addStub,getNextAvailableSortOrderStub; beforeEach(function () { // Stub add as a function that returns a get addStub = sinon.stub(models.Newsletter, 'add').returns({get: getStub}); + getNextAvailableSortOrderStub = sinon.stub(models.Newsletter, 'getNextAvailableSortOrder').returns(1); }); it('rejects if called with no data', async function () { assert.rejects(await newsletterService.add, {name: 'TypeError'}); sinon.assert.notCalled(addStub); + sinon.assert.notCalled(getNextAvailableSortOrderStub); }); it('will attempt to add empty object without verification', async function () { const result = await newsletterService.add({}); assert.equal(result.meta, undefined); // meta property has not been added - sinon.assert.calledOnceWithExactly(addStub, {}, undefined); + sinon.assert.calledOnce(getNextAvailableSortOrderStub); + sinon.assert.calledOnceWithExactly(addStub, {sort_order: 1}, undefined); + }); + + it('will override sort_order', async function () { + const data = {name: 'hello world', sort_order: 0}; + const options = {foo: 'bar'}; + + await newsletterService.add(data, options); + + sinon.assert.calledOnce(getNextAvailableSortOrderStub); + sinon.assert.calledOnceWithExactly(addStub, {name: 'hello world', sort_order: 1}, options); }); it('will pass object and options through to model when there are no fields needing verification', async function () { - const data = {name: 'hello world'}; + const data = {name: 'hello world', sort_order: 1}; const options = {foo: 'bar'}; const result = await newsletterService.add(data, options); @@ -101,7 +114,7 @@ describe('NewslettersService', function () { 'sender_email' ] }); - sinon.assert.calledOnceWithExactly(addStub, {name: 'hello world'}, options); + sinon.assert.calledOnceWithExactly(addStub, {name: 'hello world', sort_order: 1}, options); mockManager.assert.sentEmail({to: 'test@example.com'}); sinon.assert.calledOnceWithExactly(tokenProvider.create, {id: undefined, property: 'sender_email', value: 'test@example.com'}); });