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.

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

Fixes IllegalArgumentException due to uninitialised site in page template picker #15415

Merged
merged 6 commits into from Oct 5, 2021
Merged

Fixes IllegalArgumentException due to uninitialised site in page template picker #15415

merged 6 commits into from Oct 5, 2021

Conversation

@antonis
Copy link
Member

@antonis antonis commented Oct 5, 2021

Fixes #15408

To test:

  • I don't have consistent steps to trigger the reported crash. A way to test this may involve using the debugger to set a null value for the SiteModel.
  • Verify that the tests pass

Regression Notes

  1. Potential unintended areas of impact
    N/A

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

  3. What automated tests I added (or what prevented me from doing so)
    Added a test case

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.
@peril-wordpress-mobile
Copy link

@peril-wordpress-mobile peril-wordpress-mobile bot commented Oct 5, 2021 •
edited
Loading

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

Loading

@antonis antonis requested review from planarvoid and ParaskP7 Oct 5, 2021
@antonis antonis added this to the 18.5 milestone Oct 5, 2021
@antonis
Copy link
Member Author

@antonis antonis commented Oct 5, 2021

Hey @ParaskP7, @planarvoid 👋
I've added you as reviewers since you seem to be familiar with this part of the codebase and handling the case where the SiteModel has not been initialised. Any suggestions for improvements or alternative approaches are more than welcome 🙇

Loading

@ParaskP7 ParaskP7 self-assigned this Oct 5, 2021
@peril-wordpress-mobile
Copy link

@peril-wordpress-mobile peril-wordpress-mobile bot commented Oct 5, 2021 •
edited
Loading

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

Loading

@ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Oct 5, 2021

👋 @antonis !

I've added you as reviewers since you seem to be familiar with this part of the codebase and handling the case where the SiteModel has not been initialised. Any suggestions for improvements or alternative approaches are more than welcome 🙇

Thanks for creating the fix, I am on it! 🙏

Loading

ParaskP7
ParaskP7 approved these changes Oct 5, 2021
View changes
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

👋 @antonis !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟

I have left one suggestion (💡) for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to think about and maybe apply this suggestions. However, feel free to ignore them and merge the PR yourself.

Extra Suggestion (💡): Consider making the SiteModel site within the SiteUtils.isBlockEditorDefaultForNewPost(...) method explicitly @Nullable to make it explicit that this method can accept a nullable site.

Loading

WordPress/src/main/java/org/wordpress/android/viewmodel/mlp/ModalLayoutPickerViewModel.kt Outdated Show resolved Hide resolved
Loading
WordPress/src/test/java/org/wordpress/android/viewmodel/mlp/ModalLayoutPickerViewModelTest.kt Show resolved Hide resolved
Loading
@ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Oct 5, 2021

👋 @antonis !

Thanks for your updates, those LGTM, I am still on the approved side, feel free to merge when CI completes.

Loading

ParaskP7
Copy link
Contributor

ParaskP7 commented on 776bcd4 Oct 5, 2021

Choose a reason for hiding this comment

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

👍

Loading

@antonis antonis merged commit 099c32a into develop Oct 5, 2021
13 checks passed
Loading
@antonis antonis deleted the fix/15408-nullSiteModel branch Oct 5, 2021
@dcalhoun dcalhoun mentioned this pull request Oct 12, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ParaskP7 ParaskP7

@planarvoid planarvoid

Assignees

@antonis antonis

@ParaskP7 ParaskP7

Labels
[Type] Crash
Projects
None yet
Milestone
18.5
Linked issues

Successfully merging this pull request may close these issues.

IllegalArgumentException: Required value was null.
2 participants
@antonis @ParaskP7