Skip to content

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Nov 13, 2024

Closes: #12913
Closes: #12919

Description

This PR fixes a crash that can occur in order details screen when tapping on of the buttons of the OrderDetailCustomerInfoView, the crash occurs if the user tries to tap on the buttons after the view was detached from the window, this can happen during transition animations, and happens more frequently on tablets given how the navigation works with the dual pane mode.

For technical side, this crash happens because when the fragment view is destroyed, the NavHostFragment will detach the navController, so if a button click listener was to be invoked after this, the findNavController will throw the exception.

Steps to reproduce

Please repeat the following on both trunk and this branch to confirm the fix.

  1. Use a physical device.
  2. Open the order list on dual pane mode (Either on a tablet, or alternatively on a phone by opening the order list when the phone is on landscape)
  3. Use two fingers to tap instantly on both:
    • The shipping section of order details.
    • The row of another order.
  4. Repeat 3 few times, and the app should crash occasionally on trunk, and shouldn't crash on this branch.

Alternatively, if the above wasn't enough to reproduce it, please apply the following patch to simulate it, the patch will trigger manually the click after the view is destroyed, which should cause the crash on trunk and would confirm the fix of this branch.

Patch
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/views/OrderDetailCustomerInfoView.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/views/OrderDetailCustomerInfoView.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/views/OrderDetailCustomerInfoView.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/views/OrderDetailCustomerInfoView.kt	(revision 564631291444e615d443e2bd4f6e00d8252602d8)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/views/OrderDetailCustomerInfoView.kt	(date 1731511463945)
@@ -40,7 +40,7 @@
         private const val TELEGRAM_PACKAGE_NAME = "org.telegram.messenger"
     }
 
-    private val binding = OrderDetailCustomerInfoBinding.inflate(LayoutInflater.from(ctx), this)
+    val binding = OrderDetailCustomerInfoBinding.inflate(LayoutInflater.from(ctx), this)
     private var isCustomerInfoViewExpanded = false
 
     override fun onSaveInstanceState(): Parcelable {
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/OrderDetailFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/OrderDetailFragment.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/OrderDetailFragment.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/OrderDetailFragment.kt	(revision 564631291444e615d443e2bd4f6e00d8252602d8)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/OrderDetailFragment.kt	(date 1731508415600)
@@ -311,7 +311,9 @@
 
     override fun onDestroyView() {
         super.onDestroyView()
+        val customerInfoBinding = binding.orderDetailCustomerInfo.binding
         _binding = null
+        customerInfoBinding.customerInfoShippingAddressSection.callOnClick()
     }
 
     fun onPrepareMenu(menu: Menu) {
  • 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.

When we don't handle this check, there are cases where the click might occur when the navController is not attached anymore, and would cause a crash.
@hichamboushaba hichamboushaba added feature: order details Related to order details. type: crash The worst kind of bug. labels Nov 13, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Nov 13, 2024

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 13, 2024

📲 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
Commitaa2a8f7
Direct Downloadwoocommerce-wear-prototype-build-pr12920-aa2a8f7.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 13, 2024

📲 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
Commitaa2a8f7
Direct Downloadwoocommerce-prototype-build-pr12920-aa2a8f7.apk

@hichamboushaba hichamboushaba marked this pull request as ready for review November 13, 2024 15:57
@hichamboushaba hichamboushaba added this to the 21.2 milestone Nov 13, 2024
@atorresveiga atorresveiga self-assigned this Nov 14, 2024
Copy link
Contributor

@atorresveiga atorresveiga left a comment

Choose a reason for hiding this comment

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

Use two fingers to tap instantly on both:
The shipping section of order details.
The row of another order.

🤯🤯
This test case is from another level @hichamboushaba. Great creativity coming up with this case 👏

I tested the PR and can confirm that the change fixes the bug.

LGTM! :shipit:

}

private fun navigateToShippingAddressEditingView(address: Address) {
if (!isAttachedToWindow) return
Copy link
Contributor

Choose a reason for hiding this comment

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

Simple and elegant solution. Great work!

@atorresveiga atorresveiga merged commit 843e1ef into trunk Nov 14, 2024
17 of 21 checks passed
@atorresveiga atorresveiga deleted the issue/12913-fix-crash-customer-info-section branch November 14, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: order details Related to order details. type: crash The worst kind of bug.
Projects
None yet
4 participants