FM2-136 : Fhir appointment resource initial implementation by jecihjoy · Pull Request #29 · Bahmni/openmrs-module-appointments

jecihjoy · 2020-04-20T10:12:06Z

Implemented translations for the following properties

Uuid
Appointment start time
Appointment end time
Appointment status
Comment
Date created
Date updated

This PR is linked to this ticket https://issues.openmrs.org/projects/FM2/issues/FM2-136

CLAassistant · 2020-04-20T10:12:14Z

All committers have signed the CLA.

… provider and appointment provider

…nd AppointmentServiceDefinition

ibacher

@jecihjoy I've added some comments here on things I think should be changed. Let me know what you think.

ibacher · 2020-04-28T14:28:47Z

fhir2-appnt-api/pom.xml

+
+    <groupId>org.openmrs.module</groupId>
+    <artifactId>fhir2-appointment-api</artifactId>
+    <version>${openmrsModuleFhirVersion}</version>


Shouldn't the version here inherit from the parent module, i.e., be tied to the Bahmni appointment module version rather than the FHIR module version?

ibacher · 2020-04-28T14:35:10Z

fhir2-appnt-api/pom.xml

+            <plugin>
+                <groupId>com.mycila</groupId>
+                <artifactId>license-maven-plugin</artifactId>
+            </plugin>
+            <plugin>
+                <groupId>org.projectlombok</groupId>
+                <artifactId>lombok-maven-plugin</artifactId>
+            </plugin>
+            <plugin>
+                <groupId>net.revelc.code.formatter</groupId>
+                <artifactId>formatter-maven-plugin</artifactId>
+            </plugin>
+            <plugin>
+                <groupId>net.revelc.code</groupId>
+                <artifactId>impsort-maven-plugin</artifactId>
+            </plugin>


While we use these plugins inside the FHIR2 module, I'd hesitate about using them here, since they aren't defined in the parent POM, so they're missing all the configuration we do. Also, that configuration may not align with Bahmni's coding standards.

ibacher · 2020-04-28T14:35:55Z

fhir2-appnt-api/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>org.projectlombok</groupId>
+            <artifactId>lombok</artifactId>


You should define a version here, since Lombok isn't defined in the parent POM.

ibacher · 2020-04-28T14:37:13Z

...pi/src/main/java/org/openmrs/module/fhirappnt/api/Impl/FhirBahmniAppointmentServiceImpl.java

+@Primary
+@Component
+@Transactional
+@Setter(AccessLevel.PACKAGE)


This isn't doing anything here if you define the setters (which is fine).

ibacher · 2020-04-28T14:41:09Z

...pi/src/main/java/org/openmrs/module/fhirappnt/api/Impl/FhirBahmniAppointmentServiceImpl.java

