-
Notifications
You must be signed in to change notification settings - Fork 134
[Shipping labels] Improve address validation errors #14468
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] Improve address validation errors #14468
Conversation
This commit updates the `AddressNormalizationModel` to include a new `errors` field. The `WooShippingNetworkingMapper` is also updated to map the errors from the DTO to the model.
The `NormalizeAddressException` class is updated to accept a map of errors instead of a single error string. This allows for more granular error reporting, specifically differentiating between general errors and address-specific errors.
The error handling in `NormalizeAddress.kt` is updated to provide more specific error messages. - If the response model is null, an "Empty response" message is used. - `NormalizeAddressException` is now used when there are specific address errors. - A general `Exception` is used for other errors, incorporating the error message from the response if available.
The `AddressStatus` enum has been converted to a sealed class. This change allows for more detailed state representation, particularly for the `VerifyFailed` status, which can now optionally include a `NormalizeAddressException`.
Generated by 🚫 Danger |
edd4641
to
70a6a58
Compare
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.
I created this class to address the LargeClass
Detekt error in the WooShippingEditAddressViewModel
class.
@@ -12,22 +12,30 @@ class NormalizeAddress @Inject constructor( | |||
private val site: SelectedSite, | |||
) { | |||
suspend operator fun invoke(address: Address): Result<AddressNormalizationModel> { | |||
return site.getOrNull()?.let { |
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.
I changed site.getOrNull()
to site.get()
to align with the approach we use for our domain classes in the shipping label screens.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
70a6a58
to
4b1fc71
Compare
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14468 +/- ##
============================================
- Coverage 37.91% 37.90% -0.02%
- Complexity 9329 9333 +4
============================================
Files 2017 2018 +1
Lines 113250 113293 +43
Branches 14985 15005 +20
============================================
+ Hits 42942 42944 +2
- Misses 66392 66413 +21
- Partials 3916 3936 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 work @irfano, works well.
I just left a question, but I'm pre-approving.
val generalError: String? | ||
get() = errors[ERROR_GENERAL] | ||
val addressError: String? | ||
get() = errors[ERROR_ADDRESS] |
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.
Just a remark here, the iOS app exposes an error for the name
field (here), and the plugin takes this a step further by accepting an error for all fields (Check lines 43 to 49 of the file address-step/fields.jsx
).
I'm not sure how to trigger any of the other errors, so I'm not sure if we need to support this or not, WDYT?
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.
I didn't add errors for the other fields because I couldn't reproduce them. I reviewed address-step/fields.jsx
but still couldn't determine the mapping. In this PR, I mapped address
key to the "Address" field, but I couldn't find the keys for the other fields.
@itsmeichigo, since you used name
for the "Name" field errors, have you found a way to reproduce the name
error?
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.
I reviewed address-step/fields.jsx but still couldn't determine the mapping
You can find all the keys by looking for getProps
on the file: name
, company
, email
, phone
, country
, address
, city
, state
and postcode
. The mapping with of the error with the key
is done on line 43.
I didn't add errors for the other fields because I couldn't reproduce them
IMO we can add them without being able to reproduce them, in case the errors happen for some specific countries, but that's now a blocker for merging this PR.
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 your input, @hichamboushaba! I’ve added errors for name
, company
, email
, phone
, city
, and postcode
. I think it’s worth testing again, could you test it once more?

I couldn’t test the errors with API Faker, so I used the Network Inspector’s Rules tab with the following mocked response:
{
"data": {
"success": false,
"errors": {
"name": "Testing name error",
"company": "Testing company error",
"address": "House number is missing",
"city": "Testing city error",
"state": "Testing state error",
"postcode": "Testing postcode error",
"email": "Testing email error",
"phone": "Testing phone error",
"general": "Address not found"
},
"isTrivialNormalization": false,
"address": {
"address_1": "60 29TH STREET #343",
"address_2": "",
"city": "SAN FRANCISCO",
"company": "",
"country": "US",
"default_address": false,
"email": "[email protected]",
"is_verified": false,
"name": "IRFAN OMUR",
"phone": "",
"postcode": "94110-4929",
"state": "CA"
}
}
}
I didn't add test for country
and state
. Country field is always dropdown, so it's not necessary to add error for it. state
can be editable for some cases but I decided not to add error for it as well since it's rarely not a dropdown, effort to add error for state field was larger and I didn't want to increase the scope of this PR. Plus we aren't even sure server send errors for states. Let me know if you think we should add it.
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 @irfano, I tested the changes using ApiFaker, and it works as expected.
I didn't add test for country and state. Country field is always dropdown, so it's not necessary to add error for it. state can be editable for some cases but I decided not to add error for it as well since it's rarely not a dropdown, effort to add error for state field was larger and I didn't want to increase the scope of this PR. Plus we aren't even sure server send errors for states. Let me know if you think we should add it.
Sounds good to me.
Previously, only a general address error was shown. Now, errors for name, company, address, city, postcode, email, and phone are surfaced individually.
Closes WOOMOB-904
Description
This PR adds support for displaying address field errors on the Edit Address screen, using the validation errors returned from the endpoint. For example, when the house number is missing, the API response looks like this:
Steps to reproduce
Testing information
Repeat the steps for the origin address as well, but ensure that unverified addresses can't be saved as origin addresses.
The tests that have been performed
Steps above
Images/gif
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.