OBPIH-5854 New approval statuses in SM list pages - Approvals lists pages (fix after QA) #4311
Conversation
| @@ -39,6 +39,7 @@ class OutboundStockMovementService { | |||
| Date createdAfter = params.createdAfter ? Date.parse("MM/dd/yyyy", params.createdAfter) : null | |||
| Date createdBefore = params.createdBefore ? Date.parse("MM/dd/yyyy", params.createdBefore) : null | |||
| List<ShipmentType> shipmentTypes = params.list("shipmentType") ? params.list("shipmentType").collect{ ShipmentType.read(it) } : null | |||
| Boolean isApprovalRequired = stockMovement?.origin?.isApprovalRequired() | |||
There was a problem hiding this comment.
not a change request: since it is a transient, you can access it through the property like: origin.approvalRequired
| if (!isApprovalRequired) { | ||
| not { | ||
| 'in'("status", [RequisitionStatus.APPROVED]) | ||
| } |
There was a problem hiding this comment.
what's the expected behavior of this else statement? this else triggers if not stockMovement.sourceType, but just wanted to hear from you what it means at this point.
Shouldn't it be rather
if (stockMovement.sourceType == RequisitionSourceType.ELECTRONIC) {
...
} else {
...
}?
There was a problem hiding this comment.
When we are on the stock movements list we don't pass any params with sourceType - it's just null. The code that you proposed was my first version of the fix, but I had to check it in the debugger and then I saw this.
| @@ -98,6 +99,20 @@ class OutboundStockMovementService { | |||
| not { | |||
There was a problem hiding this comment.
Should this not be together with the next one or it should be one of these depending on the case if it it approval required or not?
There was a problem hiding this comment.
Those nots are separated by purpose:
For example, it can be read also as
- if approval is required: check if status is in [CREATED, ISSUED, CANCELED]
- if approval is not required: check if the status is in [CREATED, ISSUED, CANCELED, APPROVED, PENDING_APPROVAL, REJECTED]
I just moved the common part higher, and because of that I got rid of one additional else and longer lists of statuses
There was a problem hiding this comment.
I think it might be easier to follow the code if it was (with some commentary around it):
if required {
not in the list of some statuses + list of other statuses
} else {
not in the list of some statuses
}
There was a problem hiding this comment.
Are you sure about that? This redundancy of statuses looks odd to me, so maybe it would be enough when I add just comments around the current code?
There was a problem hiding this comment.
I added some comments for now, if you think it's not enough, let me know
There was a problem hiding this comment.
Ok, I think this makes sense for now
| } | ||
| } | ||
| } else { | ||
| not { |
…ages (fix after QA) (#4311) * OBPIH-5854 Remove statuses that are not on the filtering options * OBPIH-5854 Add comments for filtering stock movements
No description provided.