Bug 28028 - Remove broken fix_onloan.pl maintenance script
Summary: Remove broken fix_onloan.pl maintenance script
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 23463 5579
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-23 18:11 UTC by Martin Renvoize
Modified: 2021-05-24 16:58 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This script is removed from the codebase, as it was non-functional for a long time which also suggests that it wasn't used.
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 28028: (bug 23463 follow-up) Fix fix_onloan.pl (1.01 KB, patch)
2021-04-09 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28028: (bug 23463 follow-up) Fix fix_onloan.pl (1.06 KB, patch)
2021-04-09 19:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 28028: Remove misc/migration_tools/fix_onloan.pl (1.27 KB, patch)
2021-04-15 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28028: Remove misc/migration_tools/fix_onloan.pl (1.42 KB, patch)
2021-04-15 09:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28028: Remove misc/migration_tools/fix_onloan.pl (1.49 KB, patch)
2021-04-16 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-03-23 18:11:48 UTC
Our data migrations specialists have found that the misc/migration_tools/fix_onloan.pl script was broken by 23463.

It's clear that the Koha::Items line is missing its method, but I'm also not sure that the call to store() in the loop below will actually fix the onloan field the way ModItem used it either.
Comment 1 Martin Renvoize 2021-03-24 08:23:58 UTC
Looking through the history of this script.. I'm not sure it's actually done anything for a while?

It appears that it used to update the MARC record from the items table onloan field.. I don't think store does that.. or am I missing something?
Comment 2 Jonathan Druart 2021-04-09 09:41:12 UTC
Created attachment 119365 [details] [review]
Bug 28028: (bug 23463 follow-up) Fix fix_onloan.pl

There is an obvious error in the script, the name of the method is
missing
Undefined subroutine &Koha::Items called at misc/migration_tools/fix_onloan.pl line 13.

Test plan:
Run the script and confirm that it does not explode
Comment 3 David Nind 2021-04-09 19:26:35 UTC
Created attachment 119432 [details] [review]
Bug 28028: (bug 23463 follow-up) Fix fix_onloan.pl

There is an obvious error in the script, the name of the method is
missing
Undefined subroutine &Koha::Items called at misc/migration_tools/fix_onloan.pl line 13.

Test plan:
Run the script and confirm that it does not explode

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Joonas Kylmälä 2021-04-12 07:33:41 UTC
As Martin stated earlier also, this script doesn't do anything. If I'm not mistaken the whole script can be now removed because bug 5579 is merged in and we don't store items anymore in MARC record format.
Comment 5 Katrin Fischer 2021-04-12 20:07:24 UTC
(In reply to Joonas Kylmälä from comment #4)
> As Martin stated earlier also, this script doesn't do anything. If I'm not
> mistaken the whole script can be now removed because bug 5579 is merged in
> and we don't store items anymore in MARC record format.

I believe this script might be for updating items.onloan from issues.date_due, so it would still makes total sense for migrations. You migrate the items... you migrate the issues and then update the items.onloan for the issued items.

Maybe just the script description doc is a bit off?
Comment 6 Martin Renvoize 2021-04-13 09:35:32 UTC
Yes, we may well want it to update items.on_loan.. but I don't think the store call does that any more.. so although this patch lets the script run.. I don't know that it actually makes it work as it used to.

I need to dig a little further unless someone has the time to test.

Katrins' correct though.. she's identified the exact use case where I think we were using it in the past... at migration time.
Comment 7 Joonas Kylmälä 2021-04-13 09:42:26 UTC
Hmm, strange:

If I look ModItem() subroutine on revision 0c8d8e867b18b61f39381d2061cd2124fe1c5ed3 (which is just before it was deleted from codebase it has not fetched issues.date_due value as far as I can see, I checked all the code paths in ModItem() to my best ability.

So this would conflict with what both Martin and Katrin are remembering.
Comment 8 Joonas Kylmälä 2021-04-13 10:05:20 UTC
(In reply to Joonas Kylmälä from comment #7)
> Hmm, strange:
> 
> If I look ModItem() subroutine on revision
> 0c8d8e867b18b61f39381d2061cd2124fe1c5ed3 (which is just before it was
> deleted from codebase it has not fetched issues.date_due value as far as I
> can see, I checked all the code paths in ModItem() to my best ability.
> 
> So this would conflict with what both Martin and Katrin are remembering.

Yup, I now checked out Koha version just before Bug 23463 was merged:

$ git reset --hard f04e77c7e5411ca7dcf761e7e1177d7362a10654

and the fix_onloan.pl script didn't update the items.onloan value to be same as in issues table.

Here is the steps I used to test this:

> $ git reset --hard f04e77c7e5411ca7dcf761e7e1177d7362a10654
> $ reset_all
> # Checkout an item to patron and then:
> $ koha-mysql kohadev
> > update items SET onloan = "2020-01-01" WHERE itemnumber=549
> $ koha-shell kohadev
> $ (inside koha-shell) perl fix_onloan.pl
> $ koha-mysql kohadev
> > select * from items WHERE itemnumber=549 # and notice it didn't update

So I strongly think this script has been doing nothing after bug 5579 has been merged and we stopped storing items in MARC format.
Comment 9 Jonathan Druart 2021-04-15 08:46:38 UTC
Created attachment 119607 [details] [review]
Bug 28028: Remove misc/migration_tools/fix_onloan.pl

There is an obvious error in the script since bug 23463 and it's known
to be broken for 10 years now (since bug 5579). As nobody complains we
can safely remove this script.
Comment 10 Jonathan Druart 2021-04-15 08:47:24 UTC
Thanks Joonas for the investigation. Let remove it then!
Comment 11 Joonas Kylmälä 2021-04-15 09:59:10 UTC
Created attachment 119608 [details] [review]
Bug 28028: Remove misc/migration_tools/fix_onloan.pl

There is an obvious error in the script since bug 23463 and it's known
to be broken for 10 years now (since bug 5579). As nobody complains we
can safely remove this script.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 12 Kyle M Hall 2021-04-16 16:48:18 UTC
Created attachment 119766 [details] [review]
Bug 28028: Remove misc/migration_tools/fix_onloan.pl

There is an obvious error in the script since bug 23463 and it's known
to be broken for 10 years now (since bug 5579). As nobody complains we
can safely remove this script.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Katrin Fischer 2021-04-17 16:53:42 UTC
Totally ok with removing the script, but we need to remember to update bug descriptions when changing direction :)
Comment 14 Jonathan Druart 2021-04-21 08:47:08 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 15 Fridolin Somers 2021-04-29 10:01:56 UTC
Pushed to 20.11.x for 20.11.06
Comment 16 Andrew Fuerste-Henry 2021-05-24 15:41:59 UTC
Pushed to 20.05.x for 20.05.12
Comment 17 Victor Grousset/tuxayo 2021-05-24 16:58:20 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.