OZ-545: Refactor code to use camel-openmrs-fhir and rewrite routes with Java DSL #22

VaishSiddharth · 2024-05-08T13:47:05Z

Add Odoo client
Wrap Odoo client in a camel component
Add Odoo data model
Write fhir<->odoo mappers
Migrate XML routes to Java DSL
Leverage opemrs-fhir
Add Unit tests

Ticket: https://mekomsolutions.atlassian.net/browse/OZ-545

mks-d

That's

OZ-545: Refactor code to use camel-openmrs-fhir

right?

ibacher

Thanks @VaishSiddharth Haven't gone through everything, but some quick, mostly minor comments.

ibacher · 2024-06-10T14:30:41Z

pom.xml

@@ -44,10 +44,12 @@
    <camel.version>4.1.0</camel.version>
    <spring-boot.version>3.1.4</spring-boot.version>
    <xmlRpcClientVersion>3.1.3</xmlRpcClientVersion>
+    <fhir-stu4.version>7.0.0</fhir-stu4.version>


FHIR releases post 3 use R<x> format. STU is "Standard for Trial Use"—essentially the specification version of "beta".

Suggested change

<fhir-stu4.version>7.0.0</fhir-stu4.version>

<fhir-r4.version>7.0.0</fhir-r4.version>

Thanks for pointing this out.
Lets fix this in eip-erpnext-openmrs repo as well. https://github.com/ozone-his/eip-erpnext-openmrs/blob/main/pom.xml#L61

cc: @corneliouzbett

ibacher · 2024-06-10T14:40:12Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooClient.java

+                    }
+                });
+            }
+        };


This seems a convoluted way of writing:

var config = new XmlRpcClientConfigImpl(); config.setEnabledForExtensions(true); config.setServerUrl(/* ... */); client = new XmlRpcClient(); client.setConfig(config);

The above is actually fewer lines of code (6 vs 10), easier for other maintainers to read / understand, and skips the creation of two anonymous inner classes.

ibacher · 2024-06-10T14:42:18Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooClient.java

+        try {
+            init();
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }


I'd probably do this in the opposite way, i.e., instead of authenticateIfNecessary() each function should call init() which should:

Initialize the client if it hasn't already been initialized

Authenticate the client if it hasn't already been authenticated

ibacher · 2024-06-10T14:43:06Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooClient.java

+        };
+    }
+
+    public void authenticate() throws MalformedURLException {


Why is everything but the MalformedURLException turned into an unchecked exception?

ibacher · 2024-06-10T14:43:55Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooClient.java

+        }
+    }
+
+    public Integer create(String model, List<Map<String, Object>> dataParams)


Is there a reason for favoring Integer over int here and boxed types over primitives in general?

Tried to keep the client consistent with Odoo documentation

ibacher · 2024-06-10T14:50:23Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooUtils.java

