Bug 27272

Summary: Move C4::Items::GetItemsInfo to Koha namespace
Product: Koha Reporter: Aleisha Amohia <aleisha>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: enhancement    
Priority: P5 - low CC: dcook, fridolin.somers, jonathan.druart, joonas.kylmala, m.de.rooy, martin.renvoize, nick, stalkernoid, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26587
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Another step on the old code refactoring. At this time, related to item information display. There are several DB queries that get avoided with this change as well, leading to more performant processing of requests.
Version(s) released in:
22.11.00
Bug Depends on: 29697, 30885, 30886, 31308, 31309, 31310, 31311, 31312, 31313, 31314, 31315, 31316, 31317, 31318, 31319, 31320, 31321    
Bug Blocks: 32445, 32555    
Attachments: Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove type=
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: catalogue/detail
Bug 27272: Remove type=
Bug 27272: basket/basket
Bug 27272: basket/sendbasket
Bug 27272: catalogue/imageviewer
Bug 27272: labels/label-item-search
Bug 27272: misc/migration_tools/rebuild_zebra.pl
Bug 27272: opac-detail
Bug 27272: already_reserved never set (was set before 14695)
Bug 27272: opac-reserve
Bug 27272: moredetail
Bug 27272: opac-sendbasket
Bug 27272: opac-tags
Bug 27272: serials/routing-preview
Bug 27272: tags/list.pl
Bug 27272: opac-sendshelf
Bug 27272: virtualshelves/sendshelf.pl shelves.pl
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove type= from detail.pl
Bug 27272: Remove GetItemsInfo from basket/basket
Bug 27272: Remove GetItemsInfo from basket/sendbasket
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Bug 27272: Remove GetItemsInfo labels/label-item-search
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl
Bug 27272: Remove GetItemsInfo from opac-detail
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Bug 27272: Remove GetItemsInfo import from opac-tags
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Bug 27272: Remove GetItemsInfo from tags/list.pl
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Add search_ordered sub to order items with extra conditions
Bug 27272: Allow to pass params and attributes to search_ordered
Bug 27272: Add tests for search_ordered
Bug 27272: Call search_ordered when GetItemsInfo was called before
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Add search_ordered sub to order items with extra conditions
Bug 27272: Allow to pass params and attributes to search_ordered
Bug 27272: Add tests for search_ordered
Bug 27272: Call search_ordered when GetItemsInfo was called before
Bug 27272: Add search_ordered sub to order items with extra conditions
Bug 27272: Allow to pass params and attributes to search_ordered
Bug 27272: Add tests for search_ordered
Bug 27272: Call search_ordered when GetItemsInfo was called before
Bug 27272: Fix availability - OPAC
Bug 27272: Fix library display on routing slip - to squash
Bug 27272: Remove type= from detail.pl
Bug 27272: Remove GetItemsInfo from basket/basket
Bug 27272: Remove GetItemsInfo from basket/sendbasket
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Bug 27272: Remove GetItemsInfo labels/label-item-search
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl
Bug 27272: Remove GetItemsInfo from opac-detail
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Bug 27272: Remove GetItemsInfo import from opac-tags
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Bug 27272: Remove GetItemsInfo from tags/list.pl
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Add search_ordered sub to order items with extra conditions
Bug 27272: Allow to pass params and attributes to search_ordered
Bug 27272: Add tests for search_ordered
Bug 27272: Call search_ordered when GetItemsInfo was called before
Bug 27272: Fix availability - OPAC
Bug 27272: Fix library display on routing slip - to squash
Bug 27272: Remove type= from detail.pl
Bug 27272: Remove GetItemsInfo from basket/basket
Bug 27272: Remove GetItemsInfo from basket/sendbasket
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Bug 27272: Remove GetItemsInfo labels/label-item-search
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl
Bug 27272: Remove GetItemsInfo from opac-detail
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Bug 27272: Remove GetItemsInfo import from opac-tags
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Bug 27272: Remove GetItemsInfo from tags/list.pl
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Add search_ordered sub to order items with extra conditions
Bug 27272: Allow to pass params and attributes to search_ordered
Bug 27272: Add tests for search_ordered
Bug 27272: Call search_ordered when GetItemsInfo was called before
Bug 27272: Fix availability - OPAC
Bug 27272: Fix library display on routing slip - to squash
Bug 27272: Remove type= from detail.pl
Bug 27272: Remove GetItemsInfo from basket/basket
Bug 27272: Remove GetItemsInfo from basket/sendbasket
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Bug 27272: Remove GetItemsInfo labels/label-item-search
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl
Bug 27272: Remove GetItemsInfo from opac-detail
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Bug 27272: Remove GetItemsInfo import from opac-tags
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Bug 27272: Remove GetItemsInfo from tags/list.pl
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Add search_ordered sub to order items with extra conditions
Bug 27272: Allow to pass params and attributes to search_ordered
Bug 27272: Add tests for search_ordered
Bug 27272: Call search_ordered when GetItemsInfo was called before
Bug 27272: Fix availability - OPAC
Bug 27272: Fix library display on routing slip - to squash
Bug 27272: Remove type= from detail.pl
Bug 27272: Remove GetItemsInfo from basket/basket
Bug 27272: Remove GetItemsInfo from basket/sendbasket
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Bug 27272: Remove GetItemsInfo labels/label-item-search
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl
Bug 27272: Remove GetItemsInfo from opac-detail
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Bug 27272: Remove GetItemsInfo import from opac-tags
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Bug 27272: Remove GetItemsInfo from tags/list.pl
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Add search_ordered sub to order items with extra conditions
Bug 27272: Allow to pass params and attributes to search_ordered
Bug 27272: Add tests for search_ordered
Bug 27272: Call search_ordered when GetItemsInfo was called before
Bug 27272: Fix availability - OPAC
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo

Description Aleisha Amohia 2020-12-18 02:58:09 UTC

    
Comment 1 Tomás Cohen Arazi 2020-12-18 03:08:05 UTC
This method should just be removed and a mix of things used instead:

my $biblio = Koha::Biblios->find($biblio_id, {prefetch =>['items', 'biblioitem'] });
my @items = $biblio->items;
my $biblioitem = $biblio->biblioitem;

^^^ could do most of it.
Comment 2 Aleisha Amohia 2020-12-20 19:58:01 UTC
Good tip, Tomás! I'll do that when I begin this patch.
Comment 3 Jonathan Druart 2021-12-03 15:49:07 UTC
Created attachment 128230 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Comment 4 Jonathan Druart 2021-12-03 15:49:11 UTC
Created attachment 128231 [details] [review]
Bug 27272: Remove type=

This is not related to anything known.. was it "itype" at some point?
`git log -p` will tell you that it's there for a looong time already
Comment 5 Jonathan Druart 2021-12-03 15:50:10 UTC
This is just a start! I am willing to work on that (ie. completely remove this silly subroutine). Just tell me you are ready to test the patches and I will continue!
Comment 6 Jonathan Druart 2022-06-02 07:54:58 UTC
(In reply to Jonathan Druart from comment #5)
> This is just a start! I am willing to work on that (ie. completely remove
> this silly subroutine). Just tell me you are ready to test the patches and I
> will continue!

Still nobody interested in this?
Comment 7 Marcel de Rooy 2022-06-02 08:20:03 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Jonathan Druart from comment #5)
> > This is just a start! I am willing to work on that (ie. completely remove
> > this silly subroutine). Just tell me you are ready to test the patches and I
> > will continue!
> 
> Still nobody interested in this?

Go on !
Comment 8 Tomás Cohen Arazi 2022-06-02 12:11:56 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Jonathan Druart from comment #5)
> > This is just a start! I am willing to work on that (ie. completely remove
> > this silly subroutine). Just tell me you are ready to test the patches and I
> > will continue!
> 
> Still nobody interested in this?

I'd say go for it. Next step is to just pass the items to the templates I guess?
Comment 9 Jonathan Druart 2022-06-03 06:49:05 UTC
Created attachment 135640 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Comment 10 Jonathan Druart 2022-06-03 06:49:11 UTC
Created attachment 135641 [details] [review]
Bug 27272: catalogue/detail
Comment 11 Jonathan Druart 2022-06-03 06:49:16 UTC
Created attachment 135642 [details] [review]
Bug 27272: Remove type=

This is not related to anything known.. was it "itype" at some point?
`git log -p` will tell you that it's there for a looong time already
Comment 12 Jonathan Druart 2022-06-03 06:49:20 UTC
Created attachment 135643 [details] [review]
Bug 27272: basket/basket
Comment 13 Jonathan Druart 2022-06-03 06:49:25 UTC
Created attachment 135644 [details] [review]
Bug 27272: basket/sendbasket

Why are we display with $raw?
Why are we display the location code instead of the AV's lib?
Comment 14 Jonathan Druart 2022-06-03 06:49:29 UTC
Created attachment 135645 [details] [review]
Bug 27272: catalogue/imageviewer
Comment 15 Jonathan Druart 2022-06-03 06:49:34 UTC
Created attachment 135646 [details] [review]
Bug 27272: labels/label-item-search
Comment 16 Jonathan Druart 2022-06-03 06:49:38 UTC
Created attachment 135647 [details] [review]
Bug 27272: misc/migration_tools/rebuild_zebra.pl

Hum... Item2Marc ok here?
Comment 17 Jonathan Druart 2022-06-03 06:49:43 UTC
Created attachment 135648 [details] [review]
Bug 27272: opac-detail
Comment 18 Jonathan Druart 2022-06-03 06:49:48 UTC
Created attachment 135649 [details] [review]
Bug 27272: already_reserved never set (was set before 14695)
Comment 19 Jonathan Druart 2022-06-03 06:49:53 UTC
Created attachment 135650 [details] [review]
Bug 27272: opac-reserve
Comment 20 Jonathan Druart 2022-06-03 06:49:57 UTC
Created attachment 135651 [details] [review]
Bug 27272: moredetail
Comment 21 Jonathan Druart 2022-06-03 06:50:02 UTC
Created attachment 135652 [details] [review]
Bug 27272: opac-sendbasket
Comment 22 Jonathan Druart 2022-06-03 06:50:07 UTC
Created attachment 135653 [details] [review]
Bug 27272: opac-tags
Comment 23 Jonathan Druart 2022-06-03 06:50:12 UTC
Created attachment 135654 [details] [review]
Bug 27272: serials/routing-preview
Comment 24 Jonathan Druart 2022-06-03 06:50:17 UTC
Created attachment 135655 [details] [review]
Bug 27272: tags/list.pl
Comment 25 Jonathan Druart 2022-06-03 06:50:22 UTC
Created attachment 135656 [details] [review]
Bug 27272: opac-sendshelf
Comment 26 Jonathan Druart 2022-06-03 06:50:28 UTC
Created attachment 135657 [details] [review]
Bug 27272: virtualshelves/sendshelf.pl shelves.pl
Comment 27 Jonathan Druart 2022-06-03 06:50:33 UTC
Created attachment 135658 [details] [review]
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Comment 28 Jonathan Druart 2022-06-03 06:51:50 UTC
Created attachment 135659 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Comment 29 Jonathan Druart 2022-06-03 06:52:22 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> I'd say go for it. Next step is to just pass the items to the templates I
> guess?

Yes "just"... :D

Here are the patches. What next now? Do I open one bug report per patch? IMO we should test, review and push all of them at the same time, so no need for 19 new bug reports, but I do whatever you prefer.

It's crazy the number of bugs I found and fixed.
You will find some FIXMEs I have added (some that need attention now, others that are for later). The code is way cleaner now and, I think, more efficient (perf) as well.

I haven't tested everything, and tests need test plans, but basically 80% of the work is done.

There is much more to do in these scripts, we could simplify even more. But first, this.
Comment 30 Jonathan Druart 2022-06-03 06:55:44 UTC
30 files changed, 495 insertions(+), 893 deletions(-)

Please understand that we need the patches pushed ASAP once they are ready for testing. I will NOT rebase them several months.
Comment 31 Jonathan Druart 2022-06-03 06:57:29 UTC
(In reply to Jonathan Druart from comment #29)
> It's crazy the number of bugs I found and fixed.

And some related to items visibility at the OPAC.
Comment 32 Jonathan Druart 2022-06-03 07:06:32 UTC
Ha, and question, see catalogue/detail.pl
We are using an order_by that was initially used in GetItemsInfo. Should it be the default order by for all item searches?
I am also stuck trying to replicate the previous order_by behaviour for serials (see the FIXME there)
Comment 33 Marcel de Rooy 2022-06-03 07:25:26 UTC
(In reply to Jonathan Druart from comment #29)

> I haven't tested everything, and tests need test plans, but basically 80% of
> the work is done.

If I can save 80% of my records, we're happy :)

Great work. We need some priority lane for these things.
Comment 34 Tomás Cohen Arazi 2022-06-03 11:33:30 UTC
(In reply to Marcel de Rooy from comment #33)
>
> Great work. We need some priority lane for these things.

This is a priority on this cycle.
Comment 35 Nick Clemens 2022-06-03 18:53:44 UTC
1 - View details.pl for a record - confirm barcode link takes you to correct page and other info displays correctly
2 - Add several records to a cart - find a mix with and without items
3 - Confirm the records display correctly the location and branchcodes in the cart
4 - Setup SMTP using your email. For gmail:
    Create app password:
     https://support.google.com/accounts/answer/185833?hl=en
    Add gmail server to koha-conf
 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>465</port>
    <timeout>60</timeout>
    <ssl_mode>SSL</ssl_mode>
    <user_name>kohadev@cool.com</user_name>
    <password>expialodotious</password>
    <debug>0</debug>
 </smtp_server>
5 - Send cart to yourself and verify branches show name correctly
6 - Enable LocalCoverImages system poreference
7 - Upload an image to a record (from details page, images tab)
8 - Click the image in the tab and confirm it is shown
9 - Set searchenginge system preference to zebra
10 - sudo koha-shell kohadev
11 - perl misc/migration_tools/rebuild_zebra.pl -b -v
12 - confirm searchign for item fields works
13 - Find/create a subscription (Serials->New subscription)
     Ensure in subscription details that:
       Create an item record when receiving this serial 
     is marked
14 - Receive/find an issue with an item, view in opac
15 - Confirm "Vol info" column correctly populated, and other item info displays as epxectd
16 - Place a reserve on OPAC, make sure to find/create a record with some holdable and unholdable items, and some items on loan/checked out
17 - Ensure items show correctly in form and hold can be placed
18 - In staff client, view the items tab for a record.
19 - Confirm items show correctly and links are preserved
20 - Confirm item statuses show correctly, including issued items, transit items, not for loan
21 - Repeat cart tests on the opac, ensure sent cart displays correcttly
22 - On staff client, return to subscription used previously
23 - From subscription details click 'Create routing list'
24 - Add some patrons, and a note, save
25 - Click 'save and preview' and ensure item displays correctly
26 - In the opac, add soem tags to a record (TagsEnabled system preference must be on, add from details page)
27 - In staff client browse to tools->tags, approve tag, click on term in the approved list
28 - Confirm items show correctly on the list
29 - Create a list on staff client (add items from search to new list)
30 - Send the list (Lists->clkick list name-> Send list)
31 - Confirm it is ofrmatted correctly
32 - Send list from OPAC and confirm it displays correctlyin email as well
Comment 36 Nick Clemens 2022-06-03 18:55:15 UTC
Rough test plan - Jonathan, at one point you switch from item.onloan to item.checkout - do we need this extra fetch? onloan both tells us if checked out, and provides due date
Comment 37 Katrin Fischer 2022-06-03 21:10:43 UTC
(In reply to Nick Clemens from comment #36)
> Rough test plan - Jonathan, at one point you switch from item.onloan to
> item.checkout - do we need this extra fetch? onloan both tells us if checked
> out, and provides due date

Maybe because onloan is only the date, but doesn't have the time?
Comment 38 Jonathan Druart 2022-06-07 08:33:16 UTC
(In reply to Nick Clemens from comment #36)
> Rough test plan - Jonathan, at one point you switch from item.onloan to
> item.checkout - do we need this extra fetch? onloan both tells us if checked
> out, and provides due date

From opac-reserve:

-        my $issue = Koha::Checkouts->find( { itemnumber => $itemNum } );
-        if ( $issue ) {
-            $itemLoopIter->{dateDue} = output_pref({ dt => dt_from_string($issue->date_due, 'sql'), as_due_date => 1 });
-            $itemLoopIter->{onloan} = 'onloan';

So "onloan" was actually coming from item->checkout.

Also:
                                                                [% IF ( itemLoo.dateDue ) %]
-                                                                    <span class="checkedout">Due [% itemLoo.dateDue | html %]</span>
+                                                                [% IF ( itemLoo.checkout.date_due) %]
+                                                                    <span class="checkedout">Due [% itemLoo.checkout.date_due| $KohaDates as_due_date => 1 %]</span>

=> We need the date_due from issue->checkout anyway.

Thanks a lot for the test plan, Nick!
Comment 39 Jonathan Druart 2022-06-07 08:36:30 UTC
See the following remote branch for the patches rebased against master: https://gitlab.com/joubu/Koha/-/commits/bug_27272
Comment 40 Joonas Kylmälä 2022-06-12 11:16:23 UTC
I found some issues:

- Commit "Bug 27272: basket/basket" breaks opening the basket:

> Template process failed: file error - parse error - /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/basket/basket.tt line 235: unexpected token (ELSE)


- Commit "Bug 27272: basket/sendbasket" seems to break sending the basket via email (need to fix the aformentioned basket opening bug too to get here), it complained about uncovered tests.


- The patch "Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo" needs to come after all the calls have been removed.


- Most of the commit titles don't describe the change, please add the descriptions.


I will continue review after these issues has been addressed, and also the bugs in the dependency this bug relies on have been fixed.
Comment 41 Jonathan Druart 2022-06-13 08:14:51 UTC
(In reply to Joonas Kylmälä from comment #40)
> I found some issues:

Fixed! Remote branch adjusted.
Comment 42 Martin Renvoize 2022-06-23 11:04:55 UTC
(In reply to Jonathan Druart from comment #32)
> Ha, and question, see catalogue/detail.pl
> We are using an order_by that was initially used in GetItemsInfo. Should it
> be the default order by for all item searches?
> I am also stuck trying to replicate the previous order_by behaviour for
> serials (see the FIXME there)

I wonder if we should add ordering into the Koha::Biblio->items method?

I'm not sure how it would/could affect embedding/prefetching/joining/caching...

But it would be nice to consistently order.. binding on whether the bib is a serial or otherwise to get the 'right' ordering?
Comment 43 Jonathan Druart 2022-07-06 10:19:07 UTC
Remote branch rebased against master.
Comment 44 Jonathan Druart 2022-07-08 07:15:06 UTC
(In reply to Martin Renvoize from comment #42)
> (In reply to Jonathan Druart from comment #32)
> > Ha, and question, see catalogue/detail.pl
> > We are using an order_by that was initially used in GetItemsInfo. Should it
> > be the default order by for all item searches?
> > I am also stuck trying to replicate the previous order_by behaviour for
> > serials (see the FIXME there)
> 
> I wonder if we should add ordering into the Koha::Biblio->items method?
> 
> I'm not sure how it would/could affect
> embedding/prefetching/joining/caching...
> 
> But it would be nice to consistently order.. binding on whether the bib is a
> serial or otherwise to get the 'right' ordering?

Maybe we should have an explicit "search ordered" method?
Comment 45 Jonathan Druart 2022-07-13 06:30:53 UTC
Remote branch rebased against master.
Comment 46 Joonas Kylmälä 2022-07-18 17:59:21 UTC
Doesn't apply anymore against latest master.
Comment 47 Jonathan Druart 2022-07-19 07:00:08 UTC
Remote branch rebased against master.
Comment 48 Martin Renvoize 2022-07-20 14:17:31 UTC
I think I agreed to the idea of explicit search_ordered.. thus calling it chained to get the order one wants in each case..

Can you now actually pass ordering in as attributes to the relation accessor anyway too already?

  $biblio->items({},{ order_by => whatever });
Comment 49 Jonathan Druart 2022-07-20 14:38:05 UTC
Petro, are you working on this?
Comment 50 Peter Vashchuk 2022-07-21 10:10:12 UTC
Yes, I'm on it.
Comment 51 Jonathan Druart 2022-07-21 14:03:45 UTC
Created attachment 137966 [details] [review]
Bug 27272: Remove type= from detail.pl

This is not related to anything known.. was it "itype" at some point?
`git log -p` will tell you that it's there for a looong time already
Comment 52 Jonathan Druart 2022-07-21 14:03:50 UTC
Created attachment 137967 [details] [review]
Bug 27272: Remove GetItemsInfo from basket/basket
Comment 53 Jonathan Druart 2022-07-21 14:03:56 UTC
Created attachment 137968 [details] [review]
Bug 27272: Remove GetItemsInfo from basket/sendbasket

Why are we display with $raw?
Why are we display the location code instead of the AV's lib?
Comment 54 Jonathan Druart 2022-07-21 14:04:04 UTC
Created attachment 137969 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Comment 55 Jonathan Druart 2022-07-21 14:04:14 UTC
Created attachment 137970 [details] [review]
Bug 27272: Remove GetItemsInfo labels/label-item-search

It was commented anyway
Comment 56 Jonathan Druart 2022-07-21 14:04:22 UTC
Created attachment 137971 [details] [review]
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl

Hum... Item2Marc ok here?
Comment 57 Jonathan Druart 2022-07-21 14:04:28 UTC
Created attachment 137972 [details] [review]
Bug 27272: Remove GetItemsInfo from opac-detail
Comment 58 Jonathan Druart 2022-07-21 14:04:33 UTC
Created attachment 137973 [details] [review]
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl

It was never set (was set before 14695)
Comment 59 Jonathan Druart 2022-07-21 14:04:39 UTC
Created attachment 137974 [details] [review]
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Comment 60 Jonathan Druart 2022-07-21 14:04:45 UTC
Created attachment 137975 [details] [review]
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Comment 61 Jonathan Druart 2022-07-21 14:04:50 UTC
Created attachment 137976 [details] [review]
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Comment 62 Jonathan Druart 2022-07-21 14:04:55 UTC
Created attachment 137977 [details] [review]
Bug 27272: Remove GetItemsInfo import from opac-tags
Comment 63 Jonathan Druart 2022-07-21 14:05:06 UTC
Created attachment 137978 [details] [review]
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Comment 64 Jonathan Druart 2022-07-21 14:05:15 UTC
Created attachment 137979 [details] [review]
Bug 27272: Remove GetItemsInfo from tags/list.pl
Comment 65 Jonathan Druart 2022-07-21 14:05:21 UTC
Created attachment 137980 [details] [review]
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl

And GetItemsLocationInfo from shelves.pl
Comment 66 Jonathan Druart 2022-07-21 14:05:31 UTC
Created attachment 137981 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Comment 67 Jonathan Druart 2022-07-21 14:05:37 UTC
Created attachment 137982 [details] [review]
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Comment 68 Jonathan Druart 2022-07-21 14:06:10 UTC
Patches from remote branch have been attached here.
Comment 69 Peter Vashchuk 2022-07-25 23:45:50 UTC
Created attachment 138109 [details] [review]
Bug 27272: Add search_ordered sub to order items with extra conditions
Comment 70 Jonathan Druart 2022-07-29 10:25:51 UTC
I am writing tests for the last patch.
Comment 71 Jonathan Druart 2022-07-29 11:07:25 UTC
Created attachment 138296 [details] [review]
Bug 27272: Allow to pass params and attributes to search_ordered
Comment 72 Jonathan Druart 2022-07-29 11:07:31 UTC
Created attachment 138297 [details] [review]
Bug 27272: Add tests for search_ordered
Comment 73 Jonathan Druart 2022-07-29 11:07:37 UTC
Created attachment 138298 [details] [review]
Bug 27272: Call search_ordered when GetItemsInfo was called before
Comment 74 Jonathan Druart 2022-07-29 11:08:27 UTC
Back to NQA.

Thanks, Petro!
Comment 75 Jonathan Druart 2022-07-29 11:53:21 UTC
Created attachment 138303 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Comment 76 Jonathan Druart 2022-07-29 11:53:27 UTC
Created attachment 138304 [details] [review]
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Comment 77 Jonathan Druart 2022-07-29 11:53:33 UTC
Created attachment 138305 [details] [review]
Bug 27272: Add search_ordered sub to order items with extra conditions
Comment 78 Jonathan Druart 2022-07-29 11:53:39 UTC
Created attachment 138306 [details] [review]
Bug 27272: Allow to pass params and attributes to search_ordered
Comment 79 Jonathan Druart 2022-07-29 11:53:45 UTC
Created attachment 138307 [details] [review]
Bug 27272: Add tests for search_ordered
Comment 80 Jonathan Druart 2022-07-29 11:53:52 UTC
Created attachment 138308 [details] [review]
Bug 27272: Call search_ordered when GetItemsInfo was called before
Comment 81 Joonas Kylmälä 2022-07-30 15:24:10 UTC
Did some testing, no code review yet but noticed the following problems:

1. Going to a serial biblio's detail page fails on opac, e.g. page http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=29 on KTD.

> DBIx::Class::ResultSource::_resolve_join(): No such relationship serial on Item at /kohadevbox/koha/Koha/Objects.pm line 421

Same error on the detail.pl on the staff side.


2. Go to any biblio's OPAC detail page, checkout an item for that biblio and notice the Status column is not updated and it still shows "Available". Before applying these patches the status was "Checked out"

3. serials/routing-preview.pl doesn't display the branch name anymore.

Will continue testing after this, there are still some things I want to test on the biblio detail pages that I cannot due to the internal server errors.
Comment 82 Jonathan Druart 2022-08-01 07:12:16 UTC
Created attachment 138437 [details] [review]
Bug 27272: Add search_ordered sub to order items with extra conditions
Comment 83 Jonathan Druart 2022-08-01 07:12:21 UTC
Created attachment 138438 [details] [review]
Bug 27272: Allow to pass params and attributes to search_ordered
Comment 84 Jonathan Druart 2022-08-01 07:12:27 UTC
Created attachment 138439 [details] [review]
Bug 27272: Add tests for search_ordered
Comment 85 Jonathan Druart 2022-08-01 07:12:32 UTC
Created attachment 138440 [details] [review]
Bug 27272: Call search_ordered when GetItemsInfo was called before
Comment 86 Jonathan Druart 2022-08-01 07:12:37 UTC
Created attachment 138441 [details] [review]
Bug 27272: Fix availability - OPAC

Has to move some code to a method
Comment 87 Jonathan Druart 2022-08-01 07:12:43 UTC
Created attachment 138442 [details] [review]
Bug 27272: Fix library display on routing slip - to squash
Comment 88 Jonathan Druart 2022-08-01 07:15:05 UTC
(In reply to Joonas Kylmälä from comment #81)

Thanks, Joonas!

> Did some testing, no code review yet but noticed the following problems:
> 
> 1. Going to a serial biblio's detail page fails on opac, e.g. page
> http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=29 on KTD.
> 
> > DBIx::Class::ResultSource::_resolve_join(): No such relationship serial on Item at /kohadevbox/koha/Koha/Objects.pm line 421
> 
> Same error on the detail.pl on the staff side.

My tests were wrong but passing! It's fixed now.

> 2. Go to any biblio's OPAC detail page, checkout an item for that biblio and
> notice the Status column is not updated and it still shows "Available".
> Before applying these patches the status was "Checked out"

Should be fixed, but this area is a mess. Hard to keep things clean, the idea here is to not get worse. We should focus on that once this bug is pushed.

> 3. serials/routing-preview.pl doesn't display the branch name anymore.
> 
> Will continue testing after this, there are still some things I want to test
> on the biblio detail pages that I cannot due to the internal server errors.

Oops, fixed!
Comment 89 Victor Grousset/tuxayo 2022-08-03 23:38:04 UTC Comment hidden (obsolete)
Comment 90 Victor Grousset/tuxayo 2022-08-03 23:38:29 UTC Comment hidden (obsolete)
Comment 91 Victor Grousset/tuxayo 2022-08-03 23:38:50 UTC Comment hidden (obsolete)
Comment 92 Victor Grousset/tuxayo 2022-08-03 23:39:08 UTC Comment hidden (obsolete)
Comment 93 Victor Grousset/tuxayo 2022-08-03 23:39:32 UTC Comment hidden (obsolete)
Comment 94 Victor Grousset/tuxayo 2022-08-03 23:39:51 UTC Comment hidden (obsolete)
Comment 95 Victor Grousset/tuxayo 2022-08-03 23:40:19 UTC Comment hidden (obsolete)
Comment 96 Victor Grousset/tuxayo 2022-08-03 23:40:41 UTC Comment hidden (obsolete)
Comment 97 Victor Grousset/tuxayo 2022-08-03 23:41:05 UTC Comment hidden (obsolete)
Comment 98 Victor Grousset/tuxayo 2022-08-03 23:41:29 UTC Comment hidden (obsolete)
Comment 99 Victor Grousset/tuxayo 2022-08-03 23:41:53 UTC Comment hidden (obsolete)
Comment 100 Victor Grousset/tuxayo 2022-08-03 23:42:14 UTC Comment hidden (obsolete)
Comment 101 Victor Grousset/tuxayo 2022-08-03 23:42:38 UTC Comment hidden (obsolete)
Comment 102 Victor Grousset/tuxayo 2022-08-03 23:42:59 UTC Comment hidden (obsolete)
Comment 103 Victor Grousset/tuxayo 2022-08-03 23:43:28 UTC Comment hidden (obsolete)
Comment 104 Victor Grousset/tuxayo 2022-08-03 23:43:54 UTC Comment hidden (obsolete)
Comment 105 Victor Grousset/tuxayo 2022-08-03 23:44:20 UTC Comment hidden (obsolete)
Comment 106 Victor Grousset/tuxayo 2022-08-03 23:44:41 UTC Comment hidden (obsolete)
Comment 107 Victor Grousset/tuxayo 2022-08-03 23:45:01 UTC Comment hidden (obsolete)
Comment 108 Victor Grousset/tuxayo 2022-08-03 23:45:27 UTC Comment hidden (obsolete)
Comment 109 Victor Grousset/tuxayo 2022-08-03 23:45:54 UTC Comment hidden (obsolete)
Comment 110 Victor Grousset/tuxayo 2022-08-03 23:46:15 UTC Comment hidden (obsolete)
Comment 111 Victor Grousset/tuxayo 2022-08-03 23:46:40 UTC Comment hidden (obsolete)
Comment 112 Victor Grousset/tuxayo 2022-08-03 23:49:14 UTC
Rebased.

It was tough, one patch blew up with the infamous:
error: sha1 information is lacking or useless (opac/opac-detail.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 27272: Remove GetItemsInfo from opac-detail

But thankfully it was possible to use your repo
https://gitlab.com/joubu/Koha/-/commits/bug_27272/
The latest patches aren't there so it wasn't possible to it directly (and I wasn't certain the existing commits were at their latest version).
But! Getting just the problematic commit and applying it worked :D
And the rest applied without issue on top of master.

Comparing the diffs of the previous "Remove GetItemsInfo from opac-detail" from this ticket and the rebased on showed no difference in the changed lines.
Comment 113 Victor Grousset/tuxayo 2022-08-05 01:58:10 UTC
The test suite passes.

---

Here is the only complain from the QA script:
 FAIL	C4/Items.pm
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
Is it legit?

---

Tell me (Jonathan or a QAer that looks into this) if there is particular stuff to test manually.

For lack of clues I just reread all the patches in case there where something obviously suspicious.
I haven't done development with those functions and in all the places they are used so I can't tell if the replacement of GetItemsInfo & co is correct.

Let's move on and see what QA has to say. These kind of patches with nothing clear to test can easily get stuck forever waiting for SO because testers don't know what to do.
I would happily come back if there is precise stuff to test manually.

Thanks Jonathan for the big refactoring!
Comment 114 Victor Grousset/tuxayo 2022-08-05 02:13:16 UTC Comment hidden (obsolete)
Comment 115 Victor Grousset/tuxayo 2022-08-05 02:13:21 UTC Comment hidden (obsolete)
Comment 116 Victor Grousset/tuxayo 2022-08-05 02:13:26 UTC Comment hidden (obsolete)
Comment 117 Victor Grousset/tuxayo 2022-08-05 02:13:32 UTC Comment hidden (obsolete)
Comment 118 Victor Grousset/tuxayo 2022-08-05 02:13:38 UTC Comment hidden (obsolete)
Comment 119 Victor Grousset/tuxayo 2022-08-05 02:13:44 UTC Comment hidden (obsolete)
Comment 120 Victor Grousset/tuxayo 2022-08-05 02:13:50 UTC Comment hidden (obsolete)
Comment 121 Victor Grousset/tuxayo 2022-08-05 02:13:57 UTC Comment hidden (obsolete)
Comment 122 Victor Grousset/tuxayo 2022-08-05 02:14:02 UTC Comment hidden (obsolete)
Comment 123 Victor Grousset/tuxayo 2022-08-05 02:14:08 UTC Comment hidden (obsolete)
Comment 124 Victor Grousset/tuxayo 2022-08-05 02:14:14 UTC Comment hidden (obsolete)
Comment 125 Victor Grousset/tuxayo 2022-08-05 02:14:19 UTC Comment hidden (obsolete)
Comment 126 Victor Grousset/tuxayo 2022-08-05 02:14:26 UTC Comment hidden (obsolete)
Comment 127 Victor Grousset/tuxayo 2022-08-05 02:14:32 UTC Comment hidden (obsolete)
Comment 128 Victor Grousset/tuxayo 2022-08-05 02:14:38 UTC Comment hidden (obsolete)
Comment 129 Victor Grousset/tuxayo 2022-08-05 02:14:44 UTC Comment hidden (obsolete)
Comment 130 Victor Grousset/tuxayo 2022-08-05 02:14:51 UTC Comment hidden (obsolete)
Comment 131 Victor Grousset/tuxayo 2022-08-05 02:14:56 UTC Comment hidden (obsolete)
Comment 132 Victor Grousset/tuxayo 2022-08-05 02:15:03 UTC Comment hidden (obsolete)
Comment 133 Victor Grousset/tuxayo 2022-08-05 02:15:10 UTC Comment hidden (obsolete)
Comment 134 Victor Grousset/tuxayo 2022-08-05 02:15:16 UTC Comment hidden (obsolete)
Comment 135 Victor Grousset/tuxayo 2022-08-05 02:15:23 UTC Comment hidden (obsolete)
Comment 136 Victor Grousset/tuxayo 2022-08-05 02:15:29 UTC Comment hidden (obsolete)
Comment 137 Victor Grousset/tuxayo 2022-08-05 02:17:09 UTC
(the only thing found when reviewing was a minor typo on a FIXME comment. It was fixed by amending the commit. holding_branc => holding_branch)
Comment 138 Jonathan Druart 2022-08-05 07:15:00 UTC
There is a test plan in comment 35.
Comment 139 Victor Grousset/tuxayo 2022-08-05 15:59:04 UTC
oh I missed it, thanks, I'll apply it.
Comment 140 Nick Clemens 2022-08-05 16:03:23 UTC
patch needs minor rebase

opac-details:
Template process failed: undef error - The method Koha::Item->transfertwhen is not covered by tests!
--from item-status-schema-org

Itemtype icons no longer displaying in staff interface

That's all I can touch right now. If you do not object to this becoming an omnibus and breaking it apart I will work through it in sections. I am happy to create the new bugs to help.

This is fairly large in current state - I have some slight concerns about increased db fetches with this change and would like to be bale to focus on smaller bits
Comment 141 Victor Grousset/tuxayo 2022-08-05 20:13:16 UTC
Created attachment 138714 [details] [review]
Bug 27272: Remove type= from detail.pl

This is not related to anything known.. was it "itype" at some point?
`git log -p` will tell you that it's there for a looong time already
Comment 142 Victor Grousset/tuxayo 2022-08-05 20:13:24 UTC
Created attachment 138715 [details] [review]
Bug 27272: Remove GetItemsInfo from basket/basket
Comment 143 Victor Grousset/tuxayo 2022-08-05 20:13:32 UTC
Created attachment 138716 [details] [review]
Bug 27272: Remove GetItemsInfo from basket/sendbasket

Why are we display with $raw?
Why are we display the location code instead of the AV's lib?
Comment 144 Victor Grousset/tuxayo 2022-08-05 20:13:39 UTC
Created attachment 138717 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Comment 145 Victor Grousset/tuxayo 2022-08-05 20:13:47 UTC
Created attachment 138718 [details] [review]
Bug 27272: Remove GetItemsInfo labels/label-item-search

It was commented anyway
Comment 146 Victor Grousset/tuxayo 2022-08-05 20:13:54 UTC
Created attachment 138719 [details] [review]
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl

Hum... Item2Marc ok here?
Comment 147 Victor Grousset/tuxayo 2022-08-05 20:14:01 UTC
Created attachment 138720 [details] [review]
Bug 27272: Remove GetItemsInfo from opac-detail
Comment 148 Victor Grousset/tuxayo 2022-08-05 20:14:08 UTC
Created attachment 138721 [details] [review]
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl

It was never set (was set before 14695)
Comment 149 Victor Grousset/tuxayo 2022-08-05 20:14:15 UTC
Created attachment 138722 [details] [review]
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Comment 150 Victor Grousset/tuxayo 2022-08-05 20:14:24 UTC
Created attachment 138723 [details] [review]
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Comment 151 Victor Grousset/tuxayo 2022-08-05 20:14:31 UTC
Created attachment 138724 [details] [review]
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Comment 152 Victor Grousset/tuxayo 2022-08-05 20:14:38 UTC
Created attachment 138725 [details] [review]
Bug 27272: Remove GetItemsInfo import from opac-tags
Comment 153 Victor Grousset/tuxayo 2022-08-05 20:14:50 UTC
Created attachment 138726 [details] [review]
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Comment 154 Victor Grousset/tuxayo 2022-08-05 20:14:57 UTC
Created attachment 138727 [details] [review]
Bug 27272: Remove GetItemsInfo from tags/list.pl
Comment 155 Victor Grousset/tuxayo 2022-08-05 20:15:05 UTC
Created attachment 138728 [details] [review]
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl

And GetItemsLocationInfo from shelves.pl
Comment 156 Victor Grousset/tuxayo 2022-08-05 20:15:13 UTC
Created attachment 138729 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Comment 157 Victor Grousset/tuxayo 2022-08-05 20:15:24 UTC
Created attachment 138730 [details] [review]
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Comment 158 Victor Grousset/tuxayo 2022-08-05 20:15:34 UTC
Created attachment 138731 [details] [review]
Bug 27272: Add search_ordered sub to order items with extra conditions
Comment 159 Victor Grousset/tuxayo 2022-08-05 20:15:41 UTC
Created attachment 138732 [details] [review]
Bug 27272: Allow to pass params and attributes to search_ordered
Comment 160 Victor Grousset/tuxayo 2022-08-05 20:15:49 UTC
Created attachment 138733 [details] [review]
Bug 27272: Add tests for search_ordered
Comment 161 Victor Grousset/tuxayo 2022-08-05 20:15:58 UTC
Created attachment 138734 [details] [review]
Bug 27272: Call search_ordered when GetItemsInfo was called before
Comment 162 Victor Grousset/tuxayo 2022-08-05 20:16:06 UTC
Created attachment 138735 [details] [review]
Bug 27272: Fix availability - OPAC

Has to move some code to a method
Comment 163 Victor Grousset/tuxayo 2022-08-05 20:16:13 UTC
Created attachment 138736 [details] [review]
Bug 27272: Fix library display on routing slip - to squash
Comment 164 Victor Grousset/tuxayo 2022-08-05 20:49:37 UTC
(In reply to Nick Clemens from comment #140)
> patch needs minor rebase

Here it is, hope that helps ^^
(Bug 31272 caused a bit of trouble with date stuff but it was solvable reliably :) )

> That's all I can touch right now. If you do not object to this becoming an
> omnibus and breaking it apart I will work through it in sections. I am happy
> to create the new bugs to help.

I'm not Jonathan but to help: how could this be split?

Like having a 1st ticket with the common work. And including a minor "Remove GetItemsInfo from XXX"
And then having several tickets each with a large "Remove GetItemsInfo from XXX"
And the minor ones grouped into a ticket.
Would that work well?

To have less ticket management overhead and simplify the maintenance during the review (the many rebases!), would it work well to keep everything here and have multiple people review it? Like you could focus on only some of the "Remove GetItemsInfo from XXX" patches. The signed-off lines would keep track on the which patches are QAed and which aren't.
Comment 165 Victor Grousset/tuxayo 2022-08-06 23:25:34 UTC
Issue found:

1. Apply the patches (with or without bug 30847)
2. Go to the OPAC
3. Go to any record details page
4. it blows up
   Template process failed: undef error - The method Koha::Item->transfertwhen is not covered by tests!


Another issue found:
1. after from test plan: step 14 - Receive/find an issue with an item, view in opac
2. go to the staff detail page of the record
3. it blows up
   Template process failed: undef error - The method Koha::Serial::Item->publisheddate is not covered by tests!
Comment 166 Jonathan Druart 2022-08-08 08:01:34 UTC
Created attachment 138773 [details] [review]
Bug 27272: Remove type= from detail.pl

This is not related to anything known.. was it "itype" at some point?
`git log -p` will tell you that it's there for a looong time already
Comment 167 Jonathan Druart 2022-08-08 08:01:41 UTC
Created attachment 138774 [details] [review]
Bug 27272: Remove GetItemsInfo from basket/basket
Comment 168 Jonathan Druart 2022-08-08 08:01:47 UTC
Created attachment 138775 [details] [review]
Bug 27272: Remove GetItemsInfo from basket/sendbasket

Why are we display with $raw?
Why are we display the location code instead of the AV's lib?
Comment 169 Jonathan Druart 2022-08-08 08:01:53 UTC
Created attachment 138776 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Comment 170 Jonathan Druart 2022-08-08 08:02:00 UTC
Created attachment 138777 [details] [review]
Bug 27272: Remove GetItemsInfo labels/label-item-search

It was commented anyway
Comment 171 Jonathan Druart 2022-08-08 08:02:07 UTC
Created attachment 138778 [details] [review]
Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl

Hum... Item2Marc ok here?
Comment 172 Jonathan Druart 2022-08-08 08:02:13 UTC
Created attachment 138779 [details] [review]
Bug 27272: Remove GetItemsInfo from opac-detail
Comment 173 Jonathan Druart 2022-08-08 08:02:20 UTC
Created attachment 138780 [details] [review]
Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl

It was never set (was set before 14695)
Comment 174 Jonathan Druart 2022-08-08 08:02:26 UTC
Created attachment 138781 [details] [review]
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Comment 175 Jonathan Druart 2022-08-08 08:02:33 UTC
Created attachment 138782 [details] [review]
Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Comment 176 Jonathan Druart 2022-08-08 08:02:39 UTC
Created attachment 138783 [details] [review]
Bug 27272: Remove GetItemsInfo from opac-sendbasket
Comment 177 Jonathan Druart 2022-08-08 08:02:46 UTC
Created attachment 138784 [details] [review]
Bug 27272: Remove GetItemsInfo import from opac-tags
Comment 178 Jonathan Druart 2022-08-08 08:02:52 UTC
Created attachment 138785 [details] [review]
Bug 27272: Remove GetItemsInfo from serials/routing-preview
Comment 179 Jonathan Druart 2022-08-08 08:02:59 UTC
Created attachment 138786 [details] [review]
Bug 27272: Remove GetItemsInfo from tags/list.pl
Comment 180 Jonathan Druart 2022-08-08 08:03:06 UTC
Created attachment 138787 [details] [review]
Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl

And GetItemsLocationInfo from shelves.pl
Comment 181 Jonathan Druart 2022-08-08 08:03:13 UTC
Created attachment 138788 [details] [review]
Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Comment 182 Jonathan Druart 2022-08-08 08:03:20 UTC
Created attachment 138789 [details] [review]
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Comment 183 Jonathan Druart 2022-08-08 08:03:27 UTC
Created attachment 138790 [details] [review]
Bug 27272: Add search_ordered sub to order items with extra conditions
Comment 184 Jonathan Druart 2022-08-08 08:03:34 UTC
Created attachment 138791 [details] [review]
Bug 27272: Allow to pass params and attributes to search_ordered
Comment 185 Jonathan Druart 2022-08-08 08:03:40 UTC
Created attachment 138792 [details] [review]
Bug 27272: Add tests for search_ordered
Comment 186 Jonathan Druart 2022-08-08 08:03:46 UTC
Created attachment 138793 [details] [review]
Bug 27272: Call search_ordered when GetItemsInfo was called before
Comment 187 Jonathan Druart 2022-08-08 08:03:52 UTC
Created attachment 138794 [details] [review]
Bug 27272: Fix availability - OPAC

Has to move some code to a method
Comment 188 Jonathan Druart 2022-08-08 08:04:04 UTC
(In reply to Nick Clemens from comment #140)
> patch needs minor rebase
> 
> opac-details:
> Template process failed: undef error - The method Koha::Item->transfertwhen
> is not covered by tests!
> --from item-status-schema-org

Fixed.

> Itemtype icons no longer displaying in staff interface

Fixed.

> That's all I can touch right now. If you do not object to this becoming an
> omnibus and breaking it apart I will work through it in sections. I am happy
> to create the new bugs to help.
> 
> This is fairly large in current state - I have some slight concerns about
> increased db fetches with this change and would like to be bale to focus on
> smaller bits

Which areas exactly? I am pretty sure we going globally to do better with this move, the query run by GetItemsInfo is big and is certainly the cause of some slowness.
We could split it up into several bugs, but my feeling is that it will add extra work (new bug report, more people involved in SO+QA, more time to get it in, can be released into 22.11 without all the work done, follow-ups can be harder, same for revert if we need it) for not much benefit.

(In reply to Victor Grousset/tuxayo from comment #165)
> Issue found:
> 
> 1. Apply the patches (with or without bug 30847)
> 2. Go to the OPAC
> 3. Go to any record details page
> 4. it blows up
>    Template process failed: undef error - The method
> Koha::Item->transfertwhen is not covered by tests!

This is what Nick reported.

> Another issue found:
> 1. after from test plan: step 14 - Receive/find an issue with an item, view
> in opac
> 2. go to the staff detail page of the record
> 3. it blows up
>    Template process failed: undef error - The method
> Koha::Serial::Item->publisheddate is not covered by tests!

Fixed.
Comment 189 Nick Clemens 2022-08-08 11:53:07 UTC
(In reply to Jonathan Druart from comment #188)
> I am pretty sure we going globally to do better with
> this move, the query run by GetItemsInfo is big and is certainly the cause
> of some slowness.

It's the same query as GetItemsForInventory - afaict from some profiling that is much faster than our other methods, big query, but run once

> We could split it up into several bugs, but my feeling is that it will add
> extra work (new bug report, more people involved in SO+QA, more time to get
> it in, can be released into 22.11 without all the work done, follow-ups can
> be harder, same for revert if we need it) for not much benefit.

Each patch that only deals with a single script is easier to SO/QA/rebase/revert if needed - removal from a single script doesn't rely on the other patches.
The patches near the end with search ordered can be their own, the will touch more, but it is just verifying a single routine work

It will be more people - but I think that's the benefit - more people reviewing the areas they know. When the sets are this big it is often a large commitment from a single person, and reviewing on a large level often comes out to: "Good enough, seems solid, let's catch the bugs after" 

I see the negative with a single bug as: large rebases/large testing plans/large time commitment and I don't see the benefit of this. Large patches move slower
Comment 190 Jonathan Druart 2022-08-08 14:02:45 UTC
Rebased are a nightmare when you need to maintain patches from several bug report...
Anyway, I will create the bug reports and move the patches, if you think it can help. Prove me I am wrong! :D
Comment 191 Jonathan Druart 2022-08-08 14:03:04 UTC
(In reply to Jonathan Druart from comment #166)
> Created attachment 138773 [details] [review] [review]
> Bug 27272: Remove type= from detail.pl
> 
> This is not related to anything known.. was it "itype" at some point?
> `git log -p` will tell you that it's there for a looong time already

Moved to bug 31305.
Comment 192 Jonathan Druart 2022-08-08 14:13:15 UTC
(In reply to Jonathan Druart from comment #173)
> Created attachment 138780 [details] [review] [review]
> Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl
> 
> It was never set (was set before 14695)

Moved to bug 31307.
Comment 193 Jonathan Druart 2022-08-08 14:14:39 UTC
(In reply to Jonathan Druart from comment #183)
> Created attachment 138790 [details] [review] [review]
> Bug 27272: Add search_ordered sub to order items with extra conditions

(In reply to Jonathan Druart from comment #184)
> Created attachment 138791 [details] [review] [review]
> Bug 27272: Allow to pass params and attributes to search_ordered

(In reply to Jonathan Druart from comment #185)
> Created attachment 138792 [details] [review] [review]
> Bug 27272: Add tests for search_ordered

Moved to bug 31306.
Comment 194 Jonathan Druart 2022-08-08 14:55:17 UTC
Created attachment 138832 [details] [review]
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo

With the different bug reports we removed the calls for those 3
subroutines. We can remove them now.

Test plan:
git grep their names and you should not find any occurrences in the code
base
Comment 195 Jonathan Druart 2022-08-08 14:58:02 UTC
Bug 31308 - Remove GetItemsInfo from basket/basket

Bug 31309 - Remove GetItemsInfo from basket/sendbasket

Bug 31310 - Remove GetItemsInfo from catalogue/imageviewer

Bug 31311 - Remove GetItemsInfo from labels/label-item-search

Bug 31312 - Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl

Bug 31313 - Remove GetItemsInfo from opac-detail

Bug 31314 - Remove GetItemsInfo from opac-reserve.pl

Bug 31315 - Remove GetItemsInfo from moredetail

Bug 31316 - Remove GetItemsInfo from opac-sendbasket

Bug 31317 - Remove GetItemsInfo from opac-tags

Bug 31318 - Remove GetItemsInfo from serials/routing-preview

Bug 31319 - Remove GetItemsInfo from tags/list.pl

Bug 31320 - Remove GetItemsInfo from virtualshelves/sendshelf.pl

Bug 31321 - Remove GetItemsInfo from catalogue/detail.pl
Comment 196 Jonathan Druart 2022-08-08 15:01:41 UTC
The ugly job is done.

(repeating myself) I don't think it's the way we should move this forward, but I am just doing what others think it's best.
That won't help rebase (QA will submit follow-ups on the different bugs and so more conflicts), the code may differ from when the first patch has been pushed and the last one, we could end up with only a part of the patches pushed in a major release, behaviours may be different between views, git log won't have them grouped, etc.

That's why I suggest to push everything at once, even if this is split now.
Comment 197 Tomás Cohen Arazi 2022-08-09 22:07:55 UTC
(In reply to Jonathan Druart from comment #196)
> The ugly job is done.
> 
> (repeating myself) I don't think it's the way we should move this forward,
> but I am just doing what others think it's best.
> That won't help rebase (QA will submit follow-ups on the different bugs and
> so more conflicts), the code may differ from when the first patch has been
> pushed and the last one, we could end up with only a part of the patches
> pushed in a major release, behaviours may be different between views, git
> log won't have them grouped, etc.
> 
> That's why I suggest to push everything at once, even if this is split now.

No worries. We are gonna make this a team effort as it is part of the roadmap, and we are all engaged. Rebasing won't be a problem.

Great job, Jonathan!
Comment 198 Joonas Kylmälä 2022-08-28 16:24:49 UTC
Created attachment 139928 [details] [review]
Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo

With the different bug reports we removed the calls for those 3
subroutines. We can remove them now.

Test plan:
git grep their names and you should not find any occurrences in the code
base

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 199 Joonas Kylmälä 2022-08-28 16:25:28 UTC
Trivial removal of unused subroutines, fast forwarding directly to PQA.
Comment 200 Tomás Cohen Arazi 2022-08-31 21:01:59 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!