Sharing multiple connections #4084

kwonye · 2016-05-10T17:53:52Z

To test:

Add multiple accounts to a single connection and post. It should post to the multiple accounts correctly.
Also, disconnect accounts to verify updates to accounts are correct.

Needs review: @nbradbury

nbradbury · 2016-05-10T18:18:02Z

WordPress/src/main/java/org/wordpress/android/ui/publicize/PublicizeWebViewFragment.java

@@ -118,6 +121,17 @@ public void onResume() {
        setNavigationIcon(R.drawable.ic_close_white_24dp);
    }

+    private void clearCookies() {


Am I correct in thinking removeAllCookies() will affect every WebView in the app? If so, this might be a problem since it would force the user to re-login to any authenticated page they visited in the app.

Also, keep in mind that clearing cookies is asynchronous, so it's possible that the cookies won't be cleared before the page is browsed. If this is a concern, you may want to flush the cookie manager before browsing (keeping in mind that's a blocking call).

One final nit: this seems like it belongs in a utility class since it's not tied to the publicize feature.

Am I correct in thinking removeAllCookies() will affect every WebView in the app? If so, this might be a problem since it would force the user to re-login to any authenticated page they visited in the app.

Yup, you're right. I like your emphasis on might because I went through the same thought process. I believe it is ok because I don't see too many use cases where someone has navigated far beyond WordPress-related content. And since with WordPress-related content we are passing the credentials in anyway, they'll always be logged-in.

Also, keep in mind that clearing cookies is asynchronous, so it's possible that the cookies won't be cleared before the page is browsed. If this is a concern, you may want to flush the cookie manager before browsing (keeping in mind that's a blocking call).

Good call. I'll move it to when the WebView closes (like iOS does)

One final nit: this seems like it belongs in a utility class since it's not tied to the publicize feature.

That's fair. I'll create (I couldn't find one) a WebViewUtils class.

Good call. I'll move it to when the WebView closes (like iOS does)

Sorry, didn't finish my thought there. If it's at the end, it will solve the async issue of the webview showing up before the cookies are cleared.

Would it be better to clear cookies from specific domains rather than clear them all? If so, this may help.

I tried that method (and many other similar methods) and unfortunately it doesn't seem to actually work. Setting a cookie doesn't actually overwrite it. It looks like people have had mixed results with different versions of Android.

Because of all of this I believe we should keep the deletion of all cookies.

nbradbury · 2016-05-10T18:27:51Z

When the publicize list activity is shown prior to publicize data being available, the "Loading..." message appears on top of the connections text.

nbradbury · 2016-05-10T19:32:31Z

Prior to testing this PR, I already had a test Twitter account setup with publicize. Using this PR, I attempted to add a second Twitter account. I got all the way through the process, but when I was returned to the publicize detail I saw a toast stating "an error occurred" and the second Twitter account didn't appear.

The log shows "Unexpected response code 400 for https://public-api.wordpress.com/rest/v1.1/sites/52451176/publicize-connections/new"

I tried again with the same result, and it occurs in both the emulator and on a real device.

Note that this 2nd Twitter account has "verify login requests" enabled, which required having Twitter send me a verification code in order to login with the app. I disabled this setting, but the problem still occurred.

…he center

kwonye · 2016-05-10T23:25:51Z

The log shows "Unexpected response code 400 for https://public-api.wordpress.com/rest/v1.1/sites/52451176/publicize-connections/new"

Well that's thoroughly confusing... That's not on the list of errors for that endpoint https://developer.wordpress.com/docs/api/1.1/post/sites/%24site/publicize-connections/new/#apidoc-resource-errors

kwonye · 2016-05-10T23:28:15Z

When the publicize list activity is shown prior to publicize data being available, the "Loading..." message appears on top of the connections text.

Addressed in d4efbf9

nbradbury · 2016-05-12T12:24:31Z

@kwonye Just in case you weren't aware, this is currently failing Travis.

…-user

…gment is removed

kwonye · 2016-05-12T17:35:02Z

@nbradbury ready for a second pass. I've created #4089 where we can address design issues we find.

…-user

kwonye · 2016-05-12T18:56:36Z

@kwonye Just in case you weren't aware, this is currently failing Travis.

Thanks. It's because setImgAlpha() is not available in API 14. I've changed the minAPI to 16 (which will be done by the time this feature ships according to #4052)

nbradbury · 2016-05-12T19:09:31Z

Hmm, something's very broken here.

I have a test Twitter account named AutomatticNickB and my real Twitter account is nbradbury.

AutomatticNickB was previously connected, so I disconnected it and the Twitter connected accounts list correctly showed no connected accounts.

Then I connected my nbradbury account. When I returned to the detail fragment, it showed AutomatticNickB as being connected again, and nbradbury wasn't listed at all.

Just to be clear, this is what I saw after I disconnected AutomatticNickB and connected nbradbury:

kwonye · 2016-05-15T23:54:43Z

@nbradbury I'm in the state of your "Unexpected response code 400" error that you saw. Unfortunately the ServerError is null.

Still unable to repro the incorrect Twitter account showing, but gun to my head I'm guessing they're related.

I'll continue investigating and ping you once this is ready to review again, thanks 👍

…-user

kwonye · 2016-06-29T01:09:14Z

Ok @nbradbury ready for another pass! Multi-users show the correct selection dialog now.

…Fragment to deal with rotation

nbradbury · 2016-06-29T08:08:04Z

When I attempted to connect a Twitter account, I saw the dialog below which incorrectly says I'm connecting Facebook. The description is cut off, as is the list of connected users (and I'm not sure why that list is even shown here). I also don't understand why "Nick Bradbury" is listed four times, nor why it's even in the list (I don't have a Twitter account named "Nick Bradbury").

When I canceled that dialog, I saw a toast that once again mentioned Facebook even though it was a Twitter account.

…ite of that service

kwonye · 2016-06-29T18:15:30Z

@nbradbury thanks for that, we weren't properly filtering out the different services. It's been resolved 👍

nbradbury · 2016-06-29T19:45:33Z

I'm still confused by that dialog (but it looks a lot better). When I clicked to add a Twitter connection, I signed into my AutomatticNickB account. So why does this dialog need to ask me which account to authorize?

…-user

kwonye · 2016-07-07T16:19:57Z

I'm still confused by that dialog (but it looks a lot better). When I clicked to add a Twitter connection, I signed into my AutomatticNickB account. So why does this dialog need to ask me which account to authorize?

@nbradbury unfortunately we do not get the information that tells us which account the user requested. We just get a list of all the connections available, and that dialog shows the ones that are not connected yet. The web has to do this as well unfortunately.

…-user

nbradbury · 2016-07-12T16:21:52Z

I just pulled the latest changes, and now I'm getting a ClassCastException after connecting my Twitter account:

java.lang.ClassCastException: android.support.v7.widget.RecyclerView cannot be cast to android.widget.LinearLayout
                                                                         at org.wordpress.android.ui.publicize.PublicizeAccountChooserDialogFragment.hideConnectedView(PublicizeAccountChooserDialogFragment.java:77)
                                                                         at org.wordpress.android.ui.publicize.PublicizeAccountChooserDialogFragment.configureRecyclerViews(PublicizeAccountChooserDialogFragment.java:70)
                                                                         at org.wordpress.android.ui.publicize.PublicizeAccountChooserDialogFragment.onCreateDialog(PublicizeAccountChooserDialogFragment.java:48)
                                                                         at android.support.v4.app.DialogFragment.getLayoutInflater(DialogFragment.java:307)

This occurs when the app navigates away from Twitter's "Redirecting you back to the application" page.

kwonye · 2016-07-13T00:29:51Z

Thanks @nbradbury, that was a typo on my part. I was pulling the wrong resource ID. It works now.

nbradbury · 2016-07-13T00:39:58Z

Will Kwon and others added 6 commits May 9, 2016 15:21

Always showing connect button

605d313

Configure connect button for adding another account

b29c690

Move Connect button below RecyclerView

3f372fb

Always clear cookies before loading the connect url

a564f54

Removing unused visibility and app namespace

2940999

Updating support library as its required to address a RecyclerView bug

584f42f

kwonye assigned nbradbury May 10, 2016

kwonye added Calypsofication Sharing labels May 10, 2016

kwonye changed the title ~~Feature/4071 sharing multi user~~ Sharing multiple connections May 10, 2016

nbradbury reviewed May 10, 2016
View reviewed changes

making RelativeLayout full height so that the empty text will be in t…

d4efbf9

…he center

kwonye added 3 commits May 12, 2016 10:00

Merge branch 'feature/sharing-master' into feature/4071-sharing-multi…

c1fbe49

…-user

Updating to support library 23.4.0

abc0f60

Moving the removal of cookies to a Utility class and calling when fra…

a5f711c

…gment is removed

kwonye added 2 commits May 12, 2016 10:37

Merge branch 'feature/sharing-master' into feature/4071-sharing-multi…

ea12ec2

…-user

Change minSdkVersion to 16 so that ImageView.setImgAlpha() can be called

068a6cf

kwonye added 3 commits May 15, 2016 19:38

Clearing cookies after connection request

a925ce7

Merge branch 'feature/sharing-master' into feature/4071-sharing-multi…

a7ebbeb

…-user

Only getting keyringConnectionId automatically if there is one account

9854084

kwonye added 10 commits June 28, 2016 17:11

Merge branch 'feature/sharing-master' into feature/4071-sharing-multi…

0862b84

…-user

Showing toast when cancelling

d98c8a7

Converting logic so that progress dialog can be shown

bff28c8

Removing extra spaces

23b4fe9

Cleaning up class. No functional changes.

5472f16

Removing unused CHOOSE_ACCOUNT enum value

77d349e

Removing unused method

6e78728

Renaming class to be consistent with the rest

453f9a9

Converting magic strings

da270a8

Cleaning up strings/dimension

d1cb875

kwonye removed the [Status] Not Ready for Review label Jun 29, 2016

kwonye added 2 commits June 28, 2016 18:23

Can't use same linearLayoutManager for two recyclerviews

5773983

Passing in JSONObject as String args to PublicizeAccountChooserDialog…

ccab771

…Fragment to deal with rotation

kwonye added 2 commits June 29, 2016 09:42

Making sure only the correct type of account is connected

30c4149

Only showing connection dialog if there is at least one unconnected s…

2a145e5

…ite of that service

kwonye added 2 commits July 4, 2016 19:50

Hiding connected text if there are no connected accounts

30e0989

Merge branch 'feature/sharing-master' into feature/4071-sharing-multi…

e22d218

…-user

kwonye added 2 commits July 7, 2016 17:15

Merge branch 'feature/sharing-master' into feature/4071-sharing-multi…

73a5503

…-user

Merge branch 'feature/sharing-master' into feature/4071-sharing-multi…

a6b15f9

…-user

Fixing resource that is being hidden

c5c7d90

nbradbury merged commit 087f846 into feature/sharing-master Jul 13, 2016

nbradbury deleted the feature/4071-sharing-multi-user branch July 13, 2016 00:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sharing multiple connections #4084

Sharing multiple connections #4084

kwonye commented May 10, 2016

nbradbury May 10, 2016

kwonye May 12, 2016

kwonye May 12, 2016

nbradbury May 12, 2016

kwonye May 15, 2016

nbradbury commented May 10, 2016

nbradbury commented May 10, 2016

kwonye commented May 10, 2016

kwonye commented May 10, 2016

nbradbury commented May 12, 2016

kwonye commented May 12, 2016

kwonye commented May 12, 2016

nbradbury commented May 12, 2016

kwonye commented May 15, 2016

kwonye commented Jun 29, 2016

nbradbury commented Jun 29, 2016

kwonye commented Jun 29, 2016

nbradbury commented Jun 29, 2016

kwonye commented Jul 7, 2016

nbradbury commented Jul 12, 2016

kwonye commented Jul 13, 2016

nbradbury commented Jul 13, 2016

Sharing multiple connections #4084

Sharing multiple connections #4084

Conversation

kwonye commented May 10, 2016

nbradbury May 10, 2016

Choose a reason for hiding this comment

kwonye May 12, 2016

Choose a reason for hiding this comment

kwonye May 12, 2016

Choose a reason for hiding this comment

nbradbury May 12, 2016

Choose a reason for hiding this comment

kwonye May 15, 2016

Choose a reason for hiding this comment

nbradbury commented May 10, 2016

nbradbury commented May 10, 2016

kwonye commented May 10, 2016

kwonye commented May 10, 2016

nbradbury commented May 12, 2016

kwonye commented May 12, 2016

kwonye commented May 12, 2016

nbradbury commented May 12, 2016

kwonye commented May 15, 2016

kwonye commented Jun 29, 2016

nbradbury commented Jun 29, 2016

kwonye commented Jun 29, 2016

nbradbury commented Jun 29, 2016

kwonye commented Jul 7, 2016

nbradbury commented Jul 12, 2016

kwonye commented Jul 13, 2016

nbradbury commented Jul 13, 2016