Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect ordering of subcategories #29281

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7f3e6c8
improve getCategoryOptionTree
rezkiy37 Oct 11, 2023
bc9b952
add one-line test case
rezkiy37 Oct 11, 2023
7ac0ec5
fix keys
rezkiy37 Oct 11, 2023
99da058
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 11, 2023
2f3c120
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 12, 2023
10a5788
single responsibility of filtering disabled categories
rezkiy37 Oct 12, 2023
c21dc53
create sortCategories helper
rezkiy37 Oct 15, 2023
638837e
use sortCategories helper
rezkiy37 Oct 15, 2023
44b45a5
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 15, 2023
ad432b1
minor grammar fix
rezkiy37 Oct 15, 2023
443cc02
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 16, 2023
1c447e2
improvements
rezkiy37 Oct 16, 2023
af7007d
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 18, 2023
9fa9bca
add comments
rezkiy37 Oct 18, 2023
21a25fa
handle a case with dots in category name
rezkiy37 Oct 18, 2023
e297e5f
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 19, 2023
0f8cfb5
clean the dot within category name solution
rezkiy37 Oct 19, 2023
e70e40b
add one more case
rezkiy37 Oct 19, 2023
8269bab
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 23, 2023
354b515
sort top-level category
rezkiy37 Oct 23, 2023
8a31777
expect sorted categories
rezkiy37 Oct 23, 2023
87c35b8
Merge branch 'main' of https://github.com/rezkiy37/Expensify into fix…
rezkiy37 Oct 24, 2023
015cc04
sort via underscore
rezkiy37 Oct 24, 2023
411db80
use array instead of string in lodashGet
rezkiy37 Oct 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 112 additions & 33 deletions src/libs/OptionsListUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import _ from 'underscore';
import Onyx from 'react-native-onyx';
import lodashOrderBy from 'lodash/orderBy';
import lodashGet from 'lodash/get';
import lodashSet from 'lodash/set';
import Str from 'expensify-common/lib/str';
import {parsePhoneNumber} from 'awesome-phonenumber';
import ONYXKEYS from '../ONYXKEYS';
Expand Down Expand Up @@ -664,7 +665,87 @@ function hasEnabledOptions(options) {
}

/**
* Build the options for the category tree hierarchy via indents
* Sorts categories using a simple object.
* It builds an hierarchy (based on an object), where each category has a name and other keys as subcategories.
* Via the hierarchy we avoid duplicating and sort categories one by one. Subcategories are being sorted alphabetically.
*
* @param {Object<String, {name: String, enabled: Boolean}>} categories
* @returns {Array<Object>}
*/
function sortCategories(categories) {
// Sorts categories alphabetically by name.
const sortedCategories = _.chain(categories)
.values()
.sortBy((category) => category.name)
.value();

// An object that respects nesting of categories. Also, can contain only uniq categories.
const hierarchy = {};

/**
* Iterates over all categories to set each category in a proper place in hierarchy
* It gets a path based on a category name e.g. "Parent: Child: Subcategory" -> "Parent.Child.Subcategory".
* {
* Parent: {
* name: "Parent",
* Child: {
* name: "Child"
* Subcategory: {
* name: "Subcategory"
* }
* }
* }
* }
*/
_.each(sortedCategories, (category) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You used _.reduce a few lines later, maybe this would be a bit cleaner if we used reduction instead of mutating the hierarchy object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I use the reduction to covert the hierarchy object into a sorted array. Before the reduction, we need do 2 actions:

  1. Sort all categories alphabetically. It helps with top-level categories.
  2. Build the hierarchy object. It helps sort subcategories, keep them uniquely and store initial names.

So, I don't see a variant how to combine these 2 different functions.

Copy link
Contributor

@cubuspl42 cubuspl42 Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant that the hierarchy itself could probably also be built in a mostly functionally-pure way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... Could you please share an example?
It is a simple loop right now, where the app sets values one by one. I don't see any concerns with functional programming patterns here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, let's leave this. I agree that the code is acceptable as-is.

const path = category.name.split(CONST.PARENT_CHILD_SEPARATOR);
const existedValue = lodashGet(hierarchy, path, {});

lodashSet(hierarchy, path, {
...existedValue,
name: category.name,
});
});

/**
* A recursive function to convert hierarchy into an array of category objects.
* The category object contains base 2 properties: "name" and "enabled".
* It iterates each key one by one. When a category has subcategories, goes deeper into them. Also, sorts subcategories alphabetically.
*
* @param {Object} initialHierarchy
* @returns {Array<Object>}
*/
const flatHierarchy = (initialHierarchy) =>
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
_.reduce(
initialHierarchy,
(acc, category) => {
const {name, ...subcategories} = category;

if (!_.isEmpty(name)) {
const categoryObject = {
name,
enabled: lodashGet(categories, [name, 'enabled'], false),
};

acc.push(categoryObject);
}

if (!_.isEmpty(subcategories)) {
const nestedCategories = flatHierarchy(subcategories);

acc.push(..._.sortBy(nestedCategories, 'name'));
}

return acc;
},
[],
);

return flatHierarchy(hierarchy);
}

