Skip to content
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

Normalize address #13525

Open
wants to merge 36 commits into
base: 13270-android-sdk-update-target-sdk-to-35
Choose a base branch
from

Conversation

atorresveiga
Copy link
Contributor

@atorresveiga atorresveiga commented Feb 12, 2025

Part of: #12439

Description

This PR includes the address normalization logic into the edit origin address flow.

This PR adds:

  1. The normalized address network logic (rest client, repository, dao, mapper)
  2. The select suggested address UI
  3. The normalized address use case

On top of the above, I also updated the loading UI to include a loading message while the address verification is taking place and some refactors in the view model for simplification. Handling failure will be tackled in the next PR.

⚠️ This PR depends on this other PR including the bottom sheet material 3 migration.

Testing information

  1. Open the orders tab
  2. Tap on an order eligible for shipping label creation
  3. Expand the shipment details section
  4. Expand the origin address selection (3 dots)
  5. Tap on the pencil
  6. If the address is verified, update an address field to make the address unverified
  7. Tap on validate & save
  8. Check a loading dialog is displayed while the verification is taking place
  9. Check that after the verification is completed, a confirm address screen is presented with the entered address and the suggested address
  10. Check that changing the address selection works as expected (changing the address and the button text).

The tests that have been performed

  • Checked that the happy path works as expected
  • Checked that a network request was made to verify the address
  • Checked that address selection works as expected

Images/gif

Screen_recording_20250213_120234.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@dangermattic
Copy link
Collaborator

dangermattic commented Feb 12, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Feb 12, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commitc0103d2
Direct Downloadwoocommerce-wear-prototype-build-pr13525-c0103d2.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Feb 12, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commitc0103d2
Direct Downloadwoocommerce-prototype-build-pr13525-c0103d2.apk

JorgeMucientes and others added 5 commits February 12, 2025 13:46
…material3

# Conflicts:
#	WooCommerce/src/main/kotlin/com/woocommerce/android/ui/themes/ThemePreviewScreen.kt
…ottomsheet-material3' into issue/12439-normalize-address
@wpmobilebot
Copy link
Collaborator

Project dependencies changes

The following changes in project dependencies were detected (configuration vanillaReleaseRuntimeClasspath):

list

tree
+\--- androidx.compose.material3:material3-android -> 1.3.0 (*)

@atorresveiga atorresveiga changed the base branch from trunk to issue/13495-migrate-to-use-modebottomsheet-material3 February 13, 2025 00:56
@atorresveiga atorresveiga changed the base branch from issue/13495-migrate-to-use-modebottomsheet-material3 to trunk February 13, 2025 00:58
@atorresveiga atorresveiga changed the base branch from trunk to 13270-android-sdk-update-target-sdk-to-35 February 13, 2025 15:44
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 49.67320% with 77 lines in your changes missing coverage. Please review.

Project coverage is 37.98%. Comparing base (0530338) to head (0da821e).
Report is 60 commits behind head on 13270-android-sdk-update-target-sdk-to-35.

Files with missing lines Patch % Lines
...nglabels/networking/WooShippingNetworkingMapper.kt 0.00% 27 Missing ⚠️
...android/ui/compose/component/WCModalBottomSheet.kt 0.00% 16 Missing ⚠️
...s/address/origin/WooShippingEditOriginViewModel.kt 80.82% 10 Missing and 4 partials ⚠️
...inglabels/networking/WooShippingLabelRestClient.kt 0.00% 8 Missing ⚠️
...oid/ui/orders/wooshippinglabels/networking/DTOs.kt 0.00% 5 Missing ⚠️
...inglabels/networking/WooShippingLabelRepository.kt 0.00% 5 Missing ⚠️
...oshippinglabels/address/origin/NormalizeAddress.kt 86.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@                               Coverage Diff                               @@
##             13270-android-sdk-update-target-sdk-to-35   #13525      +/-   ##
===============================================================================
+ Coverage                                        37.94%   37.98%   +0.04%     
- Complexity                                        9010     9019       +9     
===============================================================================
  Files                                             2054     2056       +2     
  Lines                                           112323   112350      +27     
  Branches                                         14202    14201       -1     
===============================================================================
+ Hits                                             42620    42680      +60     
+ Misses                                           65814    65778      -36     
- Partials                                          3889     3892       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atorresveiga atorresveiga added type: task An internally driven task. feature: shipping labels Related to creating, ordering, or printing shipping labels. labels Feb 13, 2025
@atorresveiga atorresveiga added this to the 21.8 milestone Feb 13, 2025
@atorresveiga atorresveiga changed the title Issue/12439 normalize address Normalize address Feb 13, 2025
@atorresveiga atorresveiga marked this pull request as ready for review February 13, 2025 18:10
@ThomazFB
Copy link
Contributor

@atorresveiga I noticed that due to this PR dependency and unrelated migrations, the diff is quite big and contains a lot of unrelated changes that make it challenging to understand what this PR introduced in terms of code. Should I wait for the other PR to be merged to start the review here? Or is it fine if I just review the introduced behavior with the test instructions?

@atorresveiga
Copy link
Contributor Author

Should I wait for the other PR to be merged to start the review here?

Yes, @ThomazFB let's wait for the dependency PR to get merge 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: shipping labels Related to creating, ordering, or printing shipping labels. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants