Bug 21206 - C4::Items - Remove GetItem
Summary: C4::Items - Remove GetItem
Status: RESOLVED FIXED
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: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 21184 21205
Blocks: 18252 21252 21975 22877 22675 22891
  Show dependency treegraph
 
Reported: 2018-08-10 16:02 UTC by Jonathan Druart
Modified: 2019-07-08 16:25 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Part of the ongoing effort to improve the maintainability of our codebase. This enhamcement removes the `GetItem` method from C4::Items and replaces any existing occurrences with `Koha::Items->search()`.
Version(s) released in:


Attachments
Bug 21206: Replace C4::Items::GetItem (116.11 KB, patch)
2018-08-10 16:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Remove GetItem (2.32 KB, patch)
2018-08-10 16:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Fix Returns.t (1.26 KB, patch)
2018-08-10 16:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Replace C4::Items::GetItem (116.20 KB, patch)
2018-08-22 01:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Remove GetItem (2.32 KB, patch)
2018-08-22 01:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Fix Returns.t (1.26 KB, patch)
2018-08-22 01:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Replace C4::Items::GetItem (116.29 KB, patch)
2018-08-24 19:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Remove GetItem (2.32 KB, patch)
2018-08-24 19:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Fix Returns.t (1.26 KB, patch)
2018-08-24 19:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Replace C4::Items::GetItem (116.12 KB, patch)
2018-12-04 12:07 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21206: Remove GetItem (2.38 KB, patch)
2018-12-04 12:07 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21206: Fix Returns.t (1.31 KB, patch)
2018-12-04 12:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21206: (follow-up) Fix Circulation.t (3.09 KB, patch)
2018-12-04 12:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21206: Replace C4::Items::GetItem (116.19 KB, patch)
2019-01-15 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: Remove GetItem (2.38 KB, patch)
2019-01-15 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: Fix Returns.t (1.31 KB, patch)
2019-01-15 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: (follow-up) Fix Circulation.t (3.09 KB, patch)
2019-01-15 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: (QA follow-up) Rebase problem and leftover mocked GetItem (3.11 KB, patch)
2019-01-15 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: Replace C4::Items::GetItem (116.24 KB, patch)
2019-01-18 10:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: Remove GetItem (2.43 KB, patch)
2019-01-18 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: Fix Returns.t (1.37 KB, patch)
2019-01-18 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: (follow-up) Fix Circulation.t (3.14 KB, patch)
2019-01-18 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: (QA follow-up) Rebase problem and leftover mocked GetItem (3.11 KB, patch)
2019-01-18 10:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21206: Replace C4::Items::GetItem (115.03 KB, patch)
2019-02-15 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Remove GetItem (2.43 KB, patch)
2019-02-15 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Fix Returns.t (1.37 KB, patch)
2019-02-15 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: (follow-up) Fix Circulation.t (3.14 KB, patch)
2019-02-15 13:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: (QA follow-up) Rebase problem and leftover mocked GetItem (3.11 KB, patch)
2019-02-15 13:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21206: Fix SwitchOnSiteCheckouts.t (975 bytes, patch)
2019-02-15 13:40 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 2018-08-10 16:02:36 UTC
Let's use Koha::Items!
Comment 1 Jonathan Druart 2018-08-10 16:06:18 UTC
Created attachment 77699 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Note: This is here for information purpose, feel free to test it if you
wan to play with it.

TODO: C4::Reserves::_get_itype is not longer in use

No more GetItem must be returned by:
git grep GetItem|grep -v GetItemsAvailableToFillHoldRequestsForBib|grep
-v GetItemsForInventory|grep -v GetItemsInfo|grep -v
GetItemsLocationInfo|grep -v GetItemsInCollection|grep -v
GetItemCourseReservesInfo|grep -v GetItemnumbersFromOrder|grep -v
GetItemSearchField|grep -v GetItemTypesCategorized|grep -v
GetItemNumbersFromImportBatch|cut -d':' -f1|sort|uniq
Comment 2 Jonathan Druart 2018-08-10 16:06:22 UTC
Created attachment 77700 [details] [review]
Bug 21206: Remove GetItem
Comment 3 Jonathan Druart 2018-08-10 16:06:26 UTC
Created attachment 77701 [details] [review]
Bug 21206: Fix Returns.t

we are calling several times effective_itemtype
Must be fixed later
Comment 4 Jonathan Druart 2018-08-10 16:07:51 UTC
The whole dependency tree has been pushed to https://gitlab.com/joubu/Koha/commits/bug_21206
Comment 5 Jonathan Druart 2018-08-10 16:08:37 UTC
This is ready for testing, no test plan provided.
Comment 6 Josef Moravec 2018-08-21 14:49:54 UTC
Comment on attachment 77699 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Review of attachment 77699 [details] [review]:
-----------------------------------------------------------------

