Bug 27963 - touch_all_items.pl script is not working at all
Summary: touch_all_items.pl script is not working at all
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 23463
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-16 09:40 UTC by Joonas Kylmälä
Modified: 2021-04-24 16:31 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11


Attachments
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl (1.34 KB, patch)
2021-03-16 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl (1.40 KB, patch)
2021-03-16 14:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl (1.36 KB, patch)
2021-03-16 15:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl (1.51 KB, patch)
2021-03-16 15:53 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl (1.57 KB, patch)
2021-03-23 18:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-03-16 09:40:33 UTC
The touch_all_items.pl script doesn't work at all currently, e.g. it is not modifying any of the items. Based on a quick look at the history this seems to have started after "Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests" (7e2514ac) commit. The itemcallnumber column would need to be marked as dirty using ->make_column_dirty('itemcallnumber') for this to work.
Comment 1 Joonas Kylmälä 2021-03-16 10:10:49 UTC
Maybe the issue started actually happening after "Bug 23463: Replace ModItem with Koha::Item->store" (bbb504c). The ModItem call previously handled a lot of broken data fixing as well and didn't just update the cn_sort value. ModItem for example fixed the permanent_location column and dateformats.
Comment 2 Jonathan Druart 2021-03-16 10:38:55 UTC
Created attachment 118294 [details] [review]
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl

This script is used to fix inconsistencies in the items table. Prior to
bug 23463, ModItem dealt with that.

We need to make some attributes as dirty to make sure the
Koha::Item->store method will fix them.

Test plan:
Be familiar with how touch_all_items is supposed to work and confirm
that the different inconsistencies are fixed.
Comment 3 Joonas Kylmälä 2021-03-16 10:44:46 UTC
thanks for the patch Jonathan. Since this is long running script some items are very likely deleted and we should wrap the whole modification part in an eval like it was before, don't you agree?
Comment 4 Jonathan Druart 2021-03-16 11:26:44 UTC
I thought about this when I wrote the patch and concluded that we just fetched the list before and so it should be fine.

But actually you are right, to be on the safe side we could surround the whole block with the eval to prevent this unlikely (:D) situation.
Comment 5 Joonas Kylmälä 2021-03-16 13:30:04 UTC
Jonathan, should I interpret that as you are sending v2 or?
Comment 6 Jonathan Druart 2021-03-16 14:44:52 UTC
Created attachment 118328 [details] [review]
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl

This script is used to fix inconsistencies in the items table. Prior to
bug 23463, ModItem dealt with that.

We need to make some attributes as dirty to make sure the
Koha::Item->store method will fix them.

Test plan:
Be familiar with how touch_all_items is supposed to work and confirm
that the different inconsistencies are fixed.
Comment 7 Joonas Kylmälä 2021-03-16 15:28:20 UTC
Hmm, now that you added the "return unless $item" (which is actually a really good solution I didn't think of) wrapping the whole block in the eval is not needed, only for the store it is. The call to ->make_column_dirty only throws exception if the DB column doesn't exists at all and in that case we can just halt the execution of the program completely because it won't magically appear later on. v3? :P

I also looked further now into fc09b0d491b1 / Removal of ModItem and actually since the modification script was originally *not* explicitly passing the location and date fields as the fields we wanted to update it didn't do that. Only the %derived_columns listed in 2677da8f1 commit were modified, so items.cn_sort if either itemcallnumber or items.cn_source is modified. So here we need to mark only itemcallnumber and cn_source as dirty.
Comment 8 Jonathan Druart 2021-03-16 15:37:48 UTC
Created attachment 118333 [details] [review]
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl

This script is used to fix inconsistencies in the items table. Prior to
bug 23463, ModItem dealt with that.

We need to make some attributes as dirty to make sure the
Koha::Item->store method will fix them.

Test plan:
Be familiar with how touch_all_items is supposed to work and confirm
that the different inconsistencies are fixed.
Comment 9 Joonas Kylmälä 2021-03-16 15:38:47 UTC
Thanks, I will test this now and let you know the results :)
Comment 10 Joonas Kylmälä 2021-03-16 15:53:27 UTC
Created attachment 118334 [details] [review]
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl

This script is used to fix inconsistencies in the items table. Prior to
bug 23463, ModItem dealt with that.

We need to make some attributes as dirty to make sure the
Koha::Item->store method will fix them.

Test plan:
Be familiar with how touch_all_items is supposed to work and confirm
that the different inconsistencies are fixed.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 11 Joonas Kylmälä 2021-03-16 15:54:40 UTC
Fixes the problem! :)
Comment 12 Martin Renvoize 2021-03-23 18:17:37 UTC
Created attachment 118703 [details] [review]
Bug 27963: (bug 23463 follow-up) Fix touch_all_items.pl

This script is used to fix inconsistencies in the items table. Prior to
bug 23463, ModItem dealt with that.

We need to make some attributes as dirty to make sure the
Koha::Item->store method will fix them.

Test plan:
Be familiar with how touch_all_items is supposed to work and confirm
that the different inconsistencies are fixed.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-03-23 18:19:04 UTC
Good catch.. and well worked through.

Passing QA...

However.. it did lead me to spot that migration_tools/fix_onloan is also broken by bug 23463.. I've opened bug 28028 for this one.. but I wonder if perhaps it should be an option in this script instead?
Comment 14 Jonathan Druart 2021-04-01 16:54:59 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 15 Fridolin Somers 2021-04-07 13:11:51 UTC
Pushed to 20.11.x for 20.11.05
Comment 16 Andrew Fuerste-Henry 2021-04-24 12:38:19 UTC
Pushed to 20.05.x for 20.05.11
Comment 17 Victor Grousset/tuxayo 2021-04-24 16:31:43 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.