Bug 29457

Summary: Fee Cancellation records the wrong manager_id
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Fines and feesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: andrewfh, fridolin.somers, janet.mcgowan, jonathan.druart, joonas.kylmala, kyle, m.de.rooy, nugged, tomascohen, victor, wainuiwitikapark
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Prior to this patch inadvertently the field borrowers.userid was used to fill accountslines.manager_id. This should have been borrowernumber. This report fixes that and prints a generic warning.
Version(s) released in:
22.05.00,21.11.02,21.05.08,20.11.15
Bug Depends on: 24603    
Bug Blocks:    
Attachments: Bug 29457: Pass context borrowernumber
Bug 29457: Pass context borrowernumber
Bug 29457: Database update
Bug 29457: Pass context borrowernumber
Bug 29457: (QA follow-up) Warn number of missing manager_id's
Bug 29457: Pass context borrowernumber
Bug 29457: (QA follow-up) Warn number of missing manager_id's
Bug 29457: (follow-up) Add additional information to warnings
Bug 29457: Pass context borrowernumber
Bug 29457: (QA follow-up) Report about checked manager_id's
Bug 29457: Generic warning at upgrade
Bug 29457: Pass context borrowernumber
Bug 29457: Generic warning at upgrade

Description Martin Renvoize 2021-11-11 10:55:23 UTC
With the introduction of bug 22008 it has highlighted an error in the charge cancellation controller.  We are passing '$user', which contains cardnumber/userid, to cancel when we should be passing a borrowernumber.
Comment 1 Martin Renvoize 2021-11-11 10:57:50 UTC
Created attachment 127527 [details] [review]
Bug 29457: Pass context borrowernumber

This patch updates the call to cancel such that we pass the currently
logged in users borrowernumber instead of their userid.
Comment 2 Martin Renvoize 2021-11-11 10:59:51 UTC
Without the above patch, cancelling a charge in the worst case can lead to a database error regarding key constrain failures.. or if your userid's happen to coincide with an existing borrowernumber it will record entirely the wrong user as the staff member who actioned the cancellation.
Comment 3 Joonas Kylmälä 2021-11-12 19:21:36 UTC
Created attachment 127596 [details] [review]
Bug 29457: Pass context borrowernumber

This patch updates the call to cancel such that we pass the currently
logged in users borrowernumber instead of their userid.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 4 Joonas Kylmälä 2021-11-12 19:23:01 UTC
well this is bad indeed. I wonder what should be done with those corrupted entries, delete them or move them to some temp column in case someone wants to try to use them to track down the person who did the action?
Comment 5 Martin Renvoize 2021-11-14 10:46:48 UTC Comment hidden (obsolete)
Comment 6 Martin Renvoize 2021-11-14 10:48:29 UTC
Hmm, maybe we can work backwards to get the right manager id.. I'll work on an update
Comment 7 Martin Renvoize 2021-11-17 16:50:01 UTC
Created attachment 127739 [details] [review]
Bug 29457: Database update
Comment 8 Martin Renvoize 2021-11-17 16:51:30 UTC
My database update works.. though I'm sure it could be refined.

But.. it's not idempotent and I'm not sure how to fix that or whether it matters now we have the new DB update system.
Comment 9 Joonas Kylmälä 2021-11-20 12:39:12 UTC
The userid could have been changed later on by the staff member and someone else might have started to use that same userid, thus the DB upgrade here is buggy. Maybe the best bet is just to delete or move the data to some safe place? Or provide somehow instructions for the users to do the data recovery themselves.
Comment 10 Martin Renvoize 2021-11-24 08:48:21 UTC
I don't think we can/should move the data.. there's isn't a good place to put it.

Whilst I agree the DB update isn't perfect either... perhaps move the DB update to a script that can be optionally run instead and leave the data as is otherwise.?
Comment 11 Jonathan Druart 2021-11-25 09:42:01 UTC
(In reply to Martin Renvoize from comment #2)
> Without the above patch, cancelling a charge in the worst case can lead to a
> database error regarding key constrain failures.. or if your userid's happen
> to coincide with an existing borrowernumber it will record entirely the
> wrong user as the staff member who actioned the cancellation.

It actually won't lead every often to a DBI exception, I've tried to recreate with userid="koha"/borrowernumber=51, and I saw that "koha" was passed to ->store, but "NULL" was actually in DB.
It's coming from Koha::Object->store:
134         if (   _numeric_column_type( $columns_info->{$col}->{data_type} )
135             or _decimal_column_type( $columns_info->{$col}->{data_type} )
136         ) {
137             # Has been passed but not a number, usually an empty string
138             my $value = $self->_result()->get_column($col);
139             if ( defined $value and not looks_like_number( $value ) ) {
140                 if ( $columns_info->{$col}->{is_nullable} ) {
141                     # If nullable, default to null
142                     $self->_result()->set_column($col => undef);


And that's certainly why we didn't catch that earlier.
Comment 12 Jonathan Druart 2021-11-25 09:44:15 UTC
What about a warn during the upgrade, for NULLs?

Userid that was a valid borrowernumber will never be restored correctly, no idea what we can do with that.

However the more the wait, the more we introduce bad data, we should fix ASAP.
Comment 13 Marcel de Rooy 2021-11-26 07:33:25 UTC
Looking here. 

This serves to demonstrate that the field name 'userid' is kind of confusing. E.g. user_code would already be better..
Comment 14 Marcel de Rooy 2021-11-26 07:46:12 UTC
(In reply to Jonathan Druart from comment #12)
> What about a warn during the upgrade, for NULLs?
> 
> Userid that was a valid borrowernumber will never be restored correctly, no
> idea what we can do with that.
> 
> However the more the wait, the more we introduce bad data, we should fix
> ASAP.

In most cases NULL will have been stored. So the db rev makes no sense. But the question is: Does it really hurt that the manager_id field is NULL for historic cancellations. Looks to me that it does not.
So we could just go with the first patch?
We could perhaps print the number of NULLs found in accountlines for this reason?
Comment 15 Marcel de Rooy 2021-11-26 07:59:25 UTC
Created attachment 128024 [details] [review]
Bug 29457: Pass context borrowernumber

This patch updates the call to cancel such that we pass the currently
logged in users borrowernumber instead of their userid.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2021-11-26 07:59:28 UTC
Created attachment 128025 [details] [review]
Bug 29457: (QA follow-up) Warn number of missing manager_id's

No sense in trying to match userid's with borrowernumbers. In 99%
of the cases (at least) we will find just NULLs.
We could print the number we found.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Jonathan Druart 2021-11-26 08:03:47 UTC
Joonas, Martin, do you agree with this follow-up?
Comment 18 Martin Renvoize 2021-11-26 10:29:45 UTC
Created attachment 128030 [details] [review]
Bug 29457: Pass context borrowernumber

This patch updates the call to cancel such that we pass the currently
logged in users borrowernumber instead of their userid.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-11-26 10:29:49 UTC
Created attachment 128031 [details] [review]
Bug 29457: (QA follow-up) Warn number of missing manager_id's

No sense in trying to match userid's with borrowernumbers. In 99%
of the cases (at least) we will find just NULLs.
We could print the number we found.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-11-26 10:29:52 UTC
Created attachment 128032 [details] [review]
Bug 29457: (follow-up) Add additional information to warnings

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Jonathan Druart 2021-11-26 11:17:46 UTC
-            say $out "Prior to this patch a users 'userid' was being passed into the manager_id field where borrowernumber is expected";
+            say $out "Prior to this patch a user's 'userid' was being passed into the manager_id field where borrowernumber is expected";

And at the end
+        say $out "Nothing to worry about" unless $count_bad || $count_empty
Comment 22 Marcel de Rooy 2021-11-26 11:58:45 UTC
I would push at least the fix only ?
Comment 23 Jonathan Druart 2021-12-01 09:47:03 UTC
(In reply to Marcel de Rooy from comment #22)
> I would push at least the fix only ?

Sorry, I haven't answered here. I was not confident to push this the same day of the release.
Comment 24 Fridolin Somers 2021-12-02 06:18:39 UTC
OK for push now ?
Comment 25 Jonathan Druart 2021-12-02 06:55:24 UTC
No, it's not ready.
Comment 26 Marcel de Rooy 2021-12-03 08:26:53 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2021-12-03 08:44:59 UTC
Another try to get this further ;)
Comment 28 Marcel de Rooy 2021-12-03 09:03:48 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2021-12-03 09:27:45 UTC
This seems like overkill. But it worked for me without CAST warnings etc on alphanumeric userid's :

UPDATE accountlines
LEFT JOIN borrowers ON CAST(borrowers.userid AS INT) = accountlines.manager_id
SET manager_id = borrowers.borrowernumber 
WHERE manager_id IS NOT NULL AND userid RLIKE "^[0-9]+$"  AND borrowers.borrowernumber IS NOT NULL AND credit_type_code = 'CANCELLATION'
Comment 30 Marcel de Rooy 2021-12-03 09:28:44 UTC
Note that the idea is just to list these SQL updates here.
Comment 31 Marcel de Rooy 2021-12-03 09:56:13 UTC
NOTE: This comment is referred to from the db revision of this report !

If you come here since a Koha upgrade referred you to this place, you have two options: (1) try to correct the incorrect manager_id's or (2) clear the incorrect manager_id's.
A suggestion beforehand: Make a SQL dump before running the SQL updates below on your (production) data.

OPTION 1 CORRECT
In the more exceptional case that your records contain a numeric manager_id which actually is a USERID (read: user code) and NOT a borrowernumber, you may want to run this sql statement:
UPDATE accountlines
LEFT JOIN borrowers ON CAST(borrowers.userid AS INT) = accountlines.manager_id
SET manager_id = borrowers.borrowernumber 
WHERE manager_id IS NOT NULL AND userid RLIKE "^[0-9]+$"  AND borrowers.borrowernumber IS NOT NULL AND credit_type_code = 'CANCELLATION'

OPTION 2 CLEAR
Use this SQL statement to correct records with a wrong wrong manager_id, setting them to NULL:
UPDATE accountlines SET manager_id = NULL WHERE manager_id IS NOT NULL AND credit_type_code = 'CANCELLATION'
Comment 32 Marcel de Rooy 2021-12-03 10:00:26 UTC
Created attachment 128207 [details] [review]
Bug 29457: Pass context borrowernumber

This patch updates the call to cancel such that we pass the currently
logged in users borrowernumber instead of their userid.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Marcel de Rooy 2021-12-03 10:00:30 UTC
Created attachment 128208 [details] [review]
Bug 29457: (QA follow-up) Report about checked manager_id's

We print the number of missing or incorrect manager_id's now.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2021-12-03 10:49:42 UTC
In order to prevent running the dbrev twice and cause confusion, we could not push the second patch on master and only backport it.
Since the second run might mark valid manager_id's as incorrect although they were added in a fixed system.

Could someone come theoretically from a version where the error already exists, missed the backport fix and dbrev, and now upgrade to 22.05 (and miss the warn)? Yes, could be.

If someone still wants to address this, please go ahead :)
Comment 35 Katrin Fischer 2021-12-05 09:55:59 UTC
(In reply to Marcel de Rooy from comment #34)
> In order to prevent running the dbrev twice and cause confusion, we could
> not push the second patch on master and only backport it.

I don't think that will work, because people jump versions/come from an older version without the fix directly to a new version released from master.
Comment 36 Fridolin Somers 2021-12-16 21:16:23 UTC
I change to in dicussion to remember why I dont push to master.
Comment 37 Marcel de Rooy 2021-12-17 07:01:33 UTC
(In reply to Fridolin Somers from comment #36)
> I change to in dicussion to remember why I dont push to master.

Please explain what you are doing here?
Comment 38 Martin Renvoize 2021-12-17 10:26:53 UTC
Created attachment 128649 [details] [review]
Bug 29457: Generic warning at upgrade
Comment 39 Martin Renvoize 2021-12-17 10:28:30 UTC
OK, how about that.. a super super generic note at upgrade telling people to come and read about the bug here.. no link (in case we ever switch from bugzilla), no counts (as they may well be incorrect if the update has already been run at some point) and a small note to say you might just want to ignore it entirely.
Comment 40 Marcel de Rooy 2021-12-17 10:39:06 UTC
Created attachment 128650 [details] [review]
Bug 29457: Pass context borrowernumber

This patch updates the call to cancel such that we pass the currently
logged in users borrowernumber instead of their userid.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2021-12-17 10:39:10 UTC
Created attachment 128651 [details] [review]
Bug 29457: Generic warning at upgrade

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Adding exec flag and two dots.
Comment 42 Marcel de Rooy 2021-12-17 10:39:49 UTC
This is where we ended up discussing it for 23th time ;)
Comment 43 Marcel de Rooy 2021-12-17 10:42:04 UTC
(In reply to Marcel de Rooy from comment #31)
> NOTE: This comment is referred to from the db revision of this report !

NOTE: This is no longer the case :)
Comment 44 Fridolin Somers 2021-12-22 06:55:20 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 45 Kyle M Hall 2022-01-07 11:01:47 UTC
Pushed to 21.11.x for 21.11.02
Comment 46 Andrew Fuerste-Henry 2022-01-10 15:03:00 UTC
Pushed to 21.05.x for 21.05.08
Comment 47 Victor Grousset/tuxayo 2022-02-12 13:55:34 UTC
Backported: Pushed to 20.11.x branch for 20.11.15
Comment 48 wainuiwitikapark 2022-02-15 03:59:25 UTC
Do I need to backport this to 19.11.x? I probably won't unless there is a need for it
Comment 49 wainuiwitikapark 2022-02-21 04:49:44 UTC
Not backported to 19.11.x. Please request it if you need it.