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

Auto save published post upon preview and show preview url #11638

Merged
merged 24 commits into from May 13, 2019

Conversation

@yaelirub
Copy link
Contributor

commented May 4, 2019

Fixes #9287 (also refer to #9282 )
autosave API call PR on WordPressKit

The gist:
When asking to preview a post we check if the post has unsaved changes (now also returning true if the post status is the new status: AbstractPostRemoteStatusSaved).
If the post is not draft, we call the save endpoint that checks if the post blog supports WPRestAPI since this endpoint is not available for self hosted sites.
Not WPCom - we return failure with PostServiceErrorDomain error. and show a noResultsViewController with "preview not available" - needs copy/ design approval (please see gif)
Is WPCom - we call the autosave endpoint and get the previewURL on successful response.
We pass the previewURL to the preview generator to display.

WPCom site preview published post:

preview1

Self hosted site preview published post:
IMG_4064

To test:
Following code review and testing by developers, we have discovered an issue where the preview doesn't show the changes for a Jetpack site.
A. preview post no updates

  1. go to a published post from your post list.
  2. go to preview.
  3. see the post preview with the latest content.

B. Preview post with updates

  1. go to a published post from your post list
  2. edit the post
  3. preview the post
  4. see "Generating preview..."
  5. see the post preview with the latest content.

C. preview after discard

  1. Steps in B
  2. Exit the editor
  3. Tap "Discard" when prompted in the action sheet
  4. go back to the post
  5. hit preview
  6. see the post preview with the latest (published) content.
  • I will update the release notes once approved.
  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@yaelirub yaelirub added this to the 12.5 milestone May 4, 2019

@yaelirub yaelirub added this to Backlog in Offline Support: Posting [iOS] via automation May 4, 2019

@yaelirub yaelirub changed the title Auto save published post upon preview Auto save published post upon preview and show preview url May 4, 2019

Yael Rubinstein and others added 9 commits Apr 22, 2019
Adding publish status to needsLogin
so that attemptNonceAuthenticatedRequest will get used in case of previewing published post
Adding new remote status
and setting the post status to syncedUpdated when saving the post
Passing preview url instead of string
also:

- removing cached response from WebView and clearing cookies on load request
- Now passing authenticated url to preview generator delegate
Handling self hosted sites
When calling autosave on a post in a self hosted site we return a failure.
In display preview we check to see if there was an error and if so we display "preview unavailable" view.

[needs fix] - At this point. There is an issue when discarding a post where once the post is updated to the last published, savePost returns an error and as a result we show the preview unavailable

@yaelirub yaelirub force-pushed the issue/9286_published_post_oreview branch from f6c22de to dac50f0 May 6, 2019

@yaelirub yaelirub requested review from aerych and diegoreymendez May 6, 2019

@yaelirub yaelirub moved this from Backlog to In Progress in Offline Support: Posting [iOS] May 6, 2019

@yaelirub yaelirub moved this from In Progress to In Review in Offline Support: Posting [iOS] May 6, 2019

@aerych
Copy link
Member

left a comment

Heya @yaelirub

I just had a few nitpicky comments. I still need to test the branch, cocapods are being uncooperative but I'll get them sorted directly. Just sharing the few thoughts I already had :)

WordPress/Classes/Services/PostService.h Outdated Show resolved Hide resolved
WordPress/Classes/Services/PostService.m Outdated Show resolved Hide resolved
@aerych

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Tested as described and worked for me! I also tried with a post with the pending review status. No problems.

yaelirub added 2 commits May 8, 2019
Updating according to code review
- Updating method name to autoSave
Updated according to code review
- refactored updating unattached media to its own method to be used both in uploadPost and autosave post
- updated method name from save to autosave

@yaelirub yaelirub force-pushed the issue/9286_published_post_oreview branch from 025bd89 to a39741e May 9, 2019

@aerych

This comment has been minimized.

Copy link
Member

commented May 9, 2019

So... I'm not sure what to make of this. When I was initially testing a self-hosted Jetpack connected site, I was not seeing a preview that included the changes.

Example 1 - Changes missing

autosave 2019-05-09 10_41_32
Note that the site here is a jurassic.ninja test site but its showing the same behavior that I saw on my own test site -- demonstrating that the behavior should be reproducible.

But as I continue to test I do see that changes are showing up when previewing.

Example 2 - Changes present

autosave 2 2019-05-09 11_11_44

Example 2 shows my test site but now previews are working.

I am quite perplexed.


Edit:

If no one else can reproduce I'd be inclined to disregard for now and chalk it up to something weird with my set up. Or if we want to be paranoid .jp sites could be treated like self-hosted sites.

Thoughts?

@megsfulton

This comment has been minimized.

Copy link

commented May 9, 2019

moving towards removing "local previews", we are now showing this screen when the user asks to preview a post from a self hosted site.

Some copy suggestions here to make it clear why the preview is unavailable:

Title:
Preview Unavailable for Self-Hosted Sites

Subtitle:

  1. The updates to your post need to be published to be viewed.
    or
  2. To view the changes, publish the updates to your post.

One other thing, in the gif you shared for "WPCom site preview published post:" -- I noticed "Privately Published" in the post list -- Just want to check that the "Privately Published" label there is unrelated to the preview changes in this issue and it's showing because the post you were updating was a private post?

@diegoreymendez

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Note that the site here is a jurassic.ninja test site but its showing the same behavior that I saw on my own test site -- demonstrating that the behavior should be reproducible.

But as I continue to test I do see that changes are showing up when previewing.

@aerych - Not sure if this is relevant but I recall seeing the same behavior using the web editor, where I had to preview twice for the autosave to be properly loaded.

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Or if we want to be paranoid .jp sites could be treated like self-hosted sites.

I'm testing some more but think that might be what I'd end up doing.

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@megsfulton , thanks so much for your input. I have updated the view as suggested:
Simulator Screen Shot - iPhone Xs - 2019-05-09 at 14 52 38

One other thing, in the gif you shared for "WPCom site preview published post:" -- I noticed "Privately Published" in the post list -- Just want to check that the "Privately Published" label there is unrelated to the preview changes in this issue and it's showing because the post you were updating was a private post?

The privately published label is unrelated. I set it as a private post.

@aerych

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I recall seeing the same behavior using the web editor, where I had to preview twice for the autosave to be properly loaded.

Potentially relevant. Was it when previewing a jetpack site via the wpcom rest api by any chance?

I'm testing some more but think that might be what I'd end up doing.

I'm torn about this but I also don't have another answer.

@diegoreymendez

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Potentially relevant. Was it when previewing a jetpack site via the wpcom rest api by any chance?

In my case it was one of my WP.com test sites.

My feeling around this is that it's a problem with the autosaves API, or something similar... one possibility could be to record this as a known issue and split it from this PR.

It is my opinion that this PR already offers a much better experience than what we had before, even with this bug.

@diegoreymendez
Copy link
Contributor

left a comment

Added some comments. Looking good!

WordPress/Classes/Models/AbstractPost.h Outdated Show resolved Hide resolved
WordPress/Classes/Services/PostService.h Outdated Show resolved Hide resolved
@megsfulton

This comment has been minimized.

Copy link

commented May 10, 2019

Looking at this with fresh eyes today, I think this copy might be clearer for that screen:

Preview Unavailable for Published Posts
Update the published post to view your changes.

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Thanks @megsfulton , updated:
Simulator Screen Shot - iPhone Xs - 2019-05-10 at 16 10 12

yaelirub added 3 commits May 10, 2019
@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@diegoreymendez, updated the code according to your comments.

My feeling around this is that it's a problem with the autosaves API, or something similar... one possibility could be to record this as a known issue and split it from this PR.

I can create a ticket for this. Do you agree, @aerych ?

@aerych

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I can create a ticket for this

Sounds reasonable. 👍

@aerych
aerych approved these changes May 13, 2019
@aerych

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Hmm. I marked approved but maybe I should have dismissed my earlier review instead? 🤔
Regardless, its 👍 from me with Diego's recommended changes + the conflicts resolved.

@diegoreymendez diegoreymendez self-requested a review May 13, 2019

@diegoreymendez
Copy link
Contributor

left a comment

I love the direction taken by this PR.

Very nice work @yaelirub !

@yaelirub yaelirub merged commit 74f4bfc into develop May 13, 2019

3 checks passed

Hound No violations found. Woof!
Peril All green. Woo!
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

Offline Support: Posting [iOS] automation moved this from In Review to Done (PRs) May 13, 2019

@yaelirub yaelirub deleted the issue/9286_published_post_oreview branch May 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.