-
Notifications
You must be signed in to change notification settings - Fork 86
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
BAH-3479 | Backend changes for dateless appointments #145
Conversation
…ges for priority (#104) * add database column and api changes for priority Co-authored-by: Umair Fayaz <[email protected]> * add priority to appointment audit Co-authored-by: Umair Fayaz <[email protected]> * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]>
…ests (#106) * removed invalid priority and related tests * added testcase for invalid priority
…ges for priority (#104) * add database column and api changes for priority * add priority to appointment audit * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]>
…ests (#106) * removed invalid priority and related tests * added testcase for invalid priority
As @angshu had mentioned yesterday, please ensure that an appointment should show in "Waitlist" based on status and not based on whether a date is set/no step. Can you verify code for this, and maybe add a test to ensure that even if a data is set on an appointment, it shows in waitlist if its status is waitlist. |
4e2d730
to
63f7c4e
Compare
@@ -243,27 +239,13 @@ public void shouldReturnAppointmentsForProvider() { | |||
assertEquals(1, appointments.size()); | |||
} | |||
|
|||
@Test | |||
public void shouldReturnAppointmentsForPatientBetweenGivenDatesForASpecificStatus() throws ParseException { |
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.
Why is this test removed?
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.
Apologies, since it has been added later, it was removed while cherry picking by mistake. Added it back
api/src/test/java/org/openmrs/module/appointments/dao/impl/AppointmentDaoImplIT.java
Show resolved
Hide resolved
omod/src/main/java/org/openmrs/module/appointments/web/contract/AppointmentRequest.java
Show resolved
Hide resolved
omod/src/main/java/org/openmrs/module/appointments/web/contract/AppointmentDefaultResponse.java
Show resolved
Hide resolved
@Test | ||
public void shouldMapExistingAppointmentWhenPayloadHasAppointmentPriority() throws ParseException { | ||
AppointmentRequest appointmentRequest = createAppointmentRequest(); | ||
appointmentRequest.setPriority("Routine"); |
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.
Why are getters/setters for Appointment Priority using String, instead of Enum?
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.
Since we referred appointment status, we followed the same for priority as well
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.
See comments above
api/src/main/resources/liquibase.xml
Outdated
</preConditions> | ||
<comment>Update start and end date time of patient appointment table to be nullable</comment> | ||
<sql> | ||
ALTER TABLE patient_appointment MODIFY start_date_time datetime; |
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 its better to be explicit about default as null using DEFAULT NULL
.
https://fedingo.com/how-to-modify-mysql-column-to-allow-null/
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.
default value is set to NULL now
What is the default priority assumed, if no priority is set (for backward compatibility)? Where is that code? I am guessing default is "routine" priority. thanks! |
Currently, default is not set, because showing it in the UI itself is behind a toggle. https://github.com/Bahmni/openmrs-module-appointments/pull/145/files#diff-0c4429900b456f54d785391170847fccd1f3e8f0fcb322d5ee296670318c2affR111 |
if (appointmentSearchRequest.getStartDate() != null) { | ||
criteria.addOrder(Order.asc("startDateTime")); | ||
} else { | ||
criteria.addOrder(Order.asc("dateCreated")); |
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.
is there any reason to order by date created. An appt maybe created well ahead in advance for later dated appointments. If this is done for for waitlisted appointment, then this can be combined with the status - e.g. if status = waitlist, order by dateCreated if not specifically asked for "startDateTime"
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.
refactored to combine both criteria
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 am little confused. We seem to be ordering by startdate (ascending) if its part of the search parameter. I can still want to search by status but order by startdate, no? If you want to sort based on request, then the request must send it as sort parameter, not search parameter
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.
yeah I agree. reverted it back to the existing way since that'll still sort the appointments irrespective of status
setDateCriteria(appointmentSearchRequest, criteria); | ||
setPatientCriteria(appointmentSearchRequest, criteria); | ||
setLimitCriteria(appointmentSearchRequest, criteria); | ||
setProviderCriteria(appointmentSearchRequest, criteria); | ||
setStatusCriteria(appointmentSearchRequest, criteria); | ||
|
||
String limit = Context.getAdministrationService().getGlobalProperty("webservices.rest.maxResultsDefault"); |
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.
You don't make a service call from Dao. if you need it, pass it from the service. Not here!
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.
moved it to service method
api/src/main/java/org/openmrs/module/appointments/model/AppointmentPriority.java
Outdated
Show resolved
Hide resolved
@@ -12,6 +12,7 @@ public class AppointmentQuery { | |||
private String patientUuid; | |||
private String status; | |||
private String appointmentKind; | |||
private Boolean isDatelessAppointments = 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.
am assuming this comes from frontend API call. Why should we call the parameter as "isDateLessAppointments"? if your intent is to search by waitlisted appointment, then "status" should be enough. or if you really want to search appointments without dates - atleast call the parameter as "withoutDates" or "unassingedDates". Javabeans properties for booleans do not require fields to be prefixed with "Is" - boolean fields getter can be "IsWithoutDates" or "hasUnassignedDates". An appointment is not a valid one without dates - so "isDateLessAppointments" do not make sense
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.
renamed to withoutDates
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.
"isDatelessAppointments" attribute still exists. Have you refactored the code to search by status instead of relying on "datelessAppointment" as part of search?
api/src/main/java/org/openmrs/module/appointments/dao/impl/AppointmentDaoImpl.java
Outdated
Show resolved
Hide resolved
omod/src/main/java/org/openmrs/module/appointments/web/contract/AppointmentRequest.java
Show resolved
Hide resolved
@@ -60,6 +60,10 @@ public List<AppointmentDefaultResponse> getAllAppointments(@RequestParam(value = | |||
@RequestMapping( method = RequestMethod.POST, value = "search") | |||
@ResponseBody | |||
public List<AppointmentDefaultResponse> searchAppointments( @Valid @RequestBody AppointmentQuery searchQuery) throws IOException { | |||
if(searchQuery.getIsDatelessAppointments()) { |
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.
Also you are expecting a searchquery but fetching only date-less appointment?
Does it mean
- if appointment without dates, rest of criteria do not apply? That does not seem right. I can look for an appointment with a certain priority or service for which dates have not been assinged yet - right?
please don't call service method as "searchDatelessAppointment". Call it "appointmentWithoutDatesAssigned" or something that makes more sense. DatelessAppt seems very much a technical internal thing.
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.
renamed getter as isWithoutDates() , service method as searchAppointmentsWithoutDates() and dao method as getAppointmentsWithoutDates()
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.
same question above is relevant here as well. Why not leverage status if you are looking for "waitlisted" appointments? Unless you want to return all appointments without dates?
@Test | ||
public void shouldMapExistingAppointmentWhenPayloadHasAppointmentPriority() throws ParseException { | ||
AppointmentRequest appointmentRequest = createAppointmentRequest(); | ||
appointmentRequest.setPriority("Routine"); |
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.
See comments above
I am trying to determine, if an existing adopter of Bahmni, that already has appointments in their database, uses this new module, with wait-list enabled, how will previously created appointments show up (if they have null in priority). Is that something we need to worry, or the new feature/UI will work fine, if it has older created appointments (for future dates)? |
Yeah right, the new UI will work fine for the older appointments irrespective of priority. If they want to add priority for older appointments, they can edit the appointment and capture it from the dropdown |
if (appointmentSearchRequest.getStartDate() != null) { | ||
criteria.addOrder(Order.asc("startDateTime")); | ||
} else { | ||
criteria.addOrder(Order.asc("dateCreated")); |
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 am little confused. We seem to be ordering by startdate (ascending) if its part of the search parameter. I can still want to search by status but order by startdate, no? If you want to sort based on request, then the request must send it as sort parameter, not search parameter
appointmentJson.put("endDateTime", appointment.getEndDateTime().toInstant().toString()); | ||
String startDate = (appointment.getStatus() == AppointmentStatus.WaitList && appointment.getStartDateTime() == null) ? null : appointment.getStartDateTime().toInstant().toString(); | ||
appointmentJson.put("startDateTime", startDate); | ||
String endDate = (appointment.getStatus() == AppointmentStatus.WaitList && appointment.getStartDateTime() == null) ? null : appointment.getEndDateTime().toInstant().toString(); |
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 will negate one of the criterias that a waitlisted appointment can have a date. This assumes that waitlist status appointment can not have a date. Why do we have to do this check against waitlist status. Why not simply just check if the dates are null, alternatively just get the time instant from the date?
I am assuming the method "getAppointmentAsJsonString" is used for audit purpose, and since it has the appointment object - you have all the info.
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.
yeah, this method is used for audit purpose only. This will still allow waitlist appointments with dates by falling to the else part but there is no need of this check against waitlist. I'll remove the status check here
@@ -12,6 +12,7 @@ public class AppointmentQuery { | |||
private String patientUuid; | |||
private String status; | |||
private String appointmentKind; | |||
private Boolean isDatelessAppointments = 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.
"isDatelessAppointments" attribute still exists. Have you refactored the code to search by status instead of relying on "datelessAppointment" as part of search?
@@ -68,4 +69,12 @@ public String getServiceTypeUuid() { | |||
public void setServiceTypeUuid(String serviceTypeUuid) { | |||
this.serviceTypeUuid = serviceTypeUuid; | |||
} | |||
|
|||
public Boolean getIsDatelessAppointments() { |
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.
Why do we need this attribute "isDatelessAppointment"? if your idea is to search for waitlisted appointment - do it by "status" not this unnecessary attribute. Unless within a particular status, you also want to search the appointments without dates, which I believe is not your requirement.
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 attribute was renamed to "withoutDates", not sure how it is showing "isDatelessAppointment". can we try refreshing the page once ?
@@ -60,6 +60,10 @@ public List<AppointmentDefaultResponse> getAllAppointments(@RequestParam(value = | |||
@RequestMapping( method = RequestMethod.POST, value = "search") | |||
@ResponseBody | |||
public List<AppointmentDefaultResponse> searchAppointments( @Valid @RequestBody AppointmentQuery searchQuery) throws IOException { | |||
if(searchQuery.getIsDatelessAppointments()) { |
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.
same question above is relevant here as well. Why not leverage status if you are looking for "waitlisted" appointments? Unless you want to return all appointments without dates?
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.
Few comments to be implemented are being tracked in this card BAH-3637 and will be taken up later
* Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority Co-authored-by: Umair Fayaz <[email protected]> * add priority to appointment audit Co-authored-by: Umair Fayaz <[email protected]> * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority * add priority to appointment audit * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Add logic to filter DateLess Appointments * add. appointment creation date in response (#109) * Add tests to filter DateLess Appointments * Update POM files * Fix failing Tests for Search appointments * Kavitha | add date check and tests for waitlist appointments * Kavitha | removed Invalid priority * Kavitha | added missed tests from master branch * Kavitha | rename datelessAppointments to appointmentsWithoutDates * Kavitha | add default null value to date columns in appt * Kavitha | removed status check in appointment json * Kavitha | removed sorting based on status * Kavitha | removed unused imports --------- Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]> Co-authored-by: Arjun G <[email protected]>
* BAH-3479 | Backend changes for dateless appointments (#145) * Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority Co-authored-by: Umair Fayaz <[email protected]> * add priority to appointment audit Co-authored-by: Umair Fayaz <[email protected]> * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority * add priority to appointment audit * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Add logic to filter DateLess Appointments * add. appointment creation date in response (#109) * Add tests to filter DateLess Appointments * Update POM files * Fix failing Tests for Search appointments * Kavitha | add date check and tests for waitlist appointments * Kavitha | removed Invalid priority * Kavitha | added missed tests from master branch * Kavitha | rename datelessAppointments to appointmentsWithoutDates * Kavitha | add default null value to date columns in appt * Kavitha | removed status check in appointment json * Kavitha | removed sorting based on status * Kavitha | removed unused imports --------- Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]> Co-authored-by: Arjun G <[email protected]> * Kavitha | refactor search API for appointments without dates * Kavitha|lowered test coverage ratio * add specific imports --------- Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]> Co-authored-by: Arjun G <[email protected]>
* BAH-3479 | Backend changes for dateless appointments (#145) * Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority Co-authored-by: Umair Fayaz <[email protected]> * add priority to appointment audit Co-authored-by: Umair Fayaz <[email protected]> * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority * add priority to appointment audit * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Add logic to filter DateLess Appointments * add. appointment creation date in response (#109) * Add tests to filter DateLess Appointments * Update POM files * Fix failing Tests for Search appointments * Kavitha | add date check and tests for waitlist appointments * Kavitha | removed Invalid priority * Kavitha | added missed tests from master branch * Kavitha | rename datelessAppointments to appointmentsWithoutDates * Kavitha | add default null value to date columns in appt * Kavitha | removed status check in appointment json * Kavitha | removed sorting based on status * Kavitha | removed unused imports --------- Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]> Co-authored-by: Arjun G <[email protected]> * Kavitha | refactor search API for appointments without dates * Kavitha|lowered test coverage ratio * add specific imports --------- Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]> Co-authored-by: Arjun G <[email protected]>
* BAH-3350: Appointments: search endpoint stack traces when a patient has more than one identifier of the same time (#142) * Ability to Filter on Appointments with multi-value support for search keys Ability to Filter on Awaiting Appointments with multiple values for same keys * Added test cases to increase branch coverage & set threshold to 0.60 (#152) Added test cases to increase branch coverage & set threshold to 0.60 (#152) * Kavitha|Awaiting appointments code refactor (#151) * BAH-3479 | Backend changes for dateless appointments (#145) * Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority Co-authored-by: Umair Fayaz <[email protected]> * add priority to appointment audit Co-authored-by: Umair Fayaz <[email protected]> * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Kavitha, Umair | A-1204370983916597| add database column and api changes for priority (#104) * add database column and api changes for priority * add priority to appointment audit * fixed test failure for priority * add tests for code coverage --------- Co-authored-by: Umair Fayaz <[email protected]> * Kavitha | A-1204361352115416 | removed invalid priority and related tests (#106) * removed invalid priority and related tests * added testcase for invalid priority * Backend migration to create dateless appointments (#105) * allow dateless appointments if status is waitlist (#108) * Add logic to filter DateLess Appointments * add. appointment creation date in response (#109) * Add tests to filter DateLess Appointments * Update POM files * Fix failing Tests for Search appointments * Kavitha | add date check and tests for waitlist appointments * Kavitha | removed Invalid priority * Kavitha | added missed tests from master branch * Kavitha | rename datelessAppointments to appointmentsWithoutDates * Kavitha | add default null value to date columns in appt * Kavitha | removed status check in appointment json * Kavitha | removed sorting based on status * Kavitha | removed unused imports --------- Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]> Co-authored-by: Arjun G <[email protected]> * Kavitha | refactor search API for appointments without dates * Kavitha|lowered test coverage ratio * add specific imports --------- Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]> Co-authored-by: Arjun G <[email protected]> * BAH-3860 | repeated imports in tests * BAH-3860 | fix. tests in AppointmentMapper --------- Co-authored-by: Mark Goodrich <[email protected]> Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: kalai-tw <[email protected]> Co-authored-by: kavitha-sundararajan <[email protected]> Co-authored-by: Umair Fayaz <[email protected]> Co-authored-by: Phanindra-tw <[email protected]>
JIRA - BAH-3479