+
+public class OdooUtils {
+
+    private static final Logger log = LoggerFactory.getLogger(OdooUtils.class);


Any reason not to use Lombok's @Slf4j annotation here?

ibacher · 2024-06-10T14:51:28Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooUtils.java

+            log.debug("OdooUtils: Converted map {} to object {}", data, obj);
+            return obj;
+        } catch (Exception e) {
+            throw new RuntimeException(String.format("Error converting map %s to object: %s", data, e.getMessage()));


Not sure that adding HashMap@1882378729 to this error message is helpful...

Will it not print something like

{a=1, b=2, c=3}

ibacher · 2024-06-10T14:55:16Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooUtils.java

+        }
+    }
+
+    public static Map<String, Object> convertObjectToMap(Object object) throws Exception {


This could be a one-liner:

new ObjectMapper().convertValue(object, new TypeReference<Map<String, Object>>() {});

ibacher · 2024-06-10T14:57:04Z

src/main/java/com/ozonehis/eip/odooopenmrs/component/OdooComponent.java

+        logger.info("Creating odoo endpoint with uri: {} remaining: {} parameters: {} ", uri, remaining, parameters);
+        String[] parts = remaining.split("/", 2);
+        if (parts.length != 2) {
+            throw new IllegalArgumentException("Invalid URI format. The expected format is 'odoo:method/model'");


Here it would be good to actually return the used URL no?

Till this point we don't have the URL.

We have another debug log inside OdooProducer which should print the actual endpoint

log.debug("OdooProducer: Endpoint {} Model: {}, Method {} ", getEndpoint(), model, method);

ibacher · 2024-06-10T14:57:36Z

src/main/java/com/ozonehis/eip/odooopenmrs/component/OdooEndpoint.java

+    public String getEndpointBaseUri() {
+        return super.getEndpointBaseUri();
+    }


Seems like we can forgo this, right?

rbuisson

@VaishSiddharth , you should be better off depending on Ozone directly rather than adding all the Ozone configs directly as test resources.

VaishSiddharth · 2024-06-11T08:10:09Z

As per the discussion yesterday with @rbuisson we have decided to create a new PR #27 which will be used to get all the review comments and commits which address those comments. After the review process is done we will break the PR into two

Camel routes using camel-openmrs-fhir with Java DSL
Integration Tests

I will address the comments on this PR from @ibacher and @rbuisson and merge those commits in the combined PR.
Please avoid adding comments on this PR
cc: @corneliouzbett @mks-d @wluyima

VaishSiddharth · 2024-06-11T11:07:22Z

@VaishSiddharth , you should be better off depending on Ozone directly rather than adding all the Ozone configs directly as test resources.

Fixed with this commit
886a18d

wluyima · 2024-06-24T13:34:43Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooClient.java

+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.*;


We don't use wildcard imports as a convention

wluyima · 2024-06-24T13:38:54Z

src/main/java/com/ozonehis/eip/odooopenmrs/client/OdooClient.java

+                    common_config, "authenticate", asList(getDatabase(), getUsername(), getPassword(), emptyMap()));
+        } catch (XmlRpcException e) {
+            log.error("Cannot authenticate to Odoo server error: {}", e.getMessage());
+            throw new RuntimeException("Cannot authenticate to Odoo server");


If you're re-throwing the error, you might as well skip logging a message

wluyima · 2024-06-24T13:49:29Z

src/main/java/com/ozonehis/eip/odooopenmrs/component/OdooEndpoint.java

+/**
+ * Odoo component to integrate with Odoo XML RPC.
+ */
+@UriEndpoint(firstVersion = "1.0.0", scheme = "odoo", title = "Odoo", syntax = "odoo:method/model", producerOnly = true)


Why not set the method as a header or param? I think having something like odoo:get/re.user is less intuitive.

VaishSiddharth · 2024-06-24T13:50:55Z

@wluyima There is a combined PR which is up-to-date. Please do not review this PR.
#22 (comment)

wluyima · 2024-06-24T13:53:17Z

src/main/java/com/ozonehis/eip/odooopenmrs/handlers/CountryHandler.java

+    @Autowired
+    private OdooClient odooClient;
+
+    public Integer getCountryId(String countryName) {


Shouldn't this be getCountryByName?

VaishSiddharth marked this pull request as draft May 10, 2024 13:06

mks-d changed the title ~~OZ-545 | Refactor code to use openmrs-fhir~~ OZ-545: Refactor code to use openmrs-fhir May 15, 2024

VaishSiddharth requested review from rbuisson, mks-d, ibacher and corneliouzbett June 6, 2024 10:52

mks-d reviewed Jun 6, 2024

View reviewed changes

VaishSiddharth changed the title ~~OZ-545: Refactor code to use openmrs-fhir~~ OZ-545: Refactor code to use camel-openmrs-fhir Jun 6, 2024

VaishSiddharth changed the title ~~OZ-545: Refactor code to use camel-openmrs-fhir~~ OZ-545: Refactor code to use camel-openmrs-fhir and rewrite routes with Java DSL Jun 6, 2024

ibacher reviewed Jun 10, 2024

View reviewed changes

rbuisson requested changes Jun 10, 2024

View reviewed changes

VaishSiddharth mentioned this pull request Jun 11, 2024

OZ-545 & OZ-596: [Combined] Refactor code to use camel-openmrs-fhir and rewrite routes with Java DSL with Integration Tests #27

Closed

wluyima reviewed Jun 24, 2024

View reviewed changes

VaishSiddharth force-pushed the OZ-545 branch 4 times, most recently from 09ec556 to 76467ba Compare July 16, 2024 06:53

OZ-545: Rewrite xml routes using Java DSL

47d541c

VaishSiddharth force-pushed the OZ-545 branch from 4357509 to 47d541c Compare July 16, 2024 07:17

VaishSiddharth added 2 commits July 16, 2024 12:48

OZ-545: Add unit tests

df6af35

OZ-596: Added Integration tests for routes (#26)

b58f48c

corneliouzbett marked this pull request as ready for review July 16, 2024 08:19

corneliouzbett approved these changes Jul 16, 2024

View reviewed changes

corneliouzbett merged commit 9f030ec into main Jul 16, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OZ-545: Refactor code to use camel-openmrs-fhir and rewrite routes with Java DSL #22

OZ-545: Refactor code to use camel-openmrs-fhir and rewrite routes with Java DSL #22

VaishSiddharth commented May 8, 2024 •

edited

Loading

mks-d left a comment

ibacher left a comment

ibacher Jun 10, 2024

VaishSiddharth Jun 11, 2024

ibacher Jun 10, 2024

ibacher Jun 10, 2024

ibacher Jun 10, 2024

ibacher Jun 10, 2024

VaishSiddharth Jun 11, 2024 •

edited

Loading

ibacher Jun 10, 2024

ibacher Jun 10, 2024

VaishSiddharth Jun 11, 2024

ibacher Jun 10, 2024

ibacher Jun 10, 2024

VaishSiddharth Jun 11, 2024

ibacher Jun 10, 2024

rbuisson left a comment

VaishSiddharth commented Jun 11, 2024

VaishSiddharth commented Jun 11, 2024

wluyima Jun 24, 2024

wluyima Jun 24, 2024

wluyima Jun 24, 2024

VaishSiddharth commented Jun 24, 2024

wluyima Jun 24, 2024

	<fhir-stu4.version>7.0.0</fhir-stu4.version>
	<fhir-r4.version>7.0.0</fhir-r4.version>


		public class OdooUtils {

		private static final Logger log = LoggerFactory.getLogger(OdooUtils.class);

OZ-545: Refactor code to use camel-openmrs-fhir and rewrite routes with Java DSL #22

OZ-545: Refactor code to use camel-openmrs-fhir and rewrite routes with Java DSL #22

Conversation

VaishSiddharth commented May 8, 2024 • edited Loading

mks-d left a comment

Choose a reason for hiding this comment

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

VaishSiddharth Jun 11, 2024 • edited Loading

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

rbuisson left a comment

Choose a reason for hiding this comment

VaishSiddharth commented Jun 11, 2024

VaishSiddharth commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishSiddharth commented Jun 24, 2024

Choose a reason for hiding this comment

VaishSiddharth commented May 8, 2024 •

edited

Loading

VaishSiddharth Jun 11, 2024 •

edited

Loading