Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Npe site.username must not be null #15895

Merged
merged 7 commits into from
Feb 28, 2022
Merged

Npe site.username must not be null #15895

merged 7 commits into from
Feb 28, 2022

Conversation

develric
Copy link
Contributor

@develric develric commented Feb 3, 2022 •
edited

Fixes #15540

This has a companion FluxC PR here and a Login Flow PR here.
It implements the rationale mentioned here; that is filtering out sites that are jetpack package connected from the list of sites obtained from the rest client API response.

To test

Reproduce

Complete these steps on trunk without this patch:

  • Create a jurassic.ninja site and install the Jetpack Backup plugin (do not install or activate the full Jetpack plugin)
  • Login into the app with the user you used to setup the Jetpack Backup plugin
  • During the login notice the CP JP site is present in the list
  • Complete the login, and check the site is listed in the site selector list
  • Go to comments and notice the crash in logcat

Check the fix

Complete these steps with the apk from this PR:

  • With the same CP JP site setup as above, Login into the app with the user you used to setup the Jetpack Backup plugin
  • During the login notice the CP JP site is NOT present in the list (you should not have the site even in the db cache)
  • Complete the login, and check the site is NOT listed in the site selector list (use the search to confirm it as well)
  • Smoke test the app (especially other places where you have the list of sites, like for example in the reader reblog when having multiple sites)

Regression Notes

  1. Potential unintended areas of impact
    Places in the app where we list available user sites.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing + adding unit test in the FluxC companion PR.

  3. What automated tests I added (or what prevented me from doing so)
    Added a bit of unit testing in the FluxC companion PR.

PR submission checklist:

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

@develric develric added this to the 19.2 milestone Feb 3, 2022
@develric develric requested a review from khaykov February 3, 2022 00:28
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 3, 2022 •
edited

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@develric develric mentioned this pull request Feb 3, 2022
Merged
@develric develric marked this pull request as ready for review February 3, 2022 00:52
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 3, 2022 •
edited

You can test the changes on this Pull Request by downloading the APKs:

@khaykov khaykov self-assigned this Feb 4, 2022
@develric develric modified the milestones: 19.2, 19.3 Feb 5, 2022
This was referenced Feb 6, 2022
Merged
Closed
@develric
Copy link
Contributor Author

This and linked login and fluxc lib PRs have been tested/reviewed and also confirmed by woo here. Since it's involving shared libs, to make it stay in trunk for a bit, we will merge everything in 19.4 soon after code freeze (cc @hichamboushaba ).

@develric develric modified the milestones: 19.3, 19.4 Feb 20, 2022
@develric
Merge branch 'trunk' into issue/15540-npe-username-not-null
e590c29 
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/util/SiteUtils.java
#	build.gradle
khaykov
khaykov approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@khaykov khaykov left a comment

Choose a reason for hiding this comment

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

Looks good to me! Jetpack CP sites are correctly filtered out from both the Login site picker and regular site picker. Normal JP sites appear as expected 👍

@develric develric merged commit 51bbffd into trunk Feb 28, 2022
@develric develric deleted the issue/15540-npe-username-not-null branch February 28, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khaykov khaykov khaykov approved these changes

Assignees

@khaykov khaykov

Labels
Site Picker [Type] Crash
Projects
None yet
Milestone
19.4
Development

Successfully merging this pull request may close these issues.

NullPointerException: site.username must not be null
2 participants
@develric @khaykov