-
Notifications
You must be signed in to change notification settings - Fork 16
O3-4542: Extend Visit Patient Scenario to add Condition #87
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
Conversation
@Bawanthathilan , @jayasanka-sack Can this PR be reviewed? Thanks |
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.
Thanks a lot for the PR! Please check my review comments. :)
src/test/java/org/openmrs/performance/http/DoctorHttpService.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openmrs/performance/http/DoctorHttpService.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openmrs/performance/http/DoctorHttpService.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openmrs/performance/registries/DoctorRegistry.java
Outdated
Show resolved
Hide resolved
@jayasanka-sack @Bawanthathilan , i made the requested changes can it be reviewed again, thanks |
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.
Great job! You’re making great progress. 👏
See my updated comments.
@@ -53,6 +53,12 @@ public class Constants { | |||
|
|||
public static final String ORDER = "39da3525-afe4-45ff-8977-c53b7b359158"; | |||
|
|||
public static final String BACK_PAIN = "114403AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; | |||
|
|||
public static final String ONSET_DATE = "2025-03-13T00:00:00+05:30"; |
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 date shouldn't hardcoded, instead you should calculate it based on the current time.
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.
Hi @jayasanka-sack , in this suggestion according to my understanding OnSet date refers to date when the condition appeared which would generally be before the current time should i make it dynamic i.e Current date - X days or just hard code the X ? or just leave it as current date
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.
@jayasanka-sack I’m planning to create a reusable date utility function in utils/CommonUtils. since I’ve noticed many API requests in future would require require dates . The goal is to centralize date function to reduce redundancy and make it versatile . Would you have any suggestions on this ?
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.
Leaving it as current date is okay.
Re: util function, yes that's a nice idea. Open it as a separate PR.
|
||
public HttpRequestActionBuilder searchForConditions(String searchQuery) { | ||
return http("Search for Condition") | ||
.get("/openmrs/ws/rest/v1/concept?name=" + searchQuery + "&searchType=fuzzy&class=8d4918b0-c2cc-11de-8d13-0010c6dffd0f&v=custom:(uuid,display)"); |
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.
Handle this this Magic value (the uuid) properly. I guess it should be a constant.
A magic value is a literal value that appears directly in the code without explanation or context.
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.
Yes went past me , its the DIAGNOSIS_UUID i will be changing 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.
Pull Request Overview
This PR extends the API coverage to include condition management during the visit patient scenario.
- Added a step in the visit patient scenario to add a condition.
- Introduced an addCondition chain in the doctor registry to trigger condition-related API calls.
- Implemented searchForConditions and saveCondition endpoints in the HTTP service along with defining related constants.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/test/java/org/openmrs/performance/scenarios/VisitPatientScenario.java | Added steps to add a condition in the patient visit scenario. |
src/test/java/org/openmrs/performance/registries/DoctorRegistry.java | Added new chain method (addCondition) to simulate condition API calls. |
src/test/java/org/openmrs/performance/http/DoctorHttpService.java | Introduced searchForConditions and saveCondition endpoints. |
src/test/java/org/openmrs/performance/Constants.java | Added constants to support the condition API. |
String asprin_162_5mg = "a722710f-403b-451f-804b-09f8624b0838"; | ||
String asprinConcept = "71617AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; | ||
return exec(httpService.getActiveVisitOfPatient(patientUuid), httpService.searchForDrug("asprin"), | ||
httpService.searchForDrug("Tylenol"), | ||
httpService.saveOrder(patientUuid, visitUuid, currentUserUuid, asprin_162_5mg, asprinConcept)); | ||
|
||
return exec( | ||
httpService.getActiveVisitOfPatient(patientUuid), | ||
httpService.searchForDrug("asprin"), | ||
httpService.searchForDrug("Tylenol"), | ||
httpService.saveOrder(patientUuid, visitUuid, currentUserUuid, asprin_162_5mg, asprinConcept) |
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.
Typo in 'asprin'; consider replacing it with 'aspirin' for clarity.
Copilot uses AI. Check for mistakes.
src/test/java/org/openmrs/performance/registries/DoctorRegistry.java
Outdated
Show resolved
Hide resolved
public static String getDate(int daysToAdjust) { | ||
// Get current date-time and adjust by the specified number of days | ||
ZonedDateTime adjustedDateTime = ZonedDateTime.now().plusDays(daysToAdjust); | ||
|
||
return adjustedDateTime.format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX")); | ||
} |
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.
Split responsibilities into clearly named, single-purpose functions. It makes the code cleaner.
public static String getDate(int daysToAdjust) { | |
// Get current date-time and adjust by the specified number of days | |
ZonedDateTime adjustedDateTime = ZonedDateTime.now().plusDays(daysToAdjust); | |
return adjustedDateTime.format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX")); | |
} | |
public static String getCurrentDateTimeAsString() { | |
ZonedDateTime now = ZonedDateTime.now(); | |
return formatDateTime(now); | |
} | |
public static String getAdjustedDateTimeAsString(int daysToAdjust) { | |
ZonedDateTime adjustedDateTime = ZonedDateTime.now().plusDays(daysToAdjust); | |
return formatDateTime(adjustedDateTime); | |
} | |
private static String formatDateTime(ZonedDateTime dateTime) { | |
return dateTime.format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX")); | |
} |
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 write unit tests for above functions in src/test/java/org/openmrs/performance/utils/TestCommonUtils.java
file.
Opening a separate PR for the util would be better I guess.
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.
Sure @jayasanka-sack , will open a new PR for this
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.
Pull Request Overview
This PR extends the Visit Patient Scenario to support adding conditions for a patient by introducing new API calls.
- Added a new utility method in CommonUtils for generating formatted date strings.
- Updated scenario and registry classes to include condition-related chain steps.
- Extended DoctorHttpService and Constants to support condition search and save operations.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/test/java/org/openmrs/performance/utils/CommonUtils.java | Added a getDate method for returning formatted dates. |
src/test/java/org/openmrs/performance/scenarios/VisitPatientScenario.java | Updated the scenario to include a step for adding a condition. |
src/test/java/org/openmrs/performance/registries/DoctorRegistry.java | Introduced a new addCondition chain builder method. |
src/test/java/org/openmrs/performance/http/DoctorHttpService.java | Added endpoints for searching and saving conditions using the new utility method. |
src/test/java/org/openmrs/performance/Constants.java | Added new constants for condition operations. |
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 good for me @jayasanka-sack
JIRA LINK: https://openmrs.atlassian.net/browse/O3-4542
This PR was raised to extend the current API coverage by adding the api's to cover addition of condition