From 31e1048db3a826786b31d7871587eee3443dbcc4 Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Thu, 6 Feb 2025 11:32:22 +0100 Subject: [PATCH] Refactored `extra-attrs` output serializer util - after discussing on a code review, we agreed the existing code was very verbose and it was hard to reason about - this is because of the incredibly long if-statements we have lingering around - the general approach here was to extract all the includes into variables at the top, replace them in the code and then simplify the logic - whilst doing this, I wrote some more tests to cover the other cases - this commit also switches to assert, as it's neater to write the assertions in tests --- .../serializers/output/utils/extra-attrs.js | 50 ++++++++-------- .../output/utils/extra-attrs.test.js | 58 +++++++++++++++---- 2 files changed, 70 insertions(+), 38 deletions(-) diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/utils/extra-attrs.js b/ghost/core/core/server/api/endpoints/utils/serializers/output/utils/extra-attrs.js index c035ed35e2..f1dc5b1750 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/output/utils/extra-attrs.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/utils/extra-attrs.js @@ -8,31 +8,32 @@ const readingMinutes = require('@tryghost/helpers').utils.readingMinutes; * @returns {void} - modifies attrs */ module.exports.forPost = (options, model, attrs) => { - // This function is split up in 3 conditions for 3 different purposes: + const columnsIncludesCustomExcerpt = options.columns?.includes('custom_excerpt'); + const columnsIncludesExcerpt = options.columns?.includes('excerpt'); + const columnsIncludesPlaintext = options.columns?.includes('plaintext'); + const columnsIncludesReadingTime = options.columns?.includes('reading_time'); + const formatsIncludesPlaintext = options.formats?.includes('plaintext'); + // 1. Gets excerpt from post's plaintext. If custom_excerpt exists, it overrides the excerpt but the key remains excerpt. - if (Object.prototype.hasOwnProperty.call(options, 'columns') || options.columns?.includes('excerpt') || (options.columns?.includes('excerpt') && options.formats?.includes('plaintext'))) { - if (options.columns?.includes('excerpt')) { - if (!attrs.custom_excerpt || attrs.custom_excerpt === null) { - let plaintext = model.get('plaintext'); - if (plaintext) { - attrs.excerpt = plaintext.substring(0, 500); - } else { - attrs.excerpt = null; - } - if (!options.columns.includes('custom_excerpt')) { - delete attrs.custom_excerpt; - } + if (columnsIncludesExcerpt) { + if (!attrs.custom_excerpt) { + let plaintext = model.get('plaintext'); + if (plaintext) { + attrs.excerpt = plaintext.substring(0, 500); } else { - attrs.excerpt = attrs.custom_excerpt; - if (!options.columns?.includes('custom_excerpt')) { - delete attrs.custom_excerpt; - } + attrs.excerpt = null; } + } else { + attrs.excerpt = attrs.custom_excerpt; + } + + if (!columnsIncludesCustomExcerpt) { + delete attrs.custom_excerpt; } } - // 2. Displays plaintext if requested by user as a field. Also works if used as format. - if (options.columns?.includes('plaintext') || options.formats?.includes('plaintext')) { + // 2. Displays plaintext if requested via `columns` or `formats` + if (columnsIncludesPlaintext || formatsIncludesPlaintext) { let plaintext = model.get('plaintext'); if (plaintext) { attrs.plaintext = plaintext; @@ -41,15 +42,14 @@ module.exports.forPost = (options, model, attrs) => { } } - // 3. Displays excerpt if no columns was requested - specifically needed for the Admin Posts API. - + // 3. Displays excerpt if no columns was requested - specifically needed for the Admin Posts API if (!Object.prototype.hasOwnProperty.call(options, 'columns')) { - let plaintext = model.get('plaintext'); let customExcerpt = model.get('custom_excerpt'); if (customExcerpt !== null) { attrs.excerpt = customExcerpt; } else { + const plaintext = model.get('plaintext'); if (plaintext) { attrs.excerpt = plaintext.substring(0, 500); } else { @@ -58,10 +58,8 @@ module.exports.forPost = (options, model, attrs) => { } } - // reading_time still only works when used along with formats=html. - - if (!Object.prototype.hasOwnProperty.call(options, 'columns') || - (options.columns?.includes('reading_time'))) { + // 4. Add `reading_time` if no columns were requested, or if `reading_time` was requested via `columns` + if (!Object.prototype.hasOwnProperty.call(options, 'columns') || columnsIncludesReadingTime) { if (attrs.html) { let additionalImages = 0; diff --git a/ghost/core/test/unit/api/canary/utils/serializers/output/utils/extra-attrs.test.js b/ghost/core/test/unit/api/canary/utils/serializers/output/utils/extra-attrs.test.js index 87de9ed2ea..de8c9c3ab0 100644 --- a/ghost/core/test/unit/api/canary/utils/serializers/output/utils/extra-attrs.test.js +++ b/ghost/core/test/unit/api/canary/utils/serializers/output/utils/extra-attrs.test.js @@ -1,4 +1,4 @@ -const should = require('should'); +const assert = require('assert/strict'); const sinon = require('sinon'); const extraAttrsUtil = require('../../../../../../../../core/server/api/endpoints/utils/serializers/output/utils/extra-attrs'); @@ -19,33 +19,67 @@ describe('Unit: endpoints/utils/serializers/output/utils/extra-attrs', function it('respects custom excerpt', function () { const attrs = {custom_excerpt: 'custom excerpt'}; extraAttrsUtil.forPost(options, model, attrs); - attrs.excerpt.should.eql(attrs.custom_excerpt); + assert.equal(attrs.excerpt, attrs.custom_excerpt); }); it('no custom excerpt', function () { const attrs = {}; extraAttrsUtil.forPost(options, model, attrs); - model.get.called.should.be.true(); - - attrs.excerpt.should.eql(new Array(501).join('A')); + assert.ok(model.get.called); + assert.equal(attrs.excerpt, new Array(501).join('A')); }); it('has excerpt when plaintext is null', function () { model.get.withArgs('plaintext').returns(null); const attrs = {}; extraAttrsUtil.forPost(options, model, attrs); - model.get.called.should.be.true(); - attrs.should.have.property('excerpt'); - (attrs.excerpt === null).should.be.true(); + assert.ok(model.get.called); + assert.equal(Object.prototype.hasOwnProperty.call(attrs, 'excerpt'), true); + assert.equal(attrs.excerpt, null); + }); + + it('has plaintext when columns includes plaintext', function () { + const attrs = {}; + extraAttrsUtil.forPost({ + columns: ['plaintext'] + }, model, attrs); + assert.ok(model.get.called); + assert.equal(Object.prototype.hasOwnProperty.call(attrs, 'plaintext'), true); + }); + + it('has plaintext when formats includes plaintext', function () { + const attrs = {}; + extraAttrsUtil.forPost({ + formats: ['plaintext'] + }, model, attrs); + assert.ok(model.get.called); + assert.equal(Object.prototype.hasOwnProperty.call(attrs, 'plaintext'), true); }); it('has excerpt when no columns are passed', function () { - delete options.columns; const attrs = {}; - extraAttrsUtil.forPost(options, model, attrs); - model.get.called.should.be.true(); - attrs.should.have.property('excerpt'); + extraAttrsUtil.forPost({}, model, attrs); + assert.ok(model.get.called); + assert.equal(Object.prototype.hasOwnProperty.call(attrs, 'excerpt'), true); + }); + + it('has reading_time when no columns are passed', function () { + const attrs = { + html: 'html' + }; + extraAttrsUtil.forPost({}, model, attrs); + assert.equal(Object.prototype.hasOwnProperty.call(attrs, 'reading_time'), true); + }); + + it('has reading_time when columns includes reading_time', function () { + const attrs = { + html: 'html' + }; + extraAttrsUtil.forPost({ + columns: ['reading_time'] + }, model, attrs); + assert.equal(Object.prototype.hasOwnProperty.call(attrs, 'reading_time'), true); }); }); });