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

Connect ManageLinkedAccounts to server data #23505

Merged
merged 15 commits into from Jul 5, 2018

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Jul 3, 2018

This PR connects ManageLinkedAccounts to actual data from the server!

What it does

  • Populates ManageLinkedAccounts with existing oauth authentication options
  • Connects to new oauth authentication options
  • Disconnects from existing oauth authentication options

Follow-up Work

  • Disable disconnecting for certain users / under certain circumstances
  • Handle errors from DELETE /users/auth/:id/disconnect (currently just logging to the console)

};

onFailure = (error) => {
// TODO: (madelynkasula) display error to user
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not really sure how we want to surface a DELETE /users/auth/:id/disconnect to users. some options:

  • display a notification above the table (via our React notification component)
  • display the error message next to the "Connect/Disconnect" button
  • other thoughts/options?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think either of those would work. We don't have an intermediate state yet, but I'd be most likely to put the error wherever that 'in progress' status lives - maybe on the button itself, even. Our desired outcome is probably for the user to click the disconnect button again, in case it's an intermittent issue.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Besides a couple things I think we should address pre-merge, this is all looking awesome! So excited to have this up and running.

navigateToHref(`/users/auth/${provider}/connect`);
};

disconnect = (authOptionId) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this method could be a lot simpler, either using fetch or async/await or both.

@@ -209,7 +209,8 @@
userAge: current_user.age,
userType: current_user.user_type,
isOauth: current_user.oauth?,
isPasswordRequired: current_user.encrypted_password.present?
isPasswordRequired: current_user.encrypted_password.present?,
authenticationOptions: current_user.authentication_options,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this potentially render OAuth tokens on the client? Maybe we should find a way to only include the info we need from each authentication option.

@maddiedierker
Copy link
Contributor Author

@islemaster i just added a summarize method for AuthenticationOption to avoid passing oauth tokens to the client, so we should be good there. i'm going to address simplifying the disconnect method using fetch and/or async-await in a follow-up PR so i can make sure this is merged before our bug bash today

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

👍 Thanks!

@maddiedierker maddiedierker merged commit ee12e1e into staging Jul 5, 2018
@maddiedierker maddiedierker deleted the manage-linked-accounts-hookup branch July 5, 2018 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants