Previews: Fix for previewing posts on sites with mapped domains. by aerych · Pull Request #15061 · wordpress-mobile/WordPress-iOS

aerych · 2020-10-07T03:31:32Z

This PR adds new logic to preview URLs to ensure that previewing on a site with a mapped domain will use the site's unmapped URL rather than the mapped URL. This will let our preview browser use existing auth cookies rather than having to make a complicated series of requests to obtain an authenticated session.

@ScoutHarris could I trouble you for a review? Also ccing @guarani who has looked into this previously.

** Merge after landing related changes to WordPressKit.

To test:
Prep:

Start from a fresh install so there are no cookies lingering in the cookie jar. For an extra level of paranoia, delete and reinstall for each scenario.

Scenario 1: Mapped Domain Draft Preview

Create a new draft post on a wpcom site with a mapped domain.
Preview the draft.
Confirm you see the remote preview and not an error.

Scenario 2: Mapped Domain Published Preview

Edit an already published post on a wpcom site with a mapped domain.
Preview your changes.
Confirm you see the remote preview and not an error.

Scenario 3: Unmapped Site Draft Preview

Repeat Scenario 1 for this site type. Confirm there is no regression.

Scenario 4: Unmapped Published Preview

Repeat Scenario 2 for this site type. Confirm there is no regression.

Scenario 5: Jetpack Site Draft Preview

Repeat Scenario 1 for this site type. Confirm there is no regression.

Scenario 6: Jeptack Site Published Preview

Repeat Scenario 2 for this site type. Confirm there is no regression.

Scenario 7: Self-hosted Site Draft Preview

Repeat Scenario 1 for this site type. Confirm there is no regression.

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 · 2020-10-07T03:31:37Z

You can trigger an installable build for these changes by visiting CircleCI here.

…nto issues/14220-previews-for-mapped-domains

peril-wordpress-mobile · 2020-10-07T15:38:29Z

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

ScoutHarris · 2020-10-07T17:59:28Z

WordPress/Classes/ViewRelated/Post/Preview/PreviewNonceHandler.swift

+
+    private static func unmapURL(post: AbstractPost, url: URL) -> URL? {
+        guard
+            let components = URLComponents(url: url, resolvingAgainstBaseURL: false),


Suggestion - if this was a var you wouldn't need comps below.

Oh ha! I never realized guard var was allowed! TIL. Thanks!

ScoutHarris · 2020-10-07T18:49:55Z

Hey @aerych .

Here are my testing results.

Successes:
✅ Scenario 1: Mapped Domain Draft Preview
✅ Scenario 3: Unmapped Site Draft Preview
✅ Scenario 4: Unmapped Published Preview
✅ Scenario 5: Jetpack Site Draft Preview
✅ Scenario 7: Self-hosted Site Draft Preview

Failures:
🚫 Scenario 2: Mapped Domain Published Preview - the preview doesn't show edits until after the post is saved.

Both of these behaved the same:
🚫 Scenario 6: Jetpack Site Published Preview - if it matters, I was using a self-hosted site with Jetpack installed. (via jurassic.ninja)
🚫 Not mentioned: Self-hosted Published Preview - I was using a self-hosted site without Jetpack. (via jurassic.ninja)

On a published post > Edit > Preview > shows preview successfully.
On a published post > Edit > edit the post > Preview > shows the Preview Unavailable error. Also, this was in the logs:

Error while trying to save post before preview: Optional(Error Domain=PostServiceErrorDomain Code=0 "Previews are unavailable for this kind of post." UserInfo={NSLocalizedDescription=Previews are unavailable for this kind of post.})

ScoutHarris

See testing results.

aerych · 2020-10-07T23:30:39Z

Thank you @ScoutHarris !

Scenario 2: Mapped Domain Published Preview - the preview doesn't show edits until after the post is saved

For this one I found two scenarios where the preview wouldn't show. One was when proxied 😬. The other depended on the site's permalink format. I've pushed an update that should take care of this case. Would you mind trying again?

Scenario 6: Jetpack Site Published Preview - if it matters, I was using a self-hosted site with Jetpack installed. (via jurassic.ninja)

Arg! I even had a recording of this working. :( Sadly, it looks like I was testing this under special circumstances so my expectations were off. The preview is not expected to be shown in this case so, what you saw was probably expected and not a regression, but please confirm!

Not mentioned: Self-hosted Published Preview - I was using a self-hosted site without Jetpack

Yeah. I omitted this because these previews were not expected to work in this scenario. (No autosave post type.) :)

Ready for another peek!

ScoutHarris · 2020-10-08T00:26:28Z

Hey @aerych .

For this one I found two scenarios where the preview wouldn't show. One was when proxied 😬. The other depended on the site's permalink format. I've pushed an update that should take care of this case. Would you mind trying again?

I'm not sure what's wrong but my .blog site doesn't show unsaved edits in the preview. I tried on a real device, so I wasn't proxied.

aerych · 2020-10-08T00:37:06Z

Hmm... I wonder if we might be taking different steps? This is what I'm doing:

Lemme know if you spot a difference and I'll try it and see if I can repo what you're seeing.

aerych · 2020-10-08T00:57:45Z

Just in case using a device mattered, I repeated the above test on my iPhone 11 Pro (iOS 13.7) and I'm seeing the same result. (I did use different test text just to be sure.) The previous test was in the iPhone 11 Pro (iOS 14) simulator.

I'm wondering if the issue you're seeing might be due to a glitch with the cookie auth (not sure yet how to test this). I can look into this angle tomorrow.

…nto issues/14220-previews-for-mapped-domains

aerych · 2020-10-08T21:44:49Z

That was a merry little debugging adventure :)

So happy you happened to test with an Atomic site! After logging in to your test site as an editor (thank you again!) I was able to reproduce the issue there. To recap our chat conversation, the issue turned out to be that for Atomic sites, the value of their unmapped_url is still their mapped domain name, not a wpcom subdomain like for my test site. This meant that the wpcom auth cookies weren't valid for the atomic site, so the published version of the post was shown rather than the autosaved version. This makes sense considering the way Atomic sites are configured.

Some investigation revealed the trick Calypso uses is to load wp-login.php with a redirect to the desired page. In the case of Atomic sites with SSO enabled the redirect is fairly seamless. In cases where SSO is not enabled, there is still a prompt for credentials as a semi-graceful fallback. It's not perfect, but better than the current situation.

I've updated the PR with some logic that checks for public atomic sites and rewrite the request URL to proxy through wp-login.php. I tested with your test site and it was working for me there. I also created an ephemeral atomic site to test with but, interestingly, these sites do not report themselves as being atomic (?!) via the REST API and so regular wpcom auth is attempted. This is something that needs to be addressed server-side, not in the app, so 🤷 for now. I'll open an Android issue regarding Atomic published post previews.

Ready for another peek!

cc @diegoreymendez in case you're interested in this as well.

ScoutHarris

Excellent debugging skillz!! It is in fact working as advertised. Well done!

diegoreymendez · 2020-10-09T01:45:11Z

WordPress/Classes/Utility/Networking/RequestAuthenticator.swift

+        }
+
+        authenticationService.loadAuthCookiesForWPCom(into: cookieJar, username: username, authToken: authToken, success: {
+            done()


Fairly minor, but this jumped out at me while looking at the code. I think you can simplify the success param to just be:

success: done,

(Without the {}s and ()s)

True. If you don't mind I think I'll leave it as is so it's consistent with the other methods in that class. Thanks for taking a peek!

…nto issues/14220-previews-for-mapped-domains

aerych · 2020-10-09T17:51:35Z

Thank you @ScoutHarris and @diegoreymendez !

Fixes #15266 Prior to #15061 (or b01f2e6 to be specific), post previews on sites with custom domains used the sites public URL (not the mapped `*.wordpress.com` one). This PR restores the previous behavior by reverting a small change that led to the `preview=true` query parameter being added to post preview URLs.

Previews: Fix for previewing posts on sites with mapped domains.

c896727

aerych added [Type] Enhancement labels Oct 7, 2020

aerych added this to the 16.0 milestone Oct 7, 2020

aerych requested a review from ScoutHarris October 7, 2020 03:31

aerych self-assigned this Oct 7, 2020

aerych mentioned this pull request Oct 7, 2020

Blog: Include unmapped_url key in blog options. wordpress-mobile/WordPressKit-iOS#291

Merged

1 task

Merge branch 'develop' of github.com:wordpress-mobile/WordPress-iOS i…

6cc5c83

…nto issues/14220-previews-for-mapped-domains

ScoutHarris reviewed Oct 7, 2020

View reviewed changes

ScoutHarris self-requested a review October 7, 2020 18:50

ScoutHarris suggested changes Oct 7, 2020

View reviewed changes

aerych added 2 commits October 7, 2020 14:25

Simplify things via a var instead of a let declaration

d785bfc

Previews: Improvements to nonce handler.

b01f2e6

ScoutHarris self-requested a review October 7, 2020 23:37

aerych added 2 commits October 8, 2020 10:11

Merge branch 'develop' of github.com:wordpress-mobile/WordPress-iOS i…

d83185a

…nto issues/14220-previews-for-mapped-domains

Previews: Add auth handler for non-private atomic sites.

1a8a97b

aerych mentioned this pull request Oct 8, 2020

Post Previews: Previewing changes to an already published post on an Atomic site shows the published version wordpress-mobile/WordPress-Android#13093

Closed

ScoutHarris approved these changes Oct 8, 2020

View reviewed changes

diegoreymendez reviewed Oct 9, 2020

View reviewed changes

aerych added 3 commits October 9, 2020 10:00

Update WordPressKit reference

b17692b

Updates Podfile.lock

5eb715e

Merge branch 'develop' of github.com:wordpress-mobile/WordPress-iOS i…

f79a153

…nto issues/14220-previews-for-mapped-domains

aerych added 2 commits October 9, 2020 10:48

Merge branch 'develop' of github.com:wordpress-mobile/WordPress-iOS i…

415cdfb

…nto issues/14220-previews-for-mapped-domains

Bump CI Cache version

38f1fc7

aerych merged commit 5ac8b86 into develop Oct 9, 2020

aerych deleted the issues/14220-previews-for-mapped-domains branch October 9, 2020 17:51

guarani mentioned this pull request Nov 13, 2020

Restore previous post preview functionality #15288

Closed

3 tasks

designsimply mentioned this pull request Nov 13, 2020

View link: sharing a link to the post displays a preview URL instead of the public one #15266

Closed

Conversation

aerych commented Oct 7, 2020

Uh oh!

peril-wordpress-mobile bot commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ScoutHarris Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

aerych Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

ScoutHarris commented Oct 7, 2020

Uh oh!

ScoutHarris left a comment

Choose a reason for hiding this comment

Uh oh!

aerych commented Oct 7, 2020

Uh oh!

ScoutHarris commented Oct 8, 2020

Uh oh!

aerych commented Oct 8, 2020

Uh oh!

aerych commented Oct 8, 2020

Uh oh!

aerych commented Oct 8, 2020

Uh oh!

ScoutHarris left a comment

Choose a reason for hiding this comment

Uh oh!

diegoreymendez Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

aerych Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

aerych commented Oct 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peril-wordpress-mobile bot commented Oct 7, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Oct 7, 2020 •

edited

Loading