(refactor) O3-4199: Bed management should use the modal system #1381

amosmachora · 2024-11-19T12:03:59Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

This PR refactors the current modal functionality in the bed management app to use the openmrs modal system as described here. When I was working on this PR I was careful only to refactor the modals, separate edit-add functionalities (beds, bed tags bed types), and not change any underlying implementation. Any change made can be considered absolutely necessary.

I also improved the UX when submitting the modals by adding loading states on the button and showing a spinner. Nothing major.

Screenshots

Short video verifying that nothing breaks after this change

bed-modals-refactor.mp4

Related Issue

https://openmrs.atlassian.net/browse/O3-4199

Other

ibacher

A couple of quick thoughts. While its a good idea to use the new modal system, I don't like the idea of having to maintain 6 forms instead of the previous 3.

ibacher · 2024-12-04T15:54:09Z

packages/esm-bed-management-app/src/bed-tag/edit-tag-form.modal.tsx

+  message: string;
+}
+
+const EditBedTagForm: React.FC<EditBedTagFormProps> = ({ editData, mutate, closeModal }) => {


I don't quite understand why we've broken things out this way instead of keeping a single, shared form used for both adding and editing a bed tag. This is way more work to maintain.

Ideally, we should maintain a single for adding/editing a tag.

ibacher · 2024-12-04T15:55:30Z

packages/esm-bed-management-app/src/bed-tag/edit-tag-form.modal.tsx

+  const [showErrorNotification, setShowErrorNotification] = useState(false);
+  const [formStateError, setFormStateError] = useState('');


It's weird to have these consts declared what is now mid-way down the file.

ibacher · 2024-12-04T15:55:57Z

packages/esm-bed-management-app/src/bed-tag/edit-tag-form.modal.tsx

+    },
+  });
+
+  const onSubmit = (formData: BedTagData) => {


Why is handleUpdateBedTag wrapped in useCallback() but this isn't?

ibacher · 2024-12-04T15:56:18Z

packages/esm-bed-management-app/src/bed-tag/edit-tag-form.modal.tsx

+
+  const onSubmit = (formData: BedTagData) => {
+    const result = BedTagAdministrationSchema.safeParse(formData);
+    if (result.success) {


Shouldn't something happen if the result is not a success?

ibacher · 2024-12-04T16:00:31Z

packages/esm-bed-management-app/src/bed-tag/edit-tag-form.modal.tsx

+  });
+
+  const onSubmit = (formData: BedTagData) => {
+    const result = BedTagAdministrationSchema.safeParse(formData);


Also, why are we parsing the form data here? What's the goal of this method?

The original code was written by @brandones in my understanding so I feel he is at a better position to answer this.

I don't think I wrote this; if I appear in the blame I probably moved the code in from somewhere else. Anyway it looks like this is outdated, so I will assume the question is resolved.

@amosmachora please take note

ibacher · 2024-12-04T16:02:50Z

packages/esm-bed-management-app/src/bed-tag/edit-tag-form.modal.tsx

+  };
+
+  const onError = (error: { [key: string]: ErrorType }) => {
+    setFormStateError(Object.entries(error)[0][1].message);


So we only support a single error? That seems wrong...

In principle, each field should display an error message related to that field and we only need a bigger message if there's an error... bigger than a field.

@ibacher the aim of this PR was to just separate out the forms. As I said in the PR description I consciously made the decision to leave everything as is to make it easier for review. If more changes like this are required I am more than happy to work on them but that might bloat the PR and make it more challenging to review.

Also as I was working on this I made the assumption that since I was working on top of already merged changes there is not much need to concentrate on the details.

Because of the way these changes are done, it's very hard for me to tell what's "new" in this PR and what's just "moved" from somewhere else.

I see. Think of it this way. the new tag form and the edit tag form both used an abstracted tag component. what I did was duplicate the tag component in both forms. So the new tag form know owns its own functionality that was previously in the tag component same as the edit one.

so nothing is new in short

ibacher · 2024-12-04T16:03:29Z

packages/esm-bed-management-app/src/bed-tag/edit-tag-form.modal.tsx

+                render={({ field, fieldState }) => (
+                  <>
+                    <TextInput
+                      id="bedTag"


Is id required here?

amosmachora · 2024-12-05T05:07:46Z

A couple of quick thoughts. While its a good idea to use the new modal system, I don't like the idea of having to maintain 6 forms instead of the previous 3.

Hello @denniskigen is at a better position to answer this question. Also the new 'architecture' is a personal preference of mine too as it allows for separation of concerns. I think it looks like more work from the onset but it simplifies work later IMO.

ibacher · 2024-12-05T15:35:19Z

I think it looks like more work from the onset but it simplifies work later IMO.

See, I see this in exactly the opposite way. Previously, we had one form to add or edit each of the three things (bed tags, etc.). Now, we have a separate form for each. That means if we decide that we need to, I don't know, collect a date the change should be effective on, we now need to add that field to both the add form and the edit form. With the previous setup, we just needed to modify the code in a single place.

While, in general, I'm in favour of breaking things up a bit, breaking things up in this way makes on-going maintenance harder, precisely because you need to know that there are two files that need to be updated for each change.

The objection here isn't necessarily to having a separate add-tag-form modal and edit-tag-form modal, but these should share a single component that actually renders the form.

amosmachora · 2024-12-05T16:26:15Z

I think it looks like more work from the onset but it simplifies work later IMO.

See, I see this in exactly the opposite way. Previously, we had one form to add or edit each of the three things (bed tags, etc.). Now, we have a separate form for each. That means if we decide that we need to, I don't know, collect a date the change should be effective on, we now need to add that field to both the add form and the edit form. With the previous setup, we just needed to modify the code in a single place.

While, in general, I'm in favour of breaking things up a bit, breaking things up in this way makes on-going maintenance harder, precisely because you need to know that there are two files that need to be updated for each change.

The objection here isn't necessarily to having a separate add-tag-form modal and edit-tag-form modal, but these should share a single component that actually renders the form.

Great points @ibacher this absolutely makes sense if you explain it that way. I guess its just a personal preference of mine and the ticket did leave room for some creativity. What do you suggest we do in that case then? should we just refactor the modals to use the modal system and leave everything as is?

amosmachora · 2024-12-05T17:23:13Z

I think it looks like more work from the onset but it simplifies work later IMO.

See, I see this in exactly the opposite way. Previously, we had one form to add or edit each of the three things (bed tags, etc.). Now, we have a separate form for each. That means if we decide that we need to, I don't know, collect a date the change should be effective on, we now need to add that field to both the add form and the edit form. With the previous setup, we just needed to modify the code in a single place.

While, in general, I'm in favour of breaking things up a bit, breaking things up in this way makes on-going maintenance harder, precisely because you need to know that there are two files that need to be updated for each change.

The objection here isn't necessarily to having a separate add-tag-form modal and edit-tag-form modal, but these should share a single component that actually renders the form.

While we are on the topic, just for fun, how about when you only need to refactor one functionality, say, the edit functionality? Isn`t there a chance that you might end up breaking two features instead of one or making an unnecessary change in the other while you only aimed at changing one thing?

ojwanganto · 2024-12-05T21:30:41Z

I think it looks like more work from the onset but it simplifies work later IMO.

See, I see this in exactly the opposite way. Previously, we had one form to add or edit each of the three things (bed tags, etc.). Now, we have a separate form for each. That means if we decide that we need to, I don't know, collect a date the change should be effective on, we now need to add that field to both the add form and the edit form. With the previous setup, we just needed to modify the code in a single place.
While, in general, I'm in favour of breaking things up a bit, breaking things up in this way makes on-going maintenance harder, precisely because you need to know that there are two files that need to be updated for each change.
The objection here isn't necessarily to having a separate add-tag-form modal and edit-tag-form modal, but these should share a single component that actually renders the form.

Great points @ibacher this absolutely makes sense if you explain it that way. I guess its just a personal preference of mine and the ticket did leave room for some creativity. What do you suggest we do in that case then? should we just refactor the modals to use the modal system and leave everything as is?

In addition to @ibacher's response, it is also a case of convention and standards which also guide code reviews. If you look at many forms in the system, the convention is to use a single form for add/edit and unit tests ensure the forms work as expected. I suggest that you only refactor the original code to use the modals. That will also align with the PR title for ease of reviews

amosmachora · 2024-12-06T03:22:09Z

I think it looks like more work from the onset but it simplifies work later IMO.

See, I see this in exactly the opposite way. Previously, we had one form to add or edit each of the three things (bed tags, etc.). Now, we have a separate form for each. That means if we decide that we need to, I don't know, collect a date the change should be effective on, we now need to add that field to both the add form and the edit form. With the previous setup, we just needed to modify the code in a single place.
While, in general, I'm in favour of breaking things up a bit, breaking things up in this way makes on-going maintenance harder, precisely because you need to know that there are two files that need to be updated for each change.
The objection here isn't necessarily to having a separate add-tag-form modal and edit-tag-form modal, but these should share a single component that actually renders the form.

Great points @ibacher this absolutely makes sense if you explain it that way. I guess its just a personal preference of mine and the ticket did leave room for some creativity. What do you suggest we do in that case then? should we just refactor the modals to use the modal system and leave everything as is?

In addition to @ibacher's response, it is also a case of convention and standards which also guide code reviews. If you look at many forms in the system, the convention is to use a single form for add/edit and unit tests ensure the forms work as expected. I suggest that you only refactor the original code to use the modals. That will also align with the PR title for ease of reviews

Perfect. Makes sense if that is the ongoing standard then there is probably no reason for this one to be different.

…he modal system

denniskigen requested a review from ibacher December 4, 2024 12:13

ibacher changed the title ~~(refactor) : bed management modal system~~ (refactor) bed management modal system Dec 4, 2024

ibacher changed the title ~~(refactor) bed management modal system~~ (refactor) O3-4199: Bed management should use the modal system Dec 4, 2024

ibacher requested changes Dec 4, 2024

View reviewed changes

amosmachora added 4 commits December 9, 2024 08:59

refactor: new bed modal

2a813dd

refactor: migrated edit bed functionality to use the modal system

db50be9

refactor: migrated edit bed type and new bed type components to use t…

9722f2c

…he modal system

refactor: migrated bed tag components to use the modal system

eea4e30

amosmachora force-pushed the refactor/bed-management-modal-system branch from 09545c7 to eea4e30 Compare December 9, 2024 06:46

amosmachora requested a review from ibacher December 9, 2024 06:51

denniskigen mentioned this pull request Dec 18, 2024

(fix) Standardize modal header close button styles #1420

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(refactor) O3-4199: Bed management should use the modal system #1381

(refactor) O3-4199: Bed management should use the modal system #1381

amosmachora commented Nov 19, 2024

ibacher left a comment

ibacher Dec 4, 2024

ojwanganto Dec 4, 2024

ibacher Dec 4, 2024

ibacher Dec 4, 2024

ibacher Dec 4, 2024

ibacher Dec 4, 2024

amosmachora Dec 5, 2024

brandones Dec 9, 2024

ojwanganto Dec 9, 2024

ibacher Dec 4, 2024

amosmachora Dec 5, 2024

ibacher Dec 5, 2024

amosmachora Dec 5, 2024

amosmachora Dec 5, 2024

ibacher Dec 4, 2024

amosmachora commented Dec 5, 2024

ibacher commented Dec 5, 2024

amosmachora commented Dec 5, 2024

amosmachora commented Dec 5, 2024

ojwanganto commented Dec 5, 2024

amosmachora commented Dec 6, 2024

		const [showErrorNotification, setShowErrorNotification] = useState(false);
		const [formStateError, setFormStateError] = useState('');

(refactor) O3-4199: Bed management should use the modal system #1381

Are you sure you want to change the base?

(refactor) O3-4199: Bed management should use the modal system #1381

Conversation

amosmachora commented Nov 19, 2024

Requirements

Summary

Screenshots

Short video verifying that nothing breaks after this change

Related Issue

Other

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amosmachora commented Dec 5, 2024

ibacher commented Dec 5, 2024

amosmachora commented Dec 5, 2024

amosmachora commented Dec 5, 2024

ojwanganto commented Dec 5, 2024

amosmachora commented Dec 6, 2024