OBPIH-6225 Add PO UoM in the PO shipment workflow#4891
Conversation
github-actions
bot
added
type: feature
| } | ||
|
|
||
| String getUnitOfMeasure() { | ||
| OrderItem orderItem = OrderItem.get(this.orderItemId) |
There was a problem hiding this comment.
Looking at how the orderItemId transient is implemented, I don't see a point in fetching the order item here. I'd rather just go with orderItems[0]
There was a problem hiding this comment.
I'm assuming he did this because that's what we do in all the previous lines but yes, doing it this way looks up all order items to get the first id, then we re-fetch that order item here to get the unit of measure. Seems like a lot of redundant work.
I agree with Kacper that we could do this more efficiently by just getting the first item (since we're already assuming there is only one).
Side note, I think it's worth looking into the performance effect of having shipmentItem <-> orderItem being a many-to-many relationship. I know it was implemented that way to be more flexible, but I would think that a shipmentItem can only reference a single order item. Anyways, something for us to look into at another point
| quantityRevised : quantityRevised, | ||
| quantityPicked : quantityPicked, | ||
| unitOfMeasure : unitOfMeasure, | ||
| packsRequested : quantityRequested / packSize, |
There was a problem hiding this comment.
are you sure we don't have to do any additional checks in terms of dividing those two values, like checking if packSize is always > 0?
…tyRequested columns on Add items table
… packs requested based on UoM
drodzewicz
force-pushed
the
feature/OBPIH-6225-add-po-uo-m-in-the-po-shipment-workflow
branch
from
d0eebd2 to
f7a934c
Compare
| label: 'react.stockMovement.quantity.label', | ||
| defaultMessage: 'Qty', | ||
| label: 'react.stockMovement.quantityPerUom.label', | ||
| defaultMessage: 'Quantity (per UoM)', |
There was a problem hiding this comment.
I think this should be something different. "Packs requested" is something different than "quantity per uom" ("quantity per uom" is "pack size")
There was a problem hiding this comment.
this name was suggested to me by Manon, I don't have any opinion about it so this needs further confirmation
| quantityRevised : quantityRevised, | ||
| quantityPicked : quantityPicked, | ||
| unitOfMeasure : unitOfMeasure, | ||
| packsRequested : packsRequest, |
There was a problem hiding this comment.
Can you call it packsRequested as well instead of packsRequest?
| const packsRequested = _.toInteger( | ||
| _.get(values, `lineItems[${rowIndex}].packsRequested`), | ||
| ); | ||
| const packSize = _.toInteger( | ||
| _.get(values, `lineItems[${rowIndex}].packSize`), | ||
| ); | ||
| return packsRequested * packSize; |
There was a problem hiding this comment.
Can you just use values.lineItems[rowIndex].packsRequested with nullsafes instead of this lodash function?
| }, | ||
| getDynamicAttr: ({ rowIndex, tableItems }) => ({ | ||
| formatValue: () => { | ||
| const row = _.get(tableItems, `[${rowIndex}]`, {}); |
| } | ||
|
|
||
| BigDecimal getPacksRequested () { | ||
| if (packSize == null || packSize == 0) { |
There was a problem hiding this comment.
so when we are checking if packSize is null or 0, we can use !packSize
There was a problem hiding this comment.
I made other changes but I'd prefer to leave this as it is now, I feel like this way is more explicit to show what are we checking instead of !packSize
awalkowiak
deleted the
feature/OBPIH-6225-add-po-uo-m-in-the-po-shipment-workflow
branch
5 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: OBPIH-6225
Description:
On add items step:
On Send page
📷 Screenshots & Recordings (optional)