-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(Parameter) - HEXA-1136 dhis2 parameter validation #1102
base: main
Are you sure you want to change the base?
Conversation
nazarfil
commented
Feb 18, 2025
•
edited
Loading
edited
- implements a generic metadata widget
- supports parameters with multiple or not
- creates a dropdown with combobox or multicombobox to select items from
- adds on scroll more fetch from backend
- consumes DHIS2 api from backend
- supports metadata with id,name or name,level or just level



8b25c63
to
aa6162e
Compare
8c37bf5
to
7175276
Compare
32c216d
to
2c60b0a
Compare
164908a
to
4606db7
Compare
4606db7
to
e981e80
Compare
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.
Great, we can discuss my comments offline if they are not clear/if that helps
type GenericConnectionWidgetProps<T> = { | ||
parameter: any; | ||
form: any; | ||
workspaceSlug: string; | ||
}; |
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.
T
is not used in this Props, so we either need to remove <T>
or use it somewhere.
Also, using any
is too generic, I guess it should be possible to restrain a bit the possible types
const GenericConnectionWidget = <T,>({ | ||
parameter, | ||
form, | ||
workspaceSlug, | ||
}: GenericConnectionWidgetProps<T>) => { | ||
if (parameter.widget in dhis2WidgetToQuery) { | ||
return ( | ||
<DHIS2Widget | ||
parameter={parameter} | ||
form={form} | ||
workspaceSlug={workspaceSlug} | ||
/> | ||
); | ||
} | ||
}; |
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.
What's the goal of this layer ? I don't think it brings much at this point and it hides types
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.
To remove the complexity of making the decision on what widget to use from the ParameterField, and then extend it with IASOWidget, etc ..
type DHIS2WidgetProps<T> = { | ||
parameter: any; | ||
form: any; | ||
workspaceSlug: string; | ||
}; |
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.
T
is not used in this Props, so we either need to remove <T>
or use it somewhere.
Also, using any
is too generic, I guess it should be possible to restrain a bit the possible types
const [query, setQuery] = useState(""); | ||
const debouncedQuery = useDebounce(query, 150); | ||
const [perPage, setPerPage] = useState(10); | ||
const [page, setPage] = useState(1); |
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.
setPage
is never used, so it means page will never change
|
||
const options = useMemo(() => { | ||
if (error) { | ||
console.error("Error fetching connection metadata:", error); |
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.
We might want to show an error to the user, not only the console
items?.filter((c) => { | ||
if (c.__typename === "DHIS2MetadataItem") { | ||
return c.name?.toLowerCase().includes(debouncedQuery.toLowerCase()); | ||
} else if (c.__typename === "DHIS2OrganisationUnitLevel") { | ||
return c.name?.toLowerCase().includes(debouncedQuery.toLowerCase()); | ||
} | ||
}); |
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.
[Minor] to avoid duplicate logic
items?.filter((c) => { | |
if (c.__typename === "DHIS2MetadataItem") { | |
return c.name?.toLowerCase().includes(debouncedQuery.toLowerCase()); | |
} else if (c.__typename === "DHIS2OrganisationUnitLevel") { | |
return c.name?.toLowerCase().includes(debouncedQuery.toLowerCase()); | |
} | |
}); | |
items.filter((c) => | |
["DHIS2MetadataItem", "DHIS2OrganisationUnitLevel"].includes(c.__typename) && | |
c.name?.toLowerCase().includes(debouncedQuery.toLowerCase()) | |
); |
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.
Also I am surprised that we have to perform filtering in the Frontend and would expect the Frontend to ask the Backend the data using parameters and get clean data in return
const displayValueHandler = (value: any) => { | ||
if (!value) return ""; | ||
|
||
if (Array.isArray(value)) { | ||
const displayedNames = value | ||
.map((item) => { | ||
if (typeof item === "object" && item !== null) { | ||
if (item.__typename === "DHIS2OrganisationUnitLevel") { | ||
return item.name ?? `${item.level}`; | ||
} | ||
} | ||
|
||
const foundItem = options.items.find((opt) => { | ||
if (opt.__typename === "DHIS2OrganisationUnitLevel") { | ||
return opt.level === item; | ||
} else { | ||
return opt.id === item; | ||
} | ||
}); | ||
if (foundItem?.__typename === "DHIS2OrganisationUnitLevel") { | ||
return foundItem.name ?? `${foundItem.level}`; | ||
} else { | ||
return foundItem?.name ?? t("Unknown ID: {{id}}", { id: item }); | ||
} | ||
}) | ||
.filter(Boolean); | ||
return displayedNames.join(", "); | ||
} | ||
|
||
if (typeof value === "object" && value !== null) { | ||
if (value.__typename === "DHIS2OrganisationUnitLevel") { | ||
return value.name ?? `${value.level}`; | ||
} | ||
|
||
return ( | ||
value.name ?? | ||
(value.level | ||
? `${value.level}` | ||
: t("Unknown ID: {{id}}", { id: value.id })) | ||
); | ||
} | ||
|
||
const selectedItem = options.items.find((item) => { | ||
if (item.__typename === "DHIS2OrganisationUnitLevel") { | ||
return item.level === value; | ||
} else { | ||
return item.id === value; | ||
} | ||
}); | ||
if (selectedItem?.__typename === "DHIS2OrganisationUnitLevel") { | ||
return selectedItem.name ?? `${selectedItem.level}`; | ||
} else { | ||
return selectedItem?.name ?? t("Unknown ID: {{id}}", { id: value }); | ||
} | ||
}; |
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.
I have the feeling that this 50-lines method is overly complex.
I used a LLM to ask for a simpler version, maybe it's a clearer approach ?
Also I am wondering why a clean label can be provided from the backend and that the frontend need to handle all that logic ?
const displayValueHandler = (value: any) => { | |
if (!value) return ""; | |
if (Array.isArray(value)) { | |
const displayedNames = value | |
.map((item) => { | |
if (typeof item === "object" && item !== null) { | |
if (item.__typename === "DHIS2OrganisationUnitLevel") { | |
return item.name ?? `${item.level}`; | |
} | |
} | |
const foundItem = options.items.find((opt) => { | |
if (opt.__typename === "DHIS2OrganisationUnitLevel") { | |
return opt.level === item; | |
} else { | |
return opt.id === item; | |
} | |
}); | |
if (foundItem?.__typename === "DHIS2OrganisationUnitLevel") { | |
return foundItem.name ?? `${foundItem.level}`; | |
} else { | |
return foundItem?.name ?? t("Unknown ID: {{id}}", { id: item }); | |
} | |
}) | |
.filter(Boolean); | |
return displayedNames.join(", "); | |
} | |
if (typeof value === "object" && value !== null) { | |
if (value.__typename === "DHIS2OrganisationUnitLevel") { | |
return value.name ?? `${value.level}`; | |
} | |
return ( | |
value.name ?? | |
(value.level | |
? `${value.level}` | |
: t("Unknown ID: {{id}}", { id: value.id })) | |
); | |
} | |
const selectedItem = options.items.find((item) => { | |
if (item.__typename === "DHIS2OrganisationUnitLevel") { | |
return item.level === value; | |
} else { | |
return item.id === value; | |
} | |
}); | |
if (selectedItem?.__typename === "DHIS2OrganisationUnitLevel") { | |
return selectedItem.name ?? `${selectedItem.level}`; | |
} else { | |
return selectedItem?.name ?? t("Unknown ID: {{id}}", { id: value }); | |
} | |
}; | |
const displayValueHandler = (value: any) => { | |
if (!value) return ""; | |
const getDisplayName = (item: any) => { | |
if (typeof item === "object" && item !== null) { | |
return item.__typename === "DHIS2OrganisationUnitLevel" ? item.name ?? `${item.level}` : item.name ?? t("Unknown ID: {{id}}", { id: item.id }); | |
} | |
const foundItem = options.items.find((opt) => | |
opt.__typename === "DHIS2OrganisationUnitLevel" ? opt.level === item : opt.id === item | |
); | |
return foundItem?.__typename === "DHIS2OrganisationUnitLevel" ? foundItem.name ?? `${foundItem.level}` : foundItem?.name ?? t("Unknown ID: {{id}}", { id: item }); | |
}; | |
return Array.isArray(value) ? value.map(getDisplayName).filter(Boolean).join(", "): getDisplayName(value); | |
}; |
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.
thanks, it does look cleaner, i will test it.
Levels with dhis2 is super unpredictable, it might have a name, and it might have an id. It alway has level.
It is more user friendly to display the name of the level, and easier to filter by id.
But in both cases level is a fallback. To display level and filter by level.
Solution could be to only use level, but will need to confirm it with implementation and Yann
const selectedIds = Array.isArray(selectedValue) | ||
? selectedValue.map((item) => item.id ?? item.level).filter(Boolean) | ||
: []; |
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.
On reading, I feel like there is a logic issue : if the parameter is multiple and the selected value is not an array, we ignore the selectedValue and return an empty array. Ignoring selection seems odd right ?
expect(pipeline.form.setFieldValue).toHaveBeenCalledWith("test_param", 3); | ||
}); | ||
}); | ||
}); |
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.
Nice tests !
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.
Thanks, I have tried as well to test the error but couldn't easily wrap and catch it, some for scrolling didn't figure out how to make the test work with related event.