diff --git a/ghost/admin/app/adapters/setting.js b/ghost/admin/app/adapters/setting.js index f6d3a1dc582..62b478c9c63 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 d8c3ca2f743..ae064bb3d7c 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 2886fcad174..787568f46fc 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 b0f133f3d19..c6ad80e8111 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 8b6289e847d..8122f2a7511 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 9306461aafb..be2737c228d 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'])); });