Skip to content

[Hack Week] Address PR review fixes for troubleshooting screen#15615

Open
hichamboushaba wants to merge 10 commits intohack-week/troubleshooting-screen-m5-fixesfrom
hack-week/troubleshooting-screen-PR-review-fixes
Open

[Hack Week] Address PR review fixes for troubleshooting screen#15615
hichamboushaba wants to merge 10 commits intohack-week/troubleshooting-screen-m5-fixesfrom
hack-week/troubleshooting-screen-PR-review-fixes

Conversation

@hichamboushaba
Copy link
Copy Markdown
Member

@hichamboushaba hichamboushaba commented Apr 3, 2026

Description

Note

Addresses PR review feedback from the troubleshooting screen PRs with the following fixes:

Key Changes:

  • Dark Mode Fix: Use color_on_surface instead of hardcoded woo_black for check icons, ensuring proper visibility in dark mode.
  • Rename: Rename ConnectivityToolTroubleshootConnection across Fragment, Screen, ViewModel, tests, and navigation graphs for consistency with the feature's user-facing name. Also renamed the connectivitytool package to troubleshooting.
  • Jetpack Error Detection: Improved the connectivity check logic to accurately identify Jetpack-related errors by inspecting the API errorCode (unknown_token or invalid_blog). Ensured this detection explicitly skips if the connection uses App Passwords.
  • Test Fixes: Fixed use case tests that were broken due to unstubbed selectedSite.get(). Added App Passwords suppression tests for all three use cases. Avoided calling selectedSite.get() twice in production code.
  • Use measureTimedValue: Replaced manual System.currentTimeMillis() timing with Kotlin's measureTimedValue across all five use cases.

Test Steps

  1. Navigate to Settings > Troubleshoot Connection and verify the screen loads correctly.
  2. Run all 5 connectivity checks and confirm the check icons are visible in both light and dark mode.
  3. Navigate to the troubleshooting screen from the Orders tab error banner and verify it works.
  4. Verify navigation back works correctly from both entry points.
  5. Test Jetpack errors using ApiFaker: Make sure to log in to the app using WordPress.com (do not use app passwords).
  6. Enable ApiFaker to mock a Jetpack error (e.g., adb shell am broadcast -a com.woocommerce.android.apifaker.ADD_ENDPOINT -e endpoint ".wc/v3/orders." -e method "GET" -e statusCode 401 -e body '{"code":"unknown_token","message":"Your token is invalid","data":{}}').
  7. Open Settings > Troubleshoot Connection and run the checks. Verify the specific check fails with the proper "Jetpack" error instead of a generic error.
  • 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.

@dangermattic
Copy link
Copy Markdown
Collaborator

dangermattic commented Apr 3, 2026

1 Error
🚫 This PR is tagged with status: do not merge label(s).

Generated by 🚫 Danger

@hichamboushaba hichamboushaba changed the title Hack week/troubleshooting screen pr review fixes [Hack Week] Address PR review fixes for troubleshooting screen Apr 3, 2026
@wpmobilebot
Copy link
Copy Markdown
Collaborator

wpmobilebot commented Apr 3, 2026

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

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Build Number736
Version24.4-rc-1
Application IDcom.woocommerce.android.prealpha
Commit79c4b93
Installation URL7tevsp7efdqn8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 71.42857% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.62%. Comparing base (1dcd4c8) to head (79c4b93).

Files with missing lines Patch % Lines
...roubleshooting/useCases/StoreOrdersCheckUseCase.kt 80.76% 0 Missing and 5 partials ⚠️
...leshooting/useCases/WPComConnectionCheckUseCase.kt 61.53% 0 Missing and 5 partials ⚠️
...hooting/useCases/InternetConnectionCheckUseCase.kt 55.55% 0 Missing and 4 partials ⚠️
...leshooting/useCases/StoreConnectionCheckUseCase.kt 66.66% 0 Missing and 3 partials ⚠️
...ubleshooting/useCases/StoreProductsCheckUseCase.kt 66.66% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@                               Coverage Diff                               @@
##             hack-week/troubleshooting-screen-m5-fixes   #15615      +/-   ##
===============================================================================
- Coverage                                        39.62%   39.62%   -0.01%     
+ Complexity                                       11363    11356       -7     
===============================================================================
  Files                                             2263     2264       +1     
  Lines                                           130681   130680       -1     
  Branches                                         18321    18332      +11     
