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

Guard for nulls in processors and abort if necessary #12114

Merged

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Jun 5, 2020

Fixes: #12104

This PR guards for nulls in the processor Json handling. While it isn't clear why ids would not be present in parsed json from the block header, returning false will leave the content unchanged. Since no changes can be made to the parsed structure without matching the id, aborting should be the safest route.

I believe the reason why the id is missing is that it is not present in the templates, though I have not yet confirmed this.

Steps to test

  • Install and launch the application
  • Tap on "Log In"
  • Log in to the app using valid credentials
  • Go to My site and tap on "Pages" icon
  • Go to Drafts and tap on "CREATE A PAGE" button
  • Select the Service template (observe template preview)
  • Tap "Apply" option
  • Insert Image block
  • Add an image with "Choose from device"
  • Leave the editor (tap back button) before the upload completes

Expected:

The app should not crash, and upload should complete successfully

  • When upload has finished (observed in the notifications) reopen the page
  • Switch to HTML view

Expected:

The url for the added image should be a remote URL (i.e. https:// not file://)

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

No changes can be made to the parsed structure if the id property cannot
be matched, so returning false will leave the content unchanged.
@mkevins mkevins added Media Gutenberg Editing and display of Gutenberg blocks. labels Jun 5, 2020
@peril-wordpress-mobile
Copy link

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

@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

@mkevins mkevins marked this pull request as ready for review June 10, 2020 09:48
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@mkevins mkevins requested a review from chipsnyder June 10, 2020 09:48
Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

Great job tracking that down! Tested the case above and it all worked well.

@mkevins mkevins merged commit 16fdfda into develop Jun 10, 2020
@mkevins mkevins deleted the issue/12104-fix-npe-in-upload-completion-processors branch June 10, 2020 23:31
@mkevins
Copy link
Contributor Author

mkevins commented Jun 10, 2020

Thanks Chip for reviewing and testing so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Editing and display of Gutenberg blocks. Media
Projects
None yet
2 participants