-
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-17192] show related stages widget on registration page #3880
base: master
Are you sure you want to change the base?
Conversation
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.
Good work!
'../../common/TEIRelationshipsWidget/RegisterTei/DataEntry/TrackedEntityInstance/dataEntryTrackedEntityInstance.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.
Can TeiPayload
be exported through the existing index file in the folder?
errorMessages, | ||
saveAttempted, | ||
actionTypesOptions, |
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: Not code implemented in this PR, but is it necessary to export the plain function?
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.
No, there is no need to export this function. I fixed it now. Good catch!
@@ -7,7 +7,7 @@ import { handleAPIResponse, REQUESTED_ENTITIES } from '../../../utils/api'; | |||
|
|||
type Props = { | |||
stageId: ?string, | |||
enrollmentId: string, | |||
enrollmentId?: string, | |||
scheduledLabel: 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.
The property is always defined, but the value can be undefined.
scheduledLabel: string, | |
enrollmentId: ?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.
Looks really solid overall @simonadomnisoru! I have a few suggestions for adjustments here and there.
.../Enrollment/EnrollmentWithFirstStageDataEntry/EnrollmentWithFirstStageDataEntry.container.js
Outdated
Show resolved
Hide resolved
programStageIdLinkedEventToRedirectTo: | ||
relatedStageLinkedEvent && linkMode === RelatedStageModes.ENTER_DATA | ||
? relatedStageLinkedEvent.programStage | ||
: undefined, |
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 this is a good time to simplify how we determine what event to redirect to. My suggestion would be that we in this function (buildTeiWithEnrollment) get the eventId for the event. For related stages we are generating the eventId on the client, we will have to do this for auto generated events too. Then we should be able to write the logic to determine the eventId (that we will redirect to). Having the related stage take precedence makes sense to me (like you already did).
This way we can simplify the epics in the chain quite a bit and the code would be easier to reason about.
Might be missing something of course. Happy to talk about 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.
To elaborate: On line 159 in useBuildEnrollmentPayload
add something like: const redirectEvent = getEventToOpen(.......);
.
- Check for
ENTER_DATA
for related stages. - Check for "Open data entry form after enrollment" on the stages. If a stage is found, Get an event for that stage among the auto-generated ones (and probably the "first stage appears on registration page" one).
Let's discuss this one.
...onents/DataEntries/converters/getConvertedRelatedStageEvent/getConvertedRelatedStageEvent.js
Outdated
Show resolved
Hide resolved
...e-core/components/DataEntries/EnrollmentRegistrationEntry/hooks/useBuildEnrollmentPayload.js
Outdated
Show resolved
Hide resolved
...RegistrationEntry/DataEntryWrapper/DataEntry/epics/addRelationshipForNewSingleEvent.epics.js
Show resolved
Hide resolved
@JoakimSM the suggestions have been implemented. Can you have another look? Thanks! |
DHIS2-17192
Tech summary:
from
part of a related stages relationship type.