Fixes Twitter publicize connections #9629
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
19 tasks
mzorz
reviewed
Apr 16, 2019
| @@ -161,7 +161,8 @@ public void onErrorResponse(VolleyError volleyError) { | |||
|
|
|||
| Map<String, String> params = new HashMap<>(); | |||
| params.put("keyring_connection_ID", Long.toString(keyringConnectionId)); | |||
| if (!externalUserId.isEmpty()) { | |||
| // Sending the external id for Twitter connections result in an error | |||
| if (!externalUserId.isEmpty() && !serviceId.equals(PublicizeConstants.TWITTER_ID)) { | |||
There was a problem hiding this comment.
wdyt about using TextUils.isEmpty(externalUserId) and PublicizeConstants.TWITTER_ID.equals(serviceId) to avoid potential NPEs?
There was a problem hiding this comment.
Ha, I have been in Kotlin world for too long, good catch! I'll make the change later today.
There was a problem hiding this comment.
Addressed in dc81204.
mzorz
approved these changes
Apr 16, 2019
|
@mzorz I've addressed the possible NPE issue and merged |
|
Thanks for this @oguzkocer ! Will merge once CircleCI goes 💚 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8652.
When we tried to add a new publicize connection with the
/sites/$site/publicize-connections/newendpoint, it complained with the following error:This is easily reproducible by:
v1.1/me/keyring-connections/IDof it askeyring_connection_IDandexternal_IDof it asexternal_user_IDand make a request to/sites/$site/publicize-connections/newfor any of your sites. You can also use the/me/publicize-connections/newto create a connection for all of your sitesThis Publicize service does not support setting an external user ID.I checked Calypso and they are not sending the
external_user_IDat all for Twitter connections. (They also send the parameters as a json object but that doesn't seem to make a difference) I initially tried to remove theexternal_user_IDfor all connection requests, but Facebook does expect it and since #8652 says the issue only affects Twitter, the most straightforward solution is to not send theexternal_user_IDparameter for it.To test:
My Site->Sharing->TwitterIn my test I was able to connect 2 different Twitter accounts without any issues and a Facebook account once I add a page to my test Facebook account. I don't have a handy test account for LinkedIn or Tumblr, but this PR shouldn't change anything about them. If anyone has it handy, it'd be good to test them just in case.
Finally, I found an issue with the Facebook connections which I'll create a new issue for.
Update release notes:
RELEASE-NOTES.txt.