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

Hide previews for published posts on Jetpack sites #10212

Conversation

maxme
Copy link
Contributor

@maxme maxme commented Jul 12, 2019

Fixes #10161: Hide previews for locally modified published posts on Jetpack connected sites

Case 1:

  • On a Jetpack site
  • Open a published post and modify its content
  • Tap the "Preview" button in the overflow menu
  • You should get a "Preview unavailable" screen

Case 2:

  • On a Jetpack site
  • Open a published post and modify its content
  • Go back on the post list, tap "Preview" under the "More" button
  • You should get a "Preview unavailable" screen

Case 3:

  • On a Jetpack site
  • Open a draft (or create a new one), modify its content
  • Tap the "Preview" button in the overflow menu
  • You should get the actual preview

Case 4:

  • On a WPCom site
  • Open a published post and modify its content
  • Tap the "Preview" button in the overflow menu
  • You should get the actual preview

Demo video

Ping @osullivanchris - I wonder if we should show a specific message in that case (maybe have a look at the convo on p77Llu-cc4-p2 first).

@maxme maxme added this to the 13.0 milestone Jul 12, 2019
@maxme
Copy link
Contributor Author

maxme commented Jul 15, 2019

Note: We should wait a week or so before merging that one, there is an upcoming API fix that might fix the underlying issue.

@jkmassel
Copy link
Contributor

I'm bumping this to 13.1 as part of the 13.0 code freeze.

If you'd like it to be released as part of 13.0, please merge it against release/13.0 then DM me, and I'll be happy to issue a new beta release! :)

@jkmassel jkmassel modified the milestones: 13.0, 13.1 Jul 30, 2019
@maxme maxme requested review from shiki and malinajirka August 1, 2019 07:22
@maxme
Copy link
Contributor Author

maxme commented Aug 1, 2019

@malinajirka @shiki - Jetpack sites are still affected by the bug (a fix was deployed for simple sites, but it doesn't fix the issue for Atomic and Jetpack sites). I think we should merge that one and revert it when the final API fix is deployed.

@shiki
Copy link
Member

shiki commented Aug 1, 2019

@maxme Quick question on Case 3. I'm testing this on a Jurassic site. For a draft that's been uploaded to the server but with no changes, when I edit it:

  • Without doing anything: Preview is unavailable
  • Changing something: Preview is unavailable

Is this expected?

Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

Tested the given cases:

  • Case 1: ✔️
  • Case 2: ✔️
  • Case 3: ❓
  • Case 4: ✔️

For Case 3, I was testing with a sandbox and a Jurassic site and for both cases, it looks like this is getting called instead:

if (!site.isUsingWpComRestApi) {
activityLauncherWrapper.showActionableEmptyView(
activity,
WPWebViewUsageCategory.REMOTE_PREVIEW_NOT_AVAILABLE,
post.title
)

I think I'm misunderstanding what a Jetpack site is. I'll need your help in testing this. 😬

Also, another question. I noticed that in handlePostListAction, we have PostListAction.PreviewPost and PostListAction.ViewPost. What is the difference between them? It looks like the More → Preview uses a different action than the Preview in the editor.

@maxme
Copy link
Contributor Author

maxme commented Aug 2, 2019

I think I'm misunderstanding what a Jetpack site is. I'll need your help in testing this. 😬

I think your jurassic.ninja site is not connected to your wordpress.com account, and act like a self hosted site instead of a Jetpack connected site:

  • A Jetpack site is a self hosted site we access via the wpcom REST API.
  • !site.isUsingWpComRestApi, means a site we don't access via wpcom REST API (currently, the only other API we support is the self hosted XMLRPC API), so it's not a Jetpack site (or a wpcom site).

we have PostListAction.PreviewPost and PostListAction.ViewPost. What is the difference between them?

The difference is that ViewPost will directly open the web view with the URL. PreviewPost will first try to upload or auto-save (depending on the post status) the post, wait for the server to reply, and then open the web view with the URL we got from server's response.

For a draft that's been uploaded to the server but with no changes, when I edit it:

  • Without doing anything: Preview is unavailable
  • Changing something: Preview is unavailable

I'll double check self hosted cases. IIRC we should show the preview in these cases for drafts.

@maxme
Copy link
Contributor Author

maxme commented Aug 2, 2019

For a draft that's been uploaded to the server but with no changes, when I edit it:

  • Without doing anything: Preview is unavailable
  • Changing something: Preview is unavailable

I'll double check self hosted cases. IIRC we should show the preview in these cases for drafts.

After checking our discussion log, these 2 cases for wpcom / jetpack or self hosted sites should upload and preview drafts.

It's not related to this fix, I'll open another PR to fix it.

@maxme
Copy link
Contributor Author

maxme commented Aug 2, 2019

@shiki I added some useless tests to document the behavior (I'll add better tests for self hosted sites in the other fix).

Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

Thank you, @maxme. LGTM! Thank you for adding tests!

The difference is that ViewPost will directly open the web view with the URL. PreviewPost will first try to upload or auto-save (depending on the post status) the post, wait for the server to reply, and then open the web view with the URL we got from server's response.

Thank you for the explanation. I guess I was just wondering why we have 2 actions that look very similar. Maybe they can be merged. 🤔 Definitely out of scope for this PR.

@Test
fun `preview available for Jetpack sites on draft with modification`() {
// Given
// next stub not used (made lenient) in case we update future logic.
Copy link
Member

Choose a reason for hiding this comment

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

This comment and lenient() is 🎉

@shiki shiki merged commit 1655dfb into feature/master-remote-previews Aug 2, 2019
@shiki shiki deleted the issue/10161-hide-autosave-preview-on-jetpack-site branch August 2, 2019 13:58
@maxme
Copy link
Contributor Author

maxme commented Aug 2, 2019

I guess I was just wondering why we have 2 actions that look very similar. Maybe they can be merged.

Yes, they could be merged. Actually, the current implementation of "preview" behaves like "view" when there is no local change. It's just a matter of internal wiring. I wonder if we should change the UI as well, I'm not sure if the distinction between View/Preview is useful for users.

@shiki
Copy link
Member

shiki commented Aug 2, 2019

I wonder if we should change the UI as well, I'm not sure if the distinction between View/Preview is useful for users.

That's a good idea.

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.

3 participants