Skip to content

Commit

Permalink
handle different optionsFieldType (#971)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
oshi97 committed Oct 11, 2021
1 parent d9bf111 commit c288da5
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 30 deletions.
57 changes: 43 additions & 14 deletions static/js/transform-facets.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -26,6 +26,7 @@ export default function transformFacets (facets, config) {
const {
fieldLabels,
optionsOrder,
optionsFieldType = 'STRING',
...filterOptionsConfig
} = config.fields[facet.fieldId];

Expand All @@ -41,23 +42,51 @@ 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,
options
};
});
}

/**
* 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()))
}
61 changes: 45 additions & 16 deletions tests/static/js/transform-facets.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ describe('optionsOrder', () => {
}
];

function createFacetsConfig(optionsOrder, fieldLabels) {
function createFacetsConfig(optionsOrder, optionsFieldType, fieldLabels) {
return {
fields: {
c_mealType: {
Expand All @@ -155,6 +155,7 @@ describe('optionsOrder', () => {
Lunch: 'a lunch',
Dinner: 'duh dinner'
},
optionsFieldType: optionsFieldType || 'STRING',
optionsOrder
}
}
Expand All @@ -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');
})
});

0 comments on commit c288da5

Please sign in to comment.