-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: [DHIS2-12362][DHIS2-12455] View and filter relationships #3241
feat: [DHIS2-12362][DHIS2-12455] View and filter relationships #3241
Conversation
…ationship/DHIS2-12455_ListApplicableRelationshipTypes # Conflicts: # i18n/en.pot # src/core_modules/capture-core/components/Pages/Enrollment/EnrollmentPageDefault/EnrollmentPageDefault.container.js
…ationship/DHIS2-12455_ListApplicableRelationshipTypes # Conflicts: # i18n/en.pot
# Conflicts: # i18n/en.pot # src/core_modules/capture-core/components/Pages/Enrollment/EnrollmentPageDefault/EnrollmentPageDefault.component.js # src/core_modules/capture-core/components/Pages/Enrollment/EnrollmentPageDefault/EnrollmentPageDefault.types.js # src/core_modules/capture-core/components/Pages/EnrollmentEditEvent/EnrollmentEditEventPage.container.js
<Route path="/" component={MainPage} /> | ||
</Switch> | ||
<> | ||
<ReactQueryDevtools /> |
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.
Not sure if or where we want this, bit helps a lot during review and development to see the caching strategy.
import { useCallback, useEffect, useState } from 'react'; | ||
import log from 'loglevel'; | ||
import moment from 'moment'; | ||
import { errorCreator } from 'capture-core-utils'; |
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.
Can we use deps from our internal repo?
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.
errorCreator
is already used by other self-contained widgets. It looks like an easy function to re-rewrite, so maybe we can move it inside each widget?
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.
This is fine @eirikhaugstulen. Helpers from capture-core-utils
will have to be available to Widget library is some way.
}, 'organisationUnitRoots'); | ||
}, 'organisationUnitRoots2222'); |
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.
Was this introduced by something you thought of @JoakimSM? Do we keep it?
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.
This looks like a typo to me
staleTime: 4 * 60 * 1000, | ||
cacheTime: 10 * 60 * 1000, |
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 do we think is a sensible amount here?
cacheTime: Infinity, | ||
staleTime: Infinity, |
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.
When do we want to refetch metadata from the API?
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 think Infinity
is fine.
}, 'organisationUnitRoots'); | ||
}, 'organisationUnitRoots2222'); |
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.
This looks like a typo to me
...ore/components/WidgetsRelationship/common/AddNewRelationship/AddNewRelationship.component.js
Outdated
Show resolved
Hide resolved
<Breadcrumbs | ||
currentStep={currentStep} | ||
onNavigate={handleNavigation} | ||
linkedEntityMetadataName={selectedLinkedEntityMetadata?.name} |
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.
export type Props = {| | ||
currentStep: $Values<typeof NEW_TRACKED_ENTITY_RELATIONSHIP_WIZARD_STEPS>, | ||
onNavigate: ($Values<typeof NEW_TRACKED_ENTITY_RELATIONSHIP_WIZARD_STEPS>) => void, | ||
linkedEntityMetadataName?: 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.
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.
Hey @simonadomnisoru,
Yes, completely agree that we should have a fallback here. The reason it's null or undefined is just because of Flow I think as it could be undefined if currentStep === 0. Just to clarify, I'm guessing that the image you provided is a bug and that we should fallback to displayName
here. Therefore, it would never actually be undefined. One fallback we could add however is to not show anything after New TEI Relationship
if the name is undefined. Then it's no longer a breadcrumb, but a label of the widget. WDYT?
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.
Agreed, fallback to displayName in useApplicableTypesAndSides
(Probably empty because toFromName is not required in the metadata)
...tTrackedEntityRelationship/NewTrackedEntityRelationship/Breadcrumbs/Breadcrumbs.component.js
Outdated
Show resolved
Hide resolved
import { useCallback, useEffect, useState } from 'react'; | ||
import log from 'loglevel'; | ||
import moment from 'moment'; | ||
import { errorCreator } from 'capture-core-utils'; |
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.
errorCreator
is already used by other self-contained widgets. It looks like an easy function to re-rewrite, so maybe we can move it inside each widget?
@@ -0,0 +1,47 @@ | |||
// @flow |
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.
This file is used from both EnrollmentPageDefault
and EnrollmentEditEvent
, but resides within EnrollmentPageDefault
. Great if we can move it to Pages/common/TEIRelationshipsWidget
addRelationshipRenderElement: HTMLDivElement, | ||
onOpenAddRelationship: () => void, | ||
onCloseAddRelationship: () => void, | ||
onLinkedRecordClick: (parameters: Url) => void, |
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.
Can we use UrlParameters
from capture-core/components/WidgetsRelationship/common/Types/RelationshipData.types.js
as the type (instead of Url
). This will fix a flow error in TrackedEntityRelationshipsWrapper.component
(that you have suppressed)
const onGoBack = () => | ||
history.push(`/enrollment?${buildUrlQueryString({ enrollmentId })}`); | ||
|
||
const onHandleScheduleSave = (eventData: Object) => { | ||
dispatch(updateEnrollmentEvents(eventId, eventData)); | ||
history.push(`enrollment?${buildUrlQueryString({ enrollmentId })}`); | ||
}; | ||
const enrollmentSite = useCommonEnrollmentDomainData(teiId, enrollmentId, programId).enrollment; | ||
const onLinkedRecordClick = (parameters) => { | ||
dispatch(clickLinkedRecord(parameters)); |
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.
clickLinkedRecord
should have been moved to TEIRelationshipsWidget
, but I'm wondering if we really need an action and an epic here. Can we simply create a reusable function / hook and place that in TEIRelationshipsWidget
? (I'm aware you didn't write this, but great if we can fix it)
export type Props = {| | ||
currentStep: $Values<typeof NEW_TRACKED_ENTITY_RELATIONSHIP_WIZARD_STEPS>, | ||
onNavigate: ($Values<typeof NEW_TRACKED_ENTITY_RELATIONSHIP_WIZARD_STEPS>) => void, | ||
linkedEntityMetadataName?: 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.
Agreed, fallback to displayName in useApplicableTypesAndSides
(Probably empty because toFromName is not required in the metadata)
@@ -0,0 +1,170 @@ | |||
// @flow |
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.
Did you make major changes in this file? (Blame says Eirik on most of the lines, but probably because of formatting?)
params: { | ||
fields: 'id,displayName,valueType', | ||
filter: `id:in:[${Object.keys(attributeIds).join(',')}]`, | ||
paging: false, |
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'm wondering if we should enable paging here. In this particular case we will probably not get a performance improvement, but I think it is/was best practice to keep the url length < 2000 characters (and there is a limit in the browsers). Can you look into how to enable paging in a generic way (i.e. use useMetadataApiQuery, but have it to the paging for us. In this particular instance we should probably wait for all the pages to be retrieved before emitting the result, but this is not always the case).
resource: 'trackedEntityAttributes', | ||
params: { | ||
fields: 'id,displayName,valueType', | ||
filter: `id:in:[${Object.keys(attributeIds).join(',')}]`, |
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.
if no attributes are found, we should avoid the api request. Might be best to compute the query later?
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.
Not sure I completely understood what you meant here, @JoakimSM. Let's have a chat the next time we speak.
@@ -0,0 +1,31 @@ | |||
// @flow | |||
import { useMemo } from 'react'; | |||
import { useApiDataQuery } from '../../../../utils/reactQueryHelpers/query/useApiDataQuery'; |
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.
NIT 1: maybe we can simplify the url here like we do for useMetadataApiQuery
?
NIT 2: I agree useApiDataQuery
is better than useDataApiQuery
. Can you change useMetadataApiQuery
(and useMetadataCustomQuery
) to use the same "format" (useApiMetadataQuery
etc)?
import { useCallback, useEffect, useState } from 'react'; | ||
import log from 'loglevel'; | ||
import moment from 'moment'; | ||
import { errorCreator } from 'capture-core-utils'; |
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.
This is fine @eirikhaugstulen. Helpers from capture-core-utils
will have to be available to Widget library is some way.
cacheTime: Infinity, | ||
staleTime: Infinity, |
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 think Infinity
is fine.
…_ViewRelationships # Conflicts: # i18n/en.pot
Thanks for the reviews, @simonadomnisoru & @JoakimSM. Should be updated now 👍 |
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.
Hey @eirikhaugstulen,
I get TypeError: trackedEntityType.name is undefined
when clicking the New Relationship
button. Can you have a look?
Screen.Recording.2023-05-31.at.09.09.03.mov
Thanks!
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.
Should the folder Types
name start with lowercase instead?
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.
Does it make sense to move this file, as well as the rest of the *.types.js
from this folder to the RelationshipsWidget/types
folder? Thank you!
…_ViewRelationships # Conflicts: # i18n/en.pot # src/core_modules/capture-core/components/Pages/EnrollmentEditEvent/EnrollmentEditEventPage.component.js # src/core_modules/capture-core/components/Pages/EnrollmentEditEvent/EnrollmentEditEventPage.container.js
🚀 Deployed on https://deploy-preview-3241--dhis2-capture.netlify.app |
This PR builds on the work of @jasminenguyennn and @JoakimSM.
Add a TrackedEntity relationships widget in the enrollment pages.
TL;DR: