OBPIH-6226 Add PO UoM in the PO receipt workflow by alannadolny · Pull Request #4889 · openboxes/openboxes

alannadolny · 2024-10-10T08:21:38Z

✨ Description of Change

Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6226

kchelstowski · 2024-10-10T09:28:03Z

grails-app/domain/org/pih/warehouse/shipping/ShipmentItem.groovy

 import org.pih.warehouse.core.ActivityCode
 import org.pih.warehouse.core.Location
 import org.pih.warehouse.core.Person
+import org.pih.warehouse.core.UnitOfMeasure


it seems not be used

kchelstowski · 2024-10-10T09:29:29Z

grails-app/domain/org/pih/warehouse/shipping/ShipmentItem.groovy

    }

+    OrderItem getOrderItem() {
+        return orderItems[0]


I'm wondering why this relationship is many-to-many

I just made this same comment on the other pr 🤔

kchelstowski · 2024-10-10T09:30:41Z

grails-app/i18n/messages.properties

 react.partialReceiving.check.label=Check
 react.partialReceiving.toReceive.label=To receive
-react.partialReceiving.receivingNow.label=Receiving now
+react.partialReceiving.receivingNowEach.label=Receiving now (each)


I think for this case, the key could stay receivingNow.

kchelstowski · 2024-10-10T09:31:39Z

src/js/components/receiving/PartialReceivingPage.jsx

+        getDynamicAttr: (props) => ({
+          hide: !props.values?.isShipmentOrderBased,
+        }),
+      },


shouldn't this field be disabled?

I think LabelField is "disabled" by default

kchelstowski · 2024-10-10T09:37:17Z

src/main/groovy/org/pih/warehouse/api/PartialReceipt.groovy

    }

+    Boolean isShipmentOrderBased() {
+        return shipment.orders.size() > 0


wasn't it supposed to be only applied to the shipment from PO?
if so, this should be changed, because currently it would return true also for returns and putaways.

Could you just use shipment.isFromPurchaseOrder instead, if it is only for POs?

kchelstowski · 2024-10-10T09:40:21Z

grails-app/domain/org/pih/warehouse/shipping/ShipmentItem.groovy

            "paymentTerm",
            "quantityInvoiced",
+            "orderItem",
+            "uom",


I don't have a strong opinion on that, but I can see that in some places we use uom and in some places we use unitOfMeasure - I think it would be good to align on one version, as more PRs related to that will come.

For now, I'd say that unitOfMeasure is more relevant, because it's currently used in OrderItem and Product domain.

kchelstowski · 2024-10-10T09:41:39Z

src/main/groovy/org/pih/warehouse/api/PartialReceiptItem.groovy

                quantityOnHand                  : quantityOnHand,
-                comment                         : comment
+                comment                         : comment,
+                uom                             : displayUom,


the same comment as in the transients

awalkowiak · 2024-10-10T15:53:25Z

src/js/components/receiving/PartialReceivingPage.jsx

+        getDynamicAttr: (props) => ({
+          hide: !props.values?.isShipmentOrderBased,
+        }),
+      },


I think LabelField is "disabled" by default

awalkowiak · 2024-10-10T15:56:40Z

src/main/groovy/org/pih/warehouse/api/PartialReceipt.groovy

    }

+    Boolean isShipmentOrderBased() {
+        return shipment.orders.size() > 0


Could you just use shipment.isFromPurchaseOrder instead, if it is only for POs?

awalkowiak · 2024-10-10T15:57:46Z

grails-app/domain/org/pih/warehouse/shipping/ShipmentItem.groovy

+        return orderItems[0]
+    }
+
+    String getUom() {


Looks like your and @drodzewicz are adding the same thing, So it would be to align on the same version (plus naming-wise I would say it should be getUnitOfMeasure)

ewaterman · 2024-10-11T16:39:23Z

src/main/groovy/org/pih/warehouse/api/PartialReceiptItem.groovy

                quantityOnHand                  : quantityOnHand,
-                comment                         : comment
+                comment                         : comment,
+                unitOfMeasure                   : displayUnitOfMeasure,


@drodzewicz is doing something very similar in StockMovementItem https://github.com/openboxes/openboxes/pull/4891/files

In Darek's pr he has "unitOfMeasure" being just the shipmentItem.unitOfMeasure and there's also a "packSize" field that has shipmentItem.quantityPerUom. Then in the frontend it renders those separately.

Is there a way we can unify our approach for both of these APIs?

kchelstowski · 2024-10-14T12:40:50Z

src/main/groovy/org/pih/warehouse/api/PartialReceipt.groovy

                requisition              : shipment?.requisition?.id,
                description              : shipment?.description,
                recipient                : recipient,
+                isShipmentOrderBased     : shipment.isFromPurchaseOrder,


I don't think that isShipmentOrderBased fits here, because returns are also order based. Let's call it as shipment's method, so isShipmentFromPurchaseOrder

alannadolny added 3 commits October 10, 2024 10:19

OBPIH-6226 Add shipped in po column, and rename two columns

1057902

OBPIH-6226 Add wrapping headers

ce32ad3

OBPIH-6226 Rename columns in export

5e37b52

alannadolny requested review from awalkowiak, drodzewicz, ewaterman and kchelstowski October 10, 2024 08:21

alannadolny self-assigned this Oct 10, 2024

github-actions bot added domain: frontend Changes or discussions relating to the frontend UI domain: backend Changes or discussions relating to the backend server domain: l10n Changes or discussions relating to localization & Internationalization labels Oct 10, 2024

kchelstowski reviewed Oct 10, 2024

View reviewed changes

awalkowiak reviewed Oct 10, 2024

View reviewed changes

alannadolny force-pushed the OBPIH-6226 branch 2 times, most recently from 895dae3 to 1fb81c2 Compare October 11, 2024 12:43

OBPIH-6226 Fixes after reviews

915a268

alannadolny force-pushed the OBPIH-6226 branch from 1fb81c2 to 915a268 Compare October 11, 2024 12:45

awalkowiak approved these changes Oct 11, 2024

View reviewed changes

ewaterman reviewed Oct 11, 2024

View reviewed changes

drodzewicz approved these changes Oct 14, 2024

View reviewed changes

kchelstowski reviewed Oct 14, 2024

View reviewed changes

OBPIH-6226 Rename isShipmentOrderBased to isShipmentFromPurchaseOrder

00332a5

kchelstowski approved these changes Oct 14, 2024

View reviewed changes

awalkowiak merged commit bda3654 into develop Oct 15, 2024

awalkowiak deleted the OBPIH-6226 branch October 15, 2024 08:45

alannadolny restored the OBPIH-6226 branch October 17, 2024 12:25

alannadolny deleted the OBPIH-6226 branch October 17, 2024 12:27

Uh oh!

Conversation

alannadolny commented Oct 10, 2024

✨ Description of Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewaterman Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kchelstowski Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ewaterman Oct 11, 2024 •

edited

Loading

kchelstowski Oct 14, 2024 •

edited

Loading