RESTWS-939: DiagnosisResource should support formFieldPath property

[IamMujuziMoses]

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-939

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

[IamMujuziMoses]

@samuelmale @mseaton @dkayiwa please review

[mseaton]

My first reaction is that I would you think you could accomplish adding this support without adding a new omod-2.5 submodule. As far as I can see in the code, you never access this property directly, only via reflection (via the string property name). You should be able to simply wrap those statements in a check as to whether version 2.5+ of core is running, rather than have an entire new submodule to support.

If we do find we need to keep the submodule, then I have feedback about changes that would be needed, but I would think we can eliminate it altogether and keep this far simpler with just a few conditional changes to the existing DiagnosisResource2_2

[IamMujuziMoses]

@mseaton were you suggesting changes something like this?

[mseaton]

Something like this, yes. Obviously you'd need to test this resource against version < 2.5 and versions >= 2.5 to ensure it works. It would be nice if there were other convenience methods in ModuleUtil that allowed you to just pass in the minimum version, but if this is the best available, that's fine. I'd at least create a convenience method in this class and use that (eg. supportsFormNamespaceAndPath() { return ModuleUtil.isOpenmrsVersionInVersions("2.5.* - 9.*") }

[IamMujuziMoses]

Are there any convenience methods I could use to test this resource against versions >= 2.5?

[IamMujuziMoses]

@mseaton please review

[samuelmale]

@IamMujuziMoses are you still working on this?

[brandones]

@samuelmale Looks like @IamMujuziMoses asked for a re-review from @mseaton four days ago. However the PIH team is about to be gone all of next week. I think it would be helpful if you, @dkayiwa , @ibacher , or someone else who understands REST WS code could provide a review (including ensuring that the concerns Mike raised are addressed).

[ibacher]

I'm actually going to take the opposite view @mseaton took above. I think we should just create the 2.5 omod and go from there. Two reasons:

1. We either need to use reflection or some other mechanism like used here to test if the "formNamespaceAndPath" property is added, whereas with a 2.5 OMOD, we do the version check once (or once-per-context refresh).
2. The FormRecordable interface was added to more than just Diagnosis in 2.5. It was also added to: Order, Condition, Allergy, and PatientState so ideally, we'd cover all of those resources in a 2.5 OMOD, which I think gives us enough content to justify creating the new sub-module.

[ibacher]

Code-wise, I think we're better-off exposing the formFieldNamespace and formFieldPath properties directly rather than the formFieldAndNamespace property for parity with the existing Obs resource, which also exposes them as two fields.

[IamMujuziMoses]

@ibacher please review

[ibacher]

Remember, we want to expose formNamespace and formPath, not formNamespaceAndPath

[brandones]

@ibacher Could you please re-review when you have the chance?

[ibacher]

LGTM. Thanks @IamMujuziMoses

Changes addressed