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

WP Stories: only allow updating a Post to the server when a Story is finished being saved #13518

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Dec 2, 2020

Fixes Automattic/stories-android#587

As the issue describes, sometimes we'd end up having a Post with local URLs pointed to by the Story block, leading to a broken experience.

The current implementation of Stories manages the creation of a new block on the MediaSaveUploadBridge, and handles the updates to an existing block on a Post through Gutenberg and the Gutenberg bridge.
In the first case (a new Story post created from scratch, without using Gutenberg) everything goes smooth because we don't need to create the actual Post or Story block until we have all the media in place.
But in this latter case, we use temporary ids to reflect the progress of a slide saving operation on the visual representation of the Story block itself in mobile Gutenberg while this is taking place. In order to identify which Story block a progress event corresponds to, the temporary id the event carries is checked against the ones being hold in the media collection that represents the block.
These temporary ids make no sense for an entity other than the WPAndroid / mobile Gutenberg implementation of the Story block integration, so sending a Post with such data to the server is inevitably breaking the relationship between a Post and the media contained in it.

OTOH, the Editor and WPAndroid know how to handle uploads and conversions from local mediaIds to remote mediaIds so, it makes sense to just hold off from updating or pushing the transient Post content with temporary ids to the server whilst the saving operation is taking place. As soon as a saving operation finishes, order is re-established and a local Media id is obtained, which can be used for reference later.

As such, this PR implements this safeguard by keeping track of the StorySaveProcessStart and StorySaveResult events in the StoriesEventListener class. This tracked information can be queried in EditPostActivity when the user taps on the UPDATE button or attempts to exit the editor prematurely before the save operation has finished, and shows a brief Toast asking the user to wait for a bit.

The amount of time the user will need to wait to tap UPDATE or exit the editor is as long as it takes for each of the Story's slides to be saved. Ideally, this is almost negligible with image backed story slides, but is noticeable for video backed slides. For example, on a random handset (as per my tests: a Pixel 2 and a Samsung S5e tablet) it takes approximately 3 to 4 seconds to save a 30 seconds video backed slide to finish saving, although saving multiple videos does not necessarily add up time linearly (given we are using coroutines to process up to 3 videos simultaneously) so, it shouldn't be much of a problem to wait a few seconds.

Ideally, we'd to implement a queueing system that waits for media to be saved first, and then use the existing queue for uploading as per with any other regular WP Media / Post item, but it may add complexity and demand more thorough testing.

To test:

  1. create a story post (go to My Site, tap on the FAB, create a Story). Add just 1 slide with an image and maybe add a text to it, then publish the post.
  2. Go to Posts list section in the app, and tap on the Post to open it in Gutenberg
  3. once the Post is rendered, select the Story block and tap on it to edit it.
  4. when the Story composer shows up, add a new slide by tapping on the + icon on the right of the bottom strip.
  5. pick a video from the picker, or tap on the camera FAB on the picker to launch the camera preview, and capture a video there.
  6. once you have this new slide with a video background, please add a text so a new video gets properly processed.
  7. tap on the ✔️ on the right-top of the screen, and the saving process will get triggered and the Gutenberg editor will show up again, reflecting the saving progress updates with a progress bar on the top border of the Story block
  8. At this point, tap on UPDATE on the navigation bar, or try tapping Android's navigation back button
  9. observe a toast appears reading Story being saved, please wait...
  10. tapping on UPDATE or the back button after the story has been saved should behave as usual.
  11. if you exited the editor, then opening it again from the Posts list should allow tapping on the Story block and editing the slides normally.

This gif shows all of these steps:
noupdatewhilesaving

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.

…ng saved, to avoid sending a Post containing a story block with temporary ids
@mzorz mzorz added this to the 16.3 ❄️ milestone Dec 2, 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.

@mzorz
Copy link
Contributor Author

mzorz commented Dec 2, 2020

Just a heads up I ended up tracking the needed improvement in Automattic/stories-android#616, and added to the Recent technical debt column cc @aforcier

@jd-alexander
Copy link
Contributor

hi @mzorz @aforcier just a heads up that I have seen this. I haven't been able to do a thorough review as yet as I am working on a fix for the File Block that needs to be shipped tomorrow. @aforcier do you have the bandwidth to take a look at this one? :)

@aforcier
Copy link
Contributor

aforcier commented Dec 4, 2020

Thanks for the update @jd-alexander - I'll review this one 👍

@aforcier aforcier self-assigned this Dec 4, 2020
@aforcier
Copy link
Contributor

aforcier commented Dec 4, 2020

Looks good @mzorz :shipit:

@aforcier aforcier merged commit 99a3eac into fix/stories-for-16.3 Dec 4, 2020
@aforcier aforcier deleted the fix/stories-wait-for-saving-finish branch December 4, 2020 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants