Skip to content

@W-20931847 - OMS multiple shipment support with pickups#3613

Merged
sf-deepali-bharmal merged 2 commits intodevelopfrom
W-20931847/refactor
Jan 29, 2026
Merged

@W-20931847 - OMS multiple shipment support with pickups#3613
sf-deepali-bharmal merged 2 commits intodevelopfrom
W-20931847/refactor

Conversation

@sf-deepali-bharmal
Copy link
Contributor

@sf-deepali-bharmal sf-deepali-bharmal commented Jan 27, 2026

Description

This PR removes reliability on indexes while traversing through omsShipping info.
Previously, shipping methods were only shown under Shipping Address. That meant OMS data (provider, status, tracking) was missing for BOPIS pickup shipments.
Shipping method is now shown separately from the shipping-address list. As a result, we can surface OMS shipping methods for both delivery and pickup, so BOPIS orders show OMS info for pickups as well.

  1. Pickup shipments (and pickup addresses from stores) are shown the same for single and multi-shipment, for both OMS and ECOM.
  2. For single-shipment OMS or ECOM we keep showing shipping address when there is one delivery shipment.
  3. For multi-shipment OMS order we don't show shipping address and ECOM delivery details and instead show only OMS shipment info from order.omsData.shipments, regardless of pickup vs delivery.
  4. For multi-shipment non-OMS /ECOM order we should multi shipment just like before i.e we show both shipping method and shipping address.

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • (change1)

How to Test-Drive This PR

  • (step1)

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@sf-deepali-bharmal sf-deepali-bharmal requested a review from a team January 27, 2026 23:39
@sf-deepali-bharmal sf-deepali-bharmal requested a review from a team as a code owner January 27, 2026 23:39
@sf-deepali-bharmal sf-deepali-bharmal added the skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated label Jan 27, 2026
</Box>
</Stack>
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation is same as before, just extracted into a function.

@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Jan 27, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@sf-deepali-bharmal sf-deepali-bharmal added skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated and removed skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated labels Jan 28, 2026
const isOmsMultiShipment = useMemo(
() =>
isOmsOrder &&
((order?.omsData?.shipments?.length ?? 0) > 1 || (order?.shipments?.length ?? 0) > 1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnt distinguish between instore and delivery shipments, if we have 1 in store and 1 delivery we'll remove the address, is that ok? It probably is but just want to make sure

Copy link
Contributor Author

@sf-deepali-bharmal sf-deepali-bharmal Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For in store - there is a existing PickupAddress component that will display pickup Addresses separately.

And we will list down all the ShippingMethods from OMS.

Image

Copy link
Contributor Author

@sf-deepali-bharmal sf-deepali-bharmal Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea is whenever there is more than one omsData.Shipments or more than one eCom shipmentAddresses , its a multi-shipment scenario -> We dont try to co-relate them irrespective of in-store or online shipments, because we don't have that much data.

PickupAddresses are already shown as is - we dont touch that and thats not a problem because PWA does not show shipping method next to it already, so we are good.

For ShippingMethod and ShippingAddress below it, we show the same pattern like before. In case of multi-shipment don't show addresses.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user’s perspective, the UX feels a bit confusing. We’re showing multiple shipping methods with different statuses (pickup, shipped, etc.), but we’re not showing which order items those methods correspond to. How will users know which items are ready for pickup versus shipped?”

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the existing BOPIS with multi-shipment work.
#3414
Looks like even without OMS PWA KIT inherently does not have the UI to map Shipping info to order line item.
But its a good point. I will look more into better multi shipment mapping in future releases.
But its not something we can cover in current PR.

ddiazccrz
ddiazccrz previously approved these changes Jan 28, 2026
const isOmsMultiShipment = useMemo(
() =>
isOmsOrder &&
((order?.omsData?.shipments?.length ?? 0) > 1 || (order?.shipments?.length ?? 0) > 1),
Copy link
Contributor

@sf-madhuri-uppu sf-madhuri-uppu Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we checking for order?.shipments?.length?
Isn't the multi shipment check specifically for omsOrders?
I think it's checking for multi shipments in both oms and normal orders array.
Can we rename the variable in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename it to - isMultiShipmentOrder ?

isOmsOrder &&
(order.omsData.shipments?.length > 1 ||
order.shipments?.length > 1)
{!isOmsMultiShipment &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we are checking only for multi shipments here. So, the variable name is confusing.
On lines 408 and below, we are showing shipping method name and status from normal order shipment even it's not from oms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to check for both oms order and multi-shipment. I renamed variable to make it cleaner.

@sf-deepali-bharmal sf-deepali-bharmal removed the skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated label Jan 29, 2026
nit

Empty commit to trigger CI

Empty commit to trigger CI
rename variable
@sf-deepali-bharmal sf-deepali-bharmal merged commit d82757a into develop Jan 29, 2026
42 checks passed
@sf-deepali-bharmal sf-deepali-bharmal deleted the W-20931847/refactor branch January 29, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants