OCLM-78 - Enable importing single concept exports from OCL #84

mseaton · 2021-10-02T21:31:21Z

This adds the ability to import a single concept export from OCL into an OpenMRS instance via the existing import page. See demo below of the code within this PR in action:

What this PR does:

Removes the "zip" file restriction, and supports uploading either a zip file or a json file, both for the existing Collection import, and for the new single-concept import
Adds a new subdirectory to the the existing .OpenMRS/ocl directory for .OpenMRS/ocl/imports.
** Copies any uploaded file into this "imports" directory once it has an import attempt, with a timestamp differentiator
** Changes the existing logic that set the import URL to the .OpenMRS/ocl/configuration/loadAtStartup for file imports, and instead set the import URL to the path to the saved file within the imports directory
Adding the necessary logic to the Importer class to support:
** Importing either zip or json
** Importing a single concept in addition to a collection
Changed Saver.saveMapping to look existing Concepts up by mapping code rather than by uuid (this was necessary as the concept I'm importing had answers that exist in my dictionary with CIEL mappings already on them, but without a CIEL uuid). Looking up by mapping allowed me to import this concept and associate the correct answers without re-importing those answers or having to change my existing uuids. Note: this likely will solve the same problem that Ellen reported in OCLOMRS-1008
Updated the UI to allow for uploading json as well as zip files, and for uploading single-concept exports as well as collection exports.

ibacher

Generally I'm in favour of these changes, especially looking up concepts by mappings before assuming they don't exist, but I think we need some additional discussion around this point:

instead set the import URL to the path to the saved file within the imports directory

The problem here is that this seems to break the "happy path" flow for OCL import. As I understand things, the idea with file import wasn't "this is a quick way to get concepts into your dictionary", but rather "our site can't connect to the OCL server; here's a way we can load the dictionary offline."

In other words, the subscription URL used would be the same regardless of whether the concept came from an export from OCL or was uploaded via a zip file. This change breaks that flow as concepts imported via a zip file will no longer have the same "identity" as concepts imported via a download from OCL.

Finally, I think there's a slightly more philosophical question: does this module exist to get content from OCL into OpenMRS or does it exist to allow OCL to be used to manage OpenMRS dictionaries, because those are slightly different outlooks that lead to somewhat different conclusions. I.e., if the purpose of this module is just to get content from OCL into OpenMRS, then this is a logical bit of functionality to have. However, if the purpose is to have collection in OCL and be able to say "this collection reflects what's on my server", this functionality kind of breaks that since what is actually on the server depends on the order in which things were processed (e.g., if there's a conflict between what was loaded locally from a file and what OCL says).

api/src/main/java/org/openmrs/module/openconceptlab/importer/Importer.java

ibacher · 2021-10-05T12:50:13Z

api/src/main/java/org/openmrs/module/openconceptlab/importer/Importer.java

+	private void closeInputStream() {
+		IOUtils.closeQuietly(in);
+	}


I'd probably rather refactor things to use try-with-resources, e.g., by returning the input stream from this method.

I'll need to look back at the code but there was a reason I needed to do it this way. I think it was because there are 2 paths here - one in which an Input Stream is coming from a File, and another in which an Input Stream is coming from the subscription. If we can make this work with a try-with-resources, I'm all for it, but we'd need to look at why this is like this.

ibacher · 2021-10-05T13:00:23Z

api/src/main/java/org/openmrs/module/openconceptlab/importer/Importer.java

+									else if (oclMapping.getToSourceName().equals(oclConcept.getSource()) && oclMapping.getToConceptCode().equals(oclConcept.getId())) {
+										log.debug("Skipping SAME-AS Mapping for OCL ID as this is added in the ImportTask, and would result in duplicate mappings");
+									}


I'll be honest: this probably a guard we should have in the ImportTask rather than here.

It's strange that this is even needed. Shouldn't we just add mappings from mappings? If the CIEL mapping will always be present under mappings, then we should just add it there and not have that other custom code that adds the CIEL mapping when the concept is created. Right?

Right... the custom code was added to work around the fact that the import of CIEL wasn't including those SAME-AS maps (and to deal with the fact that dictionaries don't necessarily have the SAME-AS mapping to their concept id.

ibacher · 2021-10-05T13:02:02Z

api/src/main/java/org/openmrs/module/openconceptlab/importer/Importer.java

+									// We do not import Q-AND-A mappings for which the imported concept is an answer
+									boolean isQuestionAnswer = "Q-AND-A".equals(oclMapping.getMapType());
+									if (isQuestionAnswer && oclMapping.getToConceptUrl().equals(oclConcept.getUrl())) {
+										log.debug("Skipping Q-AND-A mapping for which the imported concept is a mapped answer");
+									}


Is this a common occurrence? Also, isn't this something that should be fixed outside of this module, i.e., at the concept source itself?

Yes, it is common - anytime you are exporting a concept that is the answer to a question. Look at the export for "TRUE" in OCL - it has 710 "associations", most of which are Q-AND-A mappings for which it is an answer:
https://app.openconceptlab.org/#/orgs/CIEL/sources/CIEL/concepts/1065/

The assumption / decision I made was to not worry about these associations when exporting a concept that is an answer, only worry about those when exporting a question that has this answer. Make sense?

I understand that associations like this are common, but they aren't generally included in the file, depending on how it's generated. E.g., when I download that concept as JSON from the OCL Term Browser, it actually has no mappings in it at all.

Attachment

If I download the concept from the API, I can either get the concept with no mappings at:

https://api.openconceptlab.org/orgs/CIEL/sources/CIEL/concepts/1065/

Or with mappings where the concept is the from concept:

https://api.openconceptlab.org/orgs/CIEL/sources/CIEL/concepts/1065/?includeMappings=true

Or with mappings where the concept is the to concept:

https://api.openconceptlab.org/orgs/CIEL/sources/CIEL/concepts/1065/?includeInverseMappings=true

I can also combine those filter. The point was that, in general, I wouldn't expect someone to end up with a bunch of mappings where the concept is the to concept and if we do we need to guard against it more generally.

That's cool that there is that flexibility in the API. Unfortunately, that same flexibility isn't in the term browser, and the term browser that I see does contain all of those mappings. This is what I was using to download a Concept.

My experience with OCL is that there is a lot of inconsistency in each of the various pages you might go to in terms of what is included in an export file. For a Concept like this you get either JSON or ZIP. For other Collections or Dictionaries, you sometimes get JSON, sometimes CSV, sometimes ZIP formats, and these formats are not necessarily consistent with each other (as you say, sometimes they contain mappings and other details, sometimes they do not).

Ah... that's fair. Actually this would be a good thing to put more centrally in mapping exclusions because I suppose the export format could change whether intentionally or accidentally to include inverse mappings.

I see... I didn't quite get that the JSON and Zip file there give you very different versions of the concept. Well, different in respect to mappings.

@ibacher I created this issue in OCL (and it was already resolved) as I worked on this. But I don't think this is the extent of the discrepencies that exist in all of the different exports across the UI and API. If you are in close contact with the OCL team, this is an area I'd like to advocate gets:
a) work done to make exports more consistent overall, across the application
b) work done to document the export formats and what one can expect from each.
If this is already done, I may just need to be pointed to the docs.

ibacher · 2021-10-05T13:06:34Z

owa/app/js/home/home.controller.js

    }

-    function uploadZip(file) {
+    function uploadZip(file, importType) {


SHould probably rename this to uploadFile(), right?

Yes, good catch. I'm happy to go in and make various changes due to feedback like this, but let's agree first that this is even something we want in this module in the first place, and if that get's agreed I'll tidy things up. @ibacher ?

mseaton · 2021-10-05T15:01:59Z

This change breaks that flow as concepts imported via a zip file will no longer have the same "identity" as concepts imported via a download from OCL

I admit to not really understanding yet the nature of the importance of this import URL, and it was not clear to me that this had anything to do with the "identity" of a concept. It isn't clear to me that the current implementation - which just sets the import URL to the location specified in the global property "ocl/configuration/loadAtStartup" - was actually done meaningfully, as this a) doesn't point to a specific file, and b) any file placed in there is immediately deleted once it is processed at startup.

Finally, I think there's a slightly more philosophical question: does this module exist to get content from OCL into OpenMRS or does it exist to allow OCL to be used to manage OpenMRS dictionaries,

This is certainly a fair point. I don't honestly know. I approached this as follows:

I wanted to get a single concept onto my server from OCL. I wanted it to have the same UUID as the one defined on OCL. There is no way to do this in OpenMRS other than via direct SQL (you can't set a concept's uuid via the create concept UI). And I was able to download a structured JSON file from OCL that had everything in it that I needed, so this felt like a good place to start.
The "openconceptlab" module already has almost everything I needed in place (or at least I thought it did, it turned out that it didn't entirely). In that - it already had the model objects and parsers for reading in an OCL JSON export, and it already had the importers in place to create concepts/names/mappings from this Object model. So, it seemed like it made sense to build on this and not re-invent the wheel in yet another module

Personally I'd like to see this module be about how to usefully and practically manage (or simply find shared) Concepts defined in OCL and import those into one's dictionary. Subscription is one possible approach to this, but to be honest, in terms of getting comfortable adopting OCL, I'm much more comfortable starting out with a single concept, whose impact I can clearly understand and test on my dictionary, than starting with a subscription.

If we want to make this purely about subscription, that's fine, but we might find it useful to develop a small shared library (i.e. like the hapi APIs) that can be shared and used for reading OCL export files and converting these to OpenMRS concepts.

…mporter.java Co-authored-by: Ian <[email protected]>

ibacher · 2021-10-18T17:09:44Z

@mseaton Yeah, I was probably a bit over-the-top with that. I think, though, we need to distinguish between uploading Zip files (which should use the global property-stored API URL if any) and uploading JSON files (which can just use the file URL). That way we get the "load single concept" functionality without losing the offline load part.

ibacher · 2021-10-18T17:10:12Z

Also, I missed the fact that OCL now has SAME-AS mappings for CIEL... another thing to worry about.

mogoodrich · 2021-10-20T18:33:32Z

@ibacher @mseaton I've been following this all, but probably not as deeply as I could have... generally looks good to me, if there's something in particular you want my feedback on, let me know...

mseaton · 2021-10-20T18:41:02Z

generally looks good to me, if there's something in particular you want my feedback on, let me know

At the end of the day, I'm not planning to push hard on getting this merged in. If it's too controversial or inconsistent with the plan for the module, that's fine to let it sit here in a branch. I do think that working through this PR has uncovered some issues that need thought and review overall. We might need a design discussion to work through next steps here. @ibacher ?

ibacher · 2021-10-20T18:43:57Z

I'm actually working through cleaning this up a bit right now because I agree this has uncovered some issues and the idea of being able to just download a concept from OCL and import it into the UI is pretty compelling.

OCLM-78 - Enable importing single concept exports from OCL

9bfc3db

mseaton requested review from mogoodrich, moshonk and ibacher October 2, 2021 21:31

ibacher reviewed Oct 5, 2021

View reviewed changes

Update api/src/main/java/org/openmrs/module/openconceptlab/importer/I…

223c244

…mporter.java Co-authored-by: Ian <[email protected]>

Merge branch 'master' into OCLM-78

6628fb7

Changes from code review

71a7af9

ibacher mentioned this pull request Nov 15, 2021

OCLOMRS-1008: Use existing mapped concepts instead of creating new ones #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCLM-78 - Enable importing single concept exports from OCL #84

OCLM-78 - Enable importing single concept exports from OCL #84

mseaton commented Oct 2, 2021

ibacher left a comment

ibacher Oct 5, 2021

mseaton Oct 5, 2021

ibacher Oct 5, 2021

mseaton Oct 5, 2021

ibacher Oct 18, 2021

ibacher Oct 5, 2021

mseaton Oct 5, 2021

ibacher Oct 18, 2021

mseaton Oct 19, 2021 •

edited

Loading

ibacher Oct 20, 2021

ibacher Oct 20, 2021 •

edited

Loading

mseaton Oct 20, 2021

ibacher Oct 5, 2021

mseaton Oct 5, 2021 •

edited

Loading

mseaton commented Oct 5, 2021

ibacher commented Oct 18, 2021

ibacher commented Oct 18, 2021

mogoodrich commented Oct 20, 2021

mseaton commented Oct 20, 2021

ibacher commented Oct 20, 2021

OCLM-78 - Enable importing single concept exports from OCL #84

Are you sure you want to change the base?

OCLM-78 - Enable importing single concept exports from OCL #84

Conversation

mseaton commented Oct 2, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseaton Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseaton Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

mseaton commented Oct 5, 2021

ibacher commented Oct 18, 2021

ibacher commented Oct 18, 2021

mogoodrich commented Oct 20, 2021

mseaton commented Oct 20, 2021

ibacher commented Oct 20, 2021

mseaton Oct 19, 2021 •

edited

Loading

ibacher Oct 20, 2021 •

edited

Loading

mseaton Oct 5, 2021 •

edited

Loading