-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Changes from 21 commits
7f3e6c8
bc9b952
7ac0ec5
99da058
2f3c120
10a5788
c21dc53
638837e
44b45a5
ad432b1
443cc02
1c447e2
af7007d
9fa9bca
21a25fa
e297e5f
0f8cfb5
e70e40b
8269bab
354b515
8a31777
87c35b8
015cc04
411db80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -662,7 +663,94 @@ 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 = _.values(categories).sort((a, b) => { | ||
if (a.name < b.name) { | ||
return -1; | ||
} | ||
|
||
if (a.name > b.name) { | ||
return 1; | ||
} | ||
|
||
return 0; | ||
}); | ||
|
||
// 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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I use the reduction to covert the
So, I don't see a variant how to combine these 2 different functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just meant that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm... Could you please share an example? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -671,52 +759,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 | ||
|
@@ -727,13 +812,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) { | ||
|
@@ -749,7 +833,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 | ||
|
@@ -768,22 +852,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 | ||
|
@@ -796,6 +870,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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe let's use array-based There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I understand what you want here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's just a follow-up to our earlier conclusions about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, okay. |
||
.map((categoryName) => ({ | ||
name: categoryName, | ||
enabled: lodashGet(categories, `${categoryName}.enabled`, false), | ||
})) | ||
.value(); | ||
|
||
if (!_.isEmpty(filteredRecentlyUsedCategories)) { | ||
const cutRecentlyUsedCategories = filteredRecentlyUsedCategories.slice(0, maxRecentReportsToShow); | ||
|
||
|
@@ -810,6 +893,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'), | ||
|
@@ -1666,6 +1751,7 @@ export { | |
getLastMessageTextForReport, | ||
getEnabledCategoriesCount, | ||
hasEnabledOptions, | ||
sortCategories, | ||
getCategoryOptionTree, | ||
formatMemberForList, | ||
formatSectionsFromSearchTerm, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://lodash.com/docs/4.17.15#sortBy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - 015cc04.