HTML-842: Add Appointments tag #302

mogoodrich · 2024-05-21T21:44:21Z

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/HTML-

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

api/src/main/resources/moduleApplicationContext.xml

mseaton · 2024-05-22T12:20:38Z

api/src/main/java/org/openmrs/module/htmlformentry/FormEntrySession.java

@@ -28,6 +28,9 @@
 import org.openmrs.Relationship;
 import org.openmrs.api.ObsService;
 import org.openmrs.api.context.Context;
+import org.openmrs.module.appointments.model.Appointment;
+import org.openmrs.module.appointments.model.AppointmentStatus;
+import org.openmrs.module.appointments.service.AppointmentsService;


Are you sure these imports don't cause problems when HFE is run in an environment that does not have the appointments module loaded? I would be concerned that this might be an issue.

Very good point, thanks @mseaton

api/src/main/java/org/openmrs/module/htmlformentry/FormSubmissionActions.java

mseaton · 2024-05-22T12:24:44Z

api/src/main/java/org/openmrs/module/htmlformentry/element/AppointmentsElement.java

+			// first, get all scheduled appointments for this patient
+			AppointmentSearchRequest request = new AppointmentSearchRequest();
+			request.setPatientUuid(patient.getUuid());
+			request.setStartDate(new DateTime().minusYears(1000).toDate()); // hack, we want all appts for patient regardless of start date, but the search method always returns null if start date is null; this will start to fail in a thousand years


Seems like something we should ticket and fix in the appointments module - can you ticket that?

https://bahmni.atlassian.net/browse/BAH-3867

mseaton · 2024-05-22T12:28:23Z

api/src/main/java/org/openmrs/module/htmlformentry/element/AppointmentsElement.java

