Issues/update facebook sharing by aerych · Pull Request #9867 · wordpress-mobile/WordPress-iOS

aerych · 2018-07-28T22:00:55Z

This is an 11th hour patch to address Facebook's privacy changes that affect publicize sharing to Facebook Profiles. As of August 1st, sharing to Facebook Profiles is no longer allowed.

This patch changes how Facebook sharing is handled so only sharing to Facebook Pages is supported. Attempting to share to a profile will result in a notice.

An existing connection to a Facebook profile will show up as broken. On the detail page new messaging is shown.

Clicking Learn More on either screen will open Safari to an FAQ. The destination is different for each button and should match the behavior in calypso.

To test:
You'll want a Facebook account to test with.

Scenario 1:
Attempt to set up a sharing connection to a Facebook profile. You should see the prompt that the connection could not be made because their are no pages.
Confirm clicking the Learn More button opens the support page in Safari.

Scenario 2:
Create a Facebook Page.
Attempt to set up sharing to Facebook again, and this time confirm that your newly created page is shown as an option. Go ahead and make the connection.

Scenario 3:
Edit SharingDetailViewController.m so mustDisconnectFacebook always returns true.
Navigate to the Facebook connection details screen and confirm that instead of a "Reconnect" option, you see a "Learn More" option and the appropriate footer text.
Confirm that the FAQ link opens in Safari.

FYI @loremattei

@ctarda could I trouble you for a review?

cc @kwonye who is wrangling the Android version of this patch.

houndci-bot · 2018-07-28T22:02:19Z

WordPress/Classes/ViewRelated/Blog/SharingAccountViewController.swift

