TRUNK-5952:Fixing validation error on saving an order

[HerbertYiga]

Ticket Id

https://issues.openmrs.org/browse/TRUNK-5952

Description

Fixing scenario one on the ticket Id ie (1.Create a DrugOrder with an Outpatient care setting, and fill out the rest of the Drug Order fields according to what is required except for quantity and numRefills - leave these as null. Create a new OrderContext with an Outpatient care setting. Save this order with the OrderService using this Care Setting. You will get a validation error that quantity and numRefills cannot be null.)

[HerbertYiga]

cc @mseaton @dkayiwa

[coveralls]

Coverage increased (+53.4%) to 63.565% when pulling 9ff171f9b9465646dc5094d18be787ae5fd6e118 on HerbertYiga:TRUNK-5952-SECOND-PR into 8d7132e43c1a6470d6835e9a0cdc5e692088d717 on openmrs:master.

[HerbertYiga]

Hi @mseaton , a gentle request to review this,thanks

[tendomart]

@HerbertYiga any update on this?

[HerbertYiga]

y update on this?

@tendomart getting some time to resume on this

[tendomart]

@HerbertYiga i think you need to fix the failure somewhere in your test

Error:  Failures: 
Error:    DrugOrderValidatorTest.validate_shouldFailValidationIfNumberOfRefillsIsNullForOutpatientCareSetting:165 expected: <false> but was: <true>
Error:    DrugOrderValidatorTest.validate_shouldFailValidationIfQuantityIsNullForOutpatientCareSetting:145 expected: <false> but was: <true>

[HerbertYiga]

@tendomart fixing this

[HerbertYiga]

hi @mseaton so got some time to get back to this. One thing i notice is that we have a method that is always called implicitly from our service interceptor and this is, ValidateUtil.validate(mainArgument) under the RequiredDataAdvice.

This means that even on defining ValidateUtil.validate(order) just before dao.saveOrder(order) will still get scenario one from the ticket breaking,do you think we can have any other cleaner approach for this cc @ibacher

[mseaton]

One thing i notice is that we have a method that is always called implicitly from our service interceptor and this is, ValidateUtil.validate(mainArgument) under the RequiredDataAdvice

Yes, @HerbertYiga you'll see on the original ticket I made similar observations. Can we take this discussion back to the ticket? My sense is that our AOP-based validation is misguided and we should just move all validation into service methods where appropriate. Or at least for the Order saving methods, find a way to bypass validation through AOP in order for validation to happen at the appropriate time. @ibacher / @dkayiwa ?

[ibacher]

My sense is that our AOP-based validation is misguided and we should just move all validation into service methods where appropriate.

I very much agree with this. Honestly, I wish our uses of AOP in general were a little less "magical" and a little more explicit, e.g. using an annotation for a service method to opt-in to being a point-cut (as we do with authorization) would at least make it clear that we expect some processing before / after / around the method call. But validation, in particular, is especially something we should do explicitly, especially where the objects being handled have more complicated validations to run before saving, like Orders and Obs.

[dkayiwa]

I actually see some advantages for the automated AOP validation. How about simply introducing a way for some objects to back out? For instance, an annotation which tells the AOP validation about which objects to skip.

[mseaton]

I actually see some advantages for the automated AOP validation. How about simply introducing a way for some objects to back out? For instance, an annotation which tells the AOP validation about which objects to skip

Happy to discuss pros/cons and see if we can come up with a solid plan moving forward. Maybe a discussion for a TAC call? @dkayiwa / @ibacher / @bmamlin ?

[dkayiwa]

Maybe a discussion for a TAC call?

Makes sense to me.

[ibacher]

I'm happy with having a way of opting out of validation... it certainly is the less drastic change 😄.

[HerbertYiga]

@mseaton sorry missed out on the TAC call, not sure if this was brought up, otherwise i am happy to bring it for the next TAC call

[HerbertYiga]

hi @mseaton @ibacher i am happy to discuss this during monday's TAC call

[github-actions[bot]]

tl;dr our action detected no activity on this PR and will close it in 30 days if the stale label is not removed.

OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare!

This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side. Please note that this is an automated message and we might very well be the reason why there has not been any activity lately. We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs.

If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :) If you would like to continue working on it or require help from us please remove the stale label and respond by commenting on the issue.

[github-actions[bot]]

tl;dr closing this PR since it has not seen any activity in the last 6 months.

OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare! We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs. Your PR has not seen any activity in the last 6 months. This is why we have decided to close this PR for now. This allows us OpenMRS reviewers to focus our limited time to review all other PRs in a timely and professional manner.

Please feel free to reassign yourself to the issue you worked on in our JIRA when you have time to focus on it. After that reopen a new PR and we will be glad to work with you to get your contribution merged. Thank you very much for your help and understanding :)