Bug 40608 - Password not changed if PASSWORD_CHANGE letter absent
Summary: Password not changed if PASSWORD_CHANGE letter absent
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed
Depends on: 25936
Blocks:
  Show dependency treegraph
 
Reported: 2025-08-06 18:56 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-08-13 19:42 UTC (History)
5 users (show)

See Also:
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:


Attachments
Bug 40608: Regression tests (9.34 KB, patch)
2025-08-06 19:44 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40608: Continue password change even if notification fails (2.76 KB, patch)
2025-08-06 19:44 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40608: Continue password change even if notification fails (2.81 KB, patch)
2025-08-07 01:14 UTC, David Flater
Details | Diff | Splinter Review
Bug 40608: Regression tests (9.31 KB, patch)
2025-08-07 19:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40608: Continue password change even if notification fails (2.86 KB, patch)
2025-08-07 19:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40608: (follow-up) Add warning in about page (7.04 KB, patch)
2025-08-07 19:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40608: (follow-up) Some tests rely on existing data (2.14 KB, patch)
2025-08-07 19:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40608: Regression tests (9.40 KB, patch)
2025-08-08 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40608: Continue password change even if notification fails (2.96 KB, patch)
2025-08-08 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40608: (follow-up) Add warning in about page (7.14 KB, patch)
2025-08-08 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40608: (follow-up) Some tests rely on existing data (2.24 KB, patch)
2025-08-08 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40608: (QA follow-up) Delete notice in test, adjust about.tt (2.53 KB, patch)
2025-08-08 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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