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

Reconnect publicize from editor #6912

Merged
merged 5 commits into from Mar 23, 2017

Conversation

koke
Copy link
Member

@koke koke commented Mar 22, 2017

We already show if a connection is broken from post options. This lets the user fix the issue right there. I had to refactor things a little bit as the sharing detail expected the publicize services to be synced, so I removed that requirement. It's still not bulletproof as the sync can fail and there's no retry, but I'd leave that for the new network infrastructure.

I also had to fix this crazy hard to debug thing where the same authorization web view would work from the Sharing section but not when presented from the editor, because the editor enables PrivateSiteURLProtocol. I updated it so it forwards redirects to the client, but perhaps we should limit it to wordpress.com sites (i.e. exclude public-api).

Fixes #6837

To test:

  • Do a fresh install to ensure publicize services aren't cached
  • Break one of the publicize connections on the test account (e.g. remove WordPress from https://www.facebook.com/settings?tab=applications)
  • Launch the app and log in
  • Create a new post
  • Go to post options
  • Notice the exclamation icon next to the broken account and tap on it
  • The sharing details view shows
  • Tap on Reconnect
  • A web browser pops up to reconnect the account

Needs review: @aerych

If we don't propagate redirections, UIWebView gets all confused about what it
just loaded. Specifically, when reconnecting a publicize connection from the
post options, the REST API would redirect to facebook, but the web view would
think it was showing public-api.wordpress.com, and everything breaks from
Javascript to relative links.
@koke koke added this to the 7.3 milestone Mar 22, 2017
@koke koke requested a review from aerych March 22, 2017 10:07
@@ -180,7 +174,37 @@ - (void)reconnectPublicizeConnection
return;
}

[self.helper reconnectPublicizeConnection:self.publicizeConnection];
SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
self.helper = [self authorizationHelper];
Copy link
Member

Choose a reason for hiding this comment

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

Why assign self.helper here but then reassign it inside the attemptReconnect closure?

}
}

- (SharingAuthorizationHelper *)authorizationHelper
Copy link
Member

Choose a reason for hiding this comment

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

Maybe refactor this to make helper a read-only lazy getter, and logic in this method part of the implementation?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, since this is the only place self.helper is really used (apart from the dealloc) maybe we could get rid of the property completely?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only reason we keep the property is so we can set the delegate to nil on dealloc.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is necessary since the delegate is already a weak reference.

@koke
Copy link
Member Author

koke commented Mar 23, 2017

OK, here's another go at it, without the property. This felt like trying to write Swift optional style code in Obj-C, and it's so weird. Also, I'm so ready to move past success/failure async programming 😬

@aerych
Copy link
Member

aerych commented Mar 23, 2017

:shipit: yo :)

@koke koke merged commit cf9feb0 into develop Mar 23, 2017
@koke koke deleted the issue/6837-reconnect-publicize-from-editor branch March 23, 2017 14:01
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.

Publicize: Help users fix a broken publicize connection in post options
2 participants