Bug 25996 - Changes to restrictions should be logged
Summary: Changes to restrictions should be logged
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal with 7 votes (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22794
  Show dependency treegraph
 
Reported: 2020-07-15 19:21 UTC by Andrew Fuerste-Henry
Modified: 2024-04-22 13:20 UTC (History)
13 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25996: Make AddDebarment() use Koha::Patron::Restriction (2.25 KB, patch)
2024-04-22 04:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25996: Unit tests (4.86 KB, patch)
2024-04-22 04:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25996: Add logging to restrictions actions (2.14 KB, patch)
2024-04-22 04:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25996: Add restrictions to the log viewer (4.09 KB, patch)
2024-04-22 04:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25996: (follow-up) Log the entire object on deletion (2.56 KB, patch)
2024-04-22 11:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25996: Make AddDebarment() use Koha::Patron::Restriction (2.31 KB, patch)
2024-04-22 13:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25996: Unit tests (4.92 KB, patch)
2024-04-22 13:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25996: Add logging to restrictions actions (2.20 KB, patch)
2024-04-22 13:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25996: Add restrictions to the log viewer (4.14 KB, patch)
2024-04-22 13:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25996: (follow-up) Log the entire object on deletion (2.61 KB, patch)
2024-04-22 13:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review

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