+
+
+extension SharingAccountViewController: NoResultsViewControllerDelegate
+{


Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

houndci-bot · 2018-07-28T22:02:19Z

WordPress/Classes/ViewRelated/Blog/SharingAccountViewController.swift

+        let title = NSLocalizedString("No Accounts Found",
+                                      comment:"Title of an error message. There were no third-party service accounts found to setup sharing.")
+        let message = NSLocalizedString("Sorry. The social service did not tell us which account could be used for sharing.",
+                                        comment:"An error message shown if a third-party social service does not specify any accounts that an be used with publicize sharing.")


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2018-07-28T22:02:19Z

WordPress/Classes/ViewRelated/Blog/SharingAccountViewController.swift


+    fileprivate func showNoResultsViewController() {
+        let title = NSLocalizedString("No Accounts Found",
+                                      comment:"Title of an error message. There were no third-party service accounts found to setup sharing.")


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

…nto issues/update-facebook-sharing

wpmobilebot · 2018-07-28T22:27:59Z

	1 Warning
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 Danger

kwonye · 2018-07-29T18:16:36Z

@aerych I believe this should be merged into 10.5 as that's the build going to the app store tomorrow.

kwonye · 2018-07-29T18:33:19Z

WordPress/Classes/ViewRelated/Blog/SharingAccountViewController.swift

+
+extension SharingAccountViewController: NoResultsViewControllerDelegate {
+    func actionButtonPressed() {
+        if let url = URL(string: "https://en.support.wordpress.com/publicize/#facebook-pages") {


@aerych do you think we should show the blog post instead?

This should match the behavior in Calypso where the support page is linked from the message shown when attempting to link a Facebook account with no pages. The blog post is linked from the message shown for a pre-existing connection that needs to be disconnected.

Unless I somehow got that backwards 😬

aerych · 2018-07-29T19:00:26Z

I believe this should be merged into 10.5 as that's the build going to the app store tomorrow.

This isn't really a trivial patch so I'm not too keen targeting 10.5. The changes include linking to updated pod(s) and adds a new model revision -- things that really need to go through the normal QA cycle.

kwonye · 2018-07-29T23:27:49Z

This isn't really a trivial patch so I'm not too keen targeting 10.5. The changes include linking to updated pod(s) and adds a new model revision -- things that really need to go through the normal QA cycle.

@aerych totally fair, but I don't think we should wait the full two weeks to release this after the facebook change. We have 200+ people adding a connection a day (I don't know how many are facebook)

What do you think about a hotfix for 10.5.1 after testing for a couple days?

aerych · 2018-07-29T23:37:48Z

What do you think about a hotfix for 10.5.1 after testing for a couple days?

Sounds good.

ctarda · 2018-07-30T02:28:27Z

WordPress/Classes/ViewRelated/Blog/SharingAccountViewController.swift

    @objc var immutableHandler: ImmuTableViewHandler!
    @objc var delegate: SharingAccountSelectionDelegate?

+    lazy var noResultsViewController: NoResultsViewController = {


Would it make sense to declare this var as private? Or do we really need it to be fully mutable?

ctarda · 2018-07-30T02:31:19Z

WordPress/Classes/ViewRelated/Blog/SharingAccountViewController.swift



+    fileprivate func showNoResultsViewController() {
+        let title = NSLocalizedString("No Accounts Found",


This is a completely personal preference, but I tend to declare strings as constants (or as static cases in an enumeration), so that, at the call site, like here, it would read similar to this:

fileprivate func showNoResultsViewController() { noResultsViewController.configure(title: "", buttonTitle: NoResults.buttonTitle, subtitle: NoResults.message, image: nil, accessoryView: nil) noResultsViewController.delegate = self }

I have found that makes the code slightly easier to parse for me. What do you think?

I'm a fan of that when a string is reused. When used only once I like declaring a string in the context of its usage as a way of having as much context as possible in one place.

ctarda · 2018-07-30T02:35:09Z

WordPress/Classes/ViewRelated/Blog/SharingAccountViewController.swift

        var accounts = keyringAccountsFromKeyringConnections(keyringConnections)

+        if accounts.count == 0 {
+            if publicizeService.externalUsersOnly && publicizeService.serviceID == PublicizeService.facebookServiceID {


Would it make sense to encapsulate this boolean check in a method in PublicizeService, named in a way that provides a clear explanation of why we want to call showFacebookNotice()?

If I read this code, without any context, I don't really know what is going on. I mean, I can understand it, but I don't really understand why this boolean check is necessary, what is the meaning of it.

In a way, it's also leaking the abstraction a little bit. This view controller needs to know about the specific internal "state" of the PublicizeService, when it probably should not need to be aware of it.

Looking a bit closer at this. The publicizeService.externalUsersOnly is redundant here so we can remove it. This will make it a check for Facebook to see if it needs special handling which will hopefully make things a bit clearer.

ctarda · 2018-07-30T02:37:35Z

WordPress/Classes/ViewRelated/Blog/SharingConnectionsViewController.m

    cell.textLabel.text = connection.externalDisplay;

-    if ([connection isBroken]) {
+    if ([connection isBroken] || [connection mustDisconnect]) {


I tend to encapsulate these boolean checks into a single method. In my experience, that makes the code more readable, in the long term, because it provides a name that can clearly explain what is happening and why it is important.

In this case, also, I think it might make sense to encapsulate that logic in the PublicizeConnection object as well, so this view controller does not need to know about isBroken and / or mustDisconnect.

What do you think?

ctarda · 2018-07-30T02:39:24Z

WordPress/Classes/ViewRelated/Blog/SharingDetailViewController.m


+- (BOOL)mustDisconnectFacebook
+{
+    return [self.publicizeConnection mustDisconnect] && [[self.publicizeConnection service] isEqualToString: [PublicizeService facebookServiceID]];


Would this logic belong in the PublicizeConnection object?

ctarda · 2018-07-30T02:41:38Z

WordPress/Classes/ViewRelated/Blog/SharingDetailViewController.m

 - (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView
 {
-    if ([self.publicizeConnection isBroken]) {
+    if ([self.publicizeConnection isBroken] || [self mustDisconnectFacebook]) {


This boolean check is also done in SharingConnectionsViewController. In my opinion, that also supports moving this logic to the PublicizeConnection object. In this case, also, this boolean check is split in between two classes: part of the logic is defined in the "connection" object, while another part of it is in the mustDisconnectFacebook method, which also pulls properties from the connection. What do you think?

ctarda · 2018-07-30T02:43:32Z

WordPress/Classes/ViewRelated/Blog/SharingDetailViewController.m

-        NSString *title = NSLocalizedString(@"There is an issue connecting to %@. Reconnect to continue publicizing.", @"Informs the user about an issue connecting to the third-party sharing service. The `%@` is a placeholder for the service name.");
-        return [NSString stringWithFormat:title, self.publicizeConnection.label];
+
+    if (section == 1) {


I would suggest extracting each logical branch to a separate method. That way titleForFooterInSection would be easier to read and parse, which might also make it. in the long term, more difficult to get wrong.

ctarda · 2018-07-30T02:44:48Z

WordPress/Classes/ViewRelated/Blog/SharingDetailViewController.m

-        cell.textLabel.textAlignment = NSTextAlignmentCenter;
-        cell.textLabel.textColor = [WPStyleGuide jazzyOrange];
+    } else if (indexPath.section == 1) {
+        if ([self.publicizeConnection isBroken]) {


I would also suggest extracting each logical branch to a separate method, if possible. That would make this code easier to read, to begin with. Also, there seems to be an issue with this title (more in another comment), so making this code simpler to read might have the extra advantage of fixing that :)

ctarda · 2018-07-30T02:45:53Z

WordPress/Classes/ViewRelated/Blog/SharingViewController.m

    // Check if any of the connections are broken.
    for (PublicizeConnection *pubConn in connections) {
-        if ([pubConn isBroken]) {
+        if ([pubConn isBroken] || [pubConn mustDisconnect]) {


This seems to be the third different place where this boolean check is done, which, in my opinion, supports encapsulating it in the PublicizeConnetion object

ctarda · 2018-07-30T02:51:59Z

Hi @aerych! Thanks for attacking this, and thanks also for thinking of me for the review.

The code looks great, I just have a made a few comments to trigger a couple of discussions. 😉

Scenario #1 works great.

Something seems to be slightly off in the "Learn More" button title for the other two scenarios though.

For the Scenario #2, I get a button with an empty label:

And for Scenario #3, I get a "Reconnect" instead of "Learn More"

I have tested the feature by upgrading from a previous build, but also after a clean install, and on both cases I get the same results

aerych · 2018-07-30T03:08:44Z

Well, that's embarrassing 😬. I see the issue with the missing button labels now. Not sure how I missed this before. Thanks for spotting it!

aerych · 2018-07-30T04:57:52Z

Hiya @ctarda ! Thanks for the review and suggestions. :) I think I've implemented the changes the way you intended but if I misunderstood let me know and I'll revise.

(The branching logic makes me wish refactoring the table views to use ImmuTable was in scope. It would make things clearer, I think. Something for a future revision.)

Ready for another peek!

ctarda · 2018-07-30T06:02:39Z

Hi @aerych ! Thanks for taking the suggestions into consideration.

One thing that I've just noticed (it might be after the latest update, but I am not completely sure it wasn't there before) is that a pod update updates my local Podfile.lock, which I think should not happen.

When it comes to the functionality, something seems to be slightly off. Scenario 1 is always prompting that the connection could not be made because there are no pages (even though there are pages in the testing account I am using)

aerych · 2018-07-30T08:42:16Z

a pod update updates my local Podfile.lock, which I think should not happen.

Nicely spotted. It looks like the Podfile.lock comitted in 620ddb3 pointed to version 1.3, instead of 1.2.1. I've fixed the reference in a79505e

Scenario 1 is always prompting that the connection could not be made because there are no pages (even though there are pages in the testing account I am using)

Hmm... I'm not sure what this could be. This is what I'm seeing with my test Facebook account with a single Facebook page:

What are you seeing that's different?

ctarda · 2018-07-30T10:11:07Z

👋 sorry for the delay, @aerych ! This is what I see:

This is a wordpress.com site, in case it is relevant.

aerych · 2018-07-30T14:50:40Z

Closing this in favor of #9872 so we're not branching off develop.

aerych added 7 commits July 27, 2018 15:31

Sharing: Adds new field to model and bumps model version.

1e8623b

Bumps version of WordPressKit used.

620ddb3

Update PublicizeService.externalUsersOnly with value from remote.

6856967

Publicize: Wires up a no results view when there are no accounts.

170fe82

Publicize: Show facebook prompt when no pages to share to.

5a35eb5

Publicize: Handle status.

55eb943

Publicize: Handle must-disconnect facebook connections.

e84c448

aerych added the Sharing label Jul 28, 2018

aerych added this to the 10.6 milestone Jul 28, 2018

aerych requested review from ctarda and kwonye July 28, 2018 22:00

houndci-bot reviewed Jul 28, 2018

View reviewed changes

aerych added 2 commits July 28, 2018 17:05

Fix whitespace. Shut up hound.

658f604

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

4a1de8c

…nto issues/update-facebook-sharing

kwonye reviewed Jul 29, 2018

View reviewed changes

ctarda reviewed Jul 30, 2018

View reviewed changes

aerych added 2 commits July 29, 2018 23:46

Publicize: Adds helper methods to model.

96dc32c

Publicize: Changes from code review.

ba54dd7

Fixes Podfile.lock

a79505e

Merge branch 'develop' into issues/update-facebook-sharing

1c59a17

aerych mentioned this pull request Jul 30, 2018

Issues/facebook sharing #9872

Merged

aerych closed this Jul 30, 2018

aerych deleted the issues/update-facebook-sharing branch July 31, 2018 02:19



		extension SharingAccountViewController: NoResultsViewControllerDelegate
		{



		fileprivate func showNoResultsViewController() {
		let title = NSLocalizedString("No Accounts Found",

Conversation

aerych commented Jul 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Jul 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwonye commented Jul 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aerych commented Jul 29, 2018

Uh oh!

kwonye commented Jul 29, 2018

Uh oh!

aerych commented Jul 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctarda commented Jul 30, 2018

Uh oh!

aerych commented Jul 30, 2018

Uh oh!

aerych commented Jul 30, 2018

Uh oh!

ctarda commented Jul 30, 2018

Uh oh!

aerych commented Jul 30, 2018

Uh oh!

ctarda commented Jul 30, 2018

Uh oh!

aerych commented Jul 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aerych commented Jul 28, 2018 •

edited

Loading

wpmobilebot commented Jul 28, 2018 •

edited

Loading