From 31d5448ec9c5e53a01d40c7d8a5b98214f65ecea Mon Sep 17 00:00:00 2001 From: jbenezech Date: Wed, 12 Oct 2022 20:03:54 +0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20settings=20overriden=20w?= =?UTF-8?q?hen=20updated=20from=20multiple=20tabs=20(#15536)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes: https://github.com/TryGhost/Ghost/issues/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 --- ghost/admin/app/adapters/setting.js | 9 +++++++++ .../app/controllers/settings/navigation.js | 8 ++++++++ ghost/admin/app/serializers/setting.js | 17 +++++++++++++++- .../settings/code-injection-test.js | 20 ++++++++++++++----- .../tests/acceptance/settings/slack-test.js | 3 +++ .../regression/api/admin/settings.test.js | 16 +++++++++++++++ 6 files changed, 67 insertions(+), 6 deletions(-) diff --git a/ghost/admin/app/adapters/setting.js b/ghost/admin/app/adapters/setting.js index f6d3a1dc5828..62b478c9c63f 100644 --- a/ghost/admin/app/adapters/setting.js +++ b/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}); diff --git a/ghost/admin/app/controllers/settings/navigation.js b/ghost/admin/app/controllers/settings/navigation.js index d8c3ca2f7436..ae064bb3d7cb 100644 --- a/ghost/admin/app/controllers/settings/navigation.js +++ b/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) { diff --git a/ghost/admin/app/serializers/setting.js b/ghost/admin/app/serializers/setting.js index 2886fcad174a..787568f46fcc 100644 --- a/ghost/admin/app/serializers/setting.js +++ b/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); diff --git a/ghost/admin/tests/acceptance/settings/code-injection-test.js b/ghost/admin/tests/acceptance/settings/code-injection-test.js index b0f133f3d19f..c6ad80e8111a 100644 --- a/ghost/admin/tests/acceptance/settings/code-injection-test.js +++ b/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'); }); }); diff --git a/ghost/admin/tests/acceptance/settings/slack-test.js b/ghost/admin/tests/acceptance/settings/slack-test.js index 8b6289e847d5..8122f2a75113 100644 --- a/ghost/admin/tests/acceptance/settings/slack-test.js +++ b/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: [ diff --git a/ghost/core/test/regression/api/admin/settings.test.js b/ghost/core/test/regression/api/admin/settings.test.js index 9306461aafb7..be2737c228de 100644 --- a/ghost/core/test/regression/api/admin/settings.test.js +++ b/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'])); });