Skip to content

Conversation

@itsmeichigo
Copy link
Contributor

Closes WOOMOB-482

Description

This PR fixes a crash when the order details screen cannot reload after its sections are updated. Steps to reproduce:

  1. Open a completed order on the app.
  2. On the web, refund the same order
  3. On the app, pull to refresh the screen. The app would crash when rendering a row that no longer exists (e.g. shipping label related row if the app has Woo Shipping or WCS&T plugin set up).

The crash was due to a check for isViewLoaded and the table view reloading is skipped if the view is not yet loaded into memory. This check is from a legacy commit from long ago that is now redundant. Removing the check enables the table to load properly when needed.

Testing instructions

  1. Open a completed order on the app.
  2. On the web, refund the same order
  3. On the app, pull to refresh the screen. Confirm that the app does not crash.

Testing information

Tested following the steps above on simulator iPhone 16 Pro iOS 18.2 and confirmed that the app no longer crashes.

Also did some smoke testing on the order details screen and confirmed that the screen loads correctly for orders of different statuses.

Screenshots

Screen.Recording.2025-05-14.at.15.52.28.mov

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

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 all devices (phone/tablet) and no regressions are added.

@itsmeichigo itsmeichigo added this to the 22.4 milestone May 14, 2025
@itsmeichigo itsmeichigo added feature: order details Related to order details. type: crash The worst kind of bug. labels May 14, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented May 14, 2025

1 Warning
⚠️ This PR is assigned to the milestone 22.4. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@itsmeichigo itsmeichigo marked this pull request as ready for review May 14, 2025 09:14
@itsmeichigo itsmeichigo requested a review from jaclync May 14, 2025 09:14
func reloadTableViewSectionsAndData() {
configureNavigationBar()
reloadSections()
reloadTableViewDataIfPossible()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is redundant as the reloadSections method in the datasource already triggers a reload.

@wpmobilebot
Copy link
Collaborator

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

App NameWooCommerce iOS Prototype
Build Number29838
VersionPR #15633
Bundle IDcom.automattic.alpha.woocommerce
Commitde6f3fe
Installation URL47uc4tjd434a8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

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

Crash that I could repro after a full refund on web in trunk no longer happens now :shipit: thanks for fixing this!

Just wondering, since this isViewLoaded check has been around since 2018, do we know what might have changed that causes this likely recent crash? Like some new calls when isViewLoaded == true?

@jaclync
Copy link
Contributor

jaclync commented May 15, 2025

Sidenote: my test site doesn't have any shipping plugins, just WooCommerce.

@itsmeichigo
Copy link
Contributor Author

Thanks @jaclync for the review!

do we know what might have changed that causes this likely recent crash? Like some new calls when isViewLoaded == true

With many moving pieces on the order details screen over the years, I have no good answer to this. My wild guess is that the introduction of the split view might have something to do with this, as there have been significant changes to the hierarchy of the Orders tab. But I'm not 100% sure - we would need to test again with a pre-split view build to confirm that.

Sidenote: my test site doesn't have any shipping plugins, just WooCommerce.

My understanding is that the order item section goes through the most changes when an order status is changed to refunded, so that could possibly be a reason for the crash when reloading the table view.

I'm merging the PR for now.

@itsmeichigo itsmeichigo merged commit d6d6c56 into trunk May 15, 2025
14 checks passed
@itsmeichigo itsmeichigo deleted the woomob-482-invalid-row-in-cellforrowatindexpath-in branch May 15, 2025 02:35
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

Development

Successfully merging this pull request may close these issues.

5 participants