From 449bae9a48a1136121c2f5268d909ff1918db116 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Mon, 4 Mar 2019 17:00:07 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20missing=20"value"=20prop?= =?UTF-8?q?erty=20for=20settings=20Admin=20API=20v2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #10518 - we had a very generic logic to remove "unwanted" null values - copied from v0.1 - originally added in https://github.com/TryGhost/Ghost/commit/7d4107fec4673023fd54a025a08d187d7e1625d5 - this logic transformed: settings = [{key: 'key', value: null}] to [{key: 'key'}], which is wrong - i've removed this generic logic completely, because i don't know which purpose it serves - if there a specific case where we want to remove null values, we should either use the JSON schema or use a specific serializer for the target resource - added tests to proof that settings API behaves as it should - one test failed because we removed the isNull logic -> if you send published_at = null on a published post - the model layer has a piece of logic to force a date if you set published_at to null if the status is published - protected --- .../api/shared/serializers/input/all.js | 15 ------------ .../v2/utils/serializers/input/settings.js | 5 ++-- .../acceptance/old/admin/settings_spec.js | 22 +++++++++++++++-- .../regression/api/v2/admin/posts_spec.js | 4 ++-- .../api/shared/serializers/handle_spec.js | 9 +++---- .../api/shared/serializers/input/all_spec.js | 24 ------------------- 6 files changed, 28 insertions(+), 51 deletions(-) diff --git a/core/server/api/shared/serializers/input/all.js b/core/server/api/shared/serializers/input/all.js index c1671d5528..1e2927bdd6 100644 --- a/core/server/api/shared/serializers/input/all.js +++ b/core/server/api/shared/serializers/input/all.js @@ -35,20 +35,5 @@ module.exports = { } debug(frame.options); - }, - - add(apiConfig, frame) { - // CASE: will remove unwanted null values - _.each(frame.data[apiConfig.docName], (value, index) => { - if (!_.isObject(frame.data[apiConfig.docName][index])) { - return; - } - - frame.data[apiConfig.docName][index] = _.omitBy(frame.data[apiConfig.docName][index], _.isNull); - }); - }, - - edit() { - return this.add(...arguments); } }; diff --git a/core/server/api/v2/utils/serializers/input/settings.js b/core/server/api/v2/utils/serializers/input/settings.js index d7505f7fca..fc2c036a52 100644 --- a/core/server/api/v2/utils/serializers/input/settings.js +++ b/core/server/api/v2/utils/serializers/input/settings.js @@ -7,9 +7,10 @@ module.exports = { frame.data = {settings: [{key: frame.data, value: frame.options}]}; } - // prepare data + // CASE: transform objects/arrays into string (we store stringified objects in the db) + // @TODO: This belongs into the model layer? frame.data.settings.forEach((setting) => { - if (!_.isString(setting.value)) { + if (_.isObject(setting.value)) { setting.value = JSON.stringify(setting.value); } }); diff --git a/core/test/acceptance/old/admin/settings_spec.js b/core/test/acceptance/old/admin/settings_spec.js index 3bf2995f04..bd1afd43cb 100644 --- a/core/test/acceptance/old/admin/settings_spec.js +++ b/core/test/acceptance/old/admin/settings_spec.js @@ -97,7 +97,22 @@ describe('Settings API', function () { changedValue = [], settingToChange = { settings: [ - {key: 'title', value: changedValue} + { + key: 'title', + value: changedValue + }, + { + key: 'ghost_head', + value: null + }, + { + key: 'navigation', + value: {label: 'label1'} + }, + { + key: 'slack', + value: JSON.stringify({username: 'username'}) + } ] }; @@ -115,10 +130,13 @@ describe('Settings API', function () { return done(err); } - var putBody = res.body; + const putBody = res.body; res.headers['x-cache-invalidate'].should.eql('/*'); should.exist(putBody); putBody.settings[0].value.should.eql(JSON.stringify(changedValue)); + should.equal(putBody.settings[1].value, null); + should.equal(putBody.settings[2].value, JSON.stringify({label: 'label1'})); + should.equal(putBody.settings[3].value, JSON.stringify({username: 'username'})); localUtils.API.checkResponse(putBody, 'settings'); done(); }); diff --git a/core/test/regression/api/v2/admin/posts_spec.js b/core/test/regression/api/v2/admin/posts_spec.js index b848b53627..68e4b213b8 100644 --- a/core/test/regression/api/v2/admin/posts_spec.js +++ b/core/test/regression/api/v2/admin/posts_spec.js @@ -144,8 +144,8 @@ describe('Posts API', function () { .expect(200); }) .then((res) => { - // @NOTE: you cannot modify published_at manually, that's why the resource won't change. - should.not.exist(res.headers['x-cache-invalidate']); + // @NOTE: if you set published_at to null and the post is published, we set it to NOW in model layer + should.exist(res.headers['x-cache-invalidate']); should.exist(res.body.posts); should.exist(res.body.posts[0].published_at); }); diff --git a/core/test/unit/api/shared/serializers/handle_spec.js b/core/test/unit/api/shared/serializers/handle_spec.js index 6fe34b07c2..f7e61e823c 100644 --- a/core/test/unit/api/shared/serializers/handle_spec.js +++ b/core/test/unit/api/shared/serializers/handle_spec.js @@ -28,27 +28,24 @@ describe('Unit: api/shared/serializers/handle', function () { it('ensure serializers are called with apiConfig and frame', function () { const allStub = sinon.stub(); - const addStub = sinon.stub(); sinon.stub(shared.serializers.input.all, 'all').get(() => allStub); - sinon.stub(shared.serializers.input.all, 'add').get(() => addStub); const apiSerializers = { all: sinon.stub().resolves(), posts: { all: sinon.stub().resolves(), - add: sinon.stub().resolves() + browse: sinon.stub().resolves() } }; - const apiConfig = {docName: 'posts', method: 'add'}; + const apiConfig = {docName: 'posts', method: 'browse'}; const frame = {}; const stubsToCheck = [ allStub, - addStub, apiSerializers.all, apiSerializers.posts.all, - apiSerializers.posts.add + apiSerializers.posts.browse ]; return shared.serializers.handle.input(apiConfig, apiSerializers, frame) diff --git a/core/test/unit/api/shared/serializers/input/all_spec.js b/core/test/unit/api/shared/serializers/input/all_spec.js index bde03f6971..e80df14eec 100644 --- a/core/test/unit/api/shared/serializers/input/all_spec.js +++ b/core/test/unit/api/shared/serializers/input/all_spec.js @@ -78,28 +78,4 @@ describe('Unit: v2/utils/serializers/input/all', function () { }); }); }); - - describe('add', function () { - it('removes null values', function () { - const frame = { - data: { - posts: [{ - a: null, - b: true, - c: null - }] - } - }; - - const apiConfig = { - docName: 'posts' - }; - - shared.serializers.input.all.add(apiConfig, frame); - - should.not.exist(frame.data.posts[0].a); - should.exist(frame.data.posts[0].b); - should.not.exist(frame.data.posts[0].c); - }); - }); });