+@Setter(AccessLevel.PACKAGE)
+public class FhirBahmniAppointmentServiceImpl implements FhirAppointmentService {
+
+    @Autowired


If you have setters defined, I would put the @Autowired annotation on them, rather than relying on field annotation.

ibacher · 2020-04-28T14:42:53Z

pom.xml

 		<openmrsAtomfeedVersion>2.5.6</openmrsAtomfeedVersion>
 		<atomfeed.version>1.9.4</atomfeed.version>
+		<lombokVersion>1.18.10</lombokVersion>
+		<openmrsModuleFhirVersion>1.0.0-SNAPSHOT</openmrsModuleFhirVersion>


This property probably belongs in the submodule rather than here.

ibacher · 2020-04-28T14:43:02Z

pom.xml

 		<jacocoVersion>0.7.9</jacocoVersion>
 		<openmrsAtomfeedVersion>2.5.6</openmrsAtomfeedVersion>
 		<atomfeed.version>1.9.4</atomfeed.version>
+		<lombokVersion>1.18.10</lombokVersion>


This property probably belongs in the submodule rather than here.

ibacher · 2020-04-28T14:45:12Z

omod/src/main/resources/config.xml

 		<require_module version="${openmrsAtomfeedVersion}">org.ict4h.openmrs.openmrs-atomfeed</require_module>
 	</require_modules>

+	<aware_of_modules>


We need to add to this:

<conditionalResources> <conditionalResource> <path>/lib/fhir2-appointment-api-${project.parent.version}.jar</path> <loadIfModulesPresent> <openmrsModule> <moduleId>org.openmrs.module.fhir2</moduleId> <version>1.*</version> </openmrsModule> </loadIfModulesPresent> </conditionalResource> </conditionalResources>

So that the FHIR-specific stuff is only loaded if the FHIR module is present.

First, you will have to upgrade the config.xml DTD file version from 1.2 to 1.6
Either this

<!DOCTYPE module PUBLIC "-//OpenMRS//DTD OpenMRS Config 1.0//EN" "https://resources.openmrs.org/doctype/config-1.6.dtd">

or
<module configVersion="1.6">
This is because loadIfModulesPresent was introduced in version 1.6 Also, conditionalResources

kipchubett · 2020-04-28T14:47:37Z

...penmrs/module/fhirappnt/api/translators/impl/HealthCareServiceParticipantTranslatorImpl.java

+    @Autowired
+    AppointmentServiceDefinitionService appointmentServiceDefinitionService;
+


You forgot to add private modifier

kipchubett · 2020-04-28T14:55:28Z

...st/java/org/openmrs/module/fhirappnt/api/translators/impl/AppointmentTranslatorImplTest.java

+
+
+
+
+
+
+
+
+
+
+
+


Remove the blank lines

kipchubett · 2020-04-28T14:58:32Z

fhir2-appnt-api/src/main/java/org/openmrs/module/fhirappnt/api/AppointmentFhirConstants.java

+import org.openmrs.module.fhir2.FhirConstants;
+
+@NoArgsConstructor
+public class AppointmentFhirConstants extends FhirConstants {


Why do you need to extend FhirConstants and you already have fhir2-api as a dependency?

sherrif10 · 2021-02-04T10:20:45Z

@corneliouzbett Feel free to close PR FM2-136 , I hope we also need to back up with other sub tickets, i mean other sub tickets should be pushed in reference to this ticket in bamnhi not on openmrs right?

jecihjoy · 2021-02-04T10:28:31Z

@corneliouzbett Feel free to close PR FM2-136 , I hope we also need to back up with other sub tickets, i mean other sub tickets should be pushed in reference to this ticket in bamnhi not on openmrs right?

yes @sherrif10 all appointment translations logic were supposed to be done against this repository, do you wish to join efforts so that we can work on this together?

sherrif10 · 2021-02-04T12:12:53Z

Sure thanks, I will be commiting other pull request regarding appointment

…

On Thu, Feb 4, 2021, 1:28 PM jecihjoy ***@***.***> wrote: @corneliouzbett <https://github.com/corneliouzbett> Feel free to close PR FM2-136 , I hope we also need to back up with other sub tickets, i mean other sub tickets should be pushed in reference to this ticket in bamnhi not on openmrs right? yes @sherrif10 <https://github.com/sherrif10> all appointment translations logic were supposed to be done against this repository, do you wish to join efforts so that we can work on this together? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALOKS6HBDZ7WVT4LLICXYGDS5JZGDANCNFSM4MMJ53WA> .

Update module group id for kenyaemr form

FM2-136 : Fhir appointment resource initial implementation

f56afeb

FM2-161 : Created translations for appointment participants; patient,…

151eadd

… provider and appointment provider

jecihjoy force-pushed the FM2-136 branch from fdbca3a to 151eadd Compare April 28, 2020 09:26

FM2-162 : Implement translations between HealthCareService resource a…

c4c5169

…nd AppointmentServiceDefinition

ibacher suggested changes Apr 28, 2020

View reviewed changes

kipchubett reviewed Apr 28, 2020

View reviewed changes

kipchubett mentioned this pull request Feb 4, 2021

FM2-136 : Appointment resource initial implementation openmrs/openmrs-module-fhir2#136

Closed

6 tasks

jecihjoy pushed a commit to jecihjoy/openmrs-module-appointments that referenced this pull request Jan 22, 2024

Merge pull request Bahmni#29 from jecihjoy/module-deploy

aac95d1

Update module group id for kenyaemr form

		@Autowired
		AppointmentServiceDefinitionService appointmentServiceDefinitionService;

Conversation

jecihjoy commented Apr 20, 2020

Uh oh!

CLAassistant commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibacher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherrif10 commented Feb 4, 2021

Uh oh!

jecihjoy commented Feb 4, 2021

Uh oh!

sherrif10 commented Feb 4, 2021 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Apr 20, 2020 •

edited

Loading