::: C4/ILSDI/Services.pm
@@ +615,5 @@
>      # Add renewal if possible
>      my @renewal = CanBookBeRenewed( $borrowernumber, $itemnumber );
>      if ( $renewal[0] ) { AddRenewal( $borrowernumber, $itemnumber ); }
>  
> +    my $issue = $item ? $item->checkout : return; # FIXME should be handled

This does not return....

::: labels/label-edit-batch.pl
@@ +87,4 @@
>          my @numbers_list = split /\n/, $number_list; # Entries are effectively passed in as a <cr> separated list
>          foreach my $number (@numbers_list) {
>              $number =~ s/\r$//; # strip any naughty return chars
> +            if( $number_type eq "itemnumber" && Koha::ITems->find($number) ) {

ITems -> Items
Comment 7 Josef Moravec 2018-08-21 14:51:14 UTC
Also QA tool fails:

 FAIL	acqui/orderreceive.pl
   FAIL	  valid
		Global symbol "@items" requires explicit package name 
		acqui/orderreceive.pl had compilation errors.

 FAIL	catalogue/getitem-ajax.pl
   FAIL	  valid
		catalogue/getitem-ajax.pl had compilation errors.
		Global symbol "$item_unblessed" requires explicit package name 

 FAIL	misc/recreateIssueStatistics.pl
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 124)

 FAIL	t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
   FAIL	  valid
		syntax error
Comment 8 Jonathan Druart 2018-08-22 01:06:23 UTC
Created attachment 78080 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Note: This is here for information purpose, feel free to test it if you
wan to play with it.

TODO: C4::Reserves::_get_itype is not longer in use

No more GetItem must be returned by:
git grep GetItem|grep -v GetItemsAvailableToFillHoldRequestsForBib|grep
-v GetItemsForInventory|grep -v GetItemsInfo|grep -v
GetItemsLocationInfo|grep -v GetItemsInCollection|grep -v
GetItemCourseReservesInfo|grep -v GetItemnumbersFromOrder|grep -v
GetItemSearchField|grep -v GetItemTypesCategorized|grep -v
GetItemNumbersFromImportBatch|cut -d':' -f1|sort|uniq
Comment 9 Jonathan Druart 2018-08-22 01:06:28 UTC
Created attachment 78081 [details] [review]
Bug 21206: Remove GetItem
Comment 10 Jonathan Druart 2018-08-22 01:06:32 UTC
Created attachment 78082 [details] [review]
Bug 21206: Fix Returns.t

we are calling several times effective_itemtype
Must be fixed later
Comment 11 Josef Moravec 2018-08-22 09:22:08 UTC
Comment on attachment 78080 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Review of attachment 78080 [details] [review]:
-----------------------------------------------------------------

::: C4/ILSDI/Services.pm
@@ +614,5 @@
>      if ( $renewal[0] ) { AddRenewal( $borrowernumber, $itemnumber ); }
>  
> +    return unless $item; # FIXME should be handled
> +
> +    my $issue = $item->checkout;

I see still little problem here: 
If item exists and is not checked out -> boom
The check and return on item is done on line 610 here, so here should be "return unless $issue;"
Comment 12 Jonathan Druart 2018-08-24 19:03:49 UTC
Created attachment 78153 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Note: This is here for information purpose, feel free to test it if you
wan to play with it.

TODO: C4::Reserves::_get_itype is not longer in use

No more GetItem must be returned by:
git grep GetItem|grep -v GetItemsAvailableToFillHoldRequestsForBib|grep
-v GetItemsForInventory|grep -v GetItemsInfo|grep -v
GetItemsLocationInfo|grep -v GetItemsInCollection|grep -v
GetItemCourseReservesInfo|grep -v GetItemnumbersFromOrder|grep -v
GetItemSearchField|grep -v GetItemTypesCategorized|grep -v
GetItemNumbersFromImportBatch|cut -d':' -f1|sort|uniq
Comment 13 Jonathan Druart 2018-08-24 19:03:53 UTC
Created attachment 78154 [details] [review]
Bug 21206: Remove GetItem
Comment 14 Jonathan Druart 2018-08-24 19:03:57 UTC
Created attachment 78155 [details] [review]
Bug 21206: Fix Returns.t

we are calling several times effective_itemtype
Must be fixed later
Comment 15 Josef Moravec 2018-12-04 12:07:54 UTC
Created attachment 82831 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Note: This is here for information purpose, feel free to test it if you
wan to play with it.

TODO: C4::Reserves::_get_itype is not longer in use

No more GetItem must be returned by:
git grep GetItem|grep -v GetItemsAvailableToFillHoldRequestsForBib|grep
-v GetItemsForInventory|grep -v GetItemsInfo|grep -v
GetItemsLocationInfo|grep -v GetItemsInCollection|grep -v
GetItemCourseReservesInfo|grep -v GetItemnumbersFromOrder|grep -v
GetItemSearchField|grep -v GetItemTypesCategorized|grep -v
GetItemNumbersFromImportBatch|cut -d':' -f1|sort|uniq

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Josef Moravec 2018-12-04 12:07:58 UTC
Created attachment 82832 [details] [review]
Bug 21206: Remove GetItem

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Josef Moravec 2018-12-04 12:08:02 UTC
Created attachment 82833 [details] [review]
Bug 21206: Fix Returns.t

we are calling several times effective_itemtype
Must be fixed later

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Josef Moravec 2018-12-04 12:08:06 UTC
Created attachment 82834 [details] [review]
Bug 21206: (follow-up) Fix Circulation.t

Test plan:
prove t/db_dependent/Circulation.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Tomás Cohen Arazi 2019-01-15 19:37:40 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2019-01-15 19:37:44 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2019-01-15 19:37:48 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2019-01-15 19:37:52 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2019-01-15 19:37:56 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2019-01-16 18:55:11 UTC
I reviewed the patches, everything looks good. I fixed minor issues too.
Will mark PQA once I cover all the changes on the UI too.
Comment 25 Tomás Cohen Arazi 2019-01-18 10:52:55 UTC
Created attachment 84184 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Note: This is here for information purpose, feel free to test it if you
wan to play with it.

TODO: C4::Reserves::_get_itype is not longer in use

No more GetItem must be returned by:
git grep GetItem|grep -v GetItemsAvailableToFillHoldRequestsForBib|grep
-v GetItemsForInventory|grep -v GetItemsInfo|grep -v
GetItemsLocationInfo|grep -v GetItemsInCollection|grep -v
GetItemCourseReservesInfo|grep -v GetItemnumbersFromOrder|grep -v
GetItemSearchField|grep -v GetItemTypesCategorized|grep -v
GetItemNumbersFromImportBatch|cut -d':' -f1|sort|uniq

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2019-01-18 10:53:03 UTC
Created attachment 84185 [details] [review]
Bug 21206: Remove GetItem

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2019-01-18 10:53:15 UTC
Created attachment 84186 [details] [review]
Bug 21206: Fix Returns.t

we are calling several times effective_itemtype
Must be fixed later

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2019-01-18 10:53:23 UTC
Created attachment 84187 [details] [review]
Bug 21206: (follow-up) Fix Circulation.t

Test plan:
prove t/db_dependent/Circulation.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2019-01-18 10:53:30 UTC
Created attachment 84188 [details] [review]
Bug 21206: (QA follow-up) Rebase problem and leftover mocked GetItem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Nick Clemens 2019-01-28 14:41:03 UTC
Needs a rebase after removal of dropbox mode
Comment 31 Jonathan Druart 2019-02-15 13:39:41 UTC
Created attachment 85163 [details] [review]
Bug 21206: Replace C4::Items::GetItem

Note: This is here for information purpose, feel free to test it if you
wan to play with it.

TODO: C4::Reserves::_get_itype is not longer in use

No more GetItem must be returned by:
git grep GetItem|grep -v GetItemsAvailableToFillHoldRequestsForBib|grep
-v GetItemsForInventory|grep -v GetItemsInfo|grep -v
GetItemsLocationInfo|grep -v GetItemsInCollection|grep -v
GetItemCourseReservesInfo|grep -v GetItemnumbersFromOrder|grep -v
GetItemSearchField|grep -v GetItemTypesCategorized|grep -v
GetItemNumbersFromImportBatch|cut -d':' -f1|sort|uniq

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Jonathan Druart 2019-02-15 13:39:51 UTC
Created attachment 85164 [details] [review]
Bug 21206: Remove GetItem

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Jonathan Druart 2019-02-15 13:39:56 UTC
Created attachment 85165 [details] [review]
Bug 21206: Fix Returns.t

we are calling several times effective_itemtype
Must be fixed later

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Jonathan Druart 2019-02-15 13:40:05 UTC
Created attachment 85166 [details] [review]
Bug 21206: (follow-up) Fix Circulation.t

Test plan:
prove t/db_dependent/Circulation.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Jonathan Druart 2019-02-15 13:40:11 UTC
Created attachment 85167 [details] [review]
Bug 21206: (QA follow-up) Rebase problem and leftover mocked GetItem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Jonathan Druart 2019-02-15 13:40:18 UTC
Created attachment 85168 [details] [review]
Bug 21206: Fix SwitchOnSiteCheckouts.t

Fix conflict with
  commit f8544ba5799c457057d42aefc63c88e15a08b096
  Bug 21999: Move attributes to a variable to not dup them

Thanks tests!
Comment 37 Nick Clemens 2019-02-26 14:40:19 UTC
Awesome work all!

Pushed to master for 19.05
Comment 38 Martin Renvoize 2019-03-01 11:16:12 UTC
Enhancement will not be backported to 18.11.x series.