/**
* Builds the options for the category tree hierarchy via indents
*
* @param {Object[]} options - an initial object array
* @param {Boolean} options[].enabled - a flag to enable/disable option in a list
Expand All @@ -673,52 +754,49 @@ function hasEnabledOptions(options) {
* @returns {Array<Object>}
*/
function getCategoryOptionTree(options, isOneLine = false) {
const optionCollection = {};
const optionCollection = new Map();

_.each(options, (option) => {
if (!option.enabled) {
return;
}

if (isOneLine) {
if (_.has(optionCollection, option.name)) {
if (optionCollection.has(option.name)) {
return;
}

optionCollection[option.name] = {
optionCollection.set(option.name, {
text: option.name,
keyForList: option.name,
searchText: option.name,
tooltipText: option.name,
isDisabled: !option.enabled,
};
});

return;
}

option.name.split(CONST.PARENT_CHILD_SEPARATOR).forEach((optionName, index, array) => {
const indents = _.times(index, () => CONST.INDENTS).join('');
const isChild = array.length - 1 === index;
const searchText = array.slice(0, index + 1).join(CONST.PARENT_CHILD_SEPARATOR);

if (_.has(optionCollection, optionName)) {
if (optionCollection.has(searchText)) {
return;
}

optionCollection[optionName] = {
optionCollection.set(searchText, {
text: `${indents}${optionName}`,
keyForList: optionName,
searchText: array.slice(0, index + 1).join(CONST.PARENT_CHILD_SEPARATOR),
keyForList: searchText,
searchText,
tooltipText: optionName,
isDisabled: isChild ? !option.enabled : true,
};
});
});
});

return _.values(optionCollection);
return Array.from(optionCollection.values());
}

/**
* Build the section list for categories
* Builds the section list for categories
*
* @param {Object<String, {name: String, enabled: Boolean}>} categories
* @param {String[]} recentlyUsedCategories
Expand All @@ -729,13 +807,12 @@ function getCategoryOptionTree(options, isOneLine = false) {
* @returns {Array<Object>}
*/
function getCategoryListSections(categories, recentlyUsedCategories, selectedOptions, searchInputValue, maxRecentReportsToShow) {
const sortedCategories = sortCategories(categories);
const enabledCategories = _.filter(sortedCategories, (category) => category.enabled);

const categorySections = [];
const categoriesValues = _.chain(categories)
.values()
.filter((category) => category.enabled)
.value();
const numberOfCategories = _.size(enabledCategories);

const numberOfCategories = _.size(categoriesValues);
let indexOffset = 0;

if (numberOfCategories === 0 && selectedOptions.length > 0) {
Expand All @@ -751,7 +828,7 @@ function getCategoryListSections(categories, recentlyUsedCategories, selectedOpt
}

if (!_.isEmpty(searchInputValue)) {
const searchCategories = _.filter(categoriesValues, (category) => category.name.toLowerCase().includes(searchInputValue.toLowerCase()));
const searchCategories = _.filter(enabledCategories, (category) => category.name.toLowerCase().includes(searchInputValue.toLowerCase()));

categorySections.push({
// "Search" section
Expand All @@ -770,22 +847,12 @@ function getCategoryListSections(categories, recentlyUsedCategories, selectedOpt
title: '',
shouldShow: false,
indexOffset,
data: getCategoryOptionTree(categoriesValues),
data: getCategoryOptionTree(enabledCategories),
});

return categorySections;
}

const selectedOptionNames = _.map(selectedOptions, (selectedOption) => selectedOption.name);
const filteredRecentlyUsedCategories = _.map(
_.filter(recentlyUsedCategories, (category) => !_.includes(selectedOptionNames, category)),
(category) => ({
name: category,
enabled: lodashGet(categories, `${category}.enabled`, false),
}),
);
const filteredCategories = _.filter(categoriesValues, (category) => !_.includes(selectedOptionNames, category.name));

if (!_.isEmpty(selectedOptions)) {
categorySections.push({
// "Selected" section
Expand All @@ -798,6 +865,15 @@ function getCategoryListSections(categories, recentlyUsedCategories, selectedOpt
indexOffset += selectedOptions.length;
}

const selectedOptionNames = _.map(selectedOptions, (selectedOption) => selectedOption.name);
const filteredRecentlyUsedCategories = _.chain(recentlyUsedCategories)
.filter((categoryName) => !_.includes(selectedOptionNames, categoryName) && lodashGet(categories, [categoryName, 'enabled'], false))
.map((categoryName) => ({
name: categoryName,
enabled: lodashGet(categories, [categoryName, 'enabled'], false),
}))
.value();

if (!_.isEmpty(filteredRecentlyUsedCategories)) {
const cutRecentlyUsedCategories = filteredRecentlyUsedCategories.slice(0, maxRecentReportsToShow);

Expand All @@ -812,6 +888,8 @@ function getCategoryListSections(categories, recentlyUsedCategories, selectedOpt
indexOffset += filteredRecentlyUsedCategories.length;
}

const filteredCategories = _.filter(enabledCategories, (category) => !_.includes(selectedOptionNames, category.name));

categorySections.push({
// "All" section when items amount more than the threshold
title: Localize.translateLocal('common.all'),
Expand Down Expand Up @@ -1668,6 +1746,7 @@ export {
getLastMessageTextForReport,
getEnabledCategoriesCount,
hasEnabledOptions,
sortCategories,
getCategoryOptionTree,
formatMemberForList,
formatSectionsFromSearchTerm,
Expand Down
Loading