Bug 33832 - Can't change a patron's username without entering passwords
Summary: Can't change a patron's username without entering passwords
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Brendan Lawlor
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-25 11:53 UTC by Owen Leonard
Modified: 2024-04-18 14:52 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 33832: Allow updating username without changing password on member-password.pl (2.68 KB, patch)
2024-04-08 14:14 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 33832: Allow updating username without changing password on member-password.pl (3.45 KB, patch)
2024-04-08 14:28 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 33832: Allow updating username without changing password on member-password.pl (3.50 KB, patch)
2024-04-08 19:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33832: Allow updating username without changing password on member-password.pl (2.45 KB, patch)
2024-04-12 15:33 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 33832: Allow updating username without changing password on member-password.pl (2.51 KB, patch)
2024-04-12 16:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33832: Allow updating username without changing password on member-password.pl (2.57 KB, patch)
2024-04-17 09:52 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2023-05-25 11:53:21 UTC
In the staff interface, on the "Change password" page, it's not possible to update a patron's username without also entering password information. This is contrary to the help text, "Leave the field blank to leave password unchanged."

As far as I can tell this bug goes back to at least 21.05.x
Comment 1 Sam Lau 2023-05-25 14:30:33 UTC
It kind of makes sense that it behaves this way because it is on the "Change Password" page and not a page designed for just changing the username. The text at the bottom is just counterintuitive in my opinion.
Comment 2 David Cook 2023-06-02 03:08:01 UTC
Should we perhaps change this script so that it just changes the password and doesn't affect the username?
Comment 3 Sally 2023-06-06 09:25:17 UTC
(In reply to David Cook from comment #2)
> Should we perhaps change this script so that it just changes the password
> and doesn't affect the username?

We use the change username functionality; it'd be a shame to lose this.
Comment 4 Brendan Lawlor 2024-04-08 14:14:10 UTC
Created attachment 164506 [details] [review]
Bug 33832: Allow updating username without changing password on member-password.pl

This patch updates the change password page on the staff interface to allow for changing the patron's username without changing the password. If the new password is an empty string we can skip setting the patron's password and sending the new password to the template.
Comment 5 Brendan Lawlor 2024-04-08 14:28:06 UTC
Created attachment 164507 [details] [review]
Bug 33832: Allow updating username without changing password on member-password.pl

This patch updates the change password page on the staff interface to allow for changing the patron's username without changing the password. If the new password is an empty string we can skip setting the patron's password and sending the new password to the template.

Test plan:
1. From a patron record tool bar click 'Change password'
2. Notice that if you try to change the user's name without also changing the password the page just reloads and nothing happens
3. Apply patch and restart_all
4. From the patron record click 'Change password' again
5. Set the user's new username and  password eg. '1234Abc' and click 'Save'
6. Confirm that you can log in to the OPAC with the user
7. Return to the patron record and click 'Change password' again
8. This time change just the 'New username field' and click 'Save'
6. Notice that the username is updated
7. Confirm you can log into the OPAC with the new username and the original password '1234Abcd'
8. Make sure that the change password form still validates passwords for length and matching errors etc
Comment 6 ByWater Sandboxes 2024-04-08 19:06:23 UTC
Created attachment 164522 [details] [review]
Bug 33832: Allow updating username without changing password on member-password.pl

This patch updates the change password page on the staff interface to allow for changing the patron's username without changing the password. If the new password is an empty string we can skip setting the patron's password and sending the new password to the template.

Test plan:
1. From a patron record tool bar click 'Change password'
2. Notice that if you try to change the user's name without also changing the password the page just reloads and nothing happens
3. Apply patch and restart_all
4. From the patron record click 'Change password' again
5. Set the user's new username and  password eg. '1234Abc' and click 'Save'
6. Confirm that you can log in to the OPAC with the user
7. Return to the patron record and click 'Change password' again
8. This time change just the 'New username field' and click 'Save'
6. Notice that the username is updated
7. Confirm you can log into the OPAC with the new username and the original password '1234Abcd'
8. Make sure that the change password form still validates passwords for length and matching errors etc

Signed-off-by: Esther <esther@bywatersolutions.com>
Comment 7 Marcel de Rooy 2024-04-12 06:32:11 UTC
Seeing this:
if ( $op eq 'cud-update' && $newpassword.defined and not @errors ) {

What is $newpassword.defined ??
Wondering why perl does compile that?
Please explain.
Comment 8 David Cook 2024-04-12 06:50:40 UTC
(In reply to Marcel de Rooy from comment #7)
> Seeing this:
> if ( $op eq 'cud-update' && $newpassword.defined and not @errors ) {
> 
> What is $newpassword.defined ??
> Wondering why perl does compile that?
> Please explain.

It looks like a copy/paste error from the template toolkit where scalar values can have methods.

It looks like Perl compiles it because "." is used to concatenate things together. 

It looks like $newpassword.defined is equivalent to $newpassword . defined $_

"Returns a Boolean value telling whether EXPR has a value other than the undefined value undef. If EXPR is not present, $_ is checked."
https://perldoc.perl.org/functions/defined

Yikes!
Comment 9 David Cook 2024-04-12 06:52:25 UTC
Although I think using "defined" at all in this context is probably wrong anyways.
Comment 10 Marcel de Rooy 2024-04-12 06:55:44 UTC
(In reply to David Cook from comment #8)
> It looks like $newpassword.defined is equivalent to $newpassword . defined $_

Ah yes that sounds like it.
Comment 11 David Cook 2024-04-12 07:03:56 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to David Cook from comment #8)
> > It looks like $newpassword.defined is equivalent to $newpassword . defined $_
> 
> Ah yes that sounds like it.

Good catch though. When I first looked at it, I couldn't believe what I was seeing. Thought I was looking at some Ruby missing the question mark...
Comment 12 Brendan Lawlor 2024-04-12 13:21:13 UTC
(In reply to David Cook from comment #8)
> (In reply to Marcel de Rooy from comment #7)
> > Seeing this:
> > if ( $op eq 'cud-update' && $newpassword.defined and not @errors ) {
> > 
> > What is $newpassword.defined ??
> > Wondering why perl does compile that?
> > Please explain.
> 
> It looks like a copy/paste error from the template toolkit where scalar
> values can have methods.

I'm new to Perl and mixed up this syntax from template tool kit. I think it should have been written as defined($newpassword)

The idea of this patch is that if the user does not fill the form with both 'New password' and 'Confirm new password' then there's no need to check and set the password.

I think this patch is close but needs another try.
Comment 13 Owen Leonard 2024-04-12 14:52:23 UTC
It looks like the change to the template is not necessary and is causing JS errors in the browser console. Including the password-checking JS doesn't seem to cause a problem whether or not the password is being updated.
Comment 14 Brendan Lawlor 2024-04-12 15:33:58 UTC
Created attachment 164856 [details] [review]
Bug 33832: Allow updating username without changing password on member-password.pl

This patch updates the change password page on the staff interface to allow for changing the patron's username without changing the password. If the new password is an empty string we can skip setting the patron's password and sending the new password to the template.

Test plan:
1. From a patron record tool bar click 'Change password'
2. Notice that if you try to change the user's name without also changing the password the page just reloads and nothing happens
3. Apply patch and restart_all
4. From the patron record click 'Change password' again
5. Set the user's new username and  password eg. '1234Abc' and click 'Save'
6. Confirm that you can log in to the OPAC with the user
7. Return to the patron record and click 'Change password' again
8. This time change just the 'New username field' and click 'Save'
6. Notice that the username is updated
7. Confirm you can log into the OPAC with the new username and the original password '1234Abcd'
8. Make sure that the change password form still validates passwords for length and matching errors etc
Comment 15 Brendan Lawlor 2024-04-12 16:00:46 UTC
(In reply to Owen Leonard from comment #13)
> It looks like the change to the template is not necessary and is causing JS
> errors in the browser console. Including the password-checking JS doesn't
> seem to cause a problem whether or not the password is being updated.

Thanks for checking, Owen. I removed the change to the template.

I also changed the Perl that I had mixed up with template toolkit syntax to check defined($newpassword).

The reason we have to check if the new password is defined is because if the user leaves the input blank, the Perl script gets the param as an empty string. If you check the value of the empty string it evaluates to false.
Comment 16 Owen Leonard 2024-04-12 16:04:02 UTC
Created attachment 164858 [details] [review]
Bug 33832: Allow updating username without changing password on member-password.pl

This patch updates the change password page on the staff interface to
allow for changing the patron's username without changing the password.
If the new password is an empty string we can skip setting the patron's
password and sending the new password to the template.

Test plan:
1. From a patron record tool bar click 'Change password'
2. Notice that if you try to change the user's name without also
   changing the password the page just reloads and nothing happens
3. Apply patch and restart_all
4. From the patron record click 'Change password' again
5. Set the user's new username and  password eg. '1234Abc' and click
   'Save'
6. Confirm that you can log in to the OPAC with the user
7. Return to the patron record and click 'Change password' again
8. This time change just the 'New username field' and click 'Save'
6. Notice that the username is updated
7. Confirm you can log into the OPAC with the new username and the
   original password '1234Abcd'
8. Make sure that the change password form still validates passwords
   for length and matching errors etc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 17 Martin Renvoize 2024-04-17 09:52:33 UTC
Created attachment 164987 [details] [review]
Bug 33832: Allow updating username without changing password on member-password.pl

This patch updates the change password page on the staff interface to
allow for changing the patron's username without changing the password.
If the new password is an empty string we can skip setting the patron's
password and sending the new password to the template.

Test plan:
1. From a patron record tool bar click 'Change password'
2. Notice that if you try to change the user's name without also
   changing the password the page just reloads and nothing happens
3. Apply patch and restart_all
4. From the patron record click 'Change password' again
5. Set the user's new username and  password eg. '1234Abc' and click
   'Save'
6. Confirm that you can log in to the OPAC with the user
7. Return to the patron record and click 'Change password' again
8. This time change just the 'New username field' and click 'Save'
6. Notice that the username is updated
7. Confirm you can log into the OPAC with the new username and the
   original password '1234Abcd'
8. Make sure that the change password form still validates passwords
   for length and matching errors etc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2024-04-17 09:53:54 UTC
Clear code, QA scripts happy, resolves the issue..

Passing QA, thanks Brendan.
Comment 19 Katrin Fischer 2024-04-18 14:52:00 UTC
Pushed for 24.05!

Well done everyone, thank you!