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

Login with Service Unavailable - show a flash message instead of generic error popup #4606

Merged
merged 2 commits into from Sep 6, 2018

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Sep 5, 2018

@himdel
Copy link
Contributor Author

himdel commented Sep 5, 2018

With invalid login credentials (this is just a 401 request)...

before "Incorrect username or password":
master401

now "Login failed: Unauthorized":
now401

@himdel
Copy link
Contributor Author

himdel commented Sep 5, 2018

With a 503 error...

before:

master503
+
master401

(the misleading flash message would be visible under the dialog)

now:

now503

(and no popup)

@himdel
Copy link
Contributor Author

himdel commented Sep 5, 2018

I still don't like that this is giving the user less info, and doesn't mention anything about the possibility of the web services role being disabled.

(Also, it means the login error messages won't be translated (except for the "Login failed" part).)

But it is a simple fix for ...what's apparently a problem, for some reason :)

@miq-bot
Copy link
Member

miq-bot commented Sep 5, 2018

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/0aa9f5b0d95a4cd97ed2d7b1ee9c0167d4a3ea3d~...7317ef49cdaaea6417e6576169d65d60c7c23bb0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@himdel himdel closed this Sep 5, 2018
@himdel himdel reopened this Sep 5, 2018
@mzazrivec mzazrivec self-assigned this Sep 5, 2018
@mzazrivec mzazrivec added this to the Sprint 94 Ending Sep 10, 2018 milestone Sep 5, 2018
@mzazrivec
Copy link
Contributor

I'm okay with this fix. @jvlcek can I have you endorsement too? 😄

@jvlcek
Copy link
Member

jvlcek commented Sep 5, 2018

I've tested this using upstream master. It works as expected. The fix LGTM

Good job @himdel! Thank you. JoeV

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants