FM2-130:Adding support for the FHIR Media resource #328

tendomart · 2021-02-15T09:54:09Z

Description of what I changed

Adding support for the FHIR Media resource

Issue I worked on

see https://issues.openmrs.org/browse/FM2-

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

@ibacher @pmanko

https://issues.openmrs.org/browse/FM2-130

tendomart · 2021-02-15T10:00:32Z

@ibacher @pmanko

1 .I just want to know wether this Handler has the required method implementation for doing the required business.

2.Would it make sense to design multiple ComplexObsHandler implementations for various MIME Types ?

into FM2-130

ibacher

@tendomart Thanks for this! I've left you some comments here and some additional pointers on the Jira issue. Please read through them and let me know if there are any points you need me to clarify, etc.

ibacher · 2021-03-30T16:24:53Z

api/src/main/java/org/openmrs/module/fhir2/api/FhirMediaService.java

+
+public interface FhirMediaService extends FhirService<Observation> {
+
+	Observation get(@Nonnull String uuid);


This seems to be unnecessary as it's already defined by the parent interface.

ibacher · 2021-03-30T16:26:02Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirMediaDaoImpl.java

+
+@Component
+@Setter(AccessLevel.PACKAGE)
+public class FhirMediaDaoImpl extends BaseFhirDao<Obs> implements FhirMediaDao, ComplexObsHandler {


This class definitely shouldn't be a ComplexObsHandler. That interface is only for classes which implement a specific type of ComplexObs, e.g., ImageHandler or PdfHandler or something like that.

ibacher · 2021-03-30T16:26:54Z

api/src/main/java/org/openmrs/module/fhir2/api/handler/FhirMediaComplexObsHandler.java

+@Component
+@Setter(AccessLevel.PUBLIC)
+@Order(Ordered.LOWEST_PRECEDENCE)
+public class FhirMediaComplexObsHandler implements ComplexObsHandler, CustomDatatypeHandler {


We really, really don't need a FHIR-specific ComplexObsHandler we can rely on the ObsService to populate the ComplexObs data (if any)

ibacher · 2021-03-30T16:27:47Z

api/src/main/java/org/openmrs/module/fhir2/api/impl/FhirMediaServiceImpl.java

+@Transactional
+@Setter(AccessLevel.PACKAGE)
+@Getter(AccessLevel.PROTECTED)
+public class FhirMediaServiceImpl implements FhirMediaService {


Is there a reason this doesn't extend BaseFhirService as that provides default implementations for all of the methods except search.

ibacher · 2021-03-30T16:28:49Z

api/src/main/java/org/openmrs/module/fhir2/api/translators/impl/MediaTranslatorImpl.java

+
+@Component
+@Setter(AccessLevel.PACKAGE)
+public class MediaTranslatorImpl implements MediaTranslator {


For the mappings to be added here, look at the ticket and the way we generally map those properties. Most of them have overlap with Obs, so the ObservationTranslatorImpl class would be a good place to start.

ibacher · 2021-03-30T16:29:30Z

api/src/main/java/org/openmrs/module/fhir2/providers/r4/MediaFhirResourceProvider.java

+@Component("mediaFhirR4ResourceProvider")
+@Qualifier("fhirResources")
+@Setter(AccessLevel.PACKAGE)
+public class MediaFhirResourceProvider implements IResourceProvider {


We need some actual resource methods here, similar to the other FhirResourceProvider classes.

ibacher · 2021-03-30T16:29:40Z

api/src/main/resources/moduleApplicationContext.xml

+    <bean name="handler" class="org.openmrs.module.fhir2.api.handler.FhirMediaComplexObsHandler">
+        <property name="handlers">
+            <map>
+                <entry>
+                    <key><value>mediaHandler</value></key>
+                </entry>
+            </map>
+        </property>
+    </bean>


Suggested change

<bean name="handler" class="org.openmrs.module.fhir2.api.handler.FhirMediaComplexObsHandler">

<property name="handlers">

<map>

<entry>

<key><value>mediaHandler</value></key>

</entry>

</map>

</property>

</bean>

…, dao and provider classes

into FM2-130

api/src/main/java/org/openmrs/module/fhir2/api/impl/FhirMediaServiceImpl.java

corneliouzbett · 2021-04-07T06:30:32Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirMediaDaoImpl.java

+	private void handleMediaCreatedDate(Criteria criteria, DateRangeParam mediaCreatedDate) {
+		if(mediaCreatedDate != null){
+			if(lacksAlias(criteria, "dt")){
+					criteria.createAlias("mediaCreatedDate", "dt");
+			}
+		}
+	}


This is superfluous. Ideally, you should re-use already existing functionality. i.e instead of this 👇🏾

case FhirConstants.MEDIA_CREATED_DATE_TIME: entry.getValue().forEach(createdDateTime -> handleDateRange(createdDateTime.getPropertyName(), (DateRangeParam) createdDateTime.getParam()).ifPresent(criteria::add)); break;

re-use handleDateRange() which somehow you are using it. Probably, you should replace with;

case FhirConstants.DATE_RANGE_SEARCH_HANDLER: entry.getValue().forEach(dateRangeParam -> handleDateRange(dateRangeParam.getPropertyName(), (DateRangeParam) dateRangeParam.getParam()).ifPresent(criteria::add)); break;

No need for the new constant and new method handleMediaCreatedDate(). This applies to encounterReference and the subjectReference - patient reference. Try re-using already existing functionality as much as possible.

corneliouzbett · 2021-04-07T06:33:13Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/BaseDao.java

 *         crt.concept_source_id = (select concept_source_id from fhir_concept_source where url = ?)
- *     AND crt.code = ?
+ *     AND crt.code = ?fhir_concept_source


This doesn't seem right to me. Why did you change the example query?

api/src/main/java/org/openmrs/module/fhir2/api/search/SearchQueryInclude.java

tendomart · 2021-04-07T06:40:30Z

oops true .Thanks @corneliouzbett this happened in my last commit yesterday . Prior to that all was well .And i didn't get time to review yesterday .Thanks again

api/src/main/java/org/openmrs/module/fhir2/FhirConstants.java

api/src/main/java/org/openmrs/module/fhir2/api/impl/FhirMediaServiceImpl.java

tendomart · 2021-04-13T11:20:38Z

Thanks @ibacher so then what will we use to map the remaining searchParams as most of the Fhir Media Params have no direct correlations with the Openmrs Obs and no handler implementations from fhir as well .

into FM2-130

ibacher · 2021-04-21T12:59:27Z

@tendomart Hopefully it's pretty straight-forward to create handle...() methods modelled on those from Obs or elsewhere in the code base.

tendomart · 2021-04-21T15:01:52Z

Alright thanks alot .Sorry for late response , been doing some house cleaning on my Jira Dashboard .

into FM2-130

ibacher · 2021-05-06T14:55:36Z

api/src/test/java/org/openmrs/module/fhir2/api/translators/impl/MediaTranslatorImplTest.java

+package org.openmrs.module.fhir2.api.translators.impl;
+
+import static org.hamcrest.Matchers.notNullValue;
+import static org.springframework.test.util.MatcherAssertionErrors.assertThat;


Suggested change

import static org.springframework.test.util.MatcherAssertionErrors.assertThat;

import static org.hamcrest.MatcherAssert.assertThat;

ibacher · 2021-05-06T14:56:52Z

api/src/test/java/org/openmrs/module/fhir2/api/dao/impl/FhirObservationDaoImplTest.java

-import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.notNullValue;
-import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.*;


Don't use star imports

ibacher · 2021-05-06T14:57:49Z

api/src/main/java/org/openmrs/module/fhir2/providers/r3/MediaFhirResourceProvider.java

+import org.springframework.beans.factory.annotation.Qualifier;
+import org.springframework.stereotype.Component;
+
+@Component("mediaFhirR4ResourceProvider")


This needs to have a different name, e.g., mediaFhirR3ResourceProvider. This is one of the things causing testing errors.

ibacher · 2021-05-06T14:58:31Z

api/src/main/java/org/openmrs/module/fhir2/providers/r3/MediaFhirResourceProvider.java

+import org.springframework.stereotype.Component;
+
+@Component("mediaFhirR4ResourceProvider")
+@Qualifier("fhirResources")


Suggested change

@Qualifier("fhirResources")

@R3Provider

ibacher · 2021-05-06T14:58:45Z

api/src/main/java/org/openmrs/module/fhir2/providers/r4/MediaFhirResourceProvider.java

+import org.springframework.stereotype.Component;
+
+@Component("mediaFhirR4ResourceProvider")
+@Qualifier("fhirResources")


Suggested change

@Qualifier("fhirResources")

@R4Provider

tendomart · 2021-05-07T09:45:13Z

@ibacher I just want to ask ,is this the new way of , to replace @Qualifier("fhirResources") with @R4Provider

tendomart · 2021-05-27T14:23:53Z

Hi @ibacher i have tried to look around for a solution but failed to figure out why

```

@OverRide
public Obs get(@nonnull String uuid) {
return obsService.getObsByUuid(uuid);
}

@Override
public Obs createOrUpdate(@Nonnull Obs newEntry) {
	return obsService.saveObs(newEntry, FhirConstants.SAVED_SUCCESSFULLY);
}

@Override
public Obs delete(@Nonnull String uuid) {
	return super.delete(uuid);
}

either throws a NPE or returns a Null Object when actually the same implementation in `FhirConceptDaoImpl` works .Any thoughts.

ibacher · 2021-05-27T15:10:12Z

api/src/test/java/org/openmrs/module/fhir2/api/dao/impl/FhirMediaDaoImplTest.java

+
+	@Test
+	public void get_shouldGetComplexObsByUuid() {
+		assertThat(dao.get("OBS_UUID"), notNullValue());


Suggested change

assertThat(dao.get("OBS_UUID"), notNullValue());

assertThat(dao.get(OBS_UUID), notNullValue());

ibacher · 2021-05-27T15:13:11Z

api/src/test/java/org/openmrs/module/fhir2/api/dao/impl/FhirMediaDaoImplTest.java

+
+	private static final String OBS_DATA_XML = "org/openmrs/module/fhir2/api/dao/impl/FhirObservationDaoImplTest_initial_data_suppl.xml";
+
+	private static final String OBS_UUID = "759a0d9e-ccf8-4f00-a045-6a94c43fbd6b";


Where did this UUID come from? In order to be able to load it from the database, it needs to be an Obs defined correctly in data that are loaded for this test. You probably want to ensure this test data is loaded and use the UUID from that.

ibacher · 2021-05-27T15:21:20Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirMediaDaoImpl.java

+
+	@Override
+	protected void setupSearchParams(Criteria criteria, SearchParameterMap theParams) {
+		theParams.getParameters().forEach(entry -> {


Here, we'll want to ensure that we're only searching ComplexObs and not just any Obs. This looks like it could be done as something like:

if (lacksAlias(criteria, "c")) { criteria.createAlias("concept", "c"); } if (lacksAlias(criteria, "cdt")) { criteria.createAlias("c.datatype", "cdt"); } criteria.add(eq("cdt.hl7_abbreviation", "ED"));

tendomart · 2021-06-06T12:58:09Z

Thanks @ibacher for bringing this up . Iam still finding some trouble defining the search param . I have already identified them at https://www.hl7.org/fhir/DSTU2/media.html but i really don't understand the meaning of the aliases used here in any case .

For example in this case who will be using "c" ,"cd" and "EDT" . And why are you choosing , "concept" , "c.datatype" and "cdt.hl7_abbreviation" ? Only
Any clear documentation for this ?

into FM2-130

ibacher · 2021-08-31T15:47:53Z

@tendomart Sorry... I somehow missed the above question. The "c" and "cd" above are really just aliases for tables inside the criteria query. Just like in SQL you could write something like (this is similar to the query my code above is intended to generate):

select *
from concept c
join concept_data_type cdt on
  c.data_type_id = cdt.concept_data_type_id
where cdt.hl7_abbreviation = "ED"

Having an alias just allows us to refer to the property name in a shorter fashion.

The "ED" part actually comes from the code we use to determine whether or not a concept is a complex concept. See here.

FM2-130:Adding support for the FHIR Media resource

d785e97

tendomart added 9 commits February 16, 2021 04:49

Refining Media Complex Obs Handler

68e5bd4

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

d508846

into FM2-130

Adding Search params , Service and Translator Layers

39be2b1

Doing more refining

0aca54d

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

b28be07

into FM2-130

Implementing fhir media translator

53d8ff7

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

ba5c351

into FM2-130

Doing cleaning

027f483

Removing duplicate code

9fda48d

ibacher requested changes Mar 30, 2021

View reviewed changes

ibacher marked this pull request as draft March 30, 2021 16:30

tendomart added 3 commits March 31, 2021 13:10

Doing more refactorings and adding unit tests for Tranlator , service…

5041fa2

…, dao and provider classes

Fine tuning Media search params doing other changes

f4a4bed

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

1ba904b

into FM2-130

theanandankit reviewed Apr 3, 2021

View reviewed changes

api/src/main/java/org/openmrs/module/fhir2/api/impl/FhirMediaServiceImpl.java Show resolved Hide resolved

tendomart added 2 commits April 6, 2021 18:24

Doing more refactoring

dd4b80e

Removing commented out lines

da7d841

corneliouzbett requested changes Apr 7, 2021

View reviewed changes

ibacher reviewed Apr 7, 2021

View reviewed changes

api/src/main/java/org/openmrs/module/fhir2/FhirConstants.java Outdated Show resolved Hide resolved

api/src/main/java/org/openmrs/module/fhir2/api/impl/FhirMediaServiceImpl.java Outdated Show resolved Hide resolved

tendomart added 2 commits April 7, 2021 16:04

Doing more rectifications

aea79f8

Adding search params and FhirMediaDaoImplTest

b26adb3

tendomart added 3 commits April 14, 2021 18:20

Adding Media element translators

d2625e6

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

74103bc

into FM2-130

Deleting unnecessary Media Translators

119fb76

tendomart added 5 commits April 28, 2021 18:57

Refining Translators

619d08b

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

ee13f88

into FM2-130

Refining MediaContentTranslator and Accompanying tests

86e2ca9

Refactoring translators

a8c2605

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

3933575

into FM2-130

ibacher reviewed May 6, 2021

View reviewed changes

tendomart added 2 commits May 7, 2021 18:05

Rectifying project build failure

93f2f45

Doing more fixes

9c843e8

ibacher reviewed May 27, 2021

View reviewed changes

Dealing with FhirMediaDaoImplTest failing tests

bb9dbdf

tendomart added 3 commits June 7, 2021 15:18

Making FhirMediaDaoImpl CRUD methods work

b6888be

Merge branch 'master' of https://github.com/openmrs/openmrs-module-fhir2

6d2e407

into FM2-130

Removing redundant code

5c79a15

ibacher force-pushed the master branch from ccce482 to 79ae139 Compare August 2, 2021 17:34

ibacher force-pushed the master branch from 093c526 to 48f9308 Compare August 23, 2022 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FM2-130:Adding support for the FHIR Media resource #328

FM2-130:Adding support for the FHIR Media resource #328

tendomart commented Feb 15, 2021 •

edited

tendomart commented Feb 15, 2021

ibacher left a comment

ibacher Mar 30, 2021

ibacher Mar 30, 2021

ibacher Mar 30, 2021

ibacher Mar 30, 2021

ibacher Mar 30, 2021

ibacher Mar 30, 2021

ibacher Mar 30, 2021

corneliouzbett Apr 7, 2021

corneliouzbett Apr 7, 2021

tendomart commented Apr 7, 2021 •

edited

tendomart commented Apr 13, 2021

ibacher commented Apr 21, 2021

tendomart commented Apr 21, 2021

ibacher May 6, 2021

ibacher May 6, 2021

ibacher May 6, 2021

ibacher May 6, 2021

ibacher May 6, 2021

tendomart commented May 7, 2021

tendomart commented May 27, 2021

ibacher May 27, 2021

ibacher May 27, 2021

ibacher May 27, 2021

tendomart commented Jun 6, 2021 •

edited

ibacher commented Aug 31, 2021


		public interface FhirMediaService extends FhirService<Observation> {

		Observation get(@Nonnull String uuid);

	import static org.springframework.test.util.MatcherAssertionErrors.assertThat;
	import static org.hamcrest.MatcherAssert.assertThat;

	assertThat(dao.get("OBS_UUID"), notNullValue());
	assertThat(dao.get(OBS_UUID), notNullValue());


		private static final String OBS_DATA_XML = "org/openmrs/module/fhir2/api/dao/impl/FhirObservationDaoImplTest_initial_data_suppl.xml";

		private static final String OBS_UUID = "759a0d9e-ccf8-4f00-a045-6a94c43fbd6b";

FM2-130:Adding support for the FHIR Media resource #328

Are you sure you want to change the base?

FM2-130:Adding support for the FHIR Media resource #328

Conversation

tendomart commented Feb 15, 2021 • edited

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

tendomart commented Feb 15, 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

Choose a reason for hiding this comment

tendomart commented Apr 7, 2021 • edited

tendomart commented Apr 13, 2021

ibacher commented Apr 21, 2021

tendomart commented Apr 21, 2021

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

tendomart commented May 7, 2021

tendomart commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tendomart commented Jun 6, 2021 • edited

ibacher commented Aug 31, 2021

tendomart commented Feb 15, 2021 •

edited

tendomart commented Apr 7, 2021 •

edited

tendomart commented Jun 6, 2021 •

edited