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

Mobile Stories block #12939

Merged

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Sep 11, 2020

This PR adds support for the jetpack Story block to be rendered on gutenberg-mobile, and provides the bridge implementation on the WPAndroid side of things to connect the block actions to the Story composer.

Related Jetpack PR: Automattic/jetpack#17140
Related Gutenberg PR: WordPress/gutenberg#25242
Related Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2611

test block rendering

  1. create a story
  2. publish it
  3. now go to the posts list
  4. open it
  5. observe the block appears (it's not unsupported but an actual story block).

Test block editing

CASE A: the story is in memory (no prefs are used)

  1. create a story
  2. publish it
  3. open the post on gutenberg
  4. tap on the block's tool icon
  5. it should load on the StoryComposer from memory

CASE B: the story has been created on this device, not in memory

  1. create a story
  2. publish it
  3. kill the app and open it again
  4. open the post on gutenberg
  5. tap on the block
  6. it should load on the StoryComposer from reviving it from prefs (added views should be editable)

CASE C: the story has not been created on this device

  1. create a story
  2. publish it
  3. log in on another device (or uninstall the app and then re-install it)
  4. open the post on gutenberg
  5. tap on the block
  6. it should load on the StoryComposer with the flattened views only (no added views, only background media)

CASE C1: story created on another device, media not present on current device

  1. go to a device A and create a story, publish it
  2. go to a device B and, WITHOUT refreshing the media section of the site, go straight to Posts list -> refresh (or wait) and tap on the newly created Post
  3. observe the post loads
  4. when you tap on the tool icon for the story block, it gets loaded and the "Limited story editing" dialog appears.

CASE C2: story created on another device, media not present on current device, NO NETWORK

  1. go to a device A and create a story, publish it
  2. go to a device B and, WITHOUT refreshing the media section of the site, go straight to Posts list -> refresh (or wait) until it appears on the list
  3. turn airplane mode ON
  4. tap on the newly created Post (if you're debugging, a red screen will appear on Gutenberg because it can't connect to metro - at this point, turn airplane mode OFF again, as it would have gotten past the media fetching code, now tap on the "Reload" option at the bottom right of the screen and allow the block to render the remote site http URL even if MediaModels haven't been fetched)
  5. observe the post loads
  6. when you tap on the tool icon for the story block, it cannot load and the following Dialog appears.

Screen Shot 2020-09-18 at 09 53 51

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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 11, 2020

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

@mzorz mzorz changed the title added WPANdroid bridge interface OnStoryCreatorRequestListener Mobile Stories block Sep 11, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 11, 2020

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

…s and clientId from Gutenberg when block editing is requested
@mzorz mzorz added gutenberg-mobile Gutenberg Editing and display of Gutenberg blocks. labels Sep 15, 2020
…ng the limited editing dialog when a Story slide to be eidted hasnt been found in StoriesPrefs
@mzorz
Copy link
Contributor Author

mzorz commented Oct 7, 2020

Thanks a lot for your very insightful review @jd-alexander ! Addressed all your comments, ready for another round 🙇 .

For CASE C2, I was wondering if the dialog is providing enough context for the user. It's stating that the story was created on a device and can't be edited at this moment even though if you go back and refresh the list and re-enter it works. I would put a dialog that says a connectivity error ocurred and put a retry button so the media can be fetched without using leaving the editor. Let me know your thoughts on this.

I think what the dialog says is still true: a) the story has been created on another device (well, not entirely true if you re-installed, but most likely), and b) we can't allow editing at this moment.
So regarding the second part "can't be edited at this moment" is still valid if you refresh and enter back again and now are able to edit it, right?

I understand the case for ambiguity you're pointing out; so I wouldn't tie this to connectivity issues because it could be something else as well.

What about This story may have been created on a different device. We'll be able to allow limited editing as soon as media related to this Story has been refreshed on this device. Please, try again later. ? also cc @aforcier

@aforcier
Copy link
Contributor

aforcier commented Oct 8, 2020

What about This story may have been created on a different device. We'll be able to allow limited editing as soon as media related to this Story has been refreshed on this device. Please, try again later. ? also cc @aforcier

If I understand the situation, the 'created on a different device' part seems not important here - we can't let the user edit the story because we aren't able to download the media. (If it had been created on the device, we'd have the media, but that's probably not a connection the user will/should make.)

