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

feat: rework newpasswd view for CLI #394

Merged
merged 3 commits into from
Jun 27, 2024
Merged

feat: rework newpasswd view for CLI #394

merged 3 commits into from
Jun 27, 2024

Conversation

GabrielNagy
Copy link
Contributor

@GabrielNagy GabrielNagy commented Jun 25, 2024

Instead of showing both prompts from the get-go, and having Enter submit the form straight away (and only being able to navigate through it with Tab), such as:

Enter your new password
> ****
>

We now render a single input if the user has just been shown the form, with an updated prompt:

Enter your new password

New password:
> ****

When pressing Enter -- provided that the password passes our quality checks -- the form changes to:

Enter your new password (confirm)

New password:
> ****
Confirm password:
> ****

This makes it clearer that we want the user to re-type their password. As a result, Tab navigation is no longer supported in non-skippable views, and the form can only be advanced by pressing Enter.

For skippable views (i.e. with a button), Tab (and Shift-Tab) only works if the password is not filled in. This avoids opening a can of worms regarding to what fields we should be showing or not based on whether password fields are filled in. Thus, the skippable experience becomes:

Enter your new password (3 days until mandatory)

New password:
>

[ Skip ]  // Focusable

and

Enter your new password (3 days until mandatory)

New password:
> *****

[ Skip ]  // No longer focusable, have to finish filling in the form
          // (or clear it)

Otherwise, I believe we should keep the form as simple as possible, without the possibility of jumping back to the previous password field once it was filled in, at the sake of improved maintainability.

Fixes #365 / UDENG-3112

@GabrielNagy GabrielNagy changed the title Rework newpasswd view for CLI feat: rework newpasswd view for CLI Jun 25, 2024
@3v1n0
Copy link
Collaborator

3v1n0 commented Jun 25, 2024

Personally I think showing two fields is not bad once one has been filled and being able to go back and forth to fix or change them (for example you want to add a final char to both forms).

@GabrielNagy
Copy link
Contributor Author

Personally I think showing two fields is not bad once one has been filled and being able to go back and forth to fix or change them (for example you want to add a final char to both forms).

Yeah but in this case assuming the new password is invalid for any reason, the form will be cleared on submit so the user would have to start from scratch again. So as of right now it's more of a visual helper to see if the password and the confirmation are of the same length.

@denisonbarbosa
Copy link
Member

Yeah but in this case assuming the new password is invalid for any reason, the form will be cleared on submit so the user would have to start from scratch again. So as of right now it's more of a visual helper to see if the password and the confirmation are of the same length.

I'd like to keep the two input fields. I'm not sure if we can add "prefix" labels to the input fields, but it would be nice if there was something like:

Enter your new password. 
New password: xxxx
Confirm password: xxxx

And, I agree, clearing the entire form if the entries don't match is a bit annoying, so maybe clear only the confirmation input and the error message?

@GabrielNagy
Copy link
Contributor Author

I'd like to keep the two input fields. I'm not sure if we can add "prefix" labels to the input fields, but it would be nice if there was something like:

Done, I've changed the prompt/prefix for the fields. This definitely looks cleaner.

And, I agree, clearing the entire form if the entries don't match is a bit annoying, so maybe clear only the confirmation input and the error message?

I opted against doing this in the end as I don't think it adds a lot of value (other unix tools don't really do this and I think we should mimic their behavior for the most part).

I've rebased the PR on top of Marco's work since it touched similar parts of the codebase. If I get the green light on the implementation I can start updating the tests :).

@3v1n0
Copy link
Collaborator

3v1n0 commented Jun 26, 2024

UI side looks good to me, the only thing I'd probably move the fields to new line so that they have the same padding and it's easier to compare the length

E.g.:

New password:
 > ********    
Confirm password:
 > *******

Also not sure if having a way to go back to the previous field where the second is active could be convenient, as now to correct one need to cancel the confirmation request. So maybe making Escape to go back to the previous field (without clearing the content?)

Instead of showing both prompts from the get-go, and having Enter submit
the form straight away (and only being able to navigate through it with
Tab), such as:

    Enter your new password
    > ****
    >

We now render a single input if the user has just been shown the form,
with an updated prompt:

    Enter your new password

    New password:
    > ****

When pressing Enter -- provided that the password passes our quality
checks -- the form changes to:

    Enter your new password (confirm)

    New password:
    > ****
    Confirm password:
    > ****

This makes it clearer that we want the user to re-type their password.
As a result, Tab navigation is no longer supported in non-skippable
views, and the form can only be advanced by pressing Enter.

For skippable views (i.e. with a button), Tab only works if the password
is not filled in. This avoids opening a can of worms regarding to what
fields we should be showing or not based on whether password fields are
filled in. Thus, the skippable experience becomes:

    Enter your new password (3 days until mandatory)

    New password:
    >

    [ Skip ]  // Focusable

and

    Enter your new password (3 days until mandatory)

    New password:
    > *****

    [ Skip ]  // No longer focusable, have to finish filling in the form
              // (or clear it)

Otherwise, I believe we should keep the form as simple as possible,
without the possibility of jumping back to the previous password field
once it was filled in, at the sake of improved maintainability.

Fixes #365 / UDENG-3112
Mostly replacing Tab with Enter, and removing some redundant Tabs.
The error from pwquality is a bit different so we need to refresh the
relevant golden files.
@GabrielNagy GabrielNagy marked this pull request as ready for review June 27, 2024 15:54
@GabrielNagy GabrielNagy requested a review from a team as a code owner June 27, 2024 15:54
@GabrielNagy
Copy link
Contributor Author

Alright, CI passed so this is ready for a review!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.37%. Comparing base (5132a3f) to head (9332177).
Report is 21 commits behind head on main.

Files Patch % Lines
pam/internal/adapter/newpasswordmodel.go 90.90% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
- Coverage   85.31%   84.37%   -0.95%     
==========================================
  Files          76       77       +1     
  Lines        6161     6686     +525     
  Branches       75       75              
==========================================
+ Hits         5256     5641     +385     
- Misses        642      733      +91     
- Partials      263      312      +49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

All good from my side, thanks!

Maybe we should consider making the also native interface similar to this when asking for secret, but that's indeed not for now.

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

I looked at the bubble tea related code and I didn’t spot anything special :) You are the real bubbletea master anyway! Well done as usual :)

@GabrielNagy GabrielNagy merged commit e259fba into main Jun 27, 2024
6 checks passed
@GabrielNagy GabrielNagy deleted the new-password-revamp branch June 27, 2024 16:15
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.

Issue: Improve the navigation in newpassword view
5 participants