Skip to content

XWIKI-20630: Improve usability of the "Change Password" page #4188

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

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

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented May 21, 2025

Jira URL

Changes

Description

  • Updated the error message so that it's easier to understand that it's asynchronous (contrarily to live validation which happens as the user fills the form up)
  • Updated the error message creation to use a standard velocimacro.
  • Removed an unecessary line of content.

Clarifications

  • The removed info is now always shown to users and handled by the live validation script. Keeping it would have meant having duplicates.

Screenshots & Video

Not much changed visually, those are screenshots of an instance with the changes from the PR applied:
image
image

Executed Tests

Manual tests only.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • None

* Updated the error message so that it's easier to understand that it's asynchronous (contrarily to live validation which happens as the user fills the form up)
* Updated the error message creation to use a standard velocimacro.
* Removed an unecessary line of information. This info is now always shown to users and handled by the live validation script.
@Sereza7 Sereza7 requested a review from surli June 3, 2025 08:29
@Sereza7
Copy link
Contributor Author

Sereza7 commented Jun 3, 2025

Assigning @surli , this is pretty much just a leftover of #3555 that I missed when looking to update all of the boxes.

@@ -677,7 +677,7 @@ platform.core.profile.passwd.reenterPassword=Reenter password
platform.core.profile.passwd.submit=Save
platform.core.profile.passwd.cancel=Cancel and return to profile
platform.core.profile.passwd.passwordMissmatch=The two passwords do not match.
platform.core.profile.passwd.invalidOriginalPassword=Current password is invalid.
platform.core.profile.passwd.invalidOriginalPassword=Current password was invalid.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not so sure about this change, AFAICS we never use past tense for validating stuff. Check above "the two passwords do not match" etc. Feels like it would be more consistent to keep present tense.

Copy link
Contributor Author

@Sereza7 Sereza7 Jun 4, 2025

Choose a reason for hiding this comment

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

The other validations are done live (or at least now there's livevalidation blocking the submission of the form until all rules are respected, so we shouldn't get any other error unless the user bypasses livevalidation somehow :) ).
This validation happens when the user submits the form, and the password field gets emptied out. At least to me, it was weird to have the same tense to talk about two different things:

  • Livevalidation uses the present tense to say Your new password must have at least 6 characters --> the "new password" is the one currently filled up by the user.
  • The warning uses the present tense to say Your old password is invalid. --> the "old password" is the one that was filled up before the last form submission

Here, I submit the password change request for the first time with an invalid password:
Screenshot from 2025-06-04 11-00-37

Here, I filled up the whole form a second time afterwards.
Screenshot from 2025-06-04 11-01-10
This warning box is the only part of the UI that doesn't adapt when I fill the form back up and even for me knowing how this all works, it was a bit confusing, so I figured out a non technical user experience would probably be quite bad. Using the past tense is a way to clearly tell the user that we're not talking about what they currently have in the field. That's the minimal effort solution, a better solution would probably be to hide this message once the user starts filling the password field again. AFAIK, we don't want livevalidation on existing passwords, for security reasons :)

What solution do you think we should go for? This is not the main problem highlighted in the ticket description, but I figured out we might as well improve things here in this PR. If we want to discuss it further, I think we should move this change to another ticket :)

PS: The wording "Current password" is also a bit confusing. After the first form submission, "Current password" can be two things:

  • The old password if the changepassword request has failed
  • The new password if the changepassword request has succeeded

This might also be part of why I was a bit confused at first with this warning message.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying, the UX of this form is a bit terrible...
So yeah I understand why you want to have the past tense: users might read the message only "too late" after filling again the form basically. So ok if you feel it's less confusing, let's keep your change.

PS: The wording "Current password" is also a bit confusing.

Well a simple improvment we could do is to change it for Current password field was invalid..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So yeah I understand why you want to have the past tense: users might read the message only "too late" after filling again the form basically. So ok if you feel it's less confusing, let's keep your change.

When coming back to this with a fresh mind and reading your message, I figured out that moving the error message to the start of the form could also help. AFAIK, we usually put non-live validation errors before the form fields.
Let me know if you want me to move it around before a merge :)

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, we usually put non-live validation errors before the form fields.
Let me know if you want me to move it around before a merge :)

Yes it would be better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's what it looks like now:
image

Thanks for your feedback :)
I implemented what we discussed in 57078c0

Note that I also decided to change the style to use the standalone error box instead of the inline one. The inline error looked weird on the top of the page.

* Moved the async validation error before the form.
* Reworded the error message to make it a bit clearer
* Changed the async validation error from inline to standalone. Not discussed but looks better in this new context IMO.
@@ -102,12 +101,13 @@
## This key fits better.
#set ($fields[$index].label = $services.localization.render('xe.admin.passwordReset.step2.newPasswordVerification.label'))
<form class="xform third" action="" method="post" autocomplete="off">
## Async validation feedback
#if ($request.xwikipassword && !$isValidPassword)
#error($stringtool.join($errorMessage, "<br/>"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#error($stringtool.join($errorMessage, "<br/>"))
#error($stringtool.join($errorMessage, '<br/>'))

It would avoid an interpretation that is not needed.

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.

2 participants