Bug 40608

Summary: Password not changed if PASSWORD_CHANGE letter absent
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Pushed to main --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: dcook, flaterdavid, lucas, m.de.rooy, tomascohen
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:
Bug Depends on: 25936    
Bug Blocks:    
Attachments: Bug 40608: Regression tests
Bug 40608: Continue password change even if notification fails
Bug 40608: Continue password change even if notification fails
Bug 40608: Regression tests
Bug 40608: Continue password change even if notification fails
Bug 40608: (follow-up) Add warning in about page
Bug 40608: (follow-up) Some tests rely on existing data
Bug 40608: Regression tests
Bug 40608: Continue password change even if notification fails
Bug 40608: (follow-up) Add warning in about page
Bug 40608: (follow-up) Some tests rely on existing data
Bug 40608: (QA follow-up) Delete notice in test, adjust about.tt

Description Tomás Cohen Arazi (tcohen) 2025-08-06 18:56:54 UTC
When the `NotifyPasswordChange` setting is enabled but no letter for the 'email' transport is found, the current code returns before the password change is stored on the DB!
Comment 1 Tomás Cohen Arazi (tcohen) 2025-08-06 19:44:51 UTC
Created attachment 185166 [details] [review]
Bug 40608: Regression tests

When NotifyPasswordChange is enabled and a patron has a valid email address,
but there is no PASSWORD_CHANGE letter template, the set_password method
fails silently and returns undef instead of changing the password.

This adds comprehensive regression tests to demonstrate the bug:

1. Bug condition: Missing PASSWORD_CHANGE letter template causes password
   change to fail and method returns undef
2. Control test: Password change works when NotifyPasswordChange is disabled
3. Control test: Password change works when PASSWORD_CHANGE letter exists

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2025-08-06 19:44:54 UTC
Created attachment 185167 [details] [review]
Bug 40608: Continue password change even if notification fails

When NotifyPasswordChange is enabled and GetPreparedLetter fails to find
a PASSWORD_CHANGE letter template, the set_password method was returning
early with undef, preventing the password from being changed.

This fixes the issue by:
1. Removing the 'or return;' statement after GetPreparedLetter
2. Wrapping the letter processing in an if block to handle missing templates
3. Ensuring password change continues regardless of notification success

The password change should not fail just because the notification cannot
be sent. The notification is a nice-to-have feature, but the core
functionality (changing the password) should always work.

Test plan:
1. Apply the tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Verify that when the letter template exists, notifications are still sent
6. Verify that when NotifyPasswordChange is disabled, password changes work
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Flater 2025-08-07 01:14:55 UTC
Created attachment 185179 [details] [review]
Bug 40608: Continue password change even if notification fails

When NotifyPasswordChange is enabled and GetPreparedLetter fails to find
a PASSWORD_CHANGE letter template, the set_password method was returning
early with undef, preventing the password from being changed.

This fixes the issue by:
1. Removing the 'or return;' statement after GetPreparedLetter
2. Wrapping the letter processing in an if block to handle missing templates
3. Ensuring password change continues regardless of notification success

The password change should not fail just because the notification cannot
be sent. The notification is a nice-to-have feature, but the core
functionality (changing the password) should always work.

Test plan:
1. Apply the tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Verify that when the letter template exists, notifications are still sent
6. Verify that when NotifyPasswordChange is disabled, password changes work
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Flater <flaterdavid@gmail.com>
Comment 4 Marcel de Rooy 2025-08-07 14:28:02 UTC
+            $schema->resultset('Letter')->search(
Old syntax. We have  Koha::Notice::Template ?

+            # Verify no PASSWORD_CHANGE letter exists
+            my $letter_count = $schema->resultset('Letter')->search(
+                {
+                    code   => 'PASSWORD_CHANGE',
+                    module => 'members'
+                }
+            )->count;
+            is( $letter_count, 0, 'No PASSWORD_CHANGE letter template exists' );
Overkill? Testing DBIx here?

We should probably warn if they enable Notify and you have an email address but dont have a letter for it ?
Now we silently ignore.
Comment 5 Tomás Cohen Arazi (tcohen) 2025-08-07 18:35:38 UTC
(In reply to Marcel de Rooy from comment #4)
> +            $schema->resultset('Letter')->search(
> Old syntax. We have  Koha::Notice::Template ?

Good catch. I didn't notice there was a Koha::Object-based class for them.

> +            # Verify no PASSWORD_CHANGE letter exists
> +            my $letter_count = $schema->resultset('Letter')->search(
> +                {
> +                    code   => 'PASSWORD_CHANGE',
> +                    module => 'members'
> +                }
> +            )->count;
> +            is( $letter_count, 0, 'No PASSWORD_CHANGE letter template
> exists' );
> Overkill? Testing DBIx here?

Fair, can remove the test.

> We should probably warn if they enable Notify and you have an email address
> but dont have a letter for it ?
> Now we silently ignore.

We did silently ignore already, it is just that the password change was not stored. And it is not silently, as there's a warn already about the missing letter.

I think a follow-up adding a check for this in about.pl wouldn't hurt.
Comment 6 Tomás Cohen Arazi (tcohen) 2025-08-07 19:25:40 UTC
Created attachment 185238 [details] [review]
Bug 40608: Regression tests

When NotifyPasswordChange is enabled and a patron has a valid email address,
but there is no PASSWORD_CHANGE letter template, the set_password method
fails silently and returns undef instead of changing the password.

This adds comprehensive regression tests to demonstrate the bug:

1. Bug condition: Missing PASSWORD_CHANGE letter template causes password
   change to fail and method returns undef
2. Control test: Password change works when NotifyPasswordChange is disabled
3. Control test: Password change works when PASSWORD_CHANGE letter exists

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi (tcohen) 2025-08-07 19:25:43 UTC
Created attachment 185239 [details] [review]
Bug 40608: Continue password change even if notification fails

When NotifyPasswordChange is enabled and GetPreparedLetter fails to find
a PASSWORD_CHANGE letter template, the set_password method was returning
early with undef, preventing the password from being changed.

This fixes the issue by:
1. Removing the 'or return;' statement after GetPreparedLetter
2. Wrapping the letter processing in an if block to handle missing templates
3. Ensuring password change continues regardless of notification success

The password change should not fail just because the notification cannot
be sent. The notification is a nice-to-have feature, but the core
functionality (changing the password) should always work.

Test plan:
1. Apply the tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Verify that when the letter template exists, notifications are still sent
6. Verify that when NotifyPasswordChange is disabled, password changes work
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Flater <flaterdavid@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi (tcohen) 2025-08-07 19:25:46 UTC
Created attachment 185240 [details] [review]
Bug 40608: (follow-up) Add warning in about page

This patch adds a check in about.pl for when `NotifyPasswordChange` is
enabled but no notices are present on the system.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi (tcohen) 2025-08-07 19:25:49 UTC
Created attachment 185241 [details] [review]
Bug 40608: (follow-up) Some tests rely on existing data

Playing with the about page, I noticed prior tests in the codebase fail
if the letter doesn't exist.

This patch fixes that.

To test:
1. With a fresh KTD run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!
2. Delete the existing notices:
  k$ koha-mysql kohadev
   > DELETE FROM letter WHERE code="PASSWORD_CHANGE"; \q
3. Repeat 1
=> FAIL: Tests fail! Current tests print a warn (Test::NoWarning)
doesn't like this
4. Apply this patch
5. Repeat 1
=> SUCCESS: Tests now pass. The test creates its own notice.
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Marcel de Rooy 2025-08-08 07:01:38 UTC
Created attachment 185257 [details] [review]
Bug 40608: Regression tests

When NotifyPasswordChange is enabled and a patron has a valid email address,
but there is no PASSWORD_CHANGE letter template, the set_password method
fails silently and returns undef instead of changing the password.

This adds comprehensive regression tests to demonstrate the bug:

1. Bug condition: Missing PASSWORD_CHANGE letter template causes password
   change to fail and method returns undef
2. Control test: Password change works when NotifyPasswordChange is disabled
3. Control test: Password change works when PASSWORD_CHANGE letter exists

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2025-08-08 07:01:40 UTC
Created attachment 185258 [details] [review]
Bug 40608: Continue password change even if notification fails

When NotifyPasswordChange is enabled and GetPreparedLetter fails to find
a PASSWORD_CHANGE letter template, the set_password method was returning
early with undef, preventing the password from being changed.

This fixes the issue by:
1. Removing the 'or return;' statement after GetPreparedLetter
2. Wrapping the letter processing in an if block to handle missing templates
3. Ensuring password change continues regardless of notification success

The password change should not fail just because the notification cannot
be sent. The notification is a nice-to-have feature, but the core
functionality (changing the password) should always work.

Test plan:
1. Apply the tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Verify that when the letter template exists, notifications are still sent
6. Verify that when NotifyPasswordChange is disabled, password changes work
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Flater <flaterdavid@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2025-08-08 07:01:42 UTC
Created attachment 185259 [details] [review]
Bug 40608: (follow-up) Add warning in about page

This patch adds a check in about.pl for when `NotifyPasswordChange` is
enabled but no notices are present on the system.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2025-08-08 07:01:44 UTC
Created attachment 185260 [details] [review]
Bug 40608: (follow-up) Some tests rely on existing data

Playing with the about page, I noticed prior tests in the codebase fail
if the letter doesn't exist.

This patch fixes that.

To test:
1. With a fresh KTD run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!
2. Delete the existing notices:
  k$ koha-mysql kohadev
   > DELETE FROM letter WHERE code="PASSWORD_CHANGE"; \q
3. Repeat 1
=> FAIL: Tests fail! Current tests print a warn (Test::NoWarning)
doesn't like this
4. Apply this patch
5. Repeat 1
=> SUCCESS: Tests now pass. The test creates its own notice.
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2025-08-08 07:01:47 UTC
Created attachment 185261 [details] [review]
Bug 40608: (QA follow-up) Delete notice in test, adjust about.tt

[1] This prevents a warn 'Violation of unique constraint in Letter'
    from TestBuilder if the notice already exists.
[2] The about warn should no longer say that password changes will
    fail but only warn about a missing notification.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Lucas Gass (lukeg) 2025-08-13 19:41:47 UTC
Nice work everyone!

Pushed to main for 25.11