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

Stories: add bounds check in saving, don't process an empty Story block #14460

Merged
merged 5 commits into from
Apr 15, 2021

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Apr 14, 2021

Fixes #13678

The problem is there seem to be Posts with empty Story blocks in their content:

    <!-- wp:jetpack/story -->
    <div class="wp-block-jetpack-story wp-story"></div>
    <!-- /wp:jetpack/story -->

Therefore, when the findAllStoryBlocksInPostAndPerformOnEachMediaFilesJson iterator tries to find the mediaFiles attribute within the block, it fails to do so (the code is not really prepared to expect a non-existent mediaFiles attribute).

This PR adds a check to avoid processing mediaFiles for a block that simply doesn't contain any (that is, an empty block that hasn't been assigned any media files yet).

To test:

  1. create a new Post
  2. add a Story block, but don't do anything with it to it's left empty
  3. add a media block (image block, video block)
  4. add a media item to it
  5. while it's uploading, exit the editor
  6. observe it doesn't crash and finalizes uploads correctly
  7. check on your site and observe the Post renders correctly

Note: I noticed once the post may be uploaded containing a local path to the media file, but I haven't been able to reproduce, and is unrelated to this particular change.

Regression Notes

  1. Potential unintended areas of impact

Uploads of media when a Story block is also on the same Post.

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

Tried uploading media (Image block) manually.

  1. What automated tests I added (or what prevented me from doing so)

Added a unit test to check that if a Story block contains no mediaFiles array (as is the case for a Story block that has been added through the Block Picker on Gutenberg mobile), then do not try to process the array.

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.

@mzorz mzorz added this to the 17.2 milestone Apr 14, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 14, 2021

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

@mzorz mzorz requested review from renanferrari and aforcier and removed request for aforcier April 14, 2021 20:32
@mzorz
Copy link
Contributor Author

mzorz commented Apr 14, 2021

@renanferrari requesting your review since we were able to detect this one after your logging work done on #14185 and #14191 👍

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 14, 2021

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

Copy link
Member

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

Hey @mzorz, thanks once again for submitting this and for keeping me in the loop on this issue!

Code looks good and I can confirm your changes prevent the app from crashing, as it was expected. However, I noticed that even though the image upload seems to complete successfully, it still doesn't show up in the post. This is what I get instead:

On this particular post, that empty media block remained there, but in another post, it was just blank:

I'm not sure if that's the unrelated error you mentioned where the post is uploaded containing a local path to the media file or if it's something else. Other than that, everything is working as expected.

@mzorz
Copy link
Contributor Author

mzorz commented Apr 15, 2021

Thanks for testing @renanferrari ! 🙇

I'm not sure if that's the unrelated error you mentioned where the post is uploaded containing a local path to the media file or if it's something else. Other than that, everything is working as expected.

TBH I'm not sure why in one case the placeholder is shown (seems to be the block is valid though) - second image shows probably a valid block and it found the image but couldn't render it somehow, wondering are you able to select the block?

But yes in any case it's an unrelated problem 👍

Copy link
Member

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

wondering are you able to select the block?

Yes, I'm actually able to select the block in both cases:

But yes in any case it's an unrelated problem 👍

In this case, feel free to merge this once all the checks have been completed. Great job on fixing these issues 👍

@mzorz mzorz merged commit 5ecc625 into develop Apr 15, 2021
@mzorz mzorz deleted the isssue/13678-iob-empty-story-block branch April 15, 2021 21:15
@mzorz
Copy link
Contributor Author

mzorz commented Apr 15, 2021

Thank you @renanferrari 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants