[Shipping Labels] Update the purchase in-progress and failure screens #14498
Conversation
And replace it with a single Boolean. PurchaseState was confusing because it represented only the state for the API request, while the label purchase itself was asynchronous, and given we weren't using any of the states except `InProgress`, a Boolean value is clearer.
To make the code more robust
We now have two properties to better represent the actual status.
hichamboushaba
added
type: enhancement
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
hichamboushaba
force-pushed
the
issue/WOOMOB-873-improve-SL-in-progress-UI
branch
from
005c26c to
972758a
Compare
| @@ -8,22 +8,10 @@ data class ShipmentUIModel( | |||
| val localId: String, | |||
| val remoteId: String? = null, | |||
| val items: List<ShippableItemModel>, | |||
| val purchaseState: PurchaseState = PurchaseState.NoStarted, | |||
| val isPurchaseAPILoading: Boolean = false, | |||
There was a problem hiding this comment.
The PurchaseState was confusing, as we already have ShippingLabelStatus that reflects purchase status.
The confusing part here is that the Shipping Label purchase happens in two steps:
- We make an API request to the backend to trigger the purchase, then the purchase will continue async.
- We observe the purchase status which is represented by
ShippingLabelStatus.
PurchaseState was only used to show the loading UI for the first step, but its other statuses weren't used at all, so I opted to remove it and replace it with the new Boolean isPurchaseAPILoading, I hope the mention of API there makes this less confusing for future code readers.
There was a problem hiding this comment.
Yes, I agree, this simplification makes it much better.
| * A shipment is considered purchased if the label was already purchased or is in the process of being purchased. | ||
| */ | ||
| val purchased: Boolean | ||
| val isPurchasedOrInProgress: Boolean |
There was a problem hiding this comment.
As mentioned in the PR description, this change is not related to the PR itself, but an update to clarify the property role.
| @@ -39,7 +39,8 @@ data class ShippingLabelModel( | |||
| val refund: Refund?, | |||
| val products: List<Order.Item> = emptyList(), | |||
| val originAddress: Address? = null, | |||
| val destinationAddress: Address? = null | |||
| val destinationAddress: Address? = null, | |||
| val error: String? = null, | |||
There was a problem hiding this comment.
We don't cache this property, as it's used only after a purchase failure.
| if (accountSettings == null || shipments.any { it.purchaseState is PurchaseState.Error }) { | ||
| if (accountSettings == null) { |
There was a problem hiding this comment.
The condition shipments.any { it.purchaseState is PurchaseState.Error } wasn't used at all, as we never set the purchaseState to the Error state.
| handlePurchaseSuccess(it, selectedShipmentIndex) | ||
| updateShipment( | ||
| selectedShipmentIndex, | ||
| shipments.value[selectedShipmentIndex].copy( | ||
| isPurchaseAPILoading = false, | ||
| label = it.labels.firstOrNull() | ||
| ) | ||
| ) | ||
| observeShippingLabelPurchaseStatus(selectedShipmentIndex) | ||
| // TODO check if we need to track this here or in the observeShippingLabelPurchaseStatus method | ||
| analyticsTracker.track(AnalyticsEvent.WCS_PURCHASE_STEP, mapOf(KEY_STATE to "purchase_success")) |
There was a problem hiding this comment.
I removed handlePurchaseSuccess and moved its logic here, I did this because the Success part of the function was a source of confusion, as this is just an API success, and the label is yet to be purchased.
I'll revisit this in my next PR to see if we can avoid the LongMethod detekt issue.
| // TODO check if we need to track this here or in the observeShippingLabelPurchaseStatus method | ||
| analyticsTracker.track(AnalyticsEvent.WCS_PURCHASE_STEP, mapOf(KEY_STATE to "purchase_success")) |
There was a problem hiding this comment.
This is the tracking issue that I mentioned in the PR description, we track the label as a success before it's actually purchased, this is different than iOS.
hichamboushaba
added
the
status: do not merge
hichamboushaba
requested review from
JorgeMucientes and
irfano
and removed request for
JorgeMucientes
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.layout.size | ||
| import androidx.compose.foundation.shape.RoundedCornerShape | ||
| import androidx.compose.material.Divider |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
| import androidx.compose.foundation.layout.size | ||
| import androidx.compose.foundation.shape.RoundedCornerShape | ||
| import androidx.compose.material.Divider | ||
| import androidx.compose.material.DropdownMenu |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
| import androidx.compose.foundation.shape.RoundedCornerShape | ||
| import androidx.compose.material.Divider | ||
| import androidx.compose.material.DropdownMenu | ||
| import androidx.compose.material.DropdownMenuItem |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
| import androidx.compose.material.Divider | ||
| import androidx.compose.material.DropdownMenu | ||
| import androidx.compose.material.DropdownMenuItem | ||
| import androidx.compose.material.Icon |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
| import androidx.compose.material.DropdownMenu | ||
| import androidx.compose.material.DropdownMenuItem | ||
| import androidx.compose.material.Icon | ||
| import androidx.compose.material.MaterialTheme |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
| import androidx.compose.material.DropdownMenuItem | ||
| import androidx.compose.material.Icon | ||
| import androidx.compose.material.MaterialTheme | ||
| import androidx.compose.material.Text |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
| @@ -8,22 +8,10 @@ data class ShipmentUIModel( | |||
| val localId: String, | |||
| val remoteId: String? = null, | |||
| val items: List<ShippableItemModel>, | |||
| val purchaseState: PurchaseState = PurchaseState.NoStarted, | |||
| val isPurchaseAPILoading: Boolean = false, | |||
There was a problem hiding this comment.
Yes, I agree, this simplification makes it much better.
hichamboushaba
removed
the
status: do not merge
Closes WOOMOB-873
Description
This PR introduces updated UI for the purchase in-progress and failure screens, and below is a detailed list of the added changes:
Note
Some notes about the changes:
ShippingLabelPurchaseStatusSectionas a new file, but it's just a renaming of the old filePrintShippingLabelSection, and the file has423lines.ShipmentUI#isReadOnly, and a separateisPurchased, this was needed as previously in progress was treated like success, while it should be just a flag for read-only form.ShipmentUIModel#purchasedtoShipmentUIModel#isPurchasedOrInProgressto better clarify its role.Important
This PR will be followed by a second PR that:
Even though the issues above have been here before this PR, it would be better not to merge this until the second PR is done too.
Steps to reproduce
Success
Failure
Testing information
The tests that have been performed
The above.
Images/gif
success.mp4
Screen_recording_20250821_143059.mp4
failure.mp4
Screen_recording_20250821_143024.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.