EA-205: Support loading dispositionConfig.json from the file system b… #248

mogoodrich · 2024-10-18T18:02:54Z

…y default

https://openmrs.atlassian.net/browse/EA-205

…y default

.../openmrs/module/emrapi/adt/reporting/evaluator/AwaitingAdmissionVisitQueryEvaluatorTest.java

mseaton · 2024-10-19T12:51:55Z

api/src/main/java/org/openmrs/module/emrapi/disposition/DispositionServiceImpl.java

@@ -31,7 +31,11 @@ public class DispositionServiceImpl extends BaseOpenmrsService implements Dispos
    private PathMatchingResourcePatternResolver resourceResolver = new PathMatchingResourcePatternResolver();

    // TODO inject this in some better way than using a setter to override?


I see where you are going with all of this. But I wonder if we would be better served focusing on resolving this TODO in such a way that emrapi can be properly configured in a way that neither relies on java or on using hard-coded default locations. A global property to define the location as either a file relative to the application data directory or a classpath resource would be the most straightforward probably.

Then, we would just need to modify config.xml in emrapi to:

Set up this global property by default with the current classpath location value (for backwards compatibility)

Set emrapi to be aware_of iniz so that iniz could be used to set this GP value before emrapi starts up

This will allow us to still choose (in the refapp distro, eg) to put the dispositionConfig.json into a configuration/dispositions/dispositionConfig.json path without making an explicit use of this directory in the emrapi mdoule itself, or we could use some location like configuration/refapp/dispositions.json or whatever

that TODO is old, but I do see the value in just implementing that using a GP while we are at it, and I can go ahead and do that.

That being said, I still think it's good to provide some out-of-the-box defaults, and I think "configuratoin/dispoistions" in the best place for this (see my comments below) and I still want to support "dispositionConfig.json" for backwards compatibility.... (so I can implement the GP think but I still think I'd want to support the two "fallback" locations). @mseaton

See my comments below.

mseaton · 2024-10-19T12:53:27Z

api/src/main/java/org/openmrs/module/emrapi/disposition/DispositionServiceImpl.java

-    private String dispositionConfig = "dispositionConfig.json";
+    private String dispositionConfig;
+
+    private final String DEFAULT_DISPOSITION_CONFIG_LOCATION = "file:" + OpenmrsUtil.getApplicationDataDirectory() + "/configuration/dispositions/dispositionConfig.json";


There is a bit of a tension around who should define and maintain namespaces within the configuration directory. Iniz has really staked out this space, and although we have had other modules also use this space directly (reporting, addresshierarchy), it is discouraged in order to prevent conflicts and we have worked to migrate those over to iniz. If we are going to choose a default, I'd probably just add it to the top-level application data directory

I was worried about that, but @mseaton see comments on https://openmrs.atlassian.net/browse/EA-205 where Romain argues that this is the best place for it, and this would be my default ideal place for it as well, so it Mekom has no objections my preference is to put it in the configuration by default.

I don't read that in the ticket comments @mogoodrich . There is no place in those comments where Mekom agrees or advocates for a configuration/dispositions folder. I'm not inherently opposed to it, but I think if we put it there then it should ideally be managed by Iniz - eg. you'd make Iniz aware-of emrapi, and then you'd have a DispositionsLoader that read from a dispositions domain, and configures the EMR API module in whatever way is appropriate (eg. by directly calling the setting on DispositionService). Anyway, it is worth more discussion.

Interested in your thoughts, @mks-d @rbuisson ...I really do believe this belongs in the "configuration" directory, and "dispositions" really seems to follow the pattern we are using for other metadata. If you agree that by placing it in "configuration" it really should be configured by Iniz, I'm happy to create DispositionLoader if you are willing to review, but I don't want to do that until I confirm that you would prefer that over the approach in this PR.

@mogoodrich I trust that if you guys have come to the conclusion that dispositions are configurable entities that are part of OpenMRS' master data, then it can belong to Iniz.

Will that require Iniz to be aware of EMR API?

@mks-d it depends, ut generally, right now... EMR-API loads "dispositionConfig.json" directly from the configuration directory, and no changes to Iniz are needed. This works for me if it works for you, but it doesn't really make it "belong" to Iniz... it "belongs" to EMR-API and "lives" in the "configuration". If you would prefer that everything in the "configuration" directory is directly loaded by the "Iniz" module, we could change this, but, then, yes, Iniz would need to be aware of EMR API.

If we kept it as-is, we could make sure it's documented within the Iniz README, even if Iniz doesn't take much ownership for it.

mseaton · 2024-10-19T12:54:39Z

api/src/main/java/org/openmrs/module/emrapi/disposition/DispositionServiceImpl.java

@@ -62,9 +66,47 @@ public DispositionDescriptor getDispositionDescriptor() {

    @Override
    public List<Disposition> getDispositions() {
-        return getDispositionsFrom(dispositionConfig);
+
+        if (dispositionConfig !=null) {


Spacing here is off

mogoodrich · 2024-10-21T22:26:53Z

No new changes @mseaton , but flagged you for re-review so you can weigh in on my two comments.

…y default

mogoodrich · 2024-10-24T20:47:14Z

api/src/main/java/org/openmrs/module/emrapi/disposition/DispositionServiceImpl.java

                path = "file:" + OpenmrsUtil.getApplicationDataDirectory() + "/" + configFile.replace("file:","");
            }
+            else {  // this is an absolute file reference
+                path = configFile;
+            }


This is the only remaining change... Iniz is going to load via an absolute path, so EMR-API needs to be smart ignore to recognize this and not prepend the application data directory path.

We may not actually want or need this anymore, given feedback on the Iniz ticket, right @mogoodrich ? I think it's a matter of whether or not we think it is a good idea to support absolute paths or not.

Yep, your point on the other PR about the dangers of supporting absolute path makes sense, duh on my part. This can likely go away

…name

mogoodrich · 2024-10-25T13:52:01Z

So at the end of the day, loading dispositionConfig will be done by Iniz, so I've pulled out all the changes here.

However, I'm keeping the change to rename the test dispositionConfig.json to an explicit "test" name and tweaking one comment to note that we will be using Iniz to load the disposition config. Merging these changes in.

…name

# Conflicts: # api/src/test/java/org/openmrs/module/emrapi/adt/AdtServiceComponentTest.java

EA-205: Support loading dispositionConfig.json from the file system b…

afb7ce8

…y default

mogoodrich commented Oct 18, 2024

View reviewed changes

.../openmrs/module/emrapi/adt/reporting/evaluator/AwaitingAdmissionVisitQueryEvaluatorTest.java Show resolved Hide resolved

mogoodrich requested review from mseaton and rbuisson October 18, 2024 18:07

mseaton reviewed Oct 19, 2024

View reviewed changes

mogoodrich requested a review from mseaton October 21, 2024 22:26

EA-205: Support loading dispositionConfig.json from the file system b…

044d370

…y default

mogoodrich commented Oct 24, 2024

View reviewed changes

mogoodrich requested a review from mks-d October 24, 2024 20:47

(chore) move location of test disposition file to give explicit test …

96391d7

…name

mogoodrich added 2 commits October 25, 2024 09:52

(chore) move location of test disposition file to give explicit test …

17ef6d7

…name

Merge branch 'master' into EA-205

879865f

# Conflicts: # api/src/test/java/org/openmrs/module/emrapi/adt/AdtServiceComponentTest.java

mogoodrich merged commit 8b657bf into master Oct 25, 2024
1 check passed

mogoodrich deleted the EA-205 branch October 25, 2024 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EA-205: Support loading dispositionConfig.json from the file system b… #248

EA-205: Support loading dispositionConfig.json from the file system b… #248

mogoodrich commented Oct 18, 2024 •

edited

Loading

mseaton Oct 19, 2024

mogoodrich Oct 21, 2024 •

edited

Loading

mseaton Oct 22, 2024

mseaton Oct 19, 2024

mogoodrich Oct 21, 2024 •

edited

Loading

mseaton Oct 22, 2024

mogoodrich Oct 23, 2024

mks-d Oct 24, 2024

mogoodrich Oct 24, 2024

mseaton Oct 19, 2024

mogoodrich commented Oct 21, 2024

mogoodrich Oct 24, 2024

mseaton Oct 25, 2024

mogoodrich Oct 25, 2024

mogoodrich commented Oct 25, 2024

		@@ -31,7 +31,11 @@ public class DispositionServiceImpl extends BaseOpenmrsService implements Dispos
		private PathMatchingResourcePatternResolver resourceResolver = new PathMatchingResourcePatternResolver();

		// TODO inject this in some better way than using a setter to override?

EA-205: Support loading dispositionConfig.json from the file system b… #248

EA-205: Support loading dispositionConfig.json from the file system b… #248

Conversation

mogoodrich commented Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

mogoodrich Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogoodrich Oct 21, 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

mogoodrich commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogoodrich commented Oct 25, 2024

mogoodrich commented Oct 18, 2024 •

edited

Loading

mogoodrich Oct 21, 2024 •

edited

Loading

mogoodrich Oct 21, 2024 •

edited

Loading