Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@brianlovin
Copy link
Contributor

@brianlovin brianlovin commented Feb 19, 2019

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • api
  • hyperion (frontend)

@mxstbr this feels...hacky. But I'm not quite sure how to else to approach this.

Here's what's happening:

  1. User A logs in to Spectrum with GitHub
  2. User A creates another Spectrum account accidentally (or intentionally) by logging in with Twitter
  3. User A+Twitter tries to connect their GitHub account in their user settings
  4. Our auth flow detects that an account already exists with those GitHub credentials
  5. We return User A+Twitter back to their user settings, without editing their user object

The problem is people getting redirected back, seeing the 'Connect GitHub' button again, and assuming the app is broken.

This PR addresses this by passing back an error message as a query parameter. When the user settings view mounts, I check for the query param and use it to dispatch a toast and save a class variable with the error to be passed as props down to the GitHub connect button so that we can render this:

screenshot 2019-02-19 15 40 51

I'm open to ideas on this; the only obvious abstraction I can think of here would be to have a global query parser that looks for some generic ?error=foo query param in every url in order to dispatch a toast. But I'm not sure if we want to do that yet, since we don't really have any other use cases for it, nor am I sure that it's the right way for us to handle error messages.

Closes #4668

@spectrum-bot
Copy link

spectrum-bot bot commented Feb 19, 2019

Warnings
⚠️

These modified files do not have Flow enabled:

  • src/components/formElements/style.js

Generated by 🚫 dangerJS

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

I would rather make this something generic like error from the get-go, we might need to reuse that for other auth or similar errors!

@brianlovin
Copy link
Contributor Author

brianlovin commented Feb 20, 2019

Fair enough - a good use case I just thought of would be to redirect all email unsubscribe links back to the app instead of that ugly unstyled html page. We could redirect it back with some kind of query params like ?toastType=success&toastText=Unsubscribe successful! and then for this PRs use case it could be ?toastType=error&toastText=Blah...

Then I'll just need an app-level history listener to handle locations with these params.

That seem better?

@mxstbr
Copy link
Contributor

mxstbr commented Feb 20, 2019

That seems great to me! 💯

@brianlovin
Copy link
Contributor Author

Okay, I've implemented a generic toast handler component at the global level, and demonstrated a proof of concept implementation for email unsub redirects here: 788143b => everything works that I've tested locally, but I'm going to write some E2E tests to make sure things look good!

@brianlovin
Copy link
Contributor Author

Okay, this is ready for a test and review @mxstbr! I added some end to end tests as well to make sure this works :)

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Amazing, this works really well!! Liking the new styling too, much cleaner ✨

Let's shipit!

We decode the cleanParams in order to preserver special characters in the url
For example, the url /spectrum/general/another-thread~thread-2?m=MTQ4MzIyNTIwMDAwMg==
has two equals signs at the end. If we don't decode the cleanParams it will become
spectrum/general/another-thread~thread-2?m=MTQ4MzIyNTIwMDAwMg%3D%3D
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch here 💯

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

I am so shipping this!

@mxstbr
Copy link
Contributor

mxstbr commented Feb 21, 2019

Restarting e2e tests since failures seem unrelated.

Then I am shipping this.

😅

@mxstbr mxstbr merged commit 872d7b4 into alpha Feb 21, 2019
@mxstbr mxstbr deleted the github-connection-error branch February 21, 2019 09:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants