[Bug 40608] New: Password not changed if PASSWORD_CHANGE letter absent
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Bug ID: 40608 Summary: Password not changed if PASSWORD_CHANGE letter absent Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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! -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Depends on| |25936 Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25936 [Bug 25936] Notify users if their password has changed -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185166 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185166&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185167 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185167&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 David Flater <flaterdavid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185167|0 |1 is obsolete| | --- Comment #3 from David Flater <flaterdavid@gmail.com> --- Created attachment 185179 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185179&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 David Flater <flaterdavid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |flaterdavid@gmail.com Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|Signed Off |Failed QA --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- + $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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #5 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185166|0 |1 is obsolete| | Attachment #185179|0 |1 is obsolete| | --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185238 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185238&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185239 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185239&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185240 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185240&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
DELETE FROM letter WHERE code="PASSWORD_CHANGE"; \q
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #9 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185241 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185241&action=edit 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 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185238|0 |1 is obsolete| | --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 185257 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185257&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185239|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 185258 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185258&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185240|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 185259 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185259&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
DELETE FROM letter WHERE code="PASSWORD_CHANGE"; \q
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185241|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 185260 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185260&action=edit 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 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 185261 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185261&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |25.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #15 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|25.11.00 |25.11.00,25.05.04 released in| | Status|Pushed to main |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 --- Comment #16 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Nice work everyone! Pushed to 25.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable CC| |fridolin.somers@biblibre.co | |m Version(s)|25.11.00,25.05.04 |25.11.00,25.05.04,24.11.10 released in| | --- Comment #17 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 24.11.x for 24.11.10 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |Needs documenting --- Comment #18 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11.x as it wasn't backported to 24.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40608 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | CC| |martin.renvoize@openfifth.c | |o.uk -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org