===============================================================================
- Hits                                             51781    51780       -1     
+ Misses                                           73605    73604       -1     
- Partials                                          5295     5296       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hichamboushaba hichamboushaba force-pushed the hack-week/troubleshooting-screen-PR-review-fixes branch 2 times, most recently from da5d80b to e50a06b Compare April 6, 2026 16:03
@hichamboushaba hichamboushaba requested a review from Copilot April 6, 2026 16:04
@hichamboushaba hichamboushaba force-pushed the hack-week/troubleshooting-screen-PR-review-fixes branch from e50a06b to 79c4b93 Compare April 6, 2026 16:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR iterates on the Troubleshoot Connection feature by applying review feedback: improving dark-mode icon contrast, renaming the feature from “ConnectivityTool” to “TroubleshootConnection” across UI/navigation/tests, and refining “Jetpack not connected” detection to rely on API error codes (while suppressing that detection for Application Password sites).

Changes:

  • Updated Troubleshoot Connection UI icons to tint using color_on_surface to ensure visibility in dark mode.
  • Renamed ConnectivityTool → TroubleshootConnection across fragments/screens/viewmodels, tests, and navigation actions/destinations.
  • Adjusted connectivity check error classification to detect Jetpack-related failures via API errorCode (unknown_token / invalid_blog) and added/updated unit tests accordingly.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Show a summary per file
File Description
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/TroubleshootConnectionScreen.kt Uses color_on_surface for icon tint; renames screen composables to TroubleshootConnection.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/TroubleshootConnectionFragment.kt Renames fragment + wires TroubleshootConnection screen/viewmodel.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/TroubleshootConnectionViewModel.kt Renames viewmodel and keeps check orchestration consistent with new naming.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/ConnectivityCheckCardData.kt Updates imports/packages to troubleshooting namespace.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/TechnicalDetailsBottomSheet.kt Package rename to troubleshooting namespace.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/JetpackErrorUtils.kt Introduces helpers to identify Jetpack-related errors via API error codes.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreConnectionCheckUseCase.kt Uses measureTimedValue, avoids multiple selectedSite.get(), switches Jetpack detection to API error codes + app-password suppression.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreProductsCheckUseCase.kt Same as above for products check: timing + Jetpack error detection via API error code.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreOrdersCheckUseCase.kt Adds orders check use case under troubleshooting with updated Jetpack detection + timing.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/WPComConnectionCheckUseCase.kt Adds WP.com connectivity check under troubleshooting with timing via measureTimedValue.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/InternetConnectionCheckUseCase.kt Adds internet connectivity check under troubleshooting with timing via measureTimedValue.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/ErrorDetailsFormatter.kt Package rename to troubleshooting namespace.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/prefs/MainSettingsFragment.kt Updates settings navigation action to TroubleshootConnection destination.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/list/OrderListFragment.kt Updates orders banner navigation to TroubleshootConnection destination.
WooCommerce/src/main/res/navigation/nav_graph_settings.xml Renames settings action + fragment destination id/name for TroubleshootConnection.
WooCommerce/src/main/res/navigation/nav_graph_main.xml Renames orders action + fragment destination id/name for TroubleshootConnection.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/TroubleshootConnectionViewModelTest.kt Updates test package/class names and references to TroubleshootConnection types.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreConnectionCheckUseCaseTest.kt Updates tests for API error-code-based Jetpack detection + app-password suppression.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreOrdersCheckUseCaseTest.kt Adds/updates tests for Jetpack detection using WPAPI network errorCode + suppression.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreProductsCheckUseCaseTest.kt Adds/updates tests for Jetpack detection using Woo API apiErrorCode + suppression.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/WPComConnectionCheckUseCaseTest.kt Package/import updates for troubleshooting rename.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/InternetConnectionCheckUseCaseTest.kt Package/import updates for troubleshooting rename.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/ErrorDetailsFormatterTest.kt Package rename to troubleshooting namespace.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/connectivitytool/useCases/WPComConnectionCheckUseCase.kt Removes old connectivitytool WP.com check use case (moved/renamed).
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/connectivitytool/useCases/StoreOrdersCheckUseCase.kt Removes old connectivitytool orders check use case (moved/renamed).
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/connectivitytool/useCases/InternetConnectionCheckUseCase.kt Removes old connectivitytool internet check use case (moved/renamed).
Comments suppressed due to low confidence (2)

WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreConnectionCheckUseCaseTest.kt:46

  • Test name grammar: use "a GENERIC_ERROR" instead of "an GENERIC_ERROR" for readability/searchability of test cases.
    WooCommerce/src/test/kotlin/com/woocommerce/android/ui/troubleshooting/useCases/StoreConnectionCheckUseCaseTest.kt:173
  • Test name grammar: use "a TIMEOUT" instead of "an TIMEOUT" for readability/searchability of test cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hichamboushaba hichamboushaba added status: do not merge Dependent on another PR, ready for review but not ready for merge. feature: support Related to anything in the help & support section, including app logs and the Zendesk SDK. labels Apr 6, 2026
@hichamboushaba hichamboushaba marked this pull request as ready for review April 6, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: support Related to anything in the help & support section, including app logs and the Zendesk SDK. status: do not merge Dependent on another PR, ready for review but not ready for merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants