From c5d013d11c7e0e0d6a6db96184e80aeb1c9cc457 Mon Sep 17 00:00:00 2001 From: "Vangalla, Rohith" Date: Fri, 22 May 2026 11:38:28 -0500 Subject: [PATCH 1/2] Extract shared composeFilters utility to eliminate code duplication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The composeFilters() method was duplicated identically between doc-type/index.js and page/index.js, with TODO comments warning developers to 'keep in sync' between the two files. This is fragile — any change to filter composition logic requires updating two files, and forgetting one creates subtle inconsistencies in how filters behave for pages vs. other doc types. This change extracts the shared logic into lib/compose-filters.js and replaces both implementations with a single call to the shared utility. The extracted function: - Transforms a filters object (keyed by name) into an array - Normalizes inputType (defaults to 'select') - Adds null choices for non-required filters - Sets appropriate nullLabel for dynamic choices - Sets default values for checkbox filters Benefits: - Single source of truth for filter composition logic - Changes to filter behavior only need to be made in one place - Eliminates the risk of the two implementations drifting apart - Easier to unit test in isolation Addresses the TODO comments in both files: 'keep in sync with page/index.js composeFilters' 'keep in sync with doc-type/index.js composeFilters' --- packages/apostrophe/lib/compose-filters.js | 51 +++++++++++++++++++ .../modules/@apostrophecms/doc-type/index.js | 35 +------------ .../modules/@apostrophecms/page/index.js | 35 +------------ 3 files changed, 53 insertions(+), 68 deletions(-) create mode 100644 packages/apostrophe/lib/compose-filters.js diff --git a/packages/apostrophe/lib/compose-filters.js b/packages/apostrophe/lib/compose-filters.js new file mode 100644 index 0000000000..cb3c724052 --- /dev/null +++ b/packages/apostrophe/lib/compose-filters.js @@ -0,0 +1,51 @@ +/** + * Shared utility for composing filters in doc-type and page modules. + * + * Transforms a filters object (keyed by name) into an array of filter + * definitions with normalized inputType, default values, and null choices. + * + * This was previously duplicated between: + * - @apostrophecms/doc-type/index.js composeFilters() + * - @apostrophecms/page/index.js composeFilters() + * + * @param {Object} filters - An object keyed by filter name, where each value + * is a filter definition object. + * @returns {Array} An array of composed filter objects with `name` property added. + */ +module.exports = function composeFilters(filters) { + const composed = Object.entries(filters) + .map(([ name, filter ]) => ({ + name, + ...filter, + inputType: filter.inputType || 'select' + })); + + // Add a null choice if not already added or set to `required` + composed.forEach((filter) => { + if (Array.isArray(filter.choices)) { + if ( + !filter.required && + !filter.choices.find((choice) => choice.value === null) + ) { + filter.def = null; + filter.choices = filter.inputType === 'checkbox' + ? filter.choices + : filter.choices.concat({ + value: null, + label: 'apostrophe:none' + }); + } + } else { + // Dynamic choices from the REST API, but + // we need a label for "no opinion" + filter.nullLabel = filter.inputType === 'radio' + ? 'apostrophe:any' + : 'apostrophe:filterMenuChooseOne'; + } + if (filter.inputType === 'checkbox') { + filter.def = []; + } + }); + + return composed; +}; diff --git a/packages/apostrophe/modules/@apostrophecms/doc-type/index.js b/packages/apostrophe/modules/@apostrophecms/doc-type/index.js index 3b3bde0f7d..2224845e70 100644 --- a/packages/apostrophe/modules/@apostrophecms/doc-type/index.js +++ b/packages/apostrophe/modules/@apostrophecms/doc-type/index.js @@ -1675,40 +1675,7 @@ module.exports = { }, composeFilters() { - // TODO: keep in sync with page/index.js composeFilters - self.filters = Object.entries(self.filters) - .map(([ name, filter ]) => ({ - name, - ...filter, - inputType: filter.inputType || 'select' - })); - - // Add a null choice if not already added or set to `required` - self.filters.forEach((filter) => { - if (Array.isArray(filter.choices)) { - if ( - !filter.required && - !filter.choices.find((choice) => choice.value === null) - ) { - filter.def = null; - filter.choices = filter.inputType === 'checkbox' - ? filter.choices - : filter.choices.concat({ - value: null, - label: 'apostrophe:none' - }); - } - } else { - // Dynamic choices from the REST API, but - // we need a label for "no opinion" - filter.nullLabel = filter.inputType === 'radio' - ? 'apostrophe:any' - : 'apostrophe:filterMenuChooseOne'; - } - if (filter.inputType === 'checkbox') { - filter.def = []; - } - }); + self.filters = require('../../../../lib/compose-filters')(self.filters); }, composeColumns() { diff --git a/packages/apostrophe/modules/@apostrophecms/page/index.js b/packages/apostrophe/modules/@apostrophecms/page/index.js index e68c45e975..4c253e65aa 100644 --- a/packages/apostrophe/modules/@apostrophecms/page/index.js +++ b/packages/apostrophe/modules/@apostrophecms/page/index.js @@ -3304,40 +3304,7 @@ database.`); }); }, composeFilters() { - // TODO: keep in sync with doc-type/index.js composeFilters - self.filters = Object.entries(self.filters) - .map(([ name, filter ]) => ({ - name, - ...filter, - inputType: filter.inputType || 'select' - })); - - // Add a null choice if not already added or set to `required` - self.filters.forEach((filter) => { - if (Array.isArray(filter.choices)) { - if ( - !filter.required && - !filter.choices.find((choice) => choice.value === null) - ) { - filter.def = null; - filter.choices = filter.inputType === 'checkbox' - ? filter.choices - : filter.choices.concat({ - value: null, - label: 'apostrophe:none' - }); - } - } else { - // Dynamic choices from the REST API, but - // we need a label for "no opinion" - filter.nullLabel = filter.inputType === 'radio' - ? 'apostrophe:any' - : 'apostrophe:filterMenuChooseOne'; - } - if (filter.inputType === 'checkbox') { - filter.def = []; - } - }); + self.filters = require('../../../../lib/compose-filters')(self.filters); }, async getBatchArchivePatches(req, ids) { const batchReq = req.clone({ From 628815ef806c071f157b798fd03b40582f315133 Mon Sep 17 00:00:00 2001 From: "Vangalla, Rohith" Date: Fri, 12 Jun 2026 15:06:13 -0500 Subject: [PATCH 2/2] Fix require path: change from ../../../../ to ../../../ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compose-filters.js file is 3 levels up from both: - modules/@apostrophecms/doc-type/index.js - modules/@apostrophecms/page/index.js Path breakdown: - ../ → up to @apostrophecms/ - ../../ → up to modules/ - ../../../ → up to apostrophe/ (package root) - ../../../lib/compose-filters → target file Fixes MODULE_NOT_FOUND error reported in review. --- packages/apostrophe/modules/@apostrophecms/doc-type/index.js | 2 +- packages/apostrophe/modules/@apostrophecms/page/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apostrophe/modules/@apostrophecms/doc-type/index.js b/packages/apostrophe/modules/@apostrophecms/doc-type/index.js index 2224845e70..ed0593c3b0 100644 --- a/packages/apostrophe/modules/@apostrophecms/doc-type/index.js +++ b/packages/apostrophe/modules/@apostrophecms/doc-type/index.js @@ -1675,7 +1675,7 @@ module.exports = { }, composeFilters() { - self.filters = require('../../../../lib/compose-filters')(self.filters); + self.filters = require('../../../lib/compose-filters')(self.filters); }, composeColumns() { diff --git a/packages/apostrophe/modules/@apostrophecms/page/index.js b/packages/apostrophe/modules/@apostrophecms/page/index.js index 4c253e65aa..e05b07326c 100644 --- a/packages/apostrophe/modules/@apostrophecms/page/index.js +++ b/packages/apostrophe/modules/@apostrophecms/page/index.js @@ -3304,7 +3304,7 @@ database.`); }); }, composeFilters() { - self.filters = require('../../../../lib/compose-filters')(self.filters); + self.filters = require('../../../lib/compose-filters')(self.filters); }, async getBatchArchivePatches(req, ids) { const batchReq = req.clone({