Bug 23463

Summary: Move C4::Items CRUD subroutines to Koha::Item
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, aleisha, jonathan.druart, m.de.rooy, martin.renvoize, nick, philippe.blouin, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://gitlab.com/joubu/Koha/commits/bug_23463
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 18252, 20271, 24107, 8132, 25248, 25608, 25964, 26082, 26134, 26268, 26438, 26470, 26507, 26557, 26675, 26750, 26987, 26992, 27003, 27509, 27808, 27819, 27837, 27963, 28015, 28028, 28255, 29794    
Attachments: Bug 23463: Replace SearchItemsByField by SearchItems
Bug 23463: Remove uneeded SearchItemsByField subroutine
Bug 23463: fix for testbuilder - set default value items.damaged=0
Bug 23463: Replace AddItem calls with Koha::Item->store
Bug 23463: Remove _set_defaults_for_add
Bug 23463: Replace _set_derived_columns_for_add
Bug 23463: AddItem removing
Bug 23463: Replace ModItem with Koha::Item->store
Bug 23463: remove trivial UpdateHoldingbranch sub
Bug 23463: Move plugin hook
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved
Bug 23463: Remove _mod_item_dates
Bug 23463: Remove ModItem
Bug 23463: Remove DelItem
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Remove no longer needed subs related to default values
Bug 23463: Fix CancelReceipt.t
Bug 23463: Fix failing tests
Bug 23463: Getting less item-level_itypes warnings (?)
Bug 23463: Fix AutomaticItemModificationByAge.t
Bug 23463: update permanent_location if location has been updated
Bug 23463: Fix items.cn_source vs cn_source
Bug 23463: build cn_sort only if needed
Bug 23463: Replace recently added occurrences
Bug 23463: cn_sort needs to be updated if cn_source changed
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Replace SearchItemsByField by SearchItems
Bug 23463: Remove uneeded SearchItemsByField subroutine
Bug 23463: fix for testbuilder - set default value items.damaged=0
Bug 23463: Replace AddItem calls with Koha::Item->store
Bug 23463: Remove _set_defaults_for_add
Bug 23463: Replace _set_derived_columns_for_add
Bug 23463: AddItem removing
Bug 23463: Replace ModItem with Koha::Item->store
Bug 23463: remove trivial UpdateHoldingbranch sub
Bug 23463: Move plugin hook
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved
Bug 23463: Remove _mod_item_dates
Bug 23463: Remove ModItem
Bug 23463: Remove DelItem
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Remove no longer needed subs related to default values
Bug 23463: Fix CancelReceipt.t
Bug 23463: Fix failing tests
Bug 23463: Getting less item-level_itypes warnings (?)
Bug 23463: Fix AutomaticItemModificationByAge.t
Bug 23463: update permanent_location if location has been updated
Bug 23463: Fix items.cn_source vs cn_source
Bug 23463: build cn_sort only if needed
Bug 23463: Replace recently added occurrences
Bug 23463: cn_sort needs to be updated if cn_source changed
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Replace SearchItemsByField by SearchItems
Bug 23463: Remove uneeded SearchItemsByField subroutine
Bug 23463: fix for testbuilder - set default value items.damaged=0
Bug 23463: Replace AddItem calls with Koha::Item->store
Bug 23463: Remove _set_defaults_for_add
Bug 23463: Replace _set_derived_columns_for_add
Bug 23463: AddItem removing
Bug 23463: Replace ModItem with Koha::Item->store
Bug 23463: remove trivial UpdateHoldingbranch sub
Bug 23463: Move plugin hook
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved
Bug 23463: Remove _mod_item_dates
Bug 23463: Remove ModItem
Bug 23463: Remove DelItem
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Remove no longer needed subs related to default values
Bug 23463: Fix CancelReceipt.t
Bug 23463: Fix failing tests
Bug 23463: Getting less item-level_itypes warnings (?)
Bug 23463: Fix AutomaticItemModificationByAge.t
Bug 23463: update permanent_location if location has been updated
Bug 23463: Fix items.cn_source vs cn_source
Bug 23463: build cn_sort only if needed
Bug 23463: Replace recently added occurrences
Bug 23463: cn_sort needs to be updated if cn_source changed
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Fix timestamp default value
Bug 23463: Move DelItem.t and Items_DelItemCheck.t tests to a Koha/Item.t subtest
Bug 23463: (follow-up) Fix timestamp default value
Bug 23463: Fix safe_return return value in deletion tools
Bug 23463: Fix tests
Bug 23463: Fix IsItemIssued.t
Bug 23463: Fix cn_source for new item
Bug 23463: Fix XSLT.t
Bug 23463: Replace new occurrence of AddItem
Bug 23463: Use new method Koha::Object->set_or_blank
Bug 23463: (follow-up 2) Fix timestamp default value
Bug 23463: Fix items.cn_sort vs cn_sort
Bug 23463: Replace SearchItemsByField by SearchItems
Bug 23463: Remove uneeded SearchItemsByField subroutine
Bug 23463: fix for testbuilder - set default value items.damaged=0
Bug 23463: Replace AddItem calls with Koha::Item->store
Bug 23463: Remove _set_defaults_for_add
Bug 23463: Replace _set_derived_columns_for_add
Bug 23463: AddItem removing
Bug 23463: Replace ModItem with Koha::Item->store
Bug 23463: remove trivial UpdateHoldingbranch sub
Bug 23463: Move plugin hook
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved
Bug 23463: Remove _mod_item_dates
Bug 23463: Remove ModItem
Bug 23463: Remove DelItem
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Remove no longer needed subs related to default values
Bug 23463: Fix CancelReceipt.t
Bug 23463: Fix failing tests
Bug 23463: Getting less item-level_itypes warnings (?)
Bug 23463: Fix AutomaticItemModificationByAge.t
Bug 23463: update permanent_location if location has been updated
Bug 23463: Fix items.cn_source vs cn_source
Bug 23463: build cn_sort only if needed
Bug 23463: Replace recently added occurrences
Bug 23463: cn_sort needs to be updated if cn_source changed
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete
Bug 23463: Fix timestamp default value
Bug 23463: Move DelItem.t and Items_DelItemCheck.t tests to a Koha/Item.t subtest
Bug 23463: (follow-up) Fix timestamp default value
Bug 23463: Fix safe_return return value in deletion tools
Bug 23463: Fix tests
Bug 23463: Fix IsItemIssued.t
Bug 23463: Fix cn_source for new item
Bug 23463: Fix XSLT.t
Bug 23463: Replace new occurrence of AddItem
Bug 23463: Use new method Koha::Object->set_or_blank
Bug 23463: (follow-up 2) Fix timestamp default value
Bug 23463: Fix items.cn_sort vs cn_sort
Bug 23463: Replace a new occurrence of ModItem
Bug 23463: Fix CourseReserves/CourseItems.t
Bug 23463: (RM follow-up) Add try block around Illrequest item creation
Bug 23463: Fix wrong variable name
Bug 23463: Fix selenium tests (highlight a bug in date management)
Bug 23463: (follow-up 3) Fix timestamp default value
Bug 23463: (follow-up 3) Fix timestamp default value
Bug 23463: Remove double store call
Bug 26948: Regression tests

