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

Added ability for login screen to clear password on failed attempt #1142

Merged
merged 3 commits into from Oct 24, 2017
Merged

Added ability for login screen to clear password on failed attempt #1142

merged 3 commits into from Oct 24, 2017

Conversation

chalettu
Copy link
Contributor

@miq-bot add_label enhancement
@miq-bot add_label fine/no

@chalettu
Copy link
Contributor Author

@chriskacerguis chriskacerguis self-assigned this Oct 24, 2017
@chriskacerguis chriskacerguis added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 24, 2017
chriskacerguis
chriskacerguis previously approved these changes Oct 24, 2017
.catch((response) => {
if (response.status === 401) {
vm.credentials.password = ''
Notifications.message('danger', '', __('Incorrect username or password.'), false)
Copy link
Member

Choose a reason for hiding this comment

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

If the response has a server message we should probably display that here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably could but a couple of thoughts. The message that comes back just says "Authentication failed". At a minimum the message we display is a little more specific and it is also translated vs the raw message from the server. Keep in mind, I didn't add this notification message, merely moved it from the factory up to the state that it pertains to.

Copy link
Member

Choose a reason for hiding this comment

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

So a compromise if displaying the original text, 'Login failed, possibly invalid credentials.' augmented with the server feedback (which might provide additional detail depending on error)?

Copy link
Member

Choose a reason for hiding this comment

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

And normally I wouldn't press on this matter, but my 👃 is in a bz right now where not showing the full error message issssssss no bueno https://bugzilla.redhat.com/show_bug.cgi?id=1498984

exception.catch('Login failed, possibly invalid credentials.')
.catch((response) => {
if (response.status === 401) {
vm.credentials.password = ''
Copy link
Member

Choose a reason for hiding this comment

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

should vm.credentials.login also be cleared out?

Copy link
Member

Choose a reason for hiding this comment

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

I realize ref was for only password to be blank, but if it was an unsuccessful login attempt :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RFE just requested password be cleared out.

.catch((response) => {
if (response.status === 401) {
vm.credentials.password = ''
Notifications.message('danger', '', __('Incorrect username or password.'), false)
Copy link
Member

Choose a reason for hiding this comment

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

So a compromise if displaying the original text, 'Login failed, possibly invalid credentials.' augmented with the server feedback (which might provide additional detail depending on error)?

@miq-bot
Copy link
Member

miq-bot commented Oct 24, 2017

Checked commits https://github.com/chalettu/manageiq-ui-service/compare/39198558665d9a6bfb1b0fb0be27b977a4f39288~...f12dd5a69cc8a780c4ea63dfbad50b75464fee3c with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

sorry for being a pain, thanks for adding! <3 🙇‍♀️

I remember there was a time in the past where much debate happened over the message length and what exactly to say on the login page.... Accurate message reporting is kinda important though 👍

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