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!
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>
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>
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>
+ $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.
(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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
Nice work everyone! Pushed to main for 25.11