Description Jonathan Druart 2019-08-14 15:32:13 UTC

    
Comment 1 Jonathan Druart 2019-08-14 16:06:30 UTC
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
Comment 2 Jonathan Druart 2019-08-14 16:07:36 UTC
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!
Comment 3 Jonathan Druart 2019-10-24 15:57:07 UTC
Remote branch has been rebased.
Comment 4 Martin Renvoize 2019-11-01 12:04:44 UTC
Lets hit this one hard beginning of next cycle :)
Comment 5 Jonathan Druart 2019-12-02 12:04:13 UTC
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
Comment 6 Jonathan Druart 2019-12-02 12:04:16 UTC
Created attachment 95900 [details] [review]
Bug 23463: Remove uneeded SearchItemsByField subroutine

It is no longer used.
Comment 7 Jonathan Druart 2019-12-02 12:04:19 UTC
Created attachment 95901 [details] [review]
Bug 23463: fix for testbuilder - set default value items.damaged=0
Comment 8 Jonathan Druart 2019-12-02 12:04:24 UTC
Created attachment 95902 [details] [review]
Bug 23463: Replace AddItem calls with Koha::Item->store
Comment 9 Jonathan Druart 2019-12-02 12:04:28 UTC
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
Comment 10 Jonathan Druart 2019-12-02 12:04:31 UTC
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
Comment 11 Jonathan Druart 2019-12-02 12:04:34 UTC
Created attachment 95905 [details] [review]
Bug 23463: AddItem removing

