Skip to content
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-15480] widget assignee #3412

Merged
merged 29 commits into from
Jan 25, 2024
Merged

feat: [DHIS2-15480] widget assignee #3412

merged 29 commits into from
Jan 25, 2024

Conversation

simonadomnisoru
Copy link
Contributor

@simonadomnisoru simonadomnisoru commented Sep 13, 2023

DHIS2-15480

Tech summary

  • Created WidgetAssignee and used it the view and edit pages in the tracker program
  • Posting to the API is done from within the WidgetAssignee using the app runtime hooks
  • getSaveContext callback is in charge of getting all the current event information.
  • The Redux store is updated on the onSave and OnSaveError callbacks
  • Reused the WidgetAssignee in event programs

@simonadomnisoru simonadomnisoru marked this pull request as ready for review September 13, 2023 13:06
@simonadomnisoru simonadomnisoru requested a review from a team as a code owner September 13, 2023 13:06
@github-actions
Copy link

github-actions bot commented Oct 11, 2023

Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that I didn't write anything about self-contained, but would be great if we can make it a bit more contained and not use Redux in the Widget itself.

Widget interface suggestion

assignedUser?: { id: string, name: string },
writeAccess: boolean,
getSaveContext: () => Object,
onSave: (assignee) => void,
onSaveError: (prevAssignee) => void,

We can update the Redux store from the onSave and OnSaveError callbacks. Let's use mutate from app-runtime when saving (don't worry about offline support for now)

  1. Let's only send the current event and not other events in the enrollment to the api (if you have multiple events in the same enrollment, they are all sent to the api when updating the assigned user it seems). But fixing the first point might also fix this one.

  2. Make sure that after we update the event itself, the assigned user is still persisted/shown in the Widget (I saw some weird behaviour here, but couldn't instantly figure out what was going on. It also seemed to differ between v39 and v40)

I'm open for a chat about this!

@simonadomnisoru
Copy link
Contributor Author

I see that I didn't write anything about self-contained, but would be great if we can make it a bit more contained and not use Redux in the Widget itself.

Widget interface suggestion

assignedUser?: { id: string, name: string },
writeAccess: boolean,
getSaveContext: () => Object,
onSave: (assignee) => void,
onSaveError: (prevAssignee) => void,

We can update the Redux store from the onSave and OnSaveError callbacks. Let's use mutate from app-runtime when saving (don't worry about offline support for now)

  1. Let's only send the current event and not other events in the enrollment to the api (if you have multiple events in the same enrollment, they are all sent to the api when updating the assigned user it seems). But fixing the first point might also fix this one.
  2. Make sure that after we update the event itself, the assigned user is still persisted/shown in the Widget (I saw some weird behaviour here, but couldn't instantly figure out what was going on. It also seemed to differ between v39 and v40)

Hi @JoakimSM,
I implemented the suggested architecture. You can take a look?
Thanks!


export type Props = {|
assignee: UserFormField | null,
programStage: ?ProgramStage,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just pass in enabled instead of passing in the entire programStage? Easier to resume work here later

Comment on lines 16 to 18
eventAccess: {|
read: boolean,
write: boolean,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In its current state I think it would be great if we don't pass in read-access. Can we just replace this with writeAccess?

onClose={useCallback(() => setOpenStatus(false), [setOpenStatus])}
open={open}
>
<div className={classes.wrapper}>{eventAccess?.write ? renderContent() : renderNoAccess()}</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should show the assignee even if the user doesn't have write access. In read-only mode we should disable the edit button.

I also just realised we don't have any way of removing the assignment altogether. We will have to look into a way of doing this. We might just add an "x"-button for now (beside the edit button), but feel free to ask the designers for a proper design.

@simonadomnisoru
Copy link
Contributor Author

Hi @JoakimSM,
I implemented the suggested changes. Let me know if you find any other issues.
Thanks!

@eirikhaugstulen
Copy link
Contributor

Hey @simonadomnisoru,
I think this PR looks very good now!
One quick comment:
It looks like we are sending trackedEntityInstance as the payload when editing enrollment event assignee. Do you think this should be replaced with trackedEntity? I see that it's referenced quite a lot of times in the types as well, not sure if that's intentional. We also pass in followUp twice (nit).

image

Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last code changes looks good and the widget is looking and functioning satisfactory, well done 🎉

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully on 2.41,2.40.3,2.39.5 ,2.38.7 versions

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully on 2.41,2.40.3,2.39.5 ,2.38.7 versions

@simonadomnisoru simonadomnisoru merged commit d61efae into master Jan 25, 2024
36 of 37 checks passed
@simonadomnisoru simonadomnisoru deleted the DHIS2-15480 branch January 25, 2024 11:45
dhis2-bot added a commit that referenced this pull request Jan 25, 2024
# [100.51.0](v100.50.7...v100.51.0) (2024-01-25)

### Features

* [DHIS2-15480] widget assignee ([#3412](#3412)) ([d61efae](d61efae))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.51.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants