OCLOMRS-1008: Use existing mapped concepts instead of creating new ones #88
Conversation
|
I'll be honest, I don't understand the compilation failure. I'll look into it tomorrow. |
| */ | ||
| @Transactional(readOnly = true) | ||
| @Authorized(PrivilegeConstants.VIEW_CONCEPTS) | ||
| Concept getDuplicateConceptByMapping(String conceptId, String source); |
There was a problem hiding this comment.
I find this method name confusing. Can we just call it "getConceptWithSameAsMapping"? And name "conceptId" -> "code"?
There was a problem hiding this comment.
Well, the name arose because I was planning on eventually including Burke's "CODE" mapping if / when that comes about, but yeah, I suppose that's more accurate.
| <ref bean="openconceptlab.conceptService"/> | ||
| </list> | ||
| </property> | ||
| </bean> |
There was a problem hiding this comment.
Do we need to list this twice, or can we list it once and have the two services in the list?
There was a problem hiding this comment.
This is probably something we should fix at some point, but the setModuleServiceMethod() only processes the first two arguments, so it's necessary to invoke this twice.
| if (cacheService.getConceptByMapping(oclConcept.getSource(), oclConcept.getId()) != null) { | ||
| return new Item(thisImport, oclConcept, ItemState.UP_TO_DATE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you explain this block? I may need to see it better in context, but might this prevent updates from happening?
There was a problem hiding this comment.
This needs an extra guard, but the idea is this: If I have a concept in my dictionary marked SAME-AS CIEL:1066, then we shouldn't load the concept CIEL:1066 into the concept dictionary. The extra guard is to make sure that that item isn't the same as the item we're importing (based on the OCL URL).
Originally, I had this written to return an ItemState.DUPLICATE, but I wasn't sure it was worth the effort to add a new ItemState for this. I can add that back in, though, if it would make things clearer.
I had a weird experience the other day with a build failure I couldn't explain. I ended up fixing it by removing the Maven cache step from the github action. It had ended up with a bad artifact in the cache and was causing it to fail... |
Looks like you just tried this @ibacher and it didn't fix it. Weird. I'm running locally on Ubuntu 20 with Maven 3.6.3 and OpenJDK 1.8.0_292 and things work. Looks like GA is using a JDK from Azul Systems, version 1.8.0_212 and Maven 3.8.3. I suppose that might be where the issue lies? |
mseaton
left a comment
mseaton
left a comment
There was a problem hiding this comment.
LGTM, assuming we can get it to pass the build in github :)
|
Yeah... it works for me on AdoptOpenJDK 1.8.0_292 and Maven 3.8.1 locally. I've tried adjusting the build to use a simpler workflow and AdoptOpenJDK so it's as close to my environment as possible. |
…ing a SAME-AS concept mapped
ibacher
force-pushed
the
OCLOMRS-1008
branch
from
030f5ed to
9818086
Compare
ibacher
changed the title
…e concept exists
This includes one of the key features from #84 but in a slightly different way (instead of using
ConceptService#getConceptByMapping()this includes a custom implementation that depends on the SAME-AS mapping).