Description
Joonas Kylmälä
2021-03-16 09:40:33 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. 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. 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? 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. Jonathan, should I interpret that as you are sending v2 or? 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. 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. 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. Thanks, I will test this now and let you know the results :) 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> Fixes the problem! :) 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> 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? Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.05 Pushed to 20.05.x for 20.05.11 Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed. |