-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Shipping Labels Revamp] Add Customs requirement check #13413
Conversation
📲 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13413 +/- ##
============================================
+ Coverage 41.10% 41.12% +0.01%
- Complexity 6486 6497 +11
============================================
Files 1325 1326 +1
Lines 77572 77601 +29
Branches 10694 10699 +5
============================================
+ Hits 31889 31913 +24
- Misses 42850 42851 +1
- Partials 2833 2837 +4 ☔ View full report in Codecov by Sentry. |
…LabelCreationViewModel
@@ -393,6 +411,68 @@ internal fun HazmatCard( | |||
} | |||
} | |||
|
|||
@Composable | |||
private fun CustomsCard( | |||
modifier: Modifier = Modifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np: Modifier parameters should be ordered as the first optional parameter. In this case, it should be the last parameter because the other parameters are not optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @atorresveiga! I took the opportunity to also adjust this in all components I introduced that wasn't following this convention in the file: 1e64abe
import com.woocommerce.android.ui.orders.wooshippinglabels.WooShippingAddresses | ||
import javax.inject.Inject | ||
|
||
class ShouldRequireCustomsForm @Inject constructor() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @ThomazFB! I left a very minor NP, but the code looks great and is working as described.
LGTM!
Version |
Why
Fix issue #13365 by introducing both the Customs section UI and the Customs requirement logic check.
How
Customs Section UI
The Customs section UI and state data is introduced through the same Mutable state flow logic using a new
CustomsState
. When no Customs data is required, the state is set asNotRequired
, when it's required but not available, it's set asUnavailable
. TheDataAvailable
state option will be added later once the form is finished.Customs requirement check
A use case called
ShouldRequireCustomsForm
is introduced, replicating the same logic used in the old Shipping Labels implementation declared at theShippingLabelsStateMachine.
Screen Capture
How to Test
Scenario 1 - Customs required
Scenario 2 - Customs NOT required
Update release notes:
RELEASE-NOTES.txt
if necessary.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: