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

fix(login) ensure a password is set, when using password login … #392

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

edlerd
Copy link
Contributor

@edlerd edlerd commented Dec 16, 2024

to improve error message.

fixes #382

@edlerd edlerd requested a review from a team as a code owner December 16, 2024 12:39
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.77%. Comparing base (0ffe428) to head (9f3b14b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #392   +/-   ##
=======================================
  Coverage   70.77%   70.77%           
=======================================
  Files          15       15           
  Lines        1728     1728           
=======================================
  Hits         1223     1223           
  Misses        442      442           
  Partials       63       63           

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

ui/pages/login.tsx Outdated Show resolved Hide resolved
BarcoMasile
BarcoMasile previously approved these changes Dec 16, 2024
…rove error message. fixes canonical#382

Signed-off-by: David Edler <david.edler@canonical.com>
@edlerd
Copy link
Contributor Author

edlerd commented Dec 16, 2024

rebased on main and resolved conflicts

Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

LGTM, I tried it and it looks fine. Only question I have is wouldn't it be better to validate the fields before making the request? Not much of a difference tbh.

@edlerd
Copy link
Contributor Author

edlerd commented Dec 18, 2024

LGTM, I tried it and it looks fine. Only question I have is wouldn't it be better to validate the fields before making the request? Not much of a difference tbh.

I think it is obvious that an empty password will not be a valid input that can be submitted. So an error shouldn't be a surprise or cause of frustration as it is expected not to work without a password.

@nsklikas
Copy link
Contributor

That's not what I meant.

In this PR we check if the password is unset and if it is unset we set it to "' and let Kratos evaluate the error message.
What I meant is that if the password is unset, we just show an error message to the user without making a request to Kratos (thus avoiding the extra request)

@edlerd
Copy link
Contributor Author

edlerd commented Dec 18, 2024

That's not what I meant.

In this PR we check if the password is unset and if it is unset we set it to "' and let Kratos evaluate the error message. What I meant is that if the password is unset, we just show an error message to the user without making a request to Kratos (thus avoiding the extra request)

I understand your concern was about a possible optimization to avoid sending the request for validation. It is pretty hard to do the validation client side. This is due to the architecture with kratos sending flows of objects. On submit, we can't easily set an error on the password input field, because that is in a different component. We could do it via DOM manipulation. Actually, we do that for in other cases to ease the UX. I am not sure adding this complexity is worth the hassle. Especially as the extra request for validation here should be tiny and quick. I assume in the UX it doesn't make a big difference given a reasonably stable and fast connection.

@nsklikas
Copy link
Contributor

Understood, thanks

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.

Incorrect error when trying to sign in without entering a password
3 participants