Skip to content

Conversation

@antonis
Copy link
Contributor

@antonis antonis commented Sep 9, 2021

Fixes #15300

Based on #15308 and targeting release/18.2

Description

This PR fixes a NullPointerException introduced in #15134

To test:
I was not able to reproduce the conditions that lead to a crash. Thus I only validated the fix by triggering a null value for the editorTheme at debug time.

A few edge cases I tested include:

  • Starting the editor quickly after a clean install of the app
  • Starting the editor quickly in offline mode after a clean install of the app

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)
    The complexity of EditPostActivity makes it hard to add an automated tests for this part of the code.

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

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

@antonis antonis added this to the 18.2 ❄️ milestone Sep 9, 2021
@antonis antonis self-assigned this Sep 9, 2021
@antonis antonis requested review from develric and mkevins September 9, 2021 09:50
@peril-wordpress-mobile
Copy link

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

@antonis antonis marked this pull request as ready for review September 9, 2021 11:03
Base automatically changed from bug/fix-editor-session-start-event-properties to release/18.2 September 9, 2021 13:44
@peril-wordpress-mobile
Copy link

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

Generated by 🚫 dangerJS

Copy link
Contributor

@develric develric left a comment

Choose a reason for hiding this comment

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

Hey @antonis 👋 , this LGTM! Even if not consistently, I was able to get the null from getEditorThemeForSite when using a pure self-hosted site on which I've opted out from Block Editor and especially when creating a post just after app starting up. Cannot be sure that is the only edge case, but regardless the fix worked for me 👍 .

I'll let you align with @AliSoftware to make this PR through the beta update, but let me know if any action needed from my side 🙇 .

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Hey @antonis , this LGTM! Even if not consistently, I was able to get the null from getEditorThemeForSite when using a pure self-hosted site on which I've opted out from Block Editor and especially when creating a post just after app starting up. Cannot be sure that is the only edge case, but regardless the fix worked for me .

I'll let you align with @AliSoftware to make this PR through the beta update, but let me know if any action needed from my side .

Thanks for testing this and finding steps to reproduce the original issue @develric . I had tried a few things to find steps, but nothing was reproducing it, I hadn't thought to opt out of the block editor on a self-hosted site.

Code looks good, thanks for fixing Antonis! 👍

@antonis
Copy link
Contributor Author

antonis commented Sep 10, 2021

@develric, @mkevins thank you both for reviewing this so quickly 🙇
I'll proceed with merging this in release/18.2

@AliSoftware
Copy link
Contributor

This fix is now part of the new 18.2-rc-3 beta, thanks all 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants