Bug 36508 - Patron userid field can be overwritten by update_patron_categories when limiting by fines
Summary: Patron userid field can be overwritten by update_patron_categories when limit...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Nick Clemens (kidclamp)
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-03 20:35 UTC by Nick Clemens (kidclamp)
Modified: 2024-05-23 14:09 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,23.05.12
Circulation function:


Attachments
Bug 36508: Refresh patron object when updating category (1.27 KB, patch)
2024-04-03 20:38 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36508: Unit tests (1.88 KB, patch)
2024-04-25 16:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36508: Refresh patron object when updating category (1.27 KB, patch)
2024-04-25 16:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36508: Unit tests (1.93 KB, patch)
2024-04-25 16:39 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 36508: Refresh patron object when updating category (1.32 KB, patch)
2024-04-25 16:39 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 36508: Unit tests (1.99 KB, patch)
2024-04-29 17:54 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 36508: Refresh patron object when updating category (1.38 KB, patch)
2024-04-29 17:54 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-04-03 20:35:35 UTC
In Koha::Patrons->search_patrons_to_update_category we join to accountlines and sum when using the fines over and fines under parameters.

The way this sql works requires a GROUP BY - which means we limit the columns to borrowernumber

This means the object we use to update later loses many of the fields we expect to be there

We end up with entries in modification log like:
{ "address" : { "after" : null, "before" : "5129 Library Rd." }, "auth_method" : { "after" : null, "before" : "password" }, "autorenew_checkouts" : { "after" : null, "before" : 1 }, "branchcode" : { "after" : null, "before" : "MPL" }, "cardnumber" : { "after" : null, "before" : "23529000050113" }, "categorycode" : { "after" : "S", "before" : "ST" }, "checkprevcheckout" : { "after" : null, "before" : "inherit" }, "city" : { "after" : null, "before" : "Centerville, CA" }, "dateenrolled" : { "after" : null, "before" : "1985-09-09" }, "dateexpiry" : { "after" : null, "before" : "2099-12-31" }, "dateofbirth" : { "after" : null, "before" : "1952-05-27" }, "firstname" : { "after" : null, "before" : "Jordan" }, "lang" : { "after" : null, "before" : "default" }, "phone" : { "after" : null, "before" : "(212) 555-1212" }, "privacy" : { "after" : null, "before" : 1 }, "sex" : { "after" : null, "before" : "M" }, "sort1" : { "after" : null, "before" : "0.54121395064631" }, "sort2" : { "after" : null, "before" : "0.90967674824379" }, "surname" : { "after" : null, "before" : "Alford" }, "userid" : { "after" : "1", "before" : "23529000050113" }, "zipcode" : { "after" : null, "before" : "44262" } } 

Most of these are ignored, null interpreting as not being updated, however, the cardnumber does overwrite as it is treated specially.
Comment 1 Nick Clemens (kidclamp) 2024-04-03 20:38:41 UTC
Created attachment 164391 [details] [review]
Bug 36508: Refresh patron object when updating category

To test:
1 - Find a ptron, I sued #45 in KTD, note their category
2 - Update them from one category to another using finesunder:
    perl misc/cronjobs/update_patrons_category.pl -f ST -t S --finesunder=5.00 --where "me.borrowernumber=45" -v -c
3 - Check their modification log (I told you to enabled BorrowersLog, right?)
4 - See many fields reported changed
5 - Apply patch
6 - Repeat, but change the to and from options
    perl misc/cronjobs/update_patrons_category.pl -f S -t ST --finesunder=5.00 --where "me.borrowernumber=45" -v -c
7 - Note only one column changed in the logs
8 - Ask for unit tests
Comment 2 Nick Clemens (kidclamp) 2024-04-25 16:26:56 UTC
Created attachment 165571 [details] [review]
Bug 36508: Unit tests
Comment 3 Nick Clemens (kidclamp) 2024-04-25 16:26:58 UTC
Created attachment 165572 [details] [review]
Bug 36508: Refresh patron object when updating category

To test:
1 - Find a ptron, I sued #45 in KTD, note their category
2 - Update them from one category to another using finesunder:
    perl misc/cronjobs/update_patrons_category.pl -f ST -t S --finesunder=5.00 --where "me.borrowernumber=45" -v -c
3 - Check their modification log (I told you to enabled BorrowersLog, right?)
4 - See many fields reported changed
5 - Apply patch
6 - Repeat, but change the to and from options
    perl misc/cronjobs/update_patrons_category.pl -f S -t ST --finesunder=5.00 --where "me.borrowernumber=45" -v -c
7 - Note only one column changed in the logs
8 - Ask for unit tests
Comment 4 Owen Leonard 2024-04-25 16:39:41 UTC
Created attachment 165574 [details] [review]
Bug 36508: Unit tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Owen Leonard 2024-04-25 16:39:43 UTC
Created attachment 165575 [details] [review]
Bug 36508: Refresh patron object when updating category

To test:
1 - Find a ptron, I sued #45 in KTD, note their category
2 - Update them from one category to another using finesunder:
    perl misc/cronjobs/update_patrons_category.pl -f ST -t S --finesunder=5.00 --where "me.borrowernumber=45" -v -c
3 - Check their modification log (I told you to enabled BorrowersLog,
    right?)
4 - See many fields reported changed
5 - Apply patch
6 - Repeat, but change the to and from options
    perl misc/cronjobs/update_patrons_category.pl -f S -t ST --finesunder=5.00 --where "me.borrowernumber=45" -v -c
7 - Note only one column changed in the logs
8 - Ask for unit tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Tomás Cohen Arazi (tcohen) 2024-04-29 17:54:29 UTC
Created attachment 165784 [details] [review]
Bug 36508: Unit tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi (tcohen) 2024-04-29 17:54:32 UTC
Created attachment 165785 [details] [review]
Bug 36508: Refresh patron object when updating category

To test:
1 - Find a ptron, I sued #45 in KTD, note their category
2 - Update them from one category to another using finesunder:
    perl misc/cronjobs/update_patrons_category.pl -f ST -t S --finesunder=5.00 --where "me.borrowernumber=45" -v -c
3 - Check their modification log (I told you to enabled BorrowersLog,
    right?)
4 - See many fields reported changed
5 - Apply patch
6 - Repeat, but change the to and from options
    perl misc/cronjobs/update_patrons_category.pl -f S -t ST --finesunder=5.00 --where "me.borrowernumber=45" -v -c
7 - Note only one column changed in the logs
8 - Ask for unit tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Katrin Fischer 2024-04-30 12:33:14 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 9 Fridolin Somers 2024-05-22 12:37:11 UTC
Pushed to 23.11.x for 23.11.06
Comment 10 Lucas Gass (lukeg) 2024-05-23 14:09:31 UTC
Backported to 23.05.x for upcoming 23.05.12