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

@@ -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;
     }
 });
 

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);
 

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",

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