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.
Good tip, Tomás! I'll do that when I begin this patch.
Created attachment 128230 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
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
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!
(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?
(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 !
(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?
Created attachment 135640 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Created attachment 135641 [details] [review] Bug 27272: catalogue/detail
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
Created attachment 135643 [details] [review] Bug 27272: basket/basket
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?
Created attachment 135645 [details] [review] Bug 27272: catalogue/imageviewer
Created attachment 135646 [details] [review] Bug 27272: labels/label-item-search
Created attachment 135647 [details] [review] Bug 27272: misc/migration_tools/rebuild_zebra.pl Hum... Item2Marc ok here?
Created attachment 135648 [details] [review] Bug 27272: opac-detail
Created attachment 135649 [details] [review] Bug 27272: already_reserved never set (was set before 14695)
Created attachment 135650 [details] [review] Bug 27272: opac-reserve
Created attachment 135651 [details] [review] Bug 27272: moredetail
Created attachment 135652 [details] [review] Bug 27272: opac-sendbasket
Created attachment 135653 [details] [review] Bug 27272: opac-tags
Created attachment 135654 [details] [review] Bug 27272: serials/routing-preview
Created attachment 135655 [details] [review] Bug 27272: tags/list.pl
Created attachment 135656 [details] [review] Bug 27272: opac-sendshelf
Created attachment 135657 [details] [review] Bug 27272: virtualshelves/sendshelf.pl shelves.pl
Created attachment 135658 [details] [review] Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Created attachment 135659 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
(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.
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.
(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.
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)
(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.
(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.
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
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
(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?
(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!
See the following remote branch for the patches rebased against master: https://gitlab.com/joubu/Koha/-/commits/bug_27272
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.
(In reply to Joonas Kylmälä from comment #40) > I found some issues: Fixed! Remote branch adjusted.
(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?
Remote branch rebased against master.
(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?
Doesn't apply anymore against latest master.
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 });
Petro, are you working on this?
Yes, I'm on it.
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
Created attachment 137967 [details] [review] Bug 27272: Remove GetItemsInfo from basket/basket
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?
Created attachment 137969 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Created attachment 137970 [details] [review] Bug 27272: Remove GetItemsInfo labels/label-item-search It was commented anyway
Created attachment 137971 [details] [review] Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl Hum... Item2Marc ok here?
Created attachment 137972 [details] [review] Bug 27272: Remove GetItemsInfo from opac-detail
Created attachment 137973 [details] [review] Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl It was never set (was set before 14695)
Created attachment 137974 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Created attachment 137975 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Created attachment 137976 [details] [review] Bug 27272: Remove GetItemsInfo from opac-sendbasket
Created attachment 137977 [details] [review] Bug 27272: Remove GetItemsInfo import from opac-tags
Created attachment 137978 [details] [review] Bug 27272: Remove GetItemsInfo from serials/routing-preview
Created attachment 137979 [details] [review] Bug 27272: Remove GetItemsInfo from tags/list.pl
Created attachment 137980 [details] [review] Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl And GetItemsLocationInfo from shelves.pl
Created attachment 137981 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Created attachment 137982 [details] [review] Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Patches from remote branch have been attached here.
Created attachment 138109 [details] [review] Bug 27272: Add search_ordered sub to order items with extra conditions
I am writing tests for the last patch.
Created attachment 138296 [details] [review] Bug 27272: Allow to pass params and attributes to search_ordered
Created attachment 138297 [details] [review] Bug 27272: Add tests for search_ordered
Created attachment 138298 [details] [review] Bug 27272: Call search_ordered when GetItemsInfo was called before
Back to NQA. Thanks, Petro!
Created attachment 138303 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Created attachment 138304 [details] [review] Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Created attachment 138305 [details] [review] Bug 27272: Add search_ordered sub to order items with extra conditions
Created attachment 138306 [details] [review] Bug 27272: Allow to pass params and attributes to search_ordered
Created attachment 138307 [details] [review] Bug 27272: Add tests for search_ordered
Created attachment 138308 [details] [review] Bug 27272: Call search_ordered when GetItemsInfo was called before
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.
Created attachment 138437 [details] [review] Bug 27272: Add search_ordered sub to order items with extra conditions
Created attachment 138438 [details] [review] Bug 27272: Allow to pass params and attributes to search_ordered
Created attachment 138439 [details] [review] Bug 27272: Add tests for search_ordered
Created attachment 138440 [details] [review] Bug 27272: Call search_ordered when GetItemsInfo was called before
Created attachment 138441 [details] [review] Bug 27272: Fix availability - OPAC Has to move some code to a method
Created attachment 138442 [details] [review] Bug 27272: Fix library display on routing slip - to squash
(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!
Created attachment 138553 [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
Created attachment 138554 [details] [review] Bug 27272: Remove GetItemsInfo from basket/basket
Created attachment 138555 [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?
Created attachment 138556 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Created attachment 138557 [details] [review] Bug 27272: Remove GetItemsInfo labels/label-item-search It was commented anyway
Created attachment 138558 [details] [review] Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl Hum... Item2Marc ok here?
Created attachment 138559 [details] [review] Bug 27272: Remove GetItemsInfo from opac-detail
Created attachment 138560 [details] [review] Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl It was never set (was set before 14695)
Created attachment 138561 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Created attachment 138562 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Created attachment 138563 [details] [review] Bug 27272: Remove GetItemsInfo from opac-sendbasket
Created attachment 138564 [details] [review] Bug 27272: Remove GetItemsInfo import from opac-tags
Created attachment 138565 [details] [review] Bug 27272: Remove GetItemsInfo from serials/routing-preview
Created attachment 138566 [details] [review] Bug 27272: Remove GetItemsInfo from tags/list.pl
Created attachment 138567 [details] [review] Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl And GetItemsLocationInfo from shelves.pl
Created attachment 138568 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Created attachment 138569 [details] [review] Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Created attachment 138570 [details] [review] Bug 27272: Add search_ordered sub to order items with extra conditions
Created attachment 138571 [details] [review] Bug 27272: Allow to pass params and attributes to search_ordered
Created attachment 138572 [details] [review] Bug 27272: Add tests for search_ordered
Created attachment 138573 [details] [review] Bug 27272: Call search_ordered when GetItemsInfo was called before
Created attachment 138574 [details] [review] Bug 27272: Fix availability - OPAC Has to move some code to a method
Created attachment 138575 [details] [review] Bug 27272: Fix library display on routing slip - to squash
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.
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!
Created attachment 138614 [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 Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138615 [details] [review] Bug 27272: Remove GetItemsInfo from basket/basket Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138616 [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? Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138617 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/imageviewer Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138618 [details] [review] Bug 27272: Remove GetItemsInfo labels/label-item-search It was commented anyway Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138619 [details] [review] Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl Hum... Item2Marc ok here? Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138620 [details] [review] Bug 27272: Remove GetItemsInfo from opac-detail Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138621 [details] [review] Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl It was never set (was set before 14695) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138622 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138623 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138624 [details] [review] Bug 27272: Remove GetItemsInfo from opac-sendbasket Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138625 [details] [review] Bug 27272: Remove GetItemsInfo import from opac-tags Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138626 [details] [review] Bug 27272: Remove GetItemsInfo from serials/routing-preview Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138627 [details] [review] Bug 27272: Remove GetItemsInfo from tags/list.pl Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138628 [details] [review] Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl And GetItemsLocationInfo from shelves.pl Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138629 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138630 [details] [review] Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138631 [details] [review] Bug 27272: Add search_ordered sub to order items with extra conditions Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138632 [details] [review] Bug 27272: Allow to pass params and attributes to search_ordered Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138633 [details] [review] Bug 27272: Add tests for search_ordered Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138634 [details] [review] Bug 27272: Call search_ordered when GetItemsInfo was called before Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138635 [details] [review] Bug 27272: Fix availability - OPAC Has to move some code to a method Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138636 [details] [review] Bug 27272: Fix library display on routing slip - to squash Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
(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)
There is a test plan in comment 35.
oh I missed it, thanks, I'll apply it.
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
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
Created attachment 138715 [details] [review] Bug 27272: Remove GetItemsInfo from basket/basket
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?
Created attachment 138717 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Created attachment 138718 [details] [review] Bug 27272: Remove GetItemsInfo labels/label-item-search It was commented anyway
Created attachment 138719 [details] [review] Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl Hum... Item2Marc ok here?
Created attachment 138720 [details] [review] Bug 27272: Remove GetItemsInfo from opac-detail
Created attachment 138721 [details] [review] Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl It was never set (was set before 14695)
Created attachment 138722 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Created attachment 138723 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Created attachment 138724 [details] [review] Bug 27272: Remove GetItemsInfo from opac-sendbasket
Created attachment 138725 [details] [review] Bug 27272: Remove GetItemsInfo import from opac-tags
Created attachment 138726 [details] [review] Bug 27272: Remove GetItemsInfo from serials/routing-preview
Created attachment 138727 [details] [review] Bug 27272: Remove GetItemsInfo from tags/list.pl
Created attachment 138728 [details] [review] Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl And GetItemsLocationInfo from shelves.pl
Created attachment 138729 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Created attachment 138730 [details] [review] Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Created attachment 138731 [details] [review] Bug 27272: Add search_ordered sub to order items with extra conditions
Created attachment 138732 [details] [review] Bug 27272: Allow to pass params and attributes to search_ordered
Created attachment 138733 [details] [review] Bug 27272: Add tests for search_ordered
Created attachment 138734 [details] [review] Bug 27272: Call search_ordered when GetItemsInfo was called before
Created attachment 138735 [details] [review] Bug 27272: Fix availability - OPAC Has to move some code to a method
Created attachment 138736 [details] [review] Bug 27272: Fix library display on routing slip - to squash
(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.
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!
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
Created attachment 138774 [details] [review] Bug 27272: Remove GetItemsInfo from basket/basket
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?
Created attachment 138776 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/imageviewer
Created attachment 138777 [details] [review] Bug 27272: Remove GetItemsInfo labels/label-item-search It was commented anyway
Created attachment 138778 [details] [review] Bug 27272: Remove GetItemsInfo from misc/migration_tools/rebuild_zebra.pl Hum... Item2Marc ok here?
Created attachment 138779 [details] [review] Bug 27272: Remove GetItemsInfo from opac-detail
Created attachment 138780 [details] [review] Bug 27272: Remove 'already_reserved' flag from opac-reserve.pl It was never set (was set before 14695)
Created attachment 138781 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from opac-reserve
Created attachment 138782 [details] [review] Bug 27272: Remove GetHostItemsInfo and GetItemsInfo from moredetail
Created attachment 138783 [details] [review] Bug 27272: Remove GetItemsInfo from opac-sendbasket
Created attachment 138784 [details] [review] Bug 27272: Remove GetItemsInfo import from opac-tags
Created attachment 138785 [details] [review] Bug 27272: Remove GetItemsInfo from serials/routing-preview
Created attachment 138786 [details] [review] Bug 27272: Remove GetItemsInfo from tags/list.pl
Created attachment 138787 [details] [review] Bug 27272: Remove GetItemsInfo from virtualshelves/sendshelf.pl And GetItemsLocationInfo from shelves.pl
Created attachment 138788 [details] [review] Bug 27272: Remove GetItemsInfo from catalogue/detail.pl
Created attachment 138789 [details] [review] Bug 27272: Remove GetItemsInfo, GetItemsLocationInfo and GetHostItemsInfo
Created attachment 138790 [details] [review] Bug 27272: Add search_ordered sub to order items with extra conditions
Created attachment 138791 [details] [review] Bug 27272: Allow to pass params and attributes to search_ordered
Created attachment 138792 [details] [review] Bug 27272: Add tests for search_ordered
Created attachment 138793 [details] [review] Bug 27272: Call search_ordered when GetItemsInfo was called before
Created attachment 138794 [details] [review] Bug 27272: Fix availability - OPAC Has to move some code to a method
(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.
(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
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
(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.
(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.
(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.
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
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
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.
(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!
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>
Trivial removal of unused subroutines, fast forwarding directly to PQA.
Pushed to master for 22.11. Nice work everyone, thanks!