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

Fixes Publicize connection flow #10378

Merged
merged 3 commits into from Aug 13, 2019

Conversation

@malinajirka
Copy link
Contributor

commented Aug 12, 2019

Fixes #10252

This line of code was introduce in #8115. The server responds with Http Error code 400 when we pass external_user_ID parameter to a service which doesn't support it. However, the only service supporting this parameter is Facebook. Therefore the PR mentioned above broke connecting flows to all the other services.
Our users mostly connect Facebook (which works) and Twitter accounts. We introduce a workaround for Twitter in #9629. However, we didn't realize other connections might be also broken due to the same bug.
This PR fixes this issue once and for all by making sure we send external_user_ID to Facebook only.

To test:

  1. My Site
  2. Sharing
  3. Linked In
  4. Connect
  5. Complete the flow and notice the connection is successfully established

Perform the same steps for Tumblr and Twitter.

Update release notes:

  • [ x ] If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@malinajirka malinajirka added this to the 13.2 milestone Aug 12, 2019

@malinajirka malinajirka requested a review from mkevins Aug 12, 2019

@malinajirka malinajirka changed the title Fixes LinkedIn connection flow WIP: Fixes LinkedIn connection flow Aug 12, 2019

@malinajirka malinajirka changed the title WIP: Fixes LinkedIn connection flow Fixes Publicize connection flow Aug 12, 2019

@malinajirka malinajirka requested a review from mzorz Aug 12, 2019

@mkevins
Copy link
Contributor

left a comment

Nice fix. 🎉

Do we have any insight about whether this will change in the future:

the only service supporting this parameter is Facebook

I am wondering if it makes sense (in a separate PR) to modify the end-point to follow the robustness principle. I was looking around here but I didn't spend a lot of time digging, and I'm not certain that's the right place. Anyway, it's outside the scope of this PR, but I wanted to comment for posterity in case a future change brings up a similar issue.

@mkevins mkevins referenced this pull request Aug 13, 2019
3 of 4 tasks complete
@malinajirka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Thank you for the review! ;)

I am wondering if it makes sense (in a separate PR) to modify the end-point to follow the robustness principle. I was looking around here but I didn't spend a lot of time digging, and I'm not certain that's the right place. Anyway, it's outside the scope of this PR, but I wanted to comment for posterity in case a future change brings up a similar issue.

Tbh I'm a fan of the fail fast principle. Perhaps this is a case where it's on the edge, but I kind of like that the API fails right away and tells you what is wrong. Imagine you would be sending external_user_DI instead of external_user_ID. With the current implementation you can be 100% sure, that you don't have any typos in the request. The fact that we were sending Facebook's parameter to other services was a bug from the beginning imo. Either case as you mentioned this is definitely out-of-scope of this PR.

@malinajirka malinajirka merged commit d66c3ce into develop Aug 13, 2019

4 checks passed

Peril All green. Well done.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@malinajirka malinajirka deleted the issue/10252-linkedin-connection branch Aug 13, 2019

@mkevins

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

I'm also a fan of fail-fast, and you bring up a good point. I suppose in a case like this, these principles are somewhat at odds with one another, and my feeling is more-or-less to be guided by the relative stability of the (external) APIs in question. Robustness may be a maintenance win, whereas fail-fast enforces more strictly defined interfaces, often with more predictable behavior.

In this particular case, we "fail fast", but we have all the information we need at that moment to not fail. I suppose ultimately, it may be a question of where such heuristics belong. I don't have a strong opinion, but definitely think there may be value in considering whether it could make sense for that logic to exist server-side (probably informed by how often these external APIs change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.