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
Conversation
|
||
OrderContext orderContext = new OrderContext(); | ||
orderContext.setCareSetting(orderService.getCareSetting(1)); | ||
order.setQuantity(42.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @mseaton so on introducing ValidateUtil.validate(order), quantity becomes required !
3c1ad26
to
57ad0ab
Compare
Hi @mseaton a gentle request to checkout this, thanks |
@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 |
Makes perfect sense! |
thanks @dkayiwa does this mean i can go on to make a separate ticket to solve scenario one? |
@HerbertYiga how about raising a second pull request for the other scenario but using the same ticket? |
@dkayiwa thanks let me do this! |
@@ -333,9 +334,9 @@ private Order saveOrderInternal(Order order, OrderContext orderContext) { | |||
cal.set(Calendar.MILLISECOND, 0); | |||
order.setAutoExpireDate(cal.getTime()); | |||
} | |||
ValidateUtil.validate(order); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like something worth investigating. How was it breaking things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like something worth investigating. How was it breaking things?
i am going to re run the change and drop here the logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @HerbertYiga - I see the errors. Have you investigated the cause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…m the OrderContext
moving this conversation to #3812 |
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)