Trunk 5682 #3046
Trunk 5682 #3046
Conversation
There was a problem hiding this comment.
Thanks @cioan , I made some comments on code, also, let's add some tests for all of these, thanks!
| @@ -183,6 +183,35 @@ public Order getOrder(Integer orderId) throws DAOException { | |||
| cal.setTime(searchCriteria.getActivatedOnOrAfterDate()); | |||
| crit.add(Restrictions.ge("dateActivated", OpenmrsUtil.firstSecondOfDay(cal.getTime()))); | |||
| } | |||
| if (searchCriteria.getDateStoppedOnOrBeforeDate() != null) { | |||
There was a problem hiding this comment.
If this criteria is not going to take the date stopped into account, it should be renamed something like "isStopped".
I'm also fine with removing this entirely if the two getCanceledAndExpired methods get you want you need...
There was a problem hiding this comment.
Thanks @mogoodrich ! I have changed to isStopped and I added test cases for each of the new parameters we added. Thanks for reviewing this.
| if (searchCriteria.getExcludeCanceledAndExpired() == true) { | ||
| Calendar cal = Calendar.getInstance(); | ||
| // exclude expired orders (include only orders with autoExpireDate = null or autoExpireDate in the future) | ||
| crit.add(Restrictions.or(Restrictions.isNull("autoExpireDate"), Restrictions.gt("autoExpireDate", OpenmrsUtil.getLastMomentOfDay(cal.getTime())))); |
There was a problem hiding this comment.
I would think that we don't need the getLastMomentOfDay in this case... it makes sense in the getAutoExpiredOnOrBefore method above, and the getDateStoppedOnOrBefore below, but here we want to compare the auto expired date against the current datettime.... ie I could see a use case where an order might expire after 10 hours... in that case, if the order is placed at 10am then you would not want to exclude it when querying at 3pm, but you would want to exclude it when querying at 11pm.
There was a problem hiding this comment.
Thanks @mogoodrich , so you are saying to just use current time, Restrictions.gt("autoExpireDate", cal.getTime())?
There was a problem hiding this comment.
Yep! (still "or-ed" with is null, of course)
| // exclude expired orders (include only orders with autoExpireDate = null or autoExpireDate in the future) | ||
| crit.add(Restrictions.or(Restrictions.isNull("autoExpireDate"), Restrictions.gt("autoExpireDate", OpenmrsUtil.getLastMomentOfDay(cal.getTime())))); | ||
| // exclude Canceled Orders | ||
| crit.add(Restrictions.isNull("dateStopped")); |
There was a problem hiding this comment.
We should make this logic the same as the autoExpireDate. I would assume that dateStopped shouldn't be in the future, but unfortunately the OrderValidator doesn't seem to enforce this... in the case that someone set a date stopped in the future, we wouldn't want to exclude this order.
There was a problem hiding this comment.
no problem, I will change this line to:
crit.add(Restrictions.le("dateStopped", cal.getTime()));
There was a problem hiding this comment.
Actually, after reading the next comment, maybe this line should be?
crit.add(Restrictions.le("dateStopped", OpenmrsUtil.getLastMomentOfDay(cal.getTime())));
There was a problem hiding this comment.
third time's the charm:
`// exclude Canceled Orders
crit.add(Restrictions.or(
Restrictions.isNull("dateStopped"),
Restrictions.gt("dateStopped", cal.getTime())));`
| // set the date's time to the last millisecond of the date | ||
| Calendar cal = Calendar.getInstance(); | ||
| cal.setTime(searchCriteria.getCanceledOrExpiredOnOrBeforeDate()); | ||
| crit.add(Restrictions.or(Restrictions.isNotNull("dateStopped"), Restrictions.le("autoExpireDate", OpenmrsUtil.getLastMomentOfDay(cal.getTime())))); |
There was a problem hiding this comment.
Same thing in the here, let's check that date stopped is also less than the end of the current day, not just that it is null.
There was a problem hiding this comment.
Thanks @mogoodrich , does this look OK now?
crit.add(Restrictions.or(
Restrictions.and(
Restrictions.isNotNull("dateStopped"),
Restrictions.le("dateStopped", OpenmrsUtil.getLastMomentOfDay(cal.getTime()))),
Restrictions.and(
Restrictions.isNotNull("autoExpireDate"),
Restrictions.le("autoExpireDate", OpenmrsUtil.getLastMomentOfDay(cal.getTime())))));
This is related to the RESTWS-757 PR. We just needed to add a few more search parameters to the OrderSearchCriteria class.