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.
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.
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.
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>
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?
I was really disappointed in myself getting this wedding in the first place :-(. Very good question regarding the corrupt entries; I can't see any obvious way to work backwards to reconstruct them so I think my vote would be to remove them... but I'm very much open to suggestions if anyone can think of a way to do better.. perhaps assigning to the anonymous user if one exists?
Hmm, maybe we can work backwards to get the right manager id.. I'll work on an update
Created attachment 127739 [details] [review] Bug 29457: Database update
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.
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.
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.?
(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.
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.
Looking here. This serves to demonstrate that the field name 'userid' is kind of confusing. E.g. user_code would already be better..
(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?
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>
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>
Joonas, Martin, do you agree with this follow-up?
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>
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>
Created attachment 128032 [details] [review] Bug 29457: (follow-up) Add additional information to warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
- 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
I would push at least the fix only ?
(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.
OK for push now ?
No, it's not ready.
I wouldnt try to run this LOL SELECT borrowernumber FROM borrowers WHERE borrowers.userid = accountlines.manager_id ) WHERE credit_type_code = 'CANCELLATION'' Double WHERE
Another try to get this further ;)
(In reply to Marcel de Rooy from comment #26) > I wouldnt try to run this LOL > > SELECT borrowernumber FROM borrowers WHERE borrowers.userid = > accountlines.manager_id ) WHERE credit_type_code = 'CANCELLATION'' > > Double WHERE Sorry, reading half the line or something
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'
Note that the idea is just to list these SQL updates here.
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'
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>
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>
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 :)
(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.
I change to in dicussion to remember why I dont push to master.
(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?
Created attachment 128649 [details] [review] Bug 29457: Generic warning at upgrade
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.
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>
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.
This is where we ended up discussing it for 23th time ;)
(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 :)
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.02
Pushed to 21.05.x for 21.05.08
Backported: Pushed to 20.11.x branch for 20.11.15
Do I need to backport this to 19.11.x? I probably won't unless there is a need for it
Not backported to 19.11.x. Please request it if you need it.