From c288da543c1f52b803d776b6cf1f9b798ba71e1c Mon Sep 17 00:00:00 2001 From: Oliver Shi Date: Mon, 11 Oct 2021 16:56:55 -0400 Subject: [PATCH] handle different optionsFieldType (#971) Originally, I thought running a toString() on the displayName would handle number display names. But, string.prototype.localeCompare doesn't work with number strings the way I hoped it would. If you try to sort an array of numbers like [100, 20, 120], the result will be [100, 120, 20] rather than the expected [20, 100, 120]. My unit tests only had single digit numbers, so they didn't run test that case. TEST=auto J=SLAP-1631 --- static/js/transform-facets.js | 57 ++++++++++++++++++++------- tests/static/js/transform-facets.js | 61 +++++++++++++++++++++-------- 2 files changed, 88 insertions(+), 30 deletions(-) diff --git a/static/js/transform-facets.js b/static/js/transform-facets.js index f2c1ddcef..c54e85786 100644 --- a/static/js/transform-facets.js +++ b/static/js/transform-facets.js @@ -6,8 +6,8 @@ * @param {FilterOptionsConfig} config the config of the FilterOptionsConfig from answers-search-ui * @returns {(DisplayableFacet | FilterOptionsConfig)[]} */ -export default function transformFacets (facets, config) { - if(!config || !('fields' in config)) { +export default function transformFacets(facets, config) { + if (!config || !('fields' in config)) { return facets; } @@ -26,6 +26,7 @@ export default function transformFacets (facets, config) { const { fieldLabels, optionsOrder, + optionsFieldType = 'STRING', ...filterOptionsConfig } = config.fields[facet.fieldId]; @@ -41,19 +42,9 @@ export default function transformFacets (facets, config) { } if (optionsOrder) { - if (optionsOrder === 'ASC') { - options = options.sort((a, b) => { - return a.displayName.toString().localeCompare(b.displayName.toString()) - }); - } else if (optionsOrder === 'DESC') { - options = options.sort((a, b) => { - return b.displayName.toString().localeCompare(a.displayName.toString()) - }); - } else { - console.error(`Unknown facet optionsOrder "${optionsOrder}" for the "${facet.fieldId}" facet.`); - } + options = sortFacetOptions(options, optionsOrder, optionsFieldType, facet.fieldId); } - + return { ...facet, ...filterOptionsConfig, @@ -61,3 +52,41 @@ export default function transformFacets (facets, config) { }; }); } + +/** + * Sorts the facet options in place. + * + * @param {{ displayName: string }[]} options The facet options to sort. + * @param {'ASC' | 'DESC'} optionsOrder + * @param {'STRING' | 'INT'} optionsFieldType + * @param {string} fieldId + * @returns {{ displayName: string }[]} + */ +function sortFacetOptions(options, optionsOrder, optionsFieldType, fieldId) { + const getSortComparator = () => { + if (optionsFieldType === 'STRING') { + return (a, b) => a.displayName.localeCompare(b.displayName); + } else if (optionsFieldType === 'INT') { + return (a, b) => parseInt(a.displayName) - parseInt(b.displayName); + } else { + console.error(`Unknown facet optionsFieldType "${optionsFieldType}" for the "${fieldId}" facet.`); + return undefined; + } + } + const applyDirectionToComparator = (comparator) => { + if (!comparator) { + return undefined; + } + + if (optionsOrder === 'ASC') { + return comparator; + } else if (optionsOrder === 'DESC') { + return (a, b) => -1 * comparator(a, b) + } else { + console.error(`Unknown facet optionsOrder "${optionsOrder}" for the "${fieldId}" facet.`); + return undefined; + } + } + + return options.sort(applyDirectionToComparator(getSortComparator())) +} diff --git a/tests/static/js/transform-facets.js b/tests/static/js/transform-facets.js index 964a4febe..5a00be8a6 100644 --- a/tests/static/js/transform-facets.js +++ b/tests/static/js/transform-facets.js @@ -146,7 +146,7 @@ describe('optionsOrder', () => { } ]; - function createFacetsConfig(optionsOrder, fieldLabels) { + function createFacetsConfig(optionsOrder, optionsFieldType, fieldLabels) { return { fields: { c_mealType: { @@ -155,6 +155,7 @@ describe('optionsOrder', () => { Lunch: 'a lunch', Dinner: 'duh dinner' }, + optionsFieldType: optionsFieldType || 'STRING', optionsOrder } } @@ -175,37 +176,65 @@ describe('optionsOrder', () => { expect(actualOptions[2].displayName).toEqual('a lunch'); }) - it('logs an error if you use an unknown optionsOrder, and does not try to sort', () => { + it('logs an error if you use an unknown optionsOrder', () => { const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); expect(consoleError).toHaveBeenCalledTimes(0); - const actualOptions = transformFacets(facets, createFacetsConfig('PACER'))[0].options; + transformFacets(facets, createFacetsConfig('PACER'))[0].options; expect(consoleError).toHaveBeenCalledWith( 'Unknown facet optionsOrder "PACER" for the "c_mealType" facet.'); consoleError.mockRestore(); - expect(actualOptions[0].displayName).toEqual('ze breakfast'); - expect(actualOptions[1].displayName).toEqual('a lunch'); - expect(actualOptions[2].displayName).toEqual('duh dinner'); + }) + + it('logs an error if you use an unknown optionsFieldType, and does not try to sort', () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); + expect(consoleError).toHaveBeenCalledTimes(0); + transformFacets(facets, createFacetsConfig('ASC', 'FAKEINT'))[0].options; + expect(consoleError).toHaveBeenCalledWith( + 'Unknown facet optionsFieldType "FAKEINT" for the "c_mealType" facet.'); + consoleError.mockRestore(); }) it('works with ASC number display names', () => { - const actualOptions = transformFacets(facets, createFacetsConfig('ASC', { + const actualOptions = transformFacets(facets, createFacetsConfig('ASC', 'INT', { Breakfast: 3, Lunch: 2, - Dinner: 1 + Dinner: 100 }))[0].options; - expect(actualOptions[0].displayName).toEqual(1); - expect(actualOptions[1].displayName).toEqual(2); - expect(actualOptions[2].displayName).toEqual(3); + expect(actualOptions[0].displayName).toEqual(2); + expect(actualOptions[1].displayName).toEqual(3); + expect(actualOptions[2].displayName).toEqual(100); }) it('works with DESC number display names', () => { - const actualOptions = transformFacets(facets, createFacetsConfig('DESC', { + const actualOptions = transformFacets(facets, createFacetsConfig('DESC', 'INT', { Breakfast: 2, Lunch: 3, - Dinner: 1 + Dinner: 100 + }))[0].options; + expect(actualOptions[0].displayName).toEqual(100); + expect(actualOptions[1].displayName).toEqual(3); + expect(actualOptions[2].displayName).toEqual(2); + }) + + it('works with ASC number display names that need to be parsed', () => { + const actualOptions = transformFacets(facets, createFacetsConfig('ASC', 'INT', { + Breakfast: '3', + Lunch: '2', + Dinner: '100' + }))[0].options; + expect(actualOptions[0].displayName).toEqual('2'); + expect(actualOptions[1].displayName).toEqual('3'); + expect(actualOptions[2].displayName).toEqual('100'); + }) + + it('works with DESC number display names that need to be parsed', () => { + const actualOptions = transformFacets(facets, createFacetsConfig('DESC', 'INT', { + Breakfast: '2', + Lunch: '3', + Dinner: '100' }))[0].options; - expect(actualOptions[0].displayName).toEqual(3); - expect(actualOptions[1].displayName).toEqual(2); - expect(actualOptions[2].displayName).toEqual(1); + expect(actualOptions[0].displayName).toEqual('100'); + expect(actualOptions[1].displayName).toEqual('3'); + expect(actualOptions[2].displayName).toEqual('2'); }) }); \ No newline at end of file