Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRUNK-5952:Order validation is bypassed if certain fields are set from the OrderContext #3757

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@HerbertYiga
Copy link
Contributor

@HerbertYiga HerbertYiga commented Apr 28, 2021

Ticket Id

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

Description

Order validation should not be bypassed if certain fields are set from the OrderContext (STILL IN PROGRESS)

@HerbertYiga HerbertYiga changed the title TRUNK-5952:Order validation is bypassed if certain fields are set fr TRUNK-5952:Order validation is bypassed if certain fields are set from the OrderContext Apr 28, 2021
@HerbertYiga
Copy link
Contributor Author

@HerbertYiga HerbertYiga commented Apr 28, 2021

hi @mseaton @dkayiwa my work around this is to first get what breaks using the unit tests ie ( seeing that Order validation is bypassed if certain fields are set from the OrderContext ) and after then fix up the saveOrder method!

@mseaton mseaton marked this pull request as draft Apr 28, 2021
@coveralls
Copy link

@coveralls coveralls commented Apr 28, 2021

Coverage Status

Coverage increased (+53.3%) to 63.592% when pulling 57ad0ab on HerbertYiga:TRUNK-5952 into d1db9de on openmrs:master.

@HerbertYiga HerbertYiga force-pushed the HerbertYiga:TRUNK-5952 branch from 8ee23ab to 9757226 Apr 28, 2021
@HerbertYiga HerbertYiga marked this pull request as ready for review Apr 28, 2021

OrderContext orderContext = new OrderContext();
orderContext.setCareSetting(orderService.getCareSetting(1));
order.setQuantity(42.0);

This comment has been minimized.

@HerbertYiga

HerbertYiga Apr 28, 2021
Author Contributor

hi @mseaton so on introducing ValidateUtil.validate(order), quantity becomes required !

@HerbertYiga HerbertYiga force-pushed the HerbertYiga:TRUNK-5952 branch 4 times, most recently from 3c1ad26 to 57ad0ab Apr 30, 2021
@HerbertYiga
Copy link
Contributor Author

@HerbertYiga HerbertYiga commented May 4, 2021

hi @mseaton @mks-d how do things look for this?

@HerbertYiga
Copy link
Contributor Author

@HerbertYiga HerbertYiga commented May 6, 2021

Hi @mseaton a gentle request to checkout this, thanks

@mseaton
Copy link
Member

@mseaton mseaton commented May 11, 2021

@HerbertYiga my impression is this PR as is seems fine and it likely solves some of the issues present in the ticket, and is a good addition. It does not solve the entire ticket however. It solves scenario 2 that was listed (which is that an invalid order can get saved), but it does not solve scenario 1 (which is that a validation error is thrown when it should not be). I'm fine if we keep this ticket just about adding in the extra validation to deal with scenario 2, and then creating a separate ticket for scenario 1, if that makes this easier to manage. @dkayiwa ?

@HerbertYiga
Copy link
Contributor Author

@HerbertYiga HerbertYiga commented May 12, 2021

@HerbertYiga my impression is this PR as is seems fine and it likely solves some of the issues present in the ticket, and is a good addition. It does not solve the entire ticket however. It solves scenario 2 that was listed (which is that an invalid order can get saved), but it does not solve scenario 1 (which is that a validation error is thrown when it should not be). I'm fine if we keep this ticket just about adding in the extra validation to deal with scenario 2, and then creating a separate ticket for scenario 1, if that makes this easier to manage. @dkayiwa ?

thnaks @mseaton well let me go on to look through scenario one again and i make a fix on this same pr

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented May 12, 2021

I'm fine if we keep this ticket just about adding in the extra validation to deal with scenario 2, and then creating a separate ticket for scenario 1, if that makes this easier to manage. @dkayiwa ?

Makes perfect sense!

@HerbertYiga
Copy link
Contributor Author

@HerbertYiga HerbertYiga commented May 12, 2021

Makes perfect sense!

thanks @dkayiwa does this mean i can go on to make a separate ticket to solve scenario one?

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented May 12, 2021

@HerbertYiga how about raising a second pull request for the other scenario but using the same ticket?

@HerbertYiga
Copy link
Contributor Author

@HerbertYiga HerbertYiga commented May 12, 2021

how about raising a second pull request for the other scenario but using the same ticket?

@dkayiwa thanks let me do this!

@HerbertYiga
Copy link
Contributor Author

@HerbertYiga HerbertYiga commented May 13, 2021

how about raising a second pull request for the other scenario but using the same ticket?

@dkayiwa thanks let me do this!

hi @mseaton made a pr to solve scenario one here #3778

@@ -333,9 +334,9 @@ private Order saveOrderInternal(Order order, OrderContext orderContext) {
cal.set(Calendar.MILLISECOND, 0);
order.setAutoExpireDate(cal.getTime());
}
ValidateUtil.validate(order);

This comment has been minimized.

@mseaton

mseaton Jun 1, 2021
Member

Can you explain why you put this validate method here (inside an if-then block), rather that directly before the dao.saveOrder(order) method, that will get invoked in every case?

This comment has been minimized.

@HerbertYiga

HerbertYiga Jun 1, 2021
Author Contributor

Can you explain why you put this validate method here (inside an if-then block), rather that directly before the dao.saveOrder(order) method, that will get invoked in every case?

thanks @mseaton ,just a quick reply to this,i first added ValidateUtil.validate(order) before the dao.saveOrder(order) method, and it was breaking things,

This comment has been minimized.

@mseaton

mseaton Jun 1, 2021
Member

That sounds like something worth investigating. How was it breaking things?

This comment has been minimized.

@HerbertYiga

HerbertYiga Jun 1, 2021
Author Contributor

That sounds like something worth investigating. How was it breaking things?

i am going to re run the change and drop here the logs

This comment has been minimized.

@HerbertYiga

HerbertYiga Jun 2, 2021
Author Contributor

That sounds like something worth investigating. How was it breaking things?

hi @mseaton here are the full logs on putting the validate method directly before the dao.saveOrder(order),
https://pastebin.com/a8LCW0LK

This comment has been minimized.

@mseaton

mseaton Jun 2, 2021
Member

Thanks @HerbertYiga - I see the errors. Have you investigated the cause?

This comment has been minimized.

@HerbertYiga

HerbertYiga Jun 5, 2021
Author Contributor

I see the errors. Have you investigated the cause?

@mseaton well i see the cause of the error is this line -> failed to validate with reason: dateActivated: Date activated cannot be before that of the associated encounter and let me work on the fix

@HerbertYiga HerbertYiga force-pushed the HerbertYiga:TRUNK-5952 branch from 9c5f6cf to 91d3253 Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants