To recreate: 1 - verify borrower log is enabled 2 - click Edit on a patron 3 - on the edit page click "add a manual restriction" and enter a debarred comment, save the patron 4 - click Edit again, remove your restriction 5 - go to patron modification logs, see the log of your changes 6 - go back to the borrower checkout page, click to the Restrictions tab 7 - add a restriction 8 - delete the restiction 9 - go to patron modification logs, see there is not a log of your changes
Very, very bad. Much bad. Needs fixed.
Tested this on 22.11 and it's still showing as Andrew described in 2020. This needs to be fixed.
(In reply to Christopher Brannon from comment #1) > Very, very bad. Much bad. Needs fixed. I agree with Christopher. This needs to be fixed.
+1
Created attachment 165244 [details] [review] Bug 25996: Make AddDebarment() use Koha::Patron::Restriction This patch makes the AddDebarment() method use the Koha::Object-based class to ease the next steps. The current codebase makes the feature fragile otherwise, as adding a new row and then querying for the latest row of that kind seems risky, With this approach the result is the same, but we have the Koha::Patron::Restriction object for using it later in logging. I opted to return 1 as it does now. The whole module deserves to be removed in the future but it is out of the scope of this report. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/Patron/Borrower_Debarments.t => SUCCESS: Tests pass 2. Apply this tiny refactoring patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Sponsored-by: Karlsruhe Institute of Technology (KIT)
Created attachment 165245 [details] [review] Bug 25996: Unit tests Sponsored-by: Karlsruhe Institute of Technology (KIT)
Created attachment 165246 [details] [review] Bug 25996: Add logging to restrictions actions This patch adds logging for the following actions: * CREATE_RESTRICITON * MODIFY_RESTRICTION * DELETE_RESTRICTION To test: 1. Apply the unit tests 2. Run: $ ktd --shell k$ prove t/db_dependent/Patron/Borrower_Debarments.t => FAIL: The feature is not implemented! Nothing is logged! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Sponsored-by: Karlsruhe Institute of Technology (KIT)
Created attachment 165247 [details] [review] Bug 25996: Add restrictions to the log viewer This patch adds the new log actions, related to restrictions, to the log viewer. To test: 0. Set BorrowersLog to 'Log' 1. Pick a patron 2. Add a restriction and identify it's id, maybe using the DB: $ ktd --shell k$ koha-mysql kohadev > SELECT * FROM borrower_debarments; 3. Run [1]: k$ perl -MKoha::Patron::Debarments \ -e 'Koha::Patron::Debarments::ModDebarment( { borrower_debarment_id => 195, comment => "potato" } );' 4. Visit the log viewer. => SUCCESS: You can filter on 'Create restriction', 'Modify restriction' and 'Delete restriction', things are displayed correctly. [1] There's the ModDebarment method in the codebase, but I couldn't find a way to trigger it for testing, so my last resort was to do it from the CLI, manually. The tests on the previous patches cover the method anyway. Sponsored-by: Karlsruhe Institute of Technology (KIT)
Thanks Tomas for the fast implementation! The logging when creating a new restriction works as expected. But unfortunately the log for DELETE is not sufficiant because the Info field in the Log viewer is empty. Therefor we don't know which debarment_id, type, comment or expiration the deleted restriction had. But these are the most important information for us, we need to know which restriction was deleted. Would it be possible to log at least the borrower_debarment_id ? Better would be the information about all restriction fields of course ;-)
The test plans from comment 7 and 8 are working!
Created attachment 165267 [details] [review] Bug 25996: (follow-up) Log the entire object on deletion
(In reply to Michaela Sieber from comment #9) > Thanks Tomas for the fast implementation! > > The logging when creating a new restriction works as expected. > > But unfortunately the log for DELETE is not sufficiant because the Info > field in the Log viewer is empty. Therefor we don't know which debarment_id, > type, comment or expiration the deleted restriction had. > But these are the most important information for us, we need to know which > restriction was deleted. > > Would it be possible to log at least the borrower_debarment_id ? > Better would be the information about all restriction fields of course ;-) Done!
Created attachment 165281 [details] [review] Bug 25996: Make AddDebarment() use Koha::Patron::Restriction This patch makes the AddDebarment() method use the Koha::Object-based class to ease the next steps. The current codebase makes the feature fragile otherwise, as adding a new row and then querying for the latest row of that kind seems risky, With this approach the result is the same, but we have the Koha::Patron::Restriction object for using it later in logging. I opted to return 1 as it does now. The whole module deserves to be removed in the future but it is out of the scope of this report. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/Patron/Borrower_Debarments.t => SUCCESS: Tests pass 2. Apply this tiny refactoring patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Created attachment 165282 [details] [review] Bug 25996: Unit tests Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Created attachment 165283 [details] [review] Bug 25996: Add logging to restrictions actions This patch adds logging for the following actions: * CREATE_RESTRICITON * MODIFY_RESTRICTION * DELETE_RESTRICTION To test: 1. Apply the unit tests 2. Run: $ ktd --shell k$ prove t/db_dependent/Patron/Borrower_Debarments.t => FAIL: The feature is not implemented! Nothing is logged! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Created attachment 165284 [details] [review] Bug 25996: Add restrictions to the log viewer This patch adds the new log actions, related to restrictions, to the log viewer. To test: 0. Set BorrowersLog to 'Log' 1. Pick a patron 2. Add a restriction and identify it's id, maybe using the DB: $ ktd --shell k$ koha-mysql kohadev > SELECT * FROM borrower_debarments; 3. Run [1]: k$ perl -MKoha::Patron::Debarments \ -e 'Koha::Patron::Debarments::ModDebarment( { borrower_debarment_id => 195, comment => "potato" } );' 4. Visit the log viewer. => SUCCESS: You can filter on 'Create restriction', 'Modify restriction' and 'Delete restriction', things are displayed correctly. [1] There's the ModDebarment method in the codebase, but I couldn't find a way to trigger it for testing, so my last resort was to do it from the CLI, manually. The tests on the previous patches cover the method anyway. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Created attachment 165285 [details] [review] Bug 25996: (follow-up) Log the entire object on deletion Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
(In reply to Tomás Cohen Arazi from comment #12) > > But these are the most important information for us, we need to know which > > restriction was deleted. > > > > Would it be possible to log at least the borrower_debarment_id ? > > Better would be the information about all restriction fields of course ;-) > > Done! Great, thanks! Signed off :-)
Created attachment 166056 [details] [review] Bug 25996: Make AddDebarment() use Koha::Patron::Restriction This patch makes the AddDebarment() method use the Koha::Object-based class to ease the next steps. The current codebase makes the feature fragile otherwise, as adding a new row and then querying for the latest row of that kind seems risky, With this approach the result is the same, but we have the Koha::Patron::Restriction object for using it later in logging. I opted to return 1 as it does now. The whole module deserves to be removed in the future but it is out of the scope of this report. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/Patron/Borrower_Debarments.t => SUCCESS: Tests pass 2. Apply this tiny refactoring patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166057 [details] [review] Bug 25996: Unit tests Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166058 [details] [review] Bug 25996: Add logging to restrictions actions This patch adds logging for the following actions: * CREATE_RESTRICITON * MODIFY_RESTRICTION * DELETE_RESTRICTION To test: 1. Apply the unit tests 2. Run: $ ktd --shell k$ prove t/db_dependent/Patron/Borrower_Debarments.t => FAIL: The feature is not implemented! Nothing is logged! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166059 [details] [review] Bug 25996: Add restrictions to the log viewer This patch adds the new log actions, related to restrictions, to the log viewer. To test: 0. Set BorrowersLog to 'Log' 1. Pick a patron 2. Add a restriction and identify it's id, maybe using the DB: $ ktd --shell k$ koha-mysql kohadev > SELECT * FROM borrower_debarments; 3. Run [1]: k$ perl -MKoha::Patron::Debarments \ -e 'Koha::Patron::Debarments::ModDebarment( { borrower_debarment_id => 195, comment => "potato" } );' 4. Visit the log viewer. => SUCCESS: You can filter on 'Create restriction', 'Modify restriction' and 'Delete restriction', things are displayed correctly. [1] There's the ModDebarment method in the codebase, but I couldn't find a way to trigger it for testing, so my last resort was to do it from the CLI, manually. The tests on the previous patches cover the method anyway. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166060 [details] [review] Bug 25996: (follow-up) Log the entire object on deletion Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice improvement, no regressions, QA scripts happy and unit testes present and correct. Passing QA
Some notes: 1) Love to see how this replaces some SQL in Koha namespace. 2) Log viewer GUI needs some work As this is not a new module, we create new actions for create, modify and delete or restrictions, that clutter the log viewer page a little more. Something to tackle on another day (bug) 3) Add JSON soon? This development overlapped with the just pushed work from Kyle on making the data storage in action logs a bit more standardized: Bug 25159 - Action logs should be stored in JSON This is at the moment not applied to all existing logging, so definitely not a blocker here. 4) Amended BorrowersLog system preference description to include "patron restrictions" in a follow-up.
Pushed for 24.05! Well done everyone, thank you!
Not backported to 23.11.x