Bug 23463 - Move C4::Items CRUD subroutines to Koha::Item
Summary: Move C4::Items CRUD subroutines to Koha::Item
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL: https://gitlab.com/joubu/Koha/commits...
Keywords:
Depends on:
Blocks: 8132 18252 20271 24107 25248 25608
  Show dependency treegraph
 
Reported: 2019-08-14 15:32 UTC by Jonathan Druart
Modified: 2020-06-12 02:27 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 23463: Replace SearchItemsByField by SearchItems (2.25 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove uneeded SearchItemsByField subroutine (1.30 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: fix for testbuilder - set default value items.damaged=0 (756 bytes, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace AddItem calls with Koha::Item->store (79.13 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove _set_defaults_for_add (2.32 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace _set_derived_columns_for_add (1.98 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: AddItem removing (5.09 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace ModItem with Koha::Item->store (51.34 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: remove trivial UpdateHoldingbranch sub (1.66 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Move plugin hook (3.03 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort (4.02 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved (2.02 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove _mod_item_dates (1.54 KB, patch)
2019-12-02 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove ModItem (2.91 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove DelItem (7.46 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove DelItemCheck and ItemSafeToDelete (15.65 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove no longer needed subs related to default values (19.41 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix CancelReceipt.t (4.09 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix failing tests (1.49 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Getting less item-level_itypes warnings (?) (1.81 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix AutomaticItemModificationByAge.t (790 bytes, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: update permanent_location if location has been updated (2.94 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix items.cn_source vs cn_source (1.93 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: build cn_sort only if needed (1.25 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace recently added occurrences (3.64 KB, patch)
2019-12-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: cn_sort needs to be updated if cn_source changed (928 bytes, patch)
2019-12-02 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete (1005 bytes, patch)
2019-12-10 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace SearchItemsByField by SearchItems (2.31 KB, patch)
2020-01-24 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Remove uneeded SearchItemsByField subroutine (1.35 KB, patch)
2020-01-24 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: fix for testbuilder - set default value items.damaged=0 (811 bytes, patch)
2020-01-24 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Replace AddItem calls with Koha::Item->store (79.25 KB, patch)
2020-01-24 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Remove _set_defaults_for_add (2.32 KB, patch)
2020-01-24 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Replace _set_derived_columns_for_add (2.03 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: AddItem removing (5.10 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Replace ModItem with Koha::Item->store (51.23 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: remove trivial UpdateHoldingbranch sub (1.66 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Move plugin hook (3.03 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort (4.01 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved (2.08 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Remove _mod_item_dates (1.54 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Remove ModItem (2.90 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Remove DelItem (7.45 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Remove DelItemCheck and ItemSafeToDelete (15.63 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Remove no longer needed subs related to default values (19.39 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Fix CancelReceipt.t (4.09 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Fix failing tests (1.49 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Getting less item-level_itypes warnings (?) (1.81 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Fix AutomaticItemModificationByAge.t (790 bytes, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: update permanent_location if location has been updated (2.94 KB, patch)
2020-01-24 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Fix items.cn_source vs cn_source (1.93 KB, patch)
2020-01-24 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: build cn_sort only if needed (1.25 KB, patch)
2020-01-24 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Replace recently added occurrences (3.62 KB, patch)
2020-01-24 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: cn_sort needs to be updated if cn_source changed (928 bytes, patch)
2020-01-24 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete (1005 bytes, patch)
2020-01-24 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23463: Replace SearchItemsByField by SearchItems (2.40 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove uneeded SearchItemsByField subroutine (1.44 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: fix for testbuilder - set default value items.damaged=0 (898 bytes, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace AddItem calls with Koha::Item->store (77.51 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove _set_defaults_for_add (2.46 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace _set_derived_columns_for_add (2.17 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: AddItem removing (5.24 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace ModItem with Koha::Item->store (49.60 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: remove trivial UpdateHoldingbranch sub (1.80 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Move plugin hook (3.17 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort (4.15 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved (2.22 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove _mod_item_dates (1.67 KB, patch)
2020-03-20 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove ModItem (3.05 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove DelItem (7.59 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove DelItemCheck and ItemSafeToDelete (16.06 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove no longer needed subs related to default values (19.53 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix CancelReceipt.t (4.23 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix failing tests (1.63 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Getting less item-level_itypes warnings (?) (1.95 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix AutomaticItemModificationByAge.t (933 bytes, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: update permanent_location if location has been updated (3.08 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix items.cn_source vs cn_source (2.07 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: build cn_sort only if needed (1.39 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace recently added occurrences (3.76 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: cn_sort needs to be updated if cn_source changed (1.04 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete (1.77 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix timestamp default value (1.42 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Move DelItem.t and Items_DelItemCheck.t tests to a Koha/Item.t subtest (9.54 KB, patch)
2020-03-20 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up) Fix timestamp default value (2.18 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix safe_return return value in deletion tools (1.65 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix tests (6.28 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix IsItemIssued.t (1.19 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix cn_source for new item (2.28 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix XSLT.t (1.64 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace new occurrence of AddItem (1.52 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Use new method Koha::Object->set_or_blank (7.40 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up 2) Fix timestamp default value (1.01 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix items.cn_sort vs cn_sort (1.96 KB, patch)
2020-03-20 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace SearchItemsByField by SearchItems (2.37 KB, patch)
2020-03-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove uneeded SearchItemsByField subroutine (1.41 KB, patch)
2020-03-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: fix for testbuilder - set default value items.damaged=0 (868 bytes, patch)
2020-03-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace AddItem calls with Koha::Item->store (77.49 KB, patch)
2020-03-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove _set_defaults_for_add (2.43 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace _set_derived_columns_for_add (2.15 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: AddItem removing (5.21 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace ModItem with Koha::Item->store (49.58 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: remove trivial UpdateHoldingbranch sub (1.77 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Move plugin hook (3.14 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: _set_derived_columns_for_mod was only used for cn_sort (4.12 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: _do_column_fixes_for_mod dealt with integer location and timestamp moved (2.19 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove _mod_item_dates (1.65 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove ModItem (3.02 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove DelItem (7.56 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove DelItemCheck and ItemSafeToDelete (16.03 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove no longer needed subs related to default values (19.50 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix CancelReceipt.t (4.20 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix failing tests (1.60 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Getting less item-level_itypes warnings (?) (1.92 KB, patch)
2020-03-20 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix AutomaticItemModificationByAge.t (903 bytes, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: update permanent_location if location has been updated (3.05 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix items.cn_source vs cn_source (2.04 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: build cn_sort only if needed (1.36 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace recently added occurrences (3.73 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: cn_sort needs to be updated if cn_source changed (1.02 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up) Remove DelItemCheck and ItemSafeToDelete (1.74 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix timestamp default value (1.39 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Move DelItem.t and Items_DelItemCheck.t tests to a Koha/Item.t subtest (9.51 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up) Fix timestamp default value (2.15 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix safe_return return value in deletion tools (1.62 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix tests (6.20 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix IsItemIssued.t (1.16 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix cn_source for new item (2.25 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix XSLT.t (1.62 KB, patch)
2020-03-20 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace new occurrence of AddItem (1.51 KB, patch)
2020-03-20 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Use new method Koha::Object->set_or_blank (7.39 KB, patch)
2020-03-20 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up 2) Fix timestamp default value (1019 bytes, patch)
2020-03-20 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix items.cn_sort vs cn_sort (1.95 KB, patch)
2020-03-20 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Replace a new occurrence of ModItem (1.12 KB, patch)
2020-03-23 10:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix CourseReserves/CourseItems.t (975 bytes, patch)
2020-03-23 10:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (RM follow-up) Add try block around Illrequest item creation (1.17 KB, patch)
2020-03-23 10:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23463: Fix wrong variable name (1.17 KB, patch)
2020-03-23 10:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Fix selenium tests (highlight a bug in date management) (2.17 KB, patch)
2020-03-23 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up 3) Fix timestamp default value (2.58 KB, patch)
2020-03-23 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: (follow-up 3) Fix timestamp default value (2.34 KB, patch)
2020-03-30 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23463: Remove double store call (920 bytes, patch)
2020-04-17 08:42 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 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