+			appointments.sort(Comparator.comparing(Appointment::getStartDateTime).reversed());
+
+			// in VIEW mode, only show appointments linked to encounter; in EDIT mode show those linked to encounter and all scheduled appts
+			appointments.removeIf(appointment -> (context.getMode() == FormEntryContext.Mode.VIEW


I'm not entirely sure, reading this, that this does what the comment says it should do. Even if it does, I'd favor code that is easier to understand, even if it takes more statements.

Will break up into a VIEW and a ENTER/EDIT component

mseaton · 2024-05-22T12:31:37Z

api/src/main/java/org/openmrs/module/htmlformentry/widget/AppointmentsWidget.java

+		// this is to ensure that this widget consistently increments the field name sequential value only once,
+		// otherwise, if the number of matches appointments changed between when the form was opened and the form was saved,
+		// widget names would be inconsistent, wreaking havoc on the form
+		int i = 1;


I'm confused as to why you would use a sequential integer here that you have to hope is the same on render and submission. Why not just use the primary key of the appointment itself?

Fair... will correct.

mogoodrich

Ready for re-review.

Initial screenshot, though I am assuming we may tweak with layout, working on that next:

mogoodrich · 2024-05-24T19:08:05Z

api/src/main/java/org/openmrs/module/htmlformentry/widget/AppointmentsWidget.java

+		// this is to ensure that this widget consistently increments the field name sequential value only once,
+		// otherwise, if the number of matches appointments changed between when the form was opened and the form was saved,
+		// widget names would be inconsistent, wreaking havoc on the form
+		int i = 1;


Fair... will correct.

mogoodrich · 2024-05-24T19:22:38Z

api/src/main/java/org/openmrs/module/htmlformentry/element/AppointmentsElement.java

+			// first, get all scheduled appointments for this patient
+			AppointmentSearchRequest request = new AppointmentSearchRequest();
+			request.setPatientUuid(patient.getUuid());
+			request.setStartDate(new DateTime().minusYears(1000).toDate()); // hack, we want all appts for patient regardless of start date, but the search method always returns null if start date is null; this will start to fail in a thousand years


https://bahmni.atlassian.net/browse/BAH-3867

mogoodrich · 2024-05-24T19:22:53Z

api/src/main/java/org/openmrs/module/htmlformentry/element/AppointmentsElement.java

+			appointments.sort(Comparator.comparing(Appointment::getStartDateTime).reversed());
+
+			// in VIEW mode, only show appointments linked to encounter; in EDIT mode show those linked to encounter and all scheduled appts
+			appointments.removeIf(appointment -> (context.getMode() == FormEntryContext.Mode.VIEW


Will break up into a VIEW and a ENTER/EDIT component

mogoodrich · 2024-05-24T19:46:27Z

api/src/main/java/org/openmrs/module/htmlformentry/appointment/AppointmentsAbstractor.java

+ * Entry Session to avoid runtime class loading issues in instances where the Appointments module is
+ * not present
+ */
+public class AppointmentsAbstractor {


Happy to take suggestions of a better name for this class!

mseaton

Generally looks good, just one or two things that I noticed that should be fixed. Nice work!

mseaton · 2024-05-24T19:53:46Z

api/src/main/java/org/openmrs/module/htmlformentry/FormSubmissionActions.java

@@ -19,6 +19,7 @@
 import org.openmrs.ProgramWorkflowState;
 import org.openmrs.Relationship;
 import org.openmrs.api.context.Context;
+import org.openmrs.module.appointments.model.Appointment;


Still have this stray import here. Can you remove this?

Strange, I noticed a couple days ago that my editor wasn't removing all imports, not quite sure what is going on

mseaton · 2024-05-24T19:58:28Z

api/src/main/java/org/openmrs/module/htmlformentry/widget/AppointmentsWidget.java

+			        && appointment.getFulfillingEncounters().contains(context.getExistingEncounter())) {
+				checkboxWidget.setInitialValue(appointment.getUuid());
+			}
+			context.registerWidget(checkboxWidget, fieldName + "_" + appointment.getUuid());


I'd have just used appointment.id, rather than uuid, as it is more concise and if you need to use it in JS on the client side there will be less issues.

fair enough

mogoodrich · 2024-05-24T21:29:03Z

Thanks for all the comments, I had to fix one Github Copilot AI autocomplete error as well... :) Merging in...

mogoodrich added 2 commits May 17, 2024 18:10

HTML-842: Add Appointments Tag

6184b0f

HTML-842: Add Appointments Tag

f0840c8

mogoodrich marked this pull request as draft May 21, 2024 21:44

HTML-842: Add Appointments Tag

3712bfe

mogoodrich changed the title ~~Html 842~~ HTML-842: Add Appointments tag May 21, 2024

mogoodrich commented May 21, 2024

View reviewed changes

api/src/main/resources/moduleApplicationContext.xml Show resolved Hide resolved

mogoodrich marked this pull request as ready for review May 21, 2024 22:35

mogoodrich requested a review from mseaton May 21, 2024 22:35

mseaton reviewed May 22, 2024

View reviewed changes

HTML-842: Add Appointments Tag

f3e379b

mogoodrich commented May 24, 2024

View reviewed changes

mogoodrich requested a review from mseaton May 24, 2024 19:47

mseaton reviewed May 24, 2024

View reviewed changes

mogoodrich added 3 commits May 24, 2024 16:44

HTML-842: Add Appointments Tag

1df138d

HTML-842: Add Appointments Tag

7ce0acf

HTML-842: Add Appointments Tag

1840814

mogoodrich merged commit aa38901 into master May 24, 2024
1 check passed

mogoodrich deleted the HTML-842 branch May 24, 2024 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML-842: Add Appointments tag #302

HTML-842: Add Appointments tag #302

mogoodrich commented May 21, 2024

mseaton May 22, 2024

mogoodrich May 22, 2024

mseaton May 22, 2024

mogoodrich May 24, 2024

mseaton May 22, 2024

mogoodrich May 24, 2024

mseaton May 22, 2024

mogoodrich May 24, 2024

mogoodrich left a comment

mogoodrich May 24, 2024

mogoodrich May 24, 2024

mogoodrich May 24, 2024

mogoodrich May 24, 2024

mseaton left a comment

mseaton May 24, 2024

mogoodrich May 24, 2024

mseaton May 24, 2024

mogoodrich May 24, 2024

mogoodrich commented May 24, 2024

HTML-842: Add Appointments tag #302

HTML-842: Add Appointments tag #302

Conversation

mogoodrich commented May 21, 2024

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

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

mogoodrich 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

mseaton 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

mogoodrich commented May 24, 2024