Skip to content

Conversation

@hafizrahman
Copy link
Contributor

@hafizrahman hafizrahman commented Jan 8, 2025

Description

In this PR for Bulk Order Status Update project, we added a mechanism to save selected items and restore it during configuration change.

This creates a crash in a case in this situation:

  1. Open Order List,
  2. Tap an Order to open its Order Details,
  3. Rotate the screen

What happens here is that binding becomes null when navigating to Order Details but OrderListFragment is still active, so when the call for binding is done in onSaveInstanceState, it creates a crash.

This PR adds null check to prevent that crash. I also added similar check in onViewStateRestored for extra safety, and a small fix in OrderSelectionItemKeyProvider that came from Android Studio's suggestion.

Steps to reproduce

TC1:

  1. Open Order List,
  2. Tap an Order to open its Order Details,
  3. Rotate the screen,
  4. Ensure there's no crash

TC2: ensure bulk selection still work

  1. Open Order List,
  2. Long press an Order to enable bulk selection mode,
  3. select a few more Orders,
  4. Rotate the screen,
  5. Ensure there's no crash, and the selected Orders remain selected

Testing information

n/a

The tests that have been performed

I tested both TCs in phone mode as that's where the crash happens. In tablet the case is a bit different (Order List and Details appear all the time in two panel mode) and the crash does not happen in my check.

Images/gif

n/a

  • 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.

@hafizrahman hafizrahman added the feature: order list Related to the order list. label Jan 8, 2025
@hafizrahman hafizrahman added this to the 21.4 milestone Jan 8, 2025
@hafizrahman hafizrahman added the type: crash The worst kind of bug. label Jan 8, 2025
@hafizrahman hafizrahman marked this pull request as ready for review January 8, 2025 05:02
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 8, 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
Commit83a3bd1
Direct Downloadwoocommerce-wear-prototype-build-pr13257-83a3bd1.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 8, 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
Commit83a3bd1
Direct Downloadwoocommerce-prototype-build-pr13257-83a3bd1.apk

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 40.67%. Comparing base (0137ab7) to head (83a3bd1).
Report is 27 commits behind head on trunk.

Files with missing lines Patch % Lines
...android/ui/orders/OrderSelectionItemKeyProvider.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #13257   +/-   ##
=========================================
  Coverage     40.67%   40.67%           
  Complexity     6396     6396           
=========================================
  Files          1351     1351           
  Lines         77664    77664           
  Branches      10682    10682           
=========================================
  Hits          31587    31587           
  Misses        43291    43291           
  Partials       2786     2786           

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

@irfano irfano self-assigned this Jan 8, 2025
Copy link
Contributor

@irfano irfano left a comment

Choose a reason for hiding this comment

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

It no longer crashes on tablets or phones, and I couldn’t find any drawbacks to this solution. LGTM! 👍🏻

@irfano irfano merged commit 542cc48 into trunk Jan 8, 2025
16 checks passed
@irfano irfano deleted the fix/order-details-rotation-crash branch January 8, 2025 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: order list Related to the order list. type: crash The worst kind of bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants