Skip to content

Conversation

@kidinov
Copy link
Contributor

@kidinov kidinov commented May 25, 2023

Closes: #9112

Description

Sentry Issue: WOOCOMMERCE-ANDROID-6PW

To reproduce:

  • Open order
  • Add customer note/shipping address or billing address
  • Enabled don't keep activities
  • Come back

The crash happens because 3 fragments use the same ViewModel while inside of the model, it expects specific sets of arguments passed from a Bundle

In the older version of Hilt, for some unclear reason in this situation, it was a passing bundle from the first fragment in the back stack, which was OrderDetailsFragment, and therefore crash didn't happen

With the update hilt, it passes the latest bundle there - that's why it cannot recover the required orderId

The PR passes orderId to the fragments so it will be in the Bundle. The workaround is not ideal - we may want to completely redesign the architecture of the taken approach

Testing instructions

Images/gif

  • Open order
  • Add customer note/shipping address or billing address
  • Enabled don't keep activities
  • Come back
  • Notice crash doesn't happen
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@kidinov kidinov linked an issue May 25, 2023 that may be closed by this pull request
@kidinov kidinov added the type: crash The worst kind of bug. label May 25, 2023
@kidinov kidinov added this to the 13.8 milestone May 25, 2023
@kidinov kidinov requested a review from wzieba May 25, 2023 07:05
@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@wpmobilebot
Copy link
Collaborator

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

@wzieba
Copy link
Contributor

wzieba commented May 25, 2023

@kidinov 👋 I'm unable to reproduce this issue, could you please point me to what I'm doing wrong?

Thu.May.25.14.16.53.CEST.2023.mp4

@kidinov
Copy link
Contributor Author

kidinov commented May 25, 2023

@wzieba, you should hide when you are on the note adding screen, enabled don't keep activity and then come back

Or just hide and show it when don't keep activities option is active

@wzieba
Copy link
Contributor

wzieba commented May 25, 2023

I see 🤦 thanks, I was able to reproduce now 👍

Copy link
Contributor

@wzieba wzieba left a comment

Choose a reason for hiding this comment

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

Thanks! The fix works well, the app is no longer crashing.

The workaround is not ideal - we may want to completely redesign the architecture of the taken approach

I agree - I think it's fairly easy, if someone doesn't know about this issue, to introduce a similar bug in the future.

Nevertheless, for the given circumstances, the proposed fix suites well 👍 :shipit:

@wzieba wzieba merged commit e9fb302 into trunk May 25, 2023
@wzieba wzieba deleted the 9112-runtimeexception-javalangreflectinvocationtargetexception-order-crash branch May 25, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: crash The worst kind of bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RuntimeException: java.lang.reflect.InvocationTargetException

4 participants