So what about just:

Unable to load media for this story. Check your internet connection.

You did mention that

I understand the case for ambiguity you're pointing out; so I wouldn't tie this to connectivity issues because it could be something else as well.

What else could cause this state? Unless this is often flat wrong, I think pointing to the connection is a good hint at what would have to change before this screen actually works, and we can add more specific messaging if we later have other cases that lead to this state. WDYT?

@mzorz
Copy link
Contributor Author

mzorz commented Oct 8, 2020

So what about just:
"Unable to load media for this story. Check your internet connection."

When this situation is reached (media indicated by the story block can't be found), they will only be able to successfully retrieve the media if both these conditions are met:

a) they go out of the Editor and back in, with proper connectivity to their site
b) the media pointed to by the Story block still exists (they could have been deleted from the site altogether)

So I would be extra cautious (that's why I said I wouldn't tie this to connectivity issues only because it could be something else as well and we'd be asking them to refresh forever when it may actually be an unrecoverable state).

What about this:

Title: Can't edit story (as it is)
Message: This story may have been created on a different device, and as such the related media couldn't be found. We'll be able to allow limited editing after this Story's media is downloaded on this device. Verify your internet connection and if the problem persists, try going to the Media section on the app and refresh the list there.

This is still long I know 😅 , if you can come up with something shorter that still conveys the message politely that'd be great 👍 🙏

EDIT
To make it clear, these are the possible situations:

  1. we have both serialized slides and local background sources: we allow editing everything (no dialog is presented)
  2. we don't have the serialized slides, but we have access to the media pointed to by the block: we allow "limited story editing" (dialog shows up in the Story composer because things suggest you created this on another device or re-installed the app)
  3. we don't have the serialized slides nor can we find the media referenced by the block: we show this dialog.

We could also for example catch the fetchMediaList() FluxC's OnMediaListFetched event in EditPostActivity when we tried refreshing and, if it all went well and we still can't find the media, tell them this is a permanent error (there's really nothing we can do at this point), and if there was an error in the fetch, still tell them about the connectivity issues as expressed above (I'd go with some shorter version of my message or, the original message would just suffice at this point?).

@jd-alexander
Copy link
Contributor

Thanks for expounding in more detail here @aforcier @mzorz

So I would be extra cautious (that's why I said I wouldn't tie this to connectivity issues only because it could be something else as well and we'd be asking them to refresh forever when it may actually be an unrecoverable state).

I totally get this 🙏 As you described further in your comment, what we can do is show the dialog with errors we are sure of, and then for the "something else" case we just have a default dialog. I agree with @aforcier about adding more specific messages about what leads to a particular state. Just to further the discussion some more, can we do any type of logging to figure out the other cases that would cause this to fail? Also, we want to track how many failures are taking place here as well versus successes so we can make incremental improvements to have all cases covered.

This story may have been created on a different device, and as such the related media couldn't be found. We'll be able to allow limited editing after this Story's media is downloaded on this device. Verify your internet connection and if the problem persists, try going to the Media section on the app and refresh the list there.

This would be good for a "More Info" action but not for the dialog itself as you have stated it's too long 😅 but yes, this is helpful. As a user using the story functionality I wouldn't want to go to the media of the site and refresh the list there. Is there anything we can do from within the EditPostActivity to do that retry mechanism automatically?

We could also for example catch the fetchMediaList() FluxC's OnMediaListFetched event in EditPostActivity when we tried refreshing and, if it all went well and we still can't find the media, tell them this is a permanent error (there's really nothing we can do at this point), and if there was an error in the fetch, still tell them about the connectivity issues as expressed above (I'd go with some shorter version of my message or, the original message would just suffice at this point?).

This approach sounds like a good one because one thing I know for sure is that once it comes to media downloads and syncing there can be several issues and we wouldn't want any of them to cause the user too much frustration when they have created their story. So optimizing the error handling and it's communication as much as possible will definitely save us in the long term.

To summarize my thoughts, we can utilize short & specific error messages based on the error that led to a particular state eg.

  1. @aforcier 's Unable to load media for this story. Check your internet connection.
  2. We couldn't find the media for your story on this site. ( for this we would need to probably exit the editor 😢 )

Doing this will allow us to have good fallback mechanisms and context so users can figure out what's going wrong with the story they are created so it can be resolved and lead them to successful edits in the future.

@mzorz
Copy link
Contributor Author

mzorz commented Oct 8, 2020

Alright! Thank you for all the brainstorming @jd-alexander @aforcier 🙇

I implemented the following logic in 62fec15:

  1. when the Editor shows up we issue a request to update the site's media list. If there's no network prior to that (so we can't make sure this is a fresh item) or in the case the result event from FluxC is an error, we set a boolean flag internally. Then when the user actually decides to try and edit a Story block, we check this flag and show @aforcier's suggested message, slightly tweaked Unable to load media for this story. Check your internet connection and try again in a moment.. Basically: we didn't find the backing media but it may still be the case that the media exists on the remote site but we just haven't been able to refresh locally. Also: when the user presses OK on the dialog, we issue another fetchMediaList request (we could potentially implement the UI for retrying and showing progress to the user but I think it's overkill for a corner case they can circumvent also by exiting the editor and re-entering it, provided they do have connectivity this time).

  2. in the case the flag didn't return any errors but still we can't find the media Id being pointed to by the block, we do now show the second error as suggested by @jd-alexander
    We couldn't find the media for your story on this site. . Note we don't necessarily need to exit the editor, as people may still remove the block or add other blocks, etc.

To test:

For point 1 above (potentially retriable error):

  1. go to a device A and create a story, publish it
  2. go to a device B and, WITHOUT refreshing the media section of the site, go straight to Posts list -> refresh (or wait) until it appears on the list
  3. turn airplane mode ON
  4. tap on the newly created Post (if you're debugging, a red screen will appear on Gutenberg because it can't connect to metro - at this point, turn airplane mode OFF again, as it would have gotten past the media fetching code, now tap on the "Reload" option at the bottom right of the screen and allow the block to render the remote site http URL even if MediaModels haven't been fetched)
  5. observe the post loads
  6. when you tap on the tool icon for the story block, it cannot load and the following Dialog appears.

Screen Shot 2020-10-08 at 16 18 48

For point 2 above (permanent error) - after doing the above:

  1. on device A, now go to the media section of the app and delete the backing media for the story you created on step 1 before.
  2. go to device B and make sure you have airplane mode OFF now, exit the editor and on the Posts list, tap on the same Post again to open it.
  3. The media list should have been refreshed now in the background
  4. now select the Story block and tap on block's tool icon
  5. the app won't find the slides nor backing media and as such can't proceed to allow editing; this is the dialog shown:

Screen Shot 2020-10-08 at 16 21 48

Should be ready for another round, and let me know your thoughts 🙇 @jd-alexander @aforcier

Copy link
Contributor

@aforcier aforcier left a comment

Choose a reason for hiding this comment

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

I think the solution for the messaging is great, nicely done 👏

I tried out the new changes and re-tested the overall flow, everything seems to be working smoothly 🎉 Just made one wording suggestion.

I'll defer to @jd-alexander for the final :shipit:

WordPress/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@jd-alexander
Copy link
Contributor

@mzorz I am giving this a test now. I just saw the hash update so I will pull that down to ensure I have the latest changes 🙏

@jd-alexander
Copy link
Contributor

jd-alexander commented Oct 9, 2020

@mzorz I was trying to do the final review here but for some reason, the content of my post is of the initial HTML so I am wondering if I did something is off with the way how I approached this. I will revisit.

@mzorz
Copy link
Contributor Author

mzorz commented Oct 10, 2020

@mzorz I was trying to do the final review here but for some reason, the content of my post is of the initial HTML so I am wondering if I did something is off with the way how I approached this. I will revisit.

Thanks for spotting that one - that's fixed in wordpress-mobile/gutenberg-mobile#2705 - will cherry pick and update this branch here so you can continue reviewing 👍

Copy link
Contributor

@jd-alexander jd-alexander left a comment

Choose a reason for hiding this comment

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

Thanks for these changes @mzorz tested both paths and they work as expected. This is a great improvement. LGTM 🚢

@jd-alexander jd-alexander merged commit 90ccc42 into feature/jetpack-stories-block-base Oct 12, 2020
@jd-alexander jd-alexander deleted the try/jetpack-stories-block branch October 12, 2020 22:43
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants