WordPress-Android icon indicating copy to clipboard operation
WordPress-Android copied to clipboard

Fix NPE in Site Settings

Open aditi-bhatia opened this issue 1 year ago β€’ 4 comments

Fixes #20840

This PR fixes a crash in SiteSettingsFragment where mBloggingRemindersViewModel.onBlogSettingsItemClicked() is called on a null object.


To Test:

I haven't been able to reproduce this crash, so code review + smoke test My Site -> Site Settings -> Blogging -> Reminders.


Regression Notes

  1. Potential unintended areas of impact

    • None
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • None
  3. What automated tests I added (or what prevented me from doing so)

    • None

PR Submission Checklist:

  • [x] I have completed the Regression Notes.
  • [x] I have considered adding accessibility improvements for my changes.
  • [x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones):

  • [ ] ~WordPress.com sites and self-hosted Jetpack sites.~
  • [ ] ~Portrait and landscape orientations.~
  • [ ] ~Light and dark modes.~
  • [ ] ~Fonts: Larger, smaller and bold text.~
  • [ ] ~High contrast.~
  • [ ] ~Talkback.~
  • [ ] ~Languages with large words or with letters/accents not frequently used in English.~
  • [ ] ~Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)~
  • [ ] ~Large and small screen sizes. (Tablet and smaller phones)~
  • [ ] ~Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)~

aditi-bhatia avatar May 18 '24 00:05 aditi-bhatia

WordPressπŸ“² You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20845-421e2df
Commit421e2dfce5ab87003a6f35918610ae3706bedaf2
Direct Downloadwordpress-prototype-build-pr20845-421e2df.apk
Note: Google Login is not supported on these builds.

wpmobilebot avatar May 18 '24 00:05 wpmobilebot

JetpackπŸ“² You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20845-421e2df
Commit421e2dfce5ab87003a6f35918610ae3706bedaf2
Direct Downloadjetpack-prototype-build-pr20845-421e2df.apk
Note: Google Login is not supported on these builds.

wpmobilebot avatar May 18 '24 00:05 wpmobilebot

site is already being checked in onCreate and the activity is finished if the site is null.

The crash message is Attempt to invoke virtual method 'void org.wordpress.android.ui.bloggingreminders.BloggingRemindersViewModel.onBlogSettingsItemClicked(int)' on a null object reference. From the logs, I understand that the null object is BloggingRemindersViewModel, not site. Can you confirm?

irfano avatar May 20 '24 08:05 irfano

Hi @irfano thanks for taking a look πŸ‘

site is already being checked in onCreate and the activity is finished if the site is null.

From what I understand, mSite can become null after onCreate is called (deletion or removal of a site) which is why a few other functions in this class are also doing a null check. I personally feel that it's safer to have than not, but open to discussion.

From the logs, I understand that the null object is BloggingRemindersViewModel, not site.

Good point, thank you for catching that! I've added a null check for BloggingRemindersViewModel as well to address the case where it isn't initialized. What do you think?

aditi-bhatia avatar May 20 '24 22:05 aditi-bhatia

Thanks @irfano, I updated the release notes πŸ‘ For future reference, how do we decide which bug/crash fixes to include there? I noticed a few other crash fixes didn't update this file.

aditi-bhatia avatar May 21 '24 23:05 aditi-bhatia

For future reference, how do we decide which bug/crash fixes to include there?

I usually add every change that affects users to release notes, but if it's a minor change, with * indicator. More detail:

  • pbMoDN-o4-p2
  • pdcxQM-Km-p2

irfano avatar May 22 '24 12:05 irfano

Awesome, thank for you sharing!

aditi-bhatia avatar May 22 '24 18:05 aditi-bhatia