🐛 Fixed settings overriden when updated from multiple tabs (#15536)

closes: #15470

- When multiple browser tabs are open, each manipulate a different copy of ember data model, changes to the model in one tab are not reflected in the model of the other tab.
- When updating some settings, all current settings were sent to the API.
- As a result, when updating two different categories of settings (navigation/code inspection) in different tabs, the second update was overriding the first one.
- From a user perspective, this is not a natural behaviour. Only settings visible on-screen when clicking save should be modified.

Co-authored-by: Kevin Ansfield <[email protected]>

ghost/admin/app/adapters/setting.js

@@ -1,4 +1,5 @@
 import ApplicationAdapter from 'ghost-admin/adapters/application';
+import {pluralize} from 'ember-inflector';
 
 export default class Setting extends ApplicationAdapter {
     updateRecord(store, type, record) {
@@ -12,6 +13,14 @@ export default class Setting extends ApplicationAdapter {
         // an array of settings objects like the API expects
         serializer.serializeIntoHash(data, type, record);
 
+        // Do not send empty data to the API
+        // This can probably be removed then this is fixed:
+        // https://github.com/TryGhost/Ghost/blob/main/ghost/api-framework/lib/validators/input/all.js#L128
+        let root = pluralize(type.modelName);
+        if (data[root].length === 0) {
+            return Promise.resolve();
+        }
+
         // use the ApplicationAdapter's buildURL method but do not
         // pass in an id.
         return this.ajax(this.buildURL(type.modelName), 'PUT', {data});

ghost/admin/app/controllers/settings/navigation.js

@@ -124,6 +124,14 @@ export default class NavigationController extends Controller {
 
         try {
             yield RSVP.all(validationPromises);
+
+            // If some attributes have been changed, rebuild
+            // the model arrays or changes will not be detected
+            if (this.dirtyAttributes) {
+                this.settings.navigation = [...this.settings.navigation];
+                this.settings.secondaryNavigation = [...this.settings.secondaryNavigation];
+            }
+
             this.dirtyAttributes = false;
             return yield this.settings.save();
         } catch (error) {

ghost/admin/app/serializers/setting.js

@@ -8,7 +8,8 @@ export default class Setting extends ApplicationSerializer {
         options.includeId = false;
 
         let root = pluralize(type.modelName);
-        let data = this.serialize(record, options);
+        let data = Object.keys(record.record.changedAttributes()).length > 0 ?
+            this.serialize(record, options) : [];
         let payload = [];
 
         delete data.id;
@@ -21,6 +22,20 @@ export default class Setting extends ApplicationSerializer {
         hash[root] = payload;
     }
 
+    serializeAttribute(snapshot, json, key, attributes) {
+        // Only serialize attributes that have changed and 
+        // send a partial update to the API to avoid conflicts
+        // with different screens using the same model
+        // See https://github.com/TryGhost/Ghost/issues/15470
+        if (
+            !snapshot.record.get('isNew') &&
+            !snapshot.record.changedAttributes()[key]
+        ) {
+            return;
+        }
+        super.serializeAttribute(snapshot, json, key, attributes);
+    }
+
     normalizeArrayResponse(store, primaryModelClass, _payload, id, requestType) {
         let payload = {settings: [this._extractObjectFromArrayPayload(_payload)]};
         return super.normalizeArrayResponse(store, primaryModelClass, payload, id, requestType);

ghost/admin/tests/acceptance/settings/code-injection-test.js

@@ -5,7 +5,7 @@ import {
     describe,
     it
 } from 'mocha';
-import {click, currentURL, find, findAll, triggerEvent} from '@ember/test-helpers';
+import {click, currentURL, fillIn, find, findAll, triggerEvent} from '@ember/test-helpers';
 import {expect} from 'chai';
 import {setupApplicationTest} from 'ember-mocha';
 import {setupMirage} from 'ember-cli-mirage/test-support';
@@ -77,26 +77,36 @@ describe('Acceptance: Settings - Code-Injection', function () {
             expect(findAll('#ghost-foot .CodeMirror').length, 'ghost head codemirror element').to.equal(1);
             expect(find('#ghost-foot .CodeMirror'), 'ghost head editor theme').to.have.class('cm-s-xq-light');
 
+            await fillIn('#settings-code #ghost-head textarea', 'Test');
+
             await click('[data-test-save-button]');
 
             let [lastRequest] = this.server.pretender.handledRequests.slice(-1);
             let params = JSON.parse(lastRequest.requestBody);
 
-            expect(params.settings.findBy('key', 'codeinjection_head').value).to.equal(null);
+            expect(params.settings.findBy('key', 'codeinjection_head').value).to.equal('Test');
+            // update should have been partial
+            expect(params.settings.findBy('key', 'navigation')).to.be.undefined;
             expect(find('[data-test-save-button]').textContent.trim(), 'save button text').to.equal('Save');
 
+            await fillIn('#settings-code #ghost-head textarea', '');
+
             // CMD-S shortcut works
             await triggerEvent('.gh-app', 'keydown', {
                 keyCode: 83, // s
                 metaKey: ctrlOrCmd === 'command',
                 ctrlKey: ctrlOrCmd === 'ctrl'
             });
-            // we've already saved in this test so there's no on-screen indication
-            // that we've had another save, check the request was fired instead
+
             let [newRequest] = this.server.pretender.handledRequests.slice(-1);
             params = JSON.parse(newRequest.requestBody);
 
-            expect(params.settings.findBy('key', 'codeinjection_head').value).to.equal(null);
+            expect(params.settings.findBy('key', 'codeinjection_head').value).to.equal('');
+            expect(find('[data-test-save-button]').textContent.trim(), 'save button text').to.equal('Save');
+
+            // Saving when no changed have been made should work
+            // (although no api request is expected)
+            await click('[data-test-save-button]');            
             expect(find('[data-test-save-button]').textContent.trim(), 'save button text').to.equal('Save');
         });
     });

ghost/admin/tests/acceptance/settings/slack-test.js

@@ -96,6 +96,9 @@ describe('Acceptance: Settings - Integrations - Slack', function () {
             expect(find('[data-test-error="slack-url"]'), 'inline validation response')
                 .to.not.exist;
 
+            // modify model data or there will be no api call
+            await fillIn('[data-test-slack-url-input]', 'https://hooks.slack.com/services/1275958431');
+
             this.server.put('/settings/', function () {
                 return new Response(422, {}, {
                     errors: [

ghost/core/test/regression/api/admin/settings.test.js

@@ -225,6 +225,22 @@ describe('Settings API', function () {
                 .expect(422);
         });
 
+        // If this test fails, it can be safely removed
+        // but the front-end should be updated accordingly, 
+        // removing the workaround in place for this specific usecase 
+        it('Cannot send an empty array', async function () {
+            const settingsToChange = {
+                settings: []
+            };
+
+            await request.put(localUtils.API.getApiQuery('settings/'))
+                .set('Origin', config.get('url'))
+                .send(settingsToChange)
+                .expect('Content-Type', /json/)
+                .expect('Cache-Control', testUtils.cacheRules.private)
+                .expect(400);
+        });
+
         it('Cannot edit notifications key through API', async function () {
             await checkCantEdit('notifications', JSON.stringify(['do not touch me']));
         });