Conversation
…s ONLY one attrOptionCombo
…; Filter data set by the selected orgunit
…sage "Choose a data set to review even no any data set exists"
…ts descending orgunits
| * @param onClose A function to close the menu. | ||
| * | ||
| */ | ||
| export default function CategoySelect({ |
There was a problem hiding this comment.
| export default function CategoySelect({ | |
| export default function CategorySelect({ |
| if (categories.length === 1 ) { | ||
| // Extracts the single category from the categories array | ||
| const category = categories[0] | ||
| if( categories[0].categoryOptions?.length === 0 ){ |
There was a problem hiding this comment.
| if( categories[0].categoryOptions?.length === 0 ){ | |
| if( category?.categoryOptions?.length === 0 ){ |
| * @param onClose A function to close the menu. | ||
| * | ||
| */ | ||
| export default function MultipleCategoySelect({ |
There was a problem hiding this comment.
| export default function MultipleCategoySelect({ | |
| export default function MultipleCategorySelect({ |
| useEffect(() => { | ||
|
|
||
| }, [selected]) |
There was a problem hiding this comment.
Yes, correct.
I removed it.
| openedSelect, | ||
| setOpenedSelect, | ||
| } = useSelectionContext() | ||
| const { searchText, orgUnits, loading, setSearchText } = useOrgUnitSearch(); |
There was a problem hiding this comment.
I would prefer this to be its own PR. As well as the search in workflows. I think that is scope-creep, and this should only include the catcombo logic...
There was a problem hiding this comment.
"I would prefer this to be its own PR" ==> The orgUnit search logic (useOrgUnitSearch, searchText, setSearchText, etc.) should go into a separate PR by itself..
"As well as the search in workflows." ==> The search functionality added to workflows should also be in its own PR, not bundled together with others.
" I think that is scope-creep, and this should only include the catcombo logic..." --> The PR should only include the category combo (catcombo) logic and nothing else.
It seems I included too many features in a single PR, instead of focusing on one feature or idea per PR.
I will take this into consideration when I do future PR.
This one is already happened, and we cannot do anything about it, right ?
| return <OrganisationUnitTree | ||
| roots={orgUnits.map(({ id }) => id)} | ||
| onChange={onChange} | ||
| key={`${searchText}-${new Date().getTime()}`} |
There was a problem hiding this comment.
Why do you need getTime here? This would reset the state of the tree every time it re-renders?
There was a problem hiding this comment.
The first time the OrgUnitTree component is mounted, the useRootOrgData hook correctly fetches the organization unit data based on the provided rootId, and the component renders as expected:
const {
loading,
error,
data,
refetch
} = useRootOrgData(rootIds, {
isUserDataViewFallback,
suppressAlphabeticalSorting
}); // From the OrganisationUnitTree component
However, on subsequent renders — such as when performing a new search — the component receives a new rootId, but the hook does not trigger a re-fetch. Instead, it relies on cached data, which may not yet include the requested rootId.
As a result, this line:
const rootNode = data[rootId]; // Extract from the component OrganisationUnitTree.
evaluates to undefined, and trying to access rootNode.displayName causes a runtime error: "Cannot read properties of undefined (reading 'displayName')"
To fixed the issue, I used dynamic key={${searchText}-${new Date().getTime()}} which in turn triggers a fresh call to useRootOrgData. This avoids using potentially incomplete cached data and ensures the correct root node is available before render.
Let me know if this approach makes sense, or if there's a better way to safely handle missing rootId data in the component itself.
For this issue was fixed in GitHub,
Earlier, I had added a timestamp to the component’s key prop (key={${searchText}-${Date.now()}}) to solve a specific issue.
However, I forgot the reason I introduced the timestamp and recently removed it, thinking it was unnecessary. After removing it, I now realize that this could reintroduce the original bug.
Since I already made several commits after that change, instead of reverting history, I’ve just re-applied the timestamp logic and committed it again as a fix.
| export const getCategoryCombosByFilters = (metadata, workflow, orgUnit, period, calendar) => { | ||
| if(workflow == null || orgUnit == null || period == null ) return []; | ||
|
|
||
| const categoryComboList = cloneJSON(extractCategoryCombosByWorkflow(metadata, workflow)) |
There was a problem hiding this comment.
What's the reason for cloneJSON here? extractCategoryCombosByWorkflow returns a new array everytime?
There was a problem hiding this comment.
I'm cloning the categoryComboList to avoid directly modifying the original data when filtering category options based on the selected org unit and period.
Since metadata is loaded once at app initialization and shared through context, modifying it directly could lead to unexpected side effects across the app.
For example, when an org unit and period are selected, some category options may be filtered out. If we don’t clone the data, those filtered-out options would be permanently removed from the shared metadata.
By cloning, we preserve the original category combo configuration and ensure that filtering is safe and reversible.
src/utils/caterogy-combo-utils.js
Outdated
| import { areListsEqual, cloneJSON } from "./array-utils.js"; | ||
| import { isDateALessThanDateB } from "./date-utils.js"; | ||
|
|
||
| export const getCategoryCombosByFilters = (metadata, workflow, orgUnit, period, calendar) => { |
There was a problem hiding this comment.
We generally don't go above 3 parameters. So I would've preferred these to be in an object. Eg. (metadata, { workflow, orgUnit, period, calendar})This affects a lot of functions here.
There was a problem hiding this comment.
Thanks for the feedback!
I'll refactor the code to group the workflow, orgUnit, period, and calendar parameters into a single object for better readability and maintainability.
| @@ -0,0 +1,275 @@ | |||
| import { areListsEqual, cloneJSON } from "./array-utils.js"; | |||
There was a problem hiding this comment.
Typo in file name (caterogy-combo-utils
There was a problem hiding this comment.
I corrected typo in file name from 'caterogy-combo-utils' to 'category-combo-utils'
…ject for getCategoryCombosByFilters method
chauthutran
force-pushed
the
catcombo-approvals
branch
from
4d6e36e to
1f57a50
Compare
|
2 participants
No description provided.