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

Close window on auth callback error and show error to user #3143

Merged
merged 16 commits into from Sep 30, 2021

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Aug 29, 2021

Because simply receiving a "Bad request" is very bad UX
For example if the user cancels the auth flow, or doesn't give companion access
it's better to go back to the uppy screen so they can try again
UPDATE: also show message in UI

Because simply receiving a "Bad request" is very bad UX
For example if the user cancels the auth flow, or doesn't give companion access
it's better to go back
@arturi arturi self-requested a review August 29, 2021 21:57
mifi and others added 2 commits August 30, 2021 21:46
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@mifi
Copy link
Contributor Author

mifi commented Aug 30, 2021

I have now also added some extra code to show a message to the user when authentication fails / is aborted.
I added a new english string authAborted. How do we get this translated?

Screenshot 2021-08-30 at 23 06 53

It should be safe with old versions of the client, because they don't check for data.error, and if data.token is not set, they will just do nothing.

@mifi
Copy link
Contributor Author

mifi commented Aug 30, 2021

I wonder why unit tests are failing on CI but not locally...
Update: it was test:companion, not test:unit.

@mifi mifi changed the title Close window on auth callback error Close window on auth callback error and show error to user Aug 30, 2021
@Murderlon
Copy link
Member

You can merge this when you're ready :)

@mifi
Copy link
Contributor Author

mifi commented Aug 31, 2021

Cool. Does the new string get translated before merge or will it happen after merge? As in is there anything I need to do?

@Murderlon
Copy link
Member

Cool. Does the new string get translated before merge or will it happen after merge? As in is there anything I need to do?

We only update English, the other locales are non-blocking for merge and mostly community contributions.

You can add this for Dutch: Authenticatie geannuleerd.

@aduh95 could suggest French.

@mifi
Copy link
Contributor Author

mifi commented Aug 31, 2021

Gotcha. Are we using any translation service? For my app LosslessCut I use weblate, and it works quite well. Everyone is free to contribute through weblate's web UI which has translation memory and all, and weblate will automatically read english strings from github and periodically create an aggregated PR to update the other languages when people translate. It's free for open source.

mifi and others added 2 commits August 31, 2021 17:36
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95
Copy link
Member

aduh95 commented Sep 2, 2021

@aduh95 could suggest French.

Authentification interrompue

@mifi mifi merged commit 56a67de into main Sep 30, 2021
@mifi mifi deleted the improve-callback-error branch September 30, 2021 11:07
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
…it#3143)

* Close window on auth callback error

Because simply receiving a "Bad request" is very bad UX
For example if the user cancels the auth flow, or doesn't give companion access
it's better to go back

* Update packages/@uppy/companion/src/server/controllers/callback.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

* Update packages/@uppy/companion/src/server/controllers/callback.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

* Update packages/@uppy/companion/src/server/controllers/callback.js

* Update callback.js

* pull out reusable code to getDynamicStateFromRequest

* Signal auth error to client

* fix test mocking

* npm run build:locale-pack

* Update packages/@uppy/companion/src/server/controllers/callback.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

* fix lint

* add i18n string that got lost during merge

* add french translation

* add dutch translation

* add norwegian translation

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
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

4 participants