We are done with AddItem, only need to log and index.
Comment 12 Jonathan Druart 2019-12-02 12:04:38 UTC
Created attachment 95906 [details] [review]
Bug 23463: Replace ModItem with Koha::Item->store

Starting to replace the ModItem calls with Koha::Item->store
Comment 13 Jonathan Druart 2019-12-02 12:04:42 UTC
Created attachment 95907 [details] [review]
Bug 23463: remove trivial UpdateHoldingbranch sub

We do not need this subroutine, moving it where it is used.
Comment 14 Jonathan Druart 2019-12-02 12:04:46 UTC
Created attachment 95908 [details] [review]
Bug 23463: Move plugin hook

From C4::Items to Koha::Item
Comment 15 Jonathan Druart 2019-12-02 12:04:50 UTC
Created attachment 95909 [details] [review]
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort

Same as _set_defaults_for_add
Comment 16 Jonathan Druart 2019-12-02 12:04:53 UTC
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.
Comment 17 Jonathan Druart 2019-12-02 12:04:56 UTC
Created attachment 95911 [details] [review]
Bug 23463: Remove _mod_item_dates

We did not do anything useful here.
Comment 18 Jonathan Druart 2019-12-02 12:05:00 UTC
Created attachment 95912 [details] [review]
Bug 23463: Remove ModItem

There should not be any calls left
Comment 19 Jonathan Druart 2019-12-02 12:05:04 UTC
Created attachment 95913 [details] [review]
Bug 23463: Remove DelItem
Comment 20 Jonathan Druart 2019-12-02 12:05:08 UTC
Created attachment 95914 [details] [review]
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Comment 21 Jonathan Druart 2019-12-02 12:05:12 UTC
Created attachment 95915 [details] [review]
Bug 23463: Remove no longer needed subs related to default values
Comment 22 Jonathan Druart 2019-12-02 12:05:15 UTC
Created attachment 95916 [details] [review]
Bug 23463: Fix CancelReceipt.t
Comment 23 Jonathan Druart 2019-12-02 12:05:18 UTC
Created attachment 95917 [details] [review]
Bug 23463: Fix failing tests

And adding a FIXME about discard_changes
Comment 24 Jonathan Druart 2019-12-02 12:05:22 UTC
Created attachment 95918 [details] [review]
Bug 23463: Getting less item-level_itypes warnings (?)

So, we get less warnings. I do not really understand why...
Comment 25 Jonathan Druart 2019-12-02 12:05:27 UTC
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?
Comment 26 Jonathan Druart 2019-12-02 12:05:30 UTC
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.
Comment 27 Jonathan Druart 2019-12-02 12:05:34 UTC
Created attachment 95921 [details] [review]
Bug 23463: Fix items.cn_source vs cn_source
Comment 28 Jonathan Druart 2019-12-02 12:05:37 UTC
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
Comment 29 Jonathan Druart 2019-12-02 12:05:41 UTC
Created attachment 95923 [details] [review]
Bug 23463: Replace recently added occurrences

Few occurrences have been added since this patchset has been originaly
written
Comment 30 Katrin Fischer 2019-12-02 12:17:41 UTC
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.
Comment 31 Jonathan Druart 2019-12-02 13:32:33 UTC
Created attachment 95925 [details] [review]
Bug 23463: cn_sort needs to be updated if cn_source changed
Comment 32 Jonathan Druart 2019-12-02 13:32:48 UTC
(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!
Comment 33 Jonathan Druart 2019-12-10 11:58:39 UTC
Created attachment 96145 [details] [review]
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete

Wrong replacement in additem.pl
Comment 34 Martin Renvoize 2019-12-13 09:38:53 UTC
This is on my list for early January
Comment 35 Tomás Cohen Arazi 2020-01-24 19:42:51 UTC
Rebased against master. Leaving ASSIGNED while I QA this.
Comment 36 Tomás Cohen Arazi 2020-01-24 19:44:42 UTC
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>
Comment 37 Tomás Cohen Arazi 2020-01-24 19:44:46 UTC
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>
Comment 38 Tomás Cohen Arazi 2020-01-24 19:44:49 UTC
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>
Comment 39 Tomás Cohen Arazi 2020-01-24 19:44:53 UTC
Created attachment 97898 [details] [review]
Bug 23463: Replace AddItem calls with Koha::Item->store

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2020-01-24 19:44:57 UTC
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
Comment 41 Tomás Cohen Arazi 2020-01-24 19:45:01 UTC
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
Comment 42 Tomás Cohen Arazi 2020-01-24 19:45:04 UTC
Created attachment 97901 [details] [review]
Bug 23463: AddItem removing

We are done with AddItem, only need to log and index.
Comment 43 Tomás Cohen Arazi 2020-01-24 19:45:08 UTC
Created attachment 97902 [details] [review]
Bug 23463: Replace ModItem with Koha::Item->store

Starting to replace the ModItem calls with Koha::Item->store
Comment 44 Tomás Cohen Arazi 2020-01-24 19:45:11 UTC
Created attachment 97903 [details] [review]
Bug 23463: remove trivial UpdateHoldingbranch sub

We do not need this subroutine, moving it where it is used.
Comment 45 Tomás Cohen Arazi 2020-01-24 19:45:15 UTC
Created attachment 97904 [details] [review]
Bug 23463: Move plugin hook

From C4::Items to Koha::Item
Comment 46 Tomás Cohen Arazi 2020-01-24 19:45:19 UTC
Created attachment 97905 [details] [review]
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort

Same as _set_defaults_for_add
Comment 47 Tomás Cohen Arazi 2020-01-24 19:45:23 UTC
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.
Comment 48 Tomás Cohen Arazi 2020-01-24 19:45:26 UTC
Created attachment 97907 [details] [review]
Bug 23463: Remove _mod_item_dates

We did not do anything useful here.
Comment 49 Tomás Cohen Arazi 2020-01-24 19:45:30 UTC
Created attachment 97908 [details] [review]
Bug 23463: Remove ModItem

There should not be any calls left
Comment 50 Tomás Cohen Arazi 2020-01-24 19:45:34 UTC
Created attachment 97909 [details] [review]
Bug 23463: Remove DelItem
Comment 51 Tomás Cohen Arazi 2020-01-24 19:45:37 UTC
Created attachment 97910 [details] [review]
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Comment 52 Tomás Cohen Arazi 2020-01-24 19:45:41 UTC
Created attachment 97911 [details] [review]
Bug 23463: Remove no longer needed subs related to default values
Comment 53 Tomás Cohen Arazi 2020-01-24 19:45:44 UTC
Created attachment 97912 [details] [review]
Bug 23463: Fix CancelReceipt.t
Comment 54 Tomás Cohen Arazi 2020-01-24 19:45:48 UTC
Created attachment 97913 [details] [review]
Bug 23463: Fix failing tests

And adding a FIXME about discard_changes
Comment 55 Tomás Cohen Arazi 2020-01-24 19:45:52 UTC
Created attachment 97914 [details] [review]
Bug 23463: Getting less item-level_itypes warnings (?)

So, we get less warnings. I do not really understand why...
Comment 56 Tomás Cohen Arazi 2020-01-24 19:45:55 UTC
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?
Comment 57 Tomás Cohen Arazi 2020-01-24 19:45:59 UTC
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.
Comment 58 Tomás Cohen Arazi 2020-01-24 19:46:02 UTC
Created attachment 97917 [details] [review]
Bug 23463: Fix items.cn_source vs cn_source
Comment 59 Tomás Cohen Arazi 2020-01-24 19:46:06 UTC
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
Comment 60 Tomás Cohen Arazi 2020-01-24 19:46:10 UTC
Created attachment 97919 [details] [review]
Bug 23463: Replace recently added occurrences

Few occurrences have been added since this patchset has been originaly
written
Comment 61 Tomás Cohen Arazi 2020-01-24 19:46:13 UTC Comment hidden (obsolete)
Comment 62 Tomás Cohen Arazi 2020-01-24 19:46:17 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2020-01-27 15:50:12 UTC
Patches rebased and remote branch updated.
Comment 64 Tomás Cohen Arazi 2020-01-27 19:51:39 UTC
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)
Comment 65 Jonathan Druart 2020-01-28 12:01:18 UTC
(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.
Comment 66 Tomás Cohen Arazi 2020-02-06 19:14:36 UTC
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.
Comment 67 Jonathan Druart 2020-02-11 09:24:50 UTC
(In reply to Tomás Cohen Arazi from comment #66)

> Jonathan: please add my signature to your branch's commits.

Done, thanks Tomas!
Comment 68 Nick Clemens (kidclamp) 2020-03-04 16:42:07 UTC
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>)'
Comment 69 Jonathan Druart 2020-03-16 14:02:02 UTC
(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!
Comment 70 Nick Clemens (kidclamp) 2020-03-20 12:29:15 UTC
(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.
Comment 71 Jonathan Druart 2020-03-20 14:17:33 UTC
(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.
Comment 72 Nick Clemens (kidclamp) 2020-03-20 14:35:31 UTC
Last follow-up fixes those issues, everything else I tried worked well.

Passing QA, excellent work here
Comment 73 Jonathan Druart 2020-03-20 14:50:07 UTC
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>
Comment 74 Jonathan Druart 2020-03-20 14:50:11 UTC
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>
Comment 75 Jonathan Druart 2020-03-20 14:50:15 UTC
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>
Comment 76 Jonathan Druart 2020-03-20 14:50:19 UTC
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>
Comment 77 Jonathan Druart 2020-03-20 14:50:23 UTC
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>
Comment 78 Jonathan Druart 2020-03-20 14:50:28 UTC
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>
Comment 79 Jonathan Druart 2020-03-20 14:50:32 UTC
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>
Comment 80 Jonathan Druart 2020-03-20 14:50:36 UTC
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>
Comment 81 Jonathan Druart 2020-03-20 14:50:40 UTC
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>
Comment 82 Jonathan Druart 2020-03-20 14:50:45 UTC
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>
Comment 83 Jonathan Druart 2020-03-20 14:50:49 UTC
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>
Comment 84 Jonathan Druart 2020-03-20 14:50:52 UTC
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>
Comment 85 Jonathan Druart 2020-03-20 14:50:56 UTC
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>
Comment 86 Jonathan Druart 2020-03-20 14:51:00 UTC
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>
Comment 87 Jonathan Druart 2020-03-20 14:51:04 UTC
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>
Comment 88 Jonathan Druart 2020-03-20 14:51:07 UTC
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>
Comment 89 Jonathan Druart 2020-03-20 14:51:11 UTC
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>
Comment 90 Jonathan Druart 2020-03-20 14:51:15 UTC
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>
Comment 91 Jonathan Druart 2020-03-20 14:51:19 UTC
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>
Comment 92 Jonathan Druart 2020-03-20 14:51:24 UTC
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>
Comment 93 Jonathan Druart 2020-03-20 14:51:27 UTC
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>
Comment 94 Jonathan Druart 2020-03-20 14:51:31 UTC
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>
Comment 95 Jonathan Druart 2020-03-20 14:51:35 UTC
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>
Comment 96 Jonathan Druart 2020-03-20 14:51:40 UTC
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>
Comment 97 Jonathan Druart 2020-03-20 14:51:44 UTC
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>
Comment 98 Jonathan Druart 2020-03-20 14:51:47 UTC
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>
Comment 99 Jonathan Druart 2020-03-20 14:51:51 UTC
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>
Comment 100 Jonathan Druart 2020-03-20 14:51:55 UTC
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>
Comment 101 Jonathan Druart 2020-03-20 14:51:59 UTC
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>
Comment 102 Jonathan Druart 2020-03-20 14:52:03 UTC
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>
Comment 103 Jonathan Druart 2020-03-20 14:52:06 UTC
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>
Comment 104 Jonathan Druart 2020-03-20 14:52:10 UTC
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>
Comment 105 Jonathan Druart 2020-03-20 14:52:14 UTC
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>
Comment 106 Jonathan Druart 2020-03-20 14:52:18 UTC
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>
Comment 107 Jonathan Druart 2020-03-20 14:52:22 UTC
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>
Comment 108 Jonathan Druart 2020-03-20 14:52:26 UTC
Created attachment 101194 [details] [review]
Bug 23463: Replace new occurrence of AddItem

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 109 Jonathan Druart 2020-03-20 14:52:30 UTC
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>
Comment 110 Jonathan Druart 2020-03-20 14:52:34 UTC
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>
Comment 111 Jonathan Druart 2020-03-20 14:52:38 UTC
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>
Comment 112 Jonathan Druart 2020-03-20 15:12:45 UTC
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>
Comment 113 Jonathan Druart 2020-03-20 15:12:49 UTC
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>
Comment 114 Jonathan Druart 2020-03-20 15:12:53 UTC
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>
Comment 115 Jonathan Druart 2020-03-20 15:12:57 UTC
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>
Comment 116 Jonathan Druart 2020-03-20 15:13:01 UTC
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>
Comment 117 Jonathan Druart 2020-03-20 15:13:05 UTC
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>
Comment 118 Jonathan Druart 2020-03-20 15:13:08 UTC
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>
Comment 119 Jonathan Druart 2020-03-20 15:13:13 UTC
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>
Comment 120 Jonathan Druart 2020-03-20 15:13:16 UTC
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>
Comment 121 Jonathan Druart 2020-03-20 15:13:20 UTC
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>
Comment 122 Jonathan Druart 2020-03-20 15:13:24 UTC
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>
Comment 123 Jonathan Druart 2020-03-20 15:13:28 UTC
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>
Comment 124 Jonathan Druart 2020-03-20 15:13:32 UTC
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>
Comment 125 Jonathan Druart 2020-03-20 15:13:36 UTC
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>
Comment 126 Jonathan Druart 2020-03-20 15:13:39 UTC
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>
Comment 127 Jonathan Druart 2020-03-20 15:13:43 UTC
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>
Comment 128 Jonathan Druart 2020-03-20 15:13:47 UTC
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>
Comment 129 Jonathan Druart 2020-03-20 15:13:51 UTC
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>
Comment 130 Jonathan Druart 2020-03-20 15:13:55 UTC
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>
Comment 131 Jonathan Druart 2020-03-20 15:13:59 UTC
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>
Comment 132 Jonathan Druart 2020-03-20 15:14:03 UTC
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>
Comment 133 Jonathan Druart 2020-03-20 15:14:07 UTC
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>
Comment 134 Jonathan Druart 2020-03-20 15:14:11 UTC
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>
Comment 135 Jonathan Druart 2020-03-20 15:14:15 UTC
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>
Comment 136 Jonathan Druart 2020-03-20 15:14:19 UTC
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>
Comment 137 Jonathan Druart 2020-03-20 15:14:23 UTC
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>
Comment 138 Jonathan Druart 2020-03-20 15:14:27 UTC
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>
Comment 139 Jonathan Druart 2020-03-20 15:14:31 UTC
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>
Comment 140 Jonathan Druart 2020-03-20 15:14:35 UTC
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>
Comment 141 Jonathan Druart 2020-03-20 15:14:39 UTC
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>
Comment 142 Jonathan Druart 2020-03-20 15:14:43 UTC
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>
Comment 143 Jonathan Druart 2020-03-20 15:14:47 UTC
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>
Comment 144 Jonathan Druart 2020-03-20 15:14:51 UTC
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>
Comment 145 Jonathan Druart 2020-03-20 15:14:55 UTC
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>
Comment 146 Jonathan Druart 2020-03-20 15:14:59 UTC
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>
Comment 147 Jonathan Druart 2020-03-20 15:15:03 UTC
Created attachment 101281 [details] [review]
Bug 23463: Replace new occurrence of AddItem

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 148 Jonathan Druart 2020-03-20 15:15:07 UTC
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>
Comment 149 Jonathan Druart 2020-03-20 15:15:11 UTC
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>
Comment 150 Jonathan Druart 2020-03-20 15:15:15 UTC
Created attachment 101284 [details] [review]
Bug 23463: Fix items.cn_sort vs cn_sort

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 151 Martin Renvoize 2020-03-23 09:23:19 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 152 Jonathan Druart 2020-03-23 10:35:04 UTC
Created attachment 101413 [details] [review]
Bug 23463: Replace a new occurrence of ModItem

This new script have been pushed just before 23463.
Comment 153 Jonathan Druart 2020-03-23 10:35:11 UTC
Created attachment 101414 [details] [review]
Bug 23463: Fix CourseReserves/CourseItems.t
Comment 154 Martin Renvoize 2020-03-23 10:36:38 UTC
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>
Comment 155 Jonathan Druart 2020-03-23 10:37:24 UTC
Created attachment 101416 [details] [review]
Bug 23463: Fix wrong variable name

Fix for t/db_dependent/Labels/t_Batch.t
Comment 156 Jonathan Druart 2020-03-23 12:14:45 UTC
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
Comment 157 Tomás Cohen Arazi 2020-03-23 12:42:44 UTC
Congratulations, Jonathan
Comment 158 Jonathan Druart 2020-03-23 16:24:04 UTC
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?
Comment 159 Jonathan Druart 2020-03-30 09:28:16 UTC
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.
Comment 160 Martin Renvoize 2020-03-31 08:53:40 UTC
Final follow-up pushed.
Comment 161 Marcel de Rooy 2020-04-02 14:13:19 UTC
Let the customization fun start ;)
Comment 162 Joy Nelson 2020-04-03 21:16:48 UTC
not backported to 19.11.x
Comment 163 Marcel de Rooy 2020-04-16 13:38:30 UTC
    $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
Comment 164 Jonathan Druart 2020-04-16 14:05:19 UTC
(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!
Comment 165 Jonathan Druart 2020-04-17 08:42:58 UTC
Created attachment 103129 [details] [review]
Bug 23463: Remove double store call
Comment 166 Martin Renvoize 2020-04-17 09:27:45 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 167 Aleisha Amohia 2020-06-12 02:27:45 UTC
enhancement, not backported to 19.11.x
Comment 168 Blou 2020-11-12 20:26:03 UTC
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>