This patchset will be quite big and will move the following C4::Items subroutines to the Koha namespace: AddItem ModItem DelItem ItemSafeToDelete DelItemCheck SearchItemsByField UpdateHoldingbranch _koha_new_item _koha_modify_item _set_defaults_for_add _set_derived_columns_for_add _do_column_fixes_for_mod _set_derived_columns_for_mod _build_default_values_for_mod_marc _get_single_item_column _calc_items_cn_sort _mod_item_dates _after_item_action_hooks
I have pushed a WIP branch to: https://gitlab.com/joubu/Koha/commits/bug_23463 The branch is ready for testing (but still not clean enough to be integrated). Early feedbacks welcomed!
Remote branch has been rebased.
Lets hit this one hard beginning of next cycle :)
Created attachment 95899 [details] [review] Bug 23463: Replace SearchItemsByField by SearchItems The subroutine SearchItemsByField is only called once and can be replaced easily with SearchItems
Created attachment 95900 [details] [review] Bug 23463: Remove uneeded SearchItemsByField subroutine It is no longer used.
Created attachment 95901 [details] [review] Bug 23463: fix for testbuilder - set default value items.damaged=0
Created attachment 95902 [details] [review] Bug 23463: Replace AddItem calls with Koha::Item->store
Created attachment 95903 [details] [review] Bug 23463: Remove _set_defaults_for_add _set_defaults_for_add is no longer needed as it is handled by dbic and koha::object->store
Created attachment 95904 [details] [review] Bug 23463: Replace _set_derived_columns_for_add Remove the unique call to _set_derived_columns_for_add Even if design to deal with other values it only dealts with cn_sort
Created attachment 95905 [details] [review] Bug 23463: AddItem removing We are done with AddItem, only need to log and index.
Created attachment 95906 [details] [review] Bug 23463: Replace ModItem with Koha::Item->store Starting to replace the ModItem calls with Koha::Item->store
Created attachment 95907 [details] [review] Bug 23463: remove trivial UpdateHoldingbranch sub We do not need this subroutine, moving it where it is used.
Created attachment 95908 [details] [review] Bug 23463: Move plugin hook From C4::Items to Koha::Item
Created attachment 95909 [details] [review] Bug 23463: _set_derived_columns_for_mod was only used for cn_sort Same as _set_defaults_for_add
Created attachment 95910 [details] [review] Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved That's done in Koha::Item->store, and Koha::Object->store for integers.
Created attachment 95911 [details] [review] Bug 23463: Remove _mod_item_dates We did not do anything useful here.
Created attachment 95912 [details] [review] Bug 23463: Remove ModItem There should not be any calls left
Created attachment 95913 [details] [review] Bug 23463: Remove DelItem
Created attachment 95914 [details] [review] Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Created attachment 95915 [details] [review] Bug 23463: Remove no longer needed subs related to default values
Created attachment 95916 [details] [review] Bug 23463: Fix CancelReceipt.t
Created attachment 95917 [details] [review] Bug 23463: Fix failing tests And adding a FIXME about discard_changes
Created attachment 95918 [details] [review] Bug 23463: Getting less item-level_itypes warnings (?) So, we get less warnings. I do not really understand why...
Created attachment 95919 [details] [review] Bug 23463: Fix AutomaticItemModificationByAge.t We do not want to log if nothing changed How could we do that better?
Created attachment 95920 [details] [review] Bug 23463: update permanent_location if location has been updated This change to Koha::Item->store seems correct here, but tests from /db_dependent/Items.t is failing now. Adjusting them to make them pass.
Created attachment 95921 [details] [review] Bug 23463: Fix items.cn_source vs cn_source
Created attachment 95922 [details] [review] Bug 23463: build cn_sort only if needed No need to calculate cn_sort if not dirty when we store
Created attachment 95923 [details] [review] Bug 23463: Replace recently added occurrences Few occurrences have been added since this patchset has been originaly written
Comment on attachment 95922 [details] [review] Bug 23463: build cn_sort only if needed Review of attachment 95922 [details] [review]: ----------------------------------------------------------------- ::: Koha/Item.pm @@ +74,5 @@ > $self->itype($self->biblio->biblioitem->itemtype); > } > > + my %updated_columns = $self->_result->get_dirty_columns; > + if ( exists $updated_columns{itemcallnumber} ) { We should also update the cn_sort if cn_source was changed.
Created attachment 95925 [details] [review] Bug 23463: cn_sort needs to be updated if cn_source changed
(In reply to Katrin Fischer from comment #30) > Comment on attachment 95922 [details] [review] [review] > Bug 23463: build cn_sort only if needed > > Review of attachment 95922 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Item.pm > @@ +74,5 @@ > > $self->itype($self->biblio->biblioitem->itemtype); > > } > > > > + my %updated_columns = $self->_result->get_dirty_columns; > > + if ( exists $updated_columns{itemcallnumber} ) { > > We should also update the cn_sort if cn_source was changed. Indeed, thanks!
Created attachment 96145 [details] [review] Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete Wrong replacement in additem.pl
This is on my list for early January
Rebased against master. Leaving ASSIGNED while I QA this.
Created attachment 97895 [details] [review] Bug 23463: Replace SearchItemsByField by SearchItems The subroutine SearchItemsByField is only called once and can be replaced easily with SearchItems Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97896 [details] [review] Bug 23463: Remove uneeded SearchItemsByField subroutine It is no longer used. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97897 [details] [review] Bug 23463: fix for testbuilder - set default value items.damaged=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97898 [details] [review] Bug 23463: Replace AddItem calls with Koha::Item->store Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97899 [details] [review] Bug 23463: Remove _set_defaults_for_add _set_defaults_for_add is no longer needed as it is handled by dbic and koha::object->store
Created attachment 97900 [details] [review] Bug 23463: Replace _set_derived_columns_for_add Remove the unique call to _set_derived_columns_for_add Even if design to deal with other values it only dealts with cn_sort
Created attachment 97901 [details] [review] Bug 23463: AddItem removing We are done with AddItem, only need to log and index.
Created attachment 97902 [details] [review] Bug 23463: Replace ModItem with Koha::Item->store Starting to replace the ModItem calls with Koha::Item->store
Created attachment 97903 [details] [review] Bug 23463: remove trivial UpdateHoldingbranch sub We do not need this subroutine, moving it where it is used.
Created attachment 97904 [details] [review] Bug 23463: Move plugin hook From C4::Items to Koha::Item
Created attachment 97905 [details] [review] Bug 23463: _set_derived_columns_for_mod was only used for cn_sort Same as _set_defaults_for_add
Created attachment 97906 [details] [review] Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved That's done in Koha::Item->store, and Koha::Object->store for integers.
Created attachment 97907 [details] [review] Bug 23463: Remove _mod_item_dates We did not do anything useful here.
Created attachment 97908 [details] [review] Bug 23463: Remove ModItem There should not be any calls left
Created attachment 97909 [details] [review] Bug 23463: Remove DelItem
Created attachment 97910 [details] [review] Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Created attachment 97911 [details] [review] Bug 23463: Remove no longer needed subs related to default values
Created attachment 97912 [details] [review] Bug 23463: Fix CancelReceipt.t
Created attachment 97913 [details] [review] Bug 23463: Fix failing tests And adding a FIXME about discard_changes
Created attachment 97914 [details] [review] Bug 23463: Getting less item-level_itypes warnings (?) So, we get less warnings. I do not really understand why...
Created attachment 97915 [details] [review] Bug 23463: Fix AutomaticItemModificationByAge.t We do not want to log if nothing changed How could we do that better?
Created attachment 97916 [details] [review] Bug 23463: update permanent_location if location has been updated This change to Koha::Item->store seems correct here, but tests from /db_dependent/Items.t is failing now. Adjusting them to make them pass.
Created attachment 97917 [details] [review] Bug 23463: Fix items.cn_source vs cn_source
Created attachment 97918 [details] [review] Bug 23463: build cn_sort only if needed No need to calculate cn_sort if not dirty when we store
Created attachment 97919 [details] [review] Bug 23463: Replace recently added occurrences Few occurrences have been added since this patchset has been originaly written
Created attachment 97920 [details] [review] Bug 23463: cn_sort needs to be updated if cn_source changed
Created attachment 97921 [details] [review] Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete Wrong replacement in additem.pl
Patches rebased and remote branch updated.
About the timestamp handling: shouldn't we better implement Koha::Object->set_and_store of similar, that instead of overwriting the whole object, just sets the really changed fields, and we leave the DB do the heavy lifting? It looks wrong (as you also say, Jonathan) to be setting the timestamp manually on the ->store method (be it in the Koha::Item class, or as a generic approach in Koha::Object)
(In reply to Tomás Cohen Arazi from comment #64) > About the timestamp handling: shouldn't we better implement > Koha::Object->set_and_store of similar, that instead of overwriting the > whole object, just sets the really changed fields, and we leave the DB do > the heavy lifting? It looks wrong (as you also say, Jonathan) to be setting > the timestamp manually on the ->store method (be it in the Koha::Item class, > or as a generic approach in Koha::Object) I have added a follow-up to the branch. Still not optimal however. I am on a "less regressions as possible" approach here. We will certainly want/need to improve later.
I tested this branch (which Jonathan rebased against master) and: - Some tests were failing due to old and new rebase issues: fixed! - Some typos in controllers and libs: fixed! I'm not 100% comfortable with the legacy approaches to avoid regressions, but I AGREE they are better than breaking things in large scale. This way we can start writing the API for items in a way that we are sure will preserve behaviour, one page at a time, so all good! I tested: STAFF - Search results - Detail view - Edit/add/delete items - Circulation, placing holds - Course reserves - Acquisition workflows - Batch item modification - Batch item deletion OPAC - Course reserves - Search results, detail view - Reading history - Shelf browser I'm confident about this patchset at this stage. Jonathan: please add my signature to your branch's commits.
(In reply to Tomás Cohen Arazi from comment #66) > Jonathan: please add my signature to your branch's commits. Done, thanks Tomas!
Testing btoh with mariadb_d9 and mariadb_d10 I was unable to blank any fields in items, either individually or via batch edit - things like 'onloan' and 'itemlost_on' are successfully nulled as they should be. Two test failures: # Failed test 'Syntax check misc/migration_tools/fix_onloan.pl' # at /usr/share/perl5/Test/Strict.pm line 423. # Unmatched right curly bracket at misc/migration_tools/fix_onloan.pl line 20, at end of line # syntax error at misc/migration_tools/fix_onloan.pl line 20, near "}" # misc/migration_tools/fix_onloan.pl had compilation errors. # Looks like you failed 1 test of 706. # Failed test 'reference if positive itemtype notforloan value' # at t/db_dependent/XSLT.t line 57. # '<items xmlns="http://www.koha-community.org/items"><item><homebranch>FwkGdaAK</homebranch><holdingbranch>FwkGdaAK</holdingbranch><location></location><ccode></ccode><status>available</status><itemcallnumber></itemcallnumber><stocknumber></stocknumber></item></items>' # doesn't match '(?^u:<status>reference</status>)'
(In reply to Nick Clemens from comment #68) > Testing btoh with mariadb_d9 and mariadb_d10 I was unable to blank any > fields in items, either individually or via batch edit - things like > 'onloan' and 'itemlost_on' are successfully nulled as they should be. Fixed, see "Use new method Koha::Object->set_or_blank" > Two test failures: > > # Failed test 'Syntax check misc/migration_tools/fix_onloan.pl' > # at /usr/share/perl5/Test/Strict.pm line 423. > # Unmatched right curly bracket at misc/migration_tools/fix_onloan.pl line > 20, at end of line > # syntax error at misc/migration_tools/fix_onloan.pl line 20, near "}" > # misc/migration_tools/fix_onloan.pl had compilation errors. > # Looks like you failed 1 test of 706. Fixed! > # Failed test 'reference if positive itemtype notforloan value' > # at t/db_dependent/XSLT.t line 57. > # '<items > xmlns="http://www.koha-community.org/items"><item><homebranch>FwkGdaAK</ > homebranch><holdingbranch>FwkGdaAK</holdingbranch><location></ > location><ccode></ccode><status>available</status><itemcallnumber></ > itemcallnumber><stocknumber></stocknumber></item></items>' > # doesn't match '(?^u:<status>reference</status>)' Fixed!
(In reply to Jonathan Druart from comment #69) > (In reply to Nick Clemens from comment #68) > > Testing btoh with mariadb_d9 and mariadb_d10 I was unable to blank any > > fields in items, either individually or via batch edit - things like > > 'onloan' and 'itemlost_on' are successfully nulled as they should be. > > Fixed, see "Use new method Koha::Object->set_or_blank" Batch modification to blank fields is still now working. Individual edits do work. Importing and replacing items does not work - the process hangs.
(In reply to Nick Clemens from comment #70) > (In reply to Jonathan Druart from comment #69) > > (In reply to Nick Clemens from comment #68) > > > Testing btoh with mariadb_d9 and mariadb_d10 I was unable to blank any > > > fields in items, either individually or via batch edit - things like > > > 'onloan' and 'itemlost_on' are successfully nulled as they should be. > > > > Fixed, see "Use new method Koha::Object->set_or_blank" > > Batch modification to blank fields is still now working. Individual edits do > work. I fixed that one. > Importing and replacing items does not work - the process hangs. I am not sure about that one. I got "DBD::mysql::st execute failed: Incorrect integer value: 'duplicate item barcode' for column 'itemnumber'" But not sure it's a bug from master or not. Could you retry the import? If it fails, can you remove 334 close STDOUT; 335 close STDERR; from tools/manage-marc-import.pl To get the error in the logs.
Last follow-up fixes those issues, everything else I tried worked well. Passing QA, excellent work here
Created attachment 101151 [details] [review] Bug 23463: Replace SearchItemsByField by SearchItems The subroutine SearchItemsByField is only called once and can be replaced easily with SearchItems Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101152 [details] [review] Bug 23463: Remove uneeded SearchItemsByField subroutine It is no longer used. Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101153 [details] [review] Bug 23463: fix for testbuilder - set default value items.damaged=0 Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101154 [details] [review] Bug 23463: Replace AddItem calls with Koha::Item->store Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101156 [details] [review] Bug 23463: Remove _set_defaults_for_add _set_defaults_for_add is no longer needed as it is handled by dbic and koha::object->store Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101157 [details] [review] Bug 23463: Replace _set_derived_columns_for_add Remove the unique call to _set_derived_columns_for_add Even if design to deal with other values it only dealts with cn_sort Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101158 [details] [review] Bug 23463: AddItem removing We are done with AddItem, only need to log and index. Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101161 [details] [review] Bug 23463: Replace ModItem with Koha::Item->store Starting to replace the ModItem calls with Koha::Item->store Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101163 [details] [review] Bug 23463: remove trivial UpdateHoldingbranch sub We do not need this subroutine, moving it where it is used. Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101165 [details] [review] Bug 23463: Move plugin hook From C4::Items to Koha::Item Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101167 [details] [review] Bug 23463: _set_derived_columns_for_mod was only used for cn_sort Same as _set_defaults_for_add Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101169 [details] [review] Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved That's done in Koha::Item->store, and Koha::Object->store for integers. Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101170 [details] [review] Bug 23463: Remove _mod_item_dates We did not do anything useful here. Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101171 [details] [review] Bug 23463: Remove ModItem There should not be any calls left Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101172 [details] [review] Bug 23463: Remove DelItem Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101173 [details] [review] Bug 23463: Remove DelItemCheck and ItemSafeToDelete Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101174 [details] [review] Bug 23463: Remove no longer needed subs related to default values Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101175 [details] [review] Bug 23463: Fix CancelReceipt.t Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101176 [details] [review] Bug 23463: Fix failing tests And adding a FIXME about discard_changes Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101177 [details] [review] Bug 23463: Getting less item-level_itypes warnings (?) So, we get less warnings. I do not really understand why... Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101178 [details] [review] Bug 23463: Fix AutomaticItemModificationByAge.t We do not want to log if nothing changed How could we do that better? Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101179 [details] [review] Bug 23463: update permanent_location if location has been updated This change to Koha::Item->store seems correct here, but tests from /db_dependent/Items.t is failing now. Adjusting them to make them pass. Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101180 [details] [review] Bug 23463: Fix items.cn_source vs cn_source Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101181 [details] [review] Bug 23463: build cn_sort only if needed No need to calculate cn_sort if not dirty when we store Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101182 [details] [review] Bug 23463: Replace recently added occurrences Few occurrences have been added since this patchset has been originaly written Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101183 [details] [review] Bug 23463: cn_sort needs to be updated if cn_source changed Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101184 [details] [review] Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete Wrong replacement in additem.pl Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101185 [details] [review] Bug 23463: Fix timestamp default value This sounds wrong as we should let the DBMS do that, but it was failing. Here we are doing the same as Koha::Patron->store for dateenrolled To recreate the failure, prove t/db_dependent/Koha/Item.t without this patch: DBD::mysql::st execute failed: Column 'timestamp' cannot be null [for Statement "UPDATE `items` SET `more_subfields_xml` = ?, `timestamp` = ? WHERE ( `itemnumber` = ? )" with ParamValues: 0='<?xml version="1.0" encoding="UTF-8"?> Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101186 [details] [review] Bug 23463: Move DelItem.t and Items_DelItemCheck.t tests to a Koha/Item.t subtest Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101188 [details] [review] Bug 23463: (follow-up) Fix timestamp default value For discussion, this patch revert the changes made previously. This line exists in Koha::Item->store as it is the translation of: if (exists $item->{'timestamp'}) { delete $item->{'timestamp'}; } that was coming from _do_column_fixes_for_mod (called from ModItem) To preserve existing behavior I would be in favor of keeping it like that to avoid regression, and deal with it separately if we want to improve/remove it. So basically here we are setting it to undef in Koha::Item->store to make it handle correctly by the parent Koha::Object->store. I agree that's kind of weird and must be improved. Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101189 [details] [review] Bug 23463: Fix safe_return return value in deletion tools Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101190 [details] [review] Bug 23463: Fix tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101191 [details] [review] Bug 23463: Fix IsItemIssued.t It seems that some of those tests could be removed as safe_delete is widely tested in Koha/Item.t Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101192 [details] [review] Bug 23463: Fix cn_source for new item get_dirty_columns only work for existing items. This fixes t/db_dependent/ShelfBrowser.t Signed-off-by: Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101193 [details] [review] Bug 23463: Fix XSLT.t From commit bf49eecdd87e2b29760226281ab1afc0a185c7f0 Bug 23463: Replace AddItem calls with Koha::Item->store in build_sample_item: - my $itype = delete $args->{itype} - || $self->build_object( { class => 'Koha::ItemTypes' } )->itemtype; + # If itype is not passed it will be picked from the biblio (see Koha::Item->store) So before we generated a new itemtype if not passed, now we pick the one from biblioitem->itemtype. For this specific test we need to make sure they are different. Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101194 [details] [review] Bug 23463: Replace new occurrence of AddItem Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101195 [details] [review] Bug 23463: Use new method Koha::Object->set_or_blank This patch fixes an issue when editing items. * The issue Cannot blank a subfield when editing an item. If you have an item with itemcallnumber=42, then edit it, blank it and save. The itemcallnumber is still 42 * Why? (line numbers from https://gitlab.com/joubu/Koha/-/tree/bug_23463) additem (and other item's edition forms) receives a list of tags, subfields and values, generates a MARC::Record::XML then calls ModItemFromMarc: 717 my $itemtosave=MARC::Record::new_from_xml($xml, 'UTF-8'); 727 my $newitem = ModItemFromMarc($itemtosave, $biblionumber, $itemnumber); And ModItemFromMarc: 282 my $item = TransformMarcToKoha( $localitemmarc, $frameworkcode, 'items' ); 283 $item->{cn_source} = delete $item->{'items.cn_source'}; # Because of C4::Biblio::_disambiguate 284 $item_object->set($item); ModItemFromMarc never knows that the field has been blank. Prior to bug 23463 we had a map of default values, and ModItemFromMarc was doing: 426 my $item = TransformMarcToKoha( $localitemmarc, $frameworkcode, 'items' ); 427 my $default_values = _build_default_values_for_mod_marc(); 428 foreach my $item_field ( keys %$default_values ) { 429 $item->{$item_field} = $default_values->{$item_field} 430 unless exists $item->{$item_field}; 431 } I do not want to reinsert that list of default values. Here I wrote a generic method in Koha::Object to set the value passed in parameter, or "blank" if not passed. It's nulled if can be set to null in DB, or the default value is retrieved from the schema info. Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101196 [details] [review] Bug 23463: (follow-up 2) Fix timestamp default value Removing previous change, again. But this time in Object.pm It fixes t/db_dependent/Koha/Object.t and t/db_dependent/Koha/Item.t is still passing. Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101197 [details] [review] Bug 23463: Fix items.cn_sort vs cn_sort Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101246 [details] [review] Bug 23463: Replace SearchItemsByField by SearchItems The subroutine SearchItemsByField is only called once and can be replaced easily with SearchItems Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101247 [details] [review] Bug 23463: Remove uneeded SearchItemsByField subroutine It is no longer used. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101248 [details] [review] Bug 23463: fix for testbuilder - set default value items.damaged=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101249 [details] [review] Bug 23463: Replace AddItem calls with Koha::Item->store Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101250 [details] [review] Bug 23463: Remove _set_defaults_for_add _set_defaults_for_add is no longer needed as it is handled by dbic and koha::object->store Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101251 [details] [review] Bug 23463: Replace _set_derived_columns_for_add Remove the unique call to _set_derived_columns_for_add Even if design to deal with other values it only dealts with cn_sort Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101252 [details] [review] Bug 23463: AddItem removing We are done with AddItem, only need to log and index. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101253 [details] [review] Bug 23463: Replace ModItem with Koha::Item->store Starting to replace the ModItem calls with Koha::Item->store Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101254 [details] [review] Bug 23463: remove trivial UpdateHoldingbranch sub We do not need this subroutine, moving it where it is used. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101255 [details] [review] Bug 23463: Move plugin hook From C4::Items to Koha::Item Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101256 [details] [review] Bug 23463: _set_derived_columns_for_mod was only used for cn_sort Same as _set_defaults_for_add Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101257 [details] [review] Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved That's done in Koha::Item->store, and Koha::Object->store for integers. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101258 [details] [review] Bug 23463: Remove _mod_item_dates We did not do anything useful here. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101259 [details] [review] Bug 23463: Remove ModItem There should not be any calls left Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101260 [details] [review] Bug 23463: Remove DelItem Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101261 [details] [review] Bug 23463: Remove DelItemCheck and ItemSafeToDelete Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101262 [details] [review] Bug 23463: Remove no longer needed subs related to default values Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101263 [details] [review] Bug 23463: Fix CancelReceipt.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101264 [details] [review] Bug 23463: Fix failing tests And adding a FIXME about discard_changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101265 [details] [review] Bug 23463: Getting less item-level_itypes warnings (?) So, we get less warnings. I do not really understand why... Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101266 [details] [review] Bug 23463: Fix AutomaticItemModificationByAge.t We do not want to log if nothing changed How could we do that better? Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101267 [details] [review] Bug 23463: update permanent_location if location has been updated This change to Koha::Item->store seems correct here, but tests from /db_dependent/Items.t is failing now. Adjusting them to make them pass. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101268 [details] [review] Bug 23463: Fix items.cn_source vs cn_source Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101269 [details] [review] Bug 23463: build cn_sort only if needed No need to calculate cn_sort if not dirty when we store Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101270 [details] [review] Bug 23463: Replace recently added occurrences Few occurrences have been added since this patchset has been originaly written Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101271 [details] [review] Bug 23463: cn_sort needs to be updated if cn_source changed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101272 [details] [review] Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete Wrong replacement in additem.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101273 [details] [review] Bug 23463: Fix timestamp default value This sounds wrong as we should let the DBMS do that, but it was failing. Here we are doing the same as Koha::Patron->store for dateenrolled To recreate the failure, prove t/db_dependent/Koha/Item.t without this patch: DBD::mysql::st execute failed: Column 'timestamp' cannot be null [for Statement "UPDATE `items` SET `more_subfields_xml` = ?, `timestamp` = ? WHERE ( `itemnumber` = ? )" with ParamValues: 0='<?xml version="1.0" encoding="UTF-8"?> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101274 [details] [review] Bug 23463: Move DelItem.t and Items_DelItemCheck.t tests to a Koha/Item.t subtest Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101275 [details] [review] Bug 23463: (follow-up) Fix timestamp default value For discussion, this patch revert the changes made previously. This line exists in Koha::Item->store as it is the translation of: if (exists $item->{'timestamp'}) { delete $item->{'timestamp'}; } that was coming from _do_column_fixes_for_mod (called from ModItem) To preserve existing behavior I would be in favor of keeping it like that to avoid regression, and deal with it separately if we want to improve/remove it. So basically here we are setting it to undef in Koha::Item->store to make it handle correctly by the parent Koha::Object->store. I agree that's kind of weird and must be improved. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101276 [details] [review] Bug 23463: Fix safe_return return value in deletion tools Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101277 [details] [review] Bug 23463: Fix tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101278 [details] [review] Bug 23463: Fix IsItemIssued.t It seems that some of those tests could be removed as safe_delete is widely tested in Koha/Item.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101279 [details] [review] Bug 23463: Fix cn_source for new item get_dirty_columns only work for existing items. This fixes t/db_dependent/ShelfBrowser.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101280 [details] [review] Bug 23463: Fix XSLT.t From commit bf49eecdd87e2b29760226281ab1afc0a185c7f0 Bug 23463: Replace AddItem calls with Koha::Item->store in build_sample_item: - my $itype = delete $args->{itype} - || $self->build_object( { class => 'Koha::ItemTypes' } )->itemtype; + # If itype is not passed it will be picked from the biblio (see Koha::Item->store) So before we generated a new itemtype if not passed, now we pick the one from biblioitem->itemtype. For this specific test we need to make sure they are different. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101281 [details] [review] Bug 23463: Replace new occurrence of AddItem Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101282 [details] [review] Bug 23463: Use new method Koha::Object->set_or_blank This patch fixes an issue when editing items. * The issue Cannot blank a subfield when editing an item. If you have an item with itemcallnumber=42, then edit it, blank it and save. The itemcallnumber is still 42 * Why? (line numbers from https://gitlab.com/joubu/Koha/-/tree/bug_23463) additem (and other item's edition forms) receives a list of tags, subfields and values, generates a MARC::Record::XML then calls ModItemFromMarc: 717 my $itemtosave=MARC::Record::new_from_xml($xml, 'UTF-8'); 727 my $newitem = ModItemFromMarc($itemtosave, $biblionumber, $itemnumber); And ModItemFromMarc: 282 my $item = TransformMarcToKoha( $localitemmarc, $frameworkcode, 'items' ); 283 $item->{cn_source} = delete $item->{'items.cn_source'}; # Because of C4::Biblio::_disambiguate 284 $item_object->set($item); ModItemFromMarc never knows that the field has been blank. Prior to bug 23463 we had a map of default values, and ModItemFromMarc was doing: 426 my $item = TransformMarcToKoha( $localitemmarc, $frameworkcode, 'items' ); 427 my $default_values = _build_default_values_for_mod_marc(); 428 foreach my $item_field ( keys %$default_values ) { 429 $item->{$item_field} = $default_values->{$item_field} 430 unless exists $item->{$item_field}; 431 } I do not want to reinsert that list of default values. Here I wrote a generic method in Koha::Object to set the value passed in parameter, or "blank" if not passed. It's nulled if can be set to null in DB, or the default value is retrieved from the schema info. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101283 [details] [review] Bug 23463: (follow-up 2) Fix timestamp default value Removing previous change, again. But this time in Object.pm It fixes t/db_dependent/Koha/Object.t and t/db_dependent/Koha/Item.t is still passing. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101284 [details] [review] Bug 23463: Fix items.cn_sort vs cn_sort Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
Created attachment 101413 [details] [review] Bug 23463: Replace a new occurrence of ModItem This new script have been pushed just before 23463.
Created attachment 101414 [details] [review] Bug 23463: Fix CourseReserves/CourseItems.t
Created attachment 101415 [details] [review] Bug 23463: (RM follow-up) Add try block around Illrequest item creation The recently introduced Ill item creation for circulation feature was not properly handling the move from AddItem to Koha::Item->new()->store. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 101416 [details] [review] Bug 23463: Fix wrong variable name Fix for t/db_dependent/Labels/t_Batch.t
Created attachment 101432 [details] [review] Bug 23463: Fix selenium tests (highlight a bug in date management) There is something wrong, and a regression has been caught by those tests: If an invalid date is passed from the add item form, the app now crashes. Before: * if the date was completely invalid, the field was blanked silently * DateTime::Format::MySQL was used to convert dates, and it's not strict at all. For instance, what happened in the selenium tests for dateaccessionned: %Y-%m-%d was prefilled by the framework plugin, then the biblionumber was added, we ended with something like (eg for today) 2020-03-234242 (with biblionumber=4242). DateTime::Format::MySQL converts that to 2020-03-23 We must deal with invalid dates, but I do not think it is good to add it back to Koha::Item->store, we will prefer to raise the error to the end user, saying that something went wrong (and more specifically the dates). The (ugly) trick was in C4::Items::_mod_item_dates
Congratulations, Jonathan
Created attachment 101476 [details] [review] Bug 23463: (follow-up 3) Fix timestamp default value This is the only situation I found where: * t/db_dependent/Koha/Item.t is passing * t/db_dependent/Koha/Object.t is passing * MySQL 8 is happy (and not fail with "Column 'timestamp' cannot be null"), which is certainly what I missed in the previous follow-up I think we should remove the lines from the overloaded ->store method (Item and Patron), but then we need Object.t to be adjusted. I do not understand why we would need to set updated_on, maybe the test is wrong?
Created attachment 102071 [details] [review] Bug 23463: (follow-up 3) Fix timestamp default value This is the only situation I found where: * t/db_dependent/Koha/Item.t is passing * t/db_dependent/Koha/Object.t is passing * MySQL 8 is happy (and not fail with "Column 'timestamp' cannot be null"), which is certainly what I missed in the previous follow-up About the change to Object.t: the next store was called without the generated timestamp, so it is needed to call discard_changes.
Final follow-up pushed.
Let the customization fun start ;)
not backported to 19.11.x
$item_object->more_subfields_xml(_get_unlinked_subfields_xml($unlinked_item_subfields))->store; $item_object->store; Is this double store call intentional ? C4/Items ModItemFromMarc
(In reply to Marcel de Rooy from comment #163) > > $item_object- > >more_subfields_xml(_get_unlinked_subfields_xml($unlinked_item_subfields))- > >store; > $item_object->store; > > Is this double store call intentional ? > C4/Items ModItemFromMarc Nope!
Created attachment 103129 [details] [review] Bug 23463: Remove double store call
enhancement, not backported to 19.11.x
Created attachment 113581 [details] [review] Bug 26948: Regression tests We already mock the send_or_die method from Email::Stuffer; This patch updates that mock to extract the internal representation of the email encoded message and looks for a known double encoded representation of a character in the sample body. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Blou <blou@inlibro.com>