[Shipping Labels Revamp] Refactor shipping label notices #13817
Conversation
irfano
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.
|
|
📲 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}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13817 +/- ##
============================================
+ Coverage 38.04% 38.09% +0.04%
- Complexity 9221 9231 +10
============================================
Files 2074 2076 +2
Lines 114424 114394 -30
Branches 14563 14558 -5
============================================
+ Hits 43535 43579 +44
+ Misses 66939 66861 -78
- Partials 3950 3954 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ShippingAddressNotification( | ||
| addressNotification = addressNotification, | ||
| onDismiss = onDismissAddressNotification, | ||
| onAction = { | ||
| addressNotification?.let { | ||
| when { | ||
| it.isSuccess.not() && it.isDestinationNotification -> { | ||
| onEditDestinationAddress(shippingAddresses.shipTo) | ||
| } | ||
| it.isSuccess.not() && it.isDestinationNotification.not() -> { | ||
| onEditOriginAddress(shippingAddresses.shipFrom) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| ItnMissingNotification(itnNotification) | ||
| NoticeBanner(noticeBannerUiState) |
There was a problem hiding this comment.
We'll use NoticeBanner for all notice types.
| fun onDismissAddressNotification() { | ||
| uiState.update { it.copy(addressNotification = null) } | ||
| } |
There was a problem hiding this comment.
The ObserveShippingLabelNotice class manages notice dismissal events, so it's removed from the viewmodel.
| it.isSuccess.not() && it.isDestinationNotification -> { | ||
| onEditDestinationAddress(shippingAddresses.shipTo) | ||
| } | ||
| it.isSuccess.not() && it.isDestinationNotification.not() -> { | ||
| onEditOriginAddress(shippingAddresses.shipFrom) | ||
| } |
There was a problem hiding this comment.
This onAction logic has been moved into the view model.
| itnNotification = takeIf { isItnMissing }?.let { | ||
| ItnMissingNotification( | ||
| errorMessage = stringResource(R.string.woo_shipping_labels_customs_itn_required_error), | ||
| onErrorDismissed = onDismissItnNotice | ||
| ) | ||
| } |
There was a problem hiding this comment.
This logic has been moved into the ObserveShippingLabelNotice class.
There was a problem hiding this comment.
I renamed “notification” to “notice” to prevent confusion with Android system notifications.
| if (state.autoDismiss) { | ||
| // Dismiss the notice after AUTO_DISMISS_TIME passes | ||
| coroutineScope.launch { | ||
| delay(AUTO_DISMISS_TIME) | ||
| onDismissed(noticeType).invoke() | ||
| } | ||
| } |
There was a problem hiding this comment.
The auto-dismiss feature is managed exclusively here. After 2 seconds, we invoke onDismissed(), which updates the isDismissedFlow. Note the presence of isDismissedFlow in the combine parameters: whenever isDismissedFlow is updated, this transform block runs again. As a result, the new noticeBannerUiState becomes null, and the banner is removed from Compose.
| enter = fadeIn( | ||
| animationSpec = tween(durationMillis = 180) | ||
| ) + scaleIn( | ||
| animationSpec = tween(durationMillis = 180) | ||
| ) |
There was a problem hiding this comment.
I’ve removed the exit animation previously used in the current Compose implementation. The exit animation was already broken due to the line:
if (noticeBannerUiState == null) return@AnimatedVisibility
Implementing the exit animation properly requires additional development since we can’t manage the animation’s visible property based solely on the state’s nullability. The state must persist throughout the animation. This means we’ll need a new parameter to control the exit animation separately.
This PR is already large. If you think there’s value in addressing the animation issue, I can create a separate PR to handle it.
There was a problem hiding this comment.
The colors varied across different notices. I’ve updated them to match the colors specified in Figma.
There was a problem hiding this comment.
I’ve updated the red and green colors to align with the latest version from https://color-studio.blog/. While this update affects the entire app, the impact should be minimal since the adjustments are subtle.
| @@ -725,35 +725,6 @@ class WooShippingLabelCreationViewModelTest : BaseUnitTest() { | |||
| assertThat(dataState.customsState).isEqualTo(CustomsState.ItnMissing) | |||
| } | |||
|
|
|||
| @Test | |||
| fun `ItnMissing is dismissed when onDismissItnNotice is called`() = testBlocking { | |||
There was a problem hiding this comment.
Managing notice dismissals is now the responsibility of the ObserveShippingLabelNotice class.
| } | ||
|
|
||
| NoticeType.MISSING_ITN -> { | ||
| onEditCustomsClick() |
There was a problem hiding this comment.
This behavior was introduced in this PR. Tapping the missing ITN notice will open the Edit Customs screen.
| fun onDismissItnNotice() { | ||
| customsState.value = Unavailable | ||
| } |
There was a problem hiding this comment.
We no longer need to set the customsState to Unavailable, as the visibility of banners is now managed by noticeBannersUiState.
irfano
changed the title
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis pull request refactors the notification handling within the shipping label screens of the WooCommerce Android app. The previous approach using separate notification components and logic (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Shipping Label Screen"
participant VM as "WooShippingLabelCreationViewModel"
participant NoticeObserver as "ObserveShippingLabelNotice"
participant Banner as "NoticeBanner Component"
UI->>VM: Initialize screen & subscribe to notices
VM->>NoticeObserver: invoke observeNotices()
NoticeObserver-->>VM: Emit NoticeBannerUiState
VM->>UI: Update UI state with noticeBannerUiState
UI->>Banner: Render NoticeBanner(noticeBannerUiState)
Banner-->>UI: Handle user tap/dismiss events (if any)
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/components/NoticeBanner.kt (3)
34-44: Optimize visibility check logic.The current implementation has a redundant null check after the AnimatedVisibility check. The early return after already checking visibility based on nullability creates unnecessary complexity.
@Composable fun NoticeBanner(noticeBannerUiState: NoticeBannerUiState?, modifier: Modifier = Modifier) { AnimatedVisibility( visible = noticeBannerUiState != null, enter = fadeIn( animationSpec = tween(durationMillis = 180) ) + scaleIn( animationSpec = tween(durationMillis = 180) ) ) { - if (noticeBannerUiState == null) return@AnimatedVisibility + noticeBannerUiState?.let { // Move the rest of the content here with "it" as the state reference // or continue with noticeBannerUiState as non-null }
86-90: Add content description for accessibility.The icon is missing a content description, which affects screen reader accessibility.
Icon( imageVector = icon, tint = colorResource(color), - contentDescription = null + contentDescription = stringResource( + if (noticeBannerUiState.error) + R.string.error + else + R.string.success + ) )
98-104: Add content description for close button.The close icon is missing a content description, which affects screen reader accessibility.
Icon( imageVector = Icons.Outlined.Close, tint = colorResource(color), - contentDescription = null, + contentDescription = stringResource(R.string.dismiss), modifier = Modifier.clickable { noticeBannerUiState.onDismissed?.invoke() } )WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/ObserveShippingLabelNotice.kt (4)
24-27: Encapsulate concurrency interactions with state flows.
By maintainingpreviousNoticeand relying onisDismissedFlow, there's a chance multiple notices might overlap if a new notice is triggered within the auto-dismiss delay. Consider canceling or overriding the dismissal coroutine for the previous notice, if a new one is triggered, to prevent inconsistent UI states.
50-85: Consider reducing cyclomatic complexity ingetNoticeType().
Although the logic is clear, the multiple clauses in thiswhenstatement may benefit from splitting them into smaller guard functions or refactoring to enhance readability and maintainability.
137-139: Validate concurrency inonDismissed().
TheonDismissedlambda updates the state to mark the givennoticeTypeas dismissed. This is fine for simple flows, but if multiple notices need to be dismissed simultaneously, you might want to ensure no race conditions exist.Would you like guidance on concurrency testing strategies?
142-143: Reconsider the auto-dismiss duration.
AUTO_DISMISS_TIMEis set to 2 seconds. Depending on user feedback, a slightly longer duration (like 3-5 seconds) might be more user-friendly, giving them appropriate time to read the notice.WooCommerce/src/main/res/values/wc_colors_base.xml (1)
54-56: Updatedwoo_green_0,woo_green_5, andwoo_green_10.
The adjusted shades align with a more subdued green palette. Confirm that existing UI elements relying on these values still meet accessibility requirements (e.g., contrast ratio).Would you like help checking color usage for adequate contrast?
🧰 Tools
🪛 GitHub Check: Android Lint
[warning] 55-55: Unused resources
The resource R.color.woo_green_10 appears to be unusedWooCommerce/src/test/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/ObserveShippingLabelNoticeTests.kt (1)
1-165: Well-structured test class with comprehensive coverage.This test class thoroughly validates the different notification scenarios handled by the
ObserveShippingLabelNoticeclass. The tests cover all the important cases including:
- No issues
- Verified addresses (both origin and destination)
- Unverified addresses
- Missing addresses
- Multiple issues and their resolution flow
- ITN notifications
- Notification dismissal
The usage of
TestScopefor testing coroutines is appropriate, and the assertions verify the correct notification types are displayed.The constant
AUTO_DISMISS_TIMEis used on line 137 but isn't defined in this file. Consider either importing it from the component under test or defining it locally as a companion object.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/ShipmentDetails.kt(3 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/WooShippingLabelCreationScreen.kt(2 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/WooShippingLabelCreationViewModel.kt(6 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/GetAddressNotification.kt(0 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/ObserveShippingLabelNotice.kt(1 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/components/NoticeBanner.kt(1 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/components/NoticeBannerUiState.kt(1 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/purchased/WooShippingLabelPurchasedScreen.kt(0 hunks)WooCommerce/src/main/res/values/colors_base.xml(1 hunks)WooCommerce/src/main/res/values/wc_colors_base.xml(2 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/WooShippingLabelCreationViewModelTest.kt(4 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/GetAddressNotificationTests.kt(0 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/ObserveShippingLabelNoticeTests.kt(1 hunks)
💤 Files with no reviewable changes (3)
- WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/purchased/WooShippingLabelPurchasedScreen.kt
- WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/GetAddressNotification.kt
- WooCommerce/src/test/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/GetAddressNotificationTests.kt
🧰 Additional context used
🪛 GitHub Check: Android Lint
WooCommerce/src/main/res/values/wc_colors_base.xml
[warning] 23-23: Unused resources
The resource R.color.woo_red_10 appears to be unused
[warning] 55-55: Unused resources
The resource R.color.woo_green_10 appears to be unused
🔇 Additional comments (25)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/components/NoticeBannerUiState.kt (2)
5-12: Well-structured data class for notice state management.The
NoticeBannerUiStatedata class is well-designed with all necessary properties to represent a notice banner's state, including message content, type, auto-dismiss behavior, error status, and callback handlers for interactions.
14-21: Good enumeration of notice types.The
NoticeTypeenum effectively categorizes the different types of notices related to shipping labels, making the code more maintainable and easier to understand.WooCommerce/src/main/res/values/colors_base.xml (2)
335-335: Color update matches Figma specifications.The success label color has been updated from
woo_green_50towoo_green_60to match the design system in Figma.
337-338: Error color updates align with design guidelines.The error label and surface colors have been updated to match the Figma specifications, improving visual consistency across the app.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/ShipmentDetails.kt (3)
56-57: Appropriate imports for the new notice banner components.The imports for
NoticeBannerandNoticeBannerUiStateare correctly added to support the new unified notification system.
72-72: Function parameter updated to support the unified notice system.The
noticeBannerUiStateparameter correctly replaces the previous notification parameters, supporting the PR's objective of unifying notice management.
116-116: Successfully integrated the unified notice banner.The implementation correctly uses the new
NoticeBannercomponent, replacing the previous separate notification components. This aligns with the goal of simplifying notification management across the app.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/address/ObserveShippingLabelNotice.kt (2)
28-48: Validate auto-dismiss for newly triggered notices.
When a notice hasautoDismiss = true, you launch a delay ofAUTO_DISMISS_TIME. Ensure that if a higher-priority notice arrives during that interval, the system properly dismisses the older notice or cancels its coroutine. This helps prevent UI flicker or confusion if multiple notices appear in quick succession.Would you like a script to search if callers cancel or override the old coroutine?
87-135: Centralize textual messages ingetNoticeBannerUiState().
Defining the UI text in one place is good practice. However, if new notices or message variations are introduced, consider using localized resource references in a single map or table for easier extension and testing.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/WooShippingLabelCreationScreen.kt (2)
317-317: Ensure bottom sheet peek height covers new notice banners.
UsinguiState.noticeBannerUiState != nullinrequiresLargePeekHeightis a neat solution to ensure the bottom sheet accommodates the new banners. This looks correct and aligns well with the new unified notice system.
365-365: Good integration ofnoticeBannerUiState.
PassinguiState.noticeBannerUiStateto theShipmentDetailsensures the newNoticeBanneris displayed within the bottom sheet. This unification of notice logic is straightforward and more maintainable compared to the previous separate notification approach.WooCommerce/src/main/res/values/wc_colors_base.xml (3)
22-22: New color definitionwoo_red_0.
Adding#F7EBEChelps achieve a lighter variant of red for subtle error surfaces or backgrounds. Make sure it's consistently used wherever light error tones are required.
24-24: Unused resource warning forwoo_red_10.
Static analysis suggests this color might be unused. Verify its usage in layouts or themes. If truly unused, consider removing it to keep the resource set clean.Would you like a script to scan the codebase for
R.color.woo_red_10references?
59-59: Addedwoo_green_60to enrich your green palette.
Defining#007017provides a darker, more intense shade of green suitable for text or accenting success states. This is consistent with the updated palette.WooCommerce/src/test/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/WooShippingLabelCreationViewModelTest.kt (5)
19-23: Appropriate imports for the new notice system.The imports have been properly updated to use the new unified notice components.
238-238: Renamed mock to match new component.Good renaming from
getAddressNotificationtoobserveShippingLabelNotice, maintaining consistency with the new architecture.
256-256: Updated constructor parameter with the new notice component.The ViewModel constructor now correctly uses the new
observeShippingLabelNoticecomponent.
957-977: Test updated to verify notice display.This test correctly verifies that notices from
ObserveShippingLabelNoticeare now displayed in the UI state'snoticeBannerUiState.
980-996: Added test for when no notices are present.Good addition of a test to verify the null case, ensuring the UI correctly handles when no notices should be displayed.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/wooshippinglabels/WooShippingLabelCreationViewModel.kt (6)
24-29: Updated imports for the new notice system.Appropriate imports for the unified notification system.
85-85: Updated constructor with the new notice component.The ViewModel constructor now correctly includes the
observeShippingLabelNoticedependency.
145-145: Updated initialization to observe notices.The initialization block now launches the
observeNotices()method to handle notifications in the unified way.
157-183: Well-implemented notice handling with appropriate actions.The
observeNotices()method provides a clean implementation of the new notification system:
- It uses
observeShippingLabelNoticeto collect notice information- Updates the UI state with the notice banner
- Implements type-specific actions when notices are tapped:
- For unverified origin address → opens origin address editor
- For missing/unverified destination address → opens destination address editor
- For missing ITN → opens customs editor
This approach simplifies notification management by centralizing it in a single component.
589-591: Added method for split shipment functionality.New
onSplitShipment()method correctly triggers theStartSplitShipmentevent.
762-762: Updated UIControlsState to use the new notice system.The
UIControlsStatedata class now includesnoticeBannerUiStateinstead of the previous notification system, completing the refactoring.
Description
We currently use different Compose components and management methods for notice banners on the shipping label creation screens. This PR introduces a unified Compose component (
NoticeBanner), a single class to manage all banners (ObserveShippingLabelNotice), and includes several fixes.Steps to reproduce
Testing information
Unverified Origin
I don't know how to reproduce this scenario.
Missing destination
When creating an order, leave the first name, last name, and company fields empty. Feel free to test various combinations.
Unverified destination
Fill in the shipping address, but intentionally use incorrect details, such as an invalid postal code.
Verified destination
After reproducing the missing or unverified destination scenario, enter a valid address and tap the Validate & Save button.
Missing ITN
Create an order valued above $2,500. Ensure the shipping address country is different from the store address. I used TR as the shipping country, but the ITN requirement logic is still a work in progress, so I’m not certain about it. If the origin and destination addresses are valid but customs information is missing, you’ll see the corresponding notice.
The tests that have been performed
Steps above
Images/gif
This video demonstrates three notice types within a single flow: Missing Destination, Verified Destination, and Missing ITN. It also shows how the Verified Destination notice is automatically dismissed after two seconds, followed by the ITN notice being displayed.
after.webm
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: