Bug 25996

Summary: Changes to restrictions should be logged
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: PatronsAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to main --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: bdaeuber, cbrannon, clackman, clemens.tubach, gmcharlt, hebah, kebliss, kyle.m.hall, lisette.scheer, martin.renvoize, michaela.sieber, rcoert, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31458
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36664
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00
Bug Depends on:    
Bug Blocks: 22794    
Attachments: Bug 25996: Make AddDebarment() use Koha::Patron::Restriction
Bug 25996: Unit tests
Bug 25996: Add logging to restrictions actions
Bug 25996: Add restrictions to the log viewer
Bug 25996: (follow-up) Log the entire object on deletion
Bug 25996: Make AddDebarment() use Koha::Patron::Restriction
Bug 25996: Unit tests
Bug 25996: Add logging to restrictions actions
Bug 25996: Add restrictions to the log viewer
Bug 25996: (follow-up) Log the entire object on deletion
Bug 25996: Make AddDebarment() use Koha::Patron::Restriction
Bug 25996: Unit tests
Bug 25996: Add logging to restrictions actions
Bug 25996: Add restrictions to the log viewer
Bug 25996: (follow-up) Log the entire object on deletion

Description Andrew Fuerste-Henry 2020-07-15 19:21:51 UTC
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
Comment 1 Christopher Brannon 2023-08-03 22:14:58 UTC
Very, very bad.  Much bad.  Needs fixed.
Comment 2 Rebecca Coert 2023-08-15 15:03:54 UTC
Tested this on 22.11 and it's still showing as Andrew described in 2020.  This needs to be fixed.
Comment 3 Rebecca Coert 2024-01-23 22:21:36 UTC
(In reply to Christopher Brannon from comment #1)
> Very, very bad.  Much bad.  Needs fixed.

I agree with Christopher.  This needs to be fixed.
Comment 4 Michaela Sieber 2024-03-22 14:36:52 UTC
+1
Comment 5 Tomás Cohen Arazi 2024-04-22 04:23:29 UTC
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)
Comment 6 Tomás Cohen Arazi 2024-04-22 04:23:32 UTC
Created attachment 165245 [details] [review]
Bug 25996: Unit tests

Sponsored-by: Karlsruhe Institute of Technology (KIT)
Comment 7 Tomás Cohen Arazi 2024-04-22 04:23:35 UTC
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)
Comment 8 Tomás Cohen Arazi 2024-04-22 04:23:38 UTC
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)
Comment 9 Michaela Sieber 2024-04-22 07:15:27 UTC
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 ;-)
Comment 10 Clemens Tubach 2024-04-22 08:35:05 UTC
The test plans from comment 7 and 8 are working!
Comment 11 Tomás Cohen Arazi 2024-04-22 11:30:20 UTC
Created attachment 165267 [details] [review]
Bug 25996: (follow-up) Log the entire object on deletion
Comment 12 Tomás Cohen Arazi 2024-04-22 11:30:56 UTC
(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!
Comment 13 ByWater Sandboxes 2024-04-22 13:17:44 UTC
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>
Comment 14 ByWater Sandboxes 2024-04-22 13:17:47 UTC
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>
Comment 15 ByWater Sandboxes 2024-04-22 13:17:49 UTC
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>
Comment 16 ByWater Sandboxes 2024-04-22 13:17:51 UTC
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>
Comment 17 ByWater Sandboxes 2024-04-22 13:17:54 UTC
Created attachment 165285 [details] [review]
Bug 25996: (follow-up) Log the entire object on deletion

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 18 Michaela Sieber 2024-04-22 13:20:02 UTC
(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 :-)
Comment 19 Martin Renvoize 2024-05-02 10:58:23 UTC
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>
Comment 20 Martin Renvoize 2024-05-02 10:58:26 UTC
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>
Comment 21 Martin Renvoize 2024-05-02 10:58:29 UTC
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>
Comment 22 Martin Renvoize 2024-05-02 10:58:32 UTC
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>
Comment 23 Martin Renvoize 2024-05-02 10:58:35 UTC
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>
Comment 24 Martin Renvoize 2024-05-02 10:59:35 UTC
Nice improvement, no regressions, QA scripts happy and unit testes present and correct.

Passing QA
Comment 25 Katrin Fischer 2024-05-03 15:56:39 UTC
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.
Comment 26 Katrin Fischer 2024-05-03 16:02:46 UTC
Pushed for 24.05!

Well done everyone, thank you!