Details pages opac/opac-detail.pl and catalogue/detail.pl call GetItemsInfo() on their own after it being called from XSLTParse4Display() (if OPACXSLTDetailsDisplay syspref and/or XSLTDetailsDisplay are set). That imposes severe performance penalty for biblio records with large number of items. The right way would be to pass the list to XSLTParse4Display(), however that interface is very complex as is. An alternative is to return the list from XSLTParse4Display(), which is not elegant at all. If no one cries foul in the next 24 hours, I'll proceed with changing the interface.
Created attachment 22800 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
This may yield significant improvements in performance. Hairy spots: * I have removed GetItemsLocationInfo() in favour of GetItemsInfo(). In case of people not using XSLT, it may incur small penalty in virtual shelves because GetItemsInfo() is a tad heavier. However, it should be insignificant compared to the benefit when XSLT is used. * I wish I knew more about Koha internals. Because then I could have more confidence in saying that items info gathered in searchResults() is sufficient for building the display. It looked fine to me, but still I need someone more knowledgeable to confirm.
We remarked some problems of performance with biblios having a long list of items attached (serials for ex). Do you think your patch could be usefull for that? M. Saby
I believe that's exactly where it shows most benefits.
Some remarks about GetItemsInfo () : There are 5 SELECT. I suppose it would be better for performance to have a unique SELECT. And the last SELECT gets some informations which are not usefull in normal OPAC and staff display : It is used for finding the last 3 people who borrowed this item. Don't you think geting the info about the 3 last ppl who borrowed the item should be made optional (by adding a param to GetItemsInfo) ? M. Saby
I totally agree, and I may go there in some other bug report. I'll just say that at this stage 5 calls are better than 10.
(In reply to Srdjan Jankovic from comment #6) > I totally agree, and I may go there in some other bug report. Good idea! And I'm sure I've seen dozen of places in C4 where useless queries are made, or useless fields retreived (like marc field in biblioitems, etc)... Mathieu
(In reply to Srdjan Jankovic from comment #2) > This may yield significant improvements in performance. > > Hairy spots: > * I have removed GetItemsLocationInfo() in favour of GetItemsInfo(). In case > of people not using XSLT, it may incur small penalty in virtual shelves > because GetItemsInfo() is a tad heavier. However, it should be insignificant > compared to the benefit when XSLT is used. I thouht I had read somewhere that non-XSLT display were to be deprecated one day, but I cannot retreive the message. Maybe I dreamt? If so, I suppose that the small penality you are talking about won't be a problem. Mathieu
Why did you remove the hidden stuff?
(In reply to M. Tompsett from comment #9) > Why did you remove the hidden stuff? Because it makes no sense to pass on all items and then filter them in the called function. If we don't want them, then why passing them on?
Because I'm worried you are breaking the hiding functionality with this patch. I don't see any changes which reflect moving the exclusion from one place to another.
(In reply to M. Tompsett from comment #11) > Because I'm worried you are breaking the hiding functionality with this > patch. I don't see any changes which reflect moving the exclusion from one > place to another. diff --git a/C4/XSLT.pm b/C4/XSLT.pm index d85b048..0fde9dc 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -156,8 +156,17 @@ sub _get_best_default_xslt_filename { return $xslfilename; } +=head2 XSLTParse4Display( $biblionumber, $orig_record, $xslsyspref, $items, $fixamps ) + + $items => an array of items rerords, as returned from eg. GetItemsInfo + +Returns XSLT block + +=cut + sub XSLTParse4Display { - my ( $biblionumber, $orig_record, $xslsyspref, $fixamps, $hidden_items ) = @_; + my ( $biblionumber, $orig_record, $xslsyspref, $items, $fixamps ) = @_; + my $xslfilename = C4::Context->preference($xslsyspref); if ( $xslfilename =~ /^\s*"?default"?\s*$/i ) { my $htdocs; Exclusion list ($hidden_items) is dropped in favour of inclusiuon list ($items)
Comment on attachment 22800 [details] [review] bug_11213: Changed XSLTParse4Display() interface Review of attachment 22800 [details] [review]: ----------------------------------------------------------------- ::: C4/Items.pm @@ -1400,5 @@ > - my $sth = $dbh->prepare($query); > - $sth->execute($biblionumber); > - > - while ( my $data = $sth->fetchrow_hashref ) { > - $data->{location_intranet} = GetKohaAuthorisedValueLib('LOC', $data->{location}); I already mentioned this makes your code change not perfectly identical in results. ::: C4/Search.pm @@ +1848,3 @@ > # hidden based on OpacHiddenItems syspref > my @hi = C4::Items::GetHiddenItemnumbers($item); > if (scalar @hi) { There's no need to store the @hiddenitems, because you have next, which causes the current item field to be skipped from getting added to hashes. Okay. @@ +1869,5 @@ > my $userenv = C4::Context->userenv; > if ( $item->{onloan} && !(C4::Members::GetHideLostItemsPreference($userenv->{'number'}) && $item->{itemlost}) ) { > $onloan_count++; > + my $key = $prefix . $item->{onloan} . $item->{barcode}; > + $onloan_items->{$key} = { %$item }; Ingenious way to simplify the lines of code which copy keys. Though, wouldn't this result in a LARGER onloan_items hash reference? @@ +1998,4 @@ > warn $marcrecord->as_formatted if $DEBUG; > my $interface = $search_context eq 'opac' ? 'OPAC' : ''; > if (!$scan && C4::Context->preference($interface . "XSLTResultsDisplay")) { > + $oldbiblio->{XSLTResultsRecord} = XSLTParse4Display($oldbiblio->{biblionumber}, $marcrecord, $interface."XSLTResultsDisplay", [@available_items_loop, @onloan_items_loop, @other_items_loop], 1); This is where the magic happens, because all the hidden things, whether lost or OpacHiddenItem'd won't appear in any of these three arrays. Perhaps a handy debugging comment in the patch somewhere? ::: C4/VirtualShelves/Page.pm @@ +261,5 @@ > ( $items, $totitems ) = GetShelfContents( $shelfnumber, $shelflimit, $shelfoffset, $sortfield, $direction ); > for my $this_item (@$items) { > my $biblionumber = $this_item->{'biblionumber'}; > + # Getting items infos for location display > + my @items_infos = &GetItemsInfo( $this_item->{'biblionumber'}, "cn_sort" ); Isn't using $biblionumber faster than $this_item->...?
Created attachment 26394 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Sorry, but could you put the whitespace fixes in a separate patch? It makes reading the actual code changes easier. :)
Created attachment 26430 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 26431 [details] [review] bug_11213: whitespace correction
In the midst of testing... Virtual shelves: LISTS! Testing to continue. I thought something was broken, but then realized that bug 10589 and the related bugs haven't hit master.
Created attachment 26499 [details] [review] [SIGNED OFF] Bug 11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call Revised Test Plan ----------------- NOTE: The ()'s are for my note taking and related to the "blocks" in the diff. Don't worry if they don't make sense. 1) perldoc C4::Items (Items2) 2) perldoc C4::XSLT (XSLT1) 3) Log into staff client (Item1) 4) Change OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay 'OPAC' system preferences to 'default'. 5) Change the 'hidelostitems' to "Don't Show". 6) Clear XSLTResultsDisplay and XSLTDetailsDisplay 'Staff client' system preferences. 7) Search catalog for a biblio. Preferrably with - one lost, one hidden (via OpacHiddenItems system preference), one checked out (Search6,8,9,10,11) 8) Click biblio title in results. (Item3,4B,5 ; catalogue/detail 1) 9) Change XSLTResultsDisplay and XSLTDetailsDisplay 'Staff client' system preferences to 'default'. 10) Search catalog for a biblio (Search12; XSLT2,3,4,5) 11) Click biblio title in results. 12) Home -> Lists 13) Add a '+ New list' 14) Add a barcode for the biblio used in step 7 to the list (Items 4A ; Search 13,14B) 15) Search catalog for the same biblio in OPAC. (Items 7) 16) Click biblio title in results (opac-detail 1) 17) Click 'browse shelf' link in one of the items 18) Click the 'Lists' button in the search bar area and select a list (14A). 19) Run the QA test tool 20) Play with the visibility of items (hidelostitems, OpacHiddenItems) The display should stay the same before and after patch. The speed should increase though. The revised test plan should trigger every code block and every case in an if/else condition. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 26500 [details] [review] [SIGNED OFF] Bug 11213: whitespace correction Not quite what I was expecting, but it is most of the whitespace changes Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Bad news, however, C4 Libraries were modified. Tests that trigger the code need to be included, such that they work BEFORE and AFTER the patches are applied. This may involve TWO test patches: one for master, and one for master post patches. Given that it took quite a while for me to figure out how to trigger everything nicely and write a test plan which was more explicit, I'll leave this to you.
Created attachment 26695 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 26696 [details] [review] bug_11213: C4::VirtualShelves::Page::shelf_contents() * Extracted shelf items processing from shelfpage() into a separate sub shelf_contents() in order to be able to test it * Added tests for shelf_contents() witx XSLT
Created attachment 26713 [details] [review] bug_11213: C4::VirtualShelves::Page::shelf_contents() * Extracted shelf items processing from shelfpage() into a separate sub shelf_contents() in order to be able to test it * Added tests for shelf_contents() with XSLT
You modified GetItemsInfo. t/db_dependent/Items.t needs to have GetItemsInfo calls. You modified searchResults. t/db_dependent/Search.t needs to have calls to searchResults that will trigger changed code. You modified shelfpage. t/db_dependent/VirtualShelves_Page.t needs to at least have a call to shelfpage added. You moved most of the changes into the shelf_contents and properly added a test for it, but the removal of code in shelfpage still may merit a call to shelfpage too. Your tweaks in C4/XSLT don't expressly need a test, but t/db_dependent/XSLT.t probably should be created with at least a call to XSLTParse4Display. These are the tests I am expecting.
Created attachment 26718 [details] [review] bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL
Created attachment 26719 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 26720 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 26721 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
Comment on attachment 26718 [details] [review] bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL Review of attachment 26718 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Items.t @@ +29,5 @@ > } > > my $dbh = C4::Context->dbh; > +my $branches = GetBranches; > +my ($branch1, $branch2) = keys %$branches; I like the idea, but you are missing a check to make sure they are defined. And if they aren't, you need to temporarily add them and then roll them back.
(In reply to M. Tompsett from comment #30) > Comment on attachment 26718 [details] [review] [review] > bug_11213: Use branch codes from the database rather than hardcoded CPL and > MPL > > Review of attachment 26718 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: t/db_dependent/Items.t > @@ +29,5 @@ > > } > > > > my $dbh = C4::Context->dbh; > > +my $branches = GetBranches; > > +my ($branch1, $branch2) = keys %$branches; > > I like the idea, but you are missing a check to make sure they are defined. > And if they aren't, you need to temporarily add them and then roll them back. That's true, however it is not worse then it used to be. Former test depended on having CPL and MPL branches, this one depends on having *any* two branches, thus is more permissive. I'm happy to drop the patch.
First two are signed. Test plan for the remaining?
(In reply to Bernardo Gonzalez Kriegel from comment #32) > First two are signed. > Test plan for the remaining? I'm sorry, I totally lost account for this fix. Which one is missing tests please?
I'd start at comment 25. :)
(In reply to M. Tompsett from comment #34) > I'd start at comment 25. :) Mmmm, I can finish right now. My friends, what I expect is a clear list of what needs to be tested, or run, and what to expect as result. In that way it's easier. In it's current state is discouraging. Anyway, 1) In it's current state patch doesn't apply. Some patches have been pushed that gave conflicts. I solved all but I prefer the author have the last word on this 2) I run this tests, 2 Ok OK t/db_dependent/Items.t OK t/db_dependent/Search.t but this one not prove t/db_dependent/VirtualShelves_Page.t t/db_dependent/VirtualShelves_Page.t .. 1/3 # Failed test 'opac items XSLT' # at t/db_dependent/VirtualShelves_Page.t line 48. # Failed test 'intranet items XSLT' # at t/db_dependent/VirtualShelves_Page.t line 52. # Looks like you failed 2 tests of 3. t/db_dependent/VirtualShelves_Page.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/3 subtests Test Summary Report ------------------- t/db_dependent/VirtualShelves_Page.t (Wstat: 512 Tests: 3 Failed: 2) Failed tests: 2-3 Non-zero exit status: 2 Files=1, Tests=3, 2 wallclock secs ( 0.03 usr 0.01 sys + 1.68 cusr 0.10 csys = 1.82 CPU) Result: FAIL Please fix and resubmit.
Created attachment 29557 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 29558 [details] [review] bug_11213: whitespace correction
Created attachment 29559 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 29560 [details] [review] bug_11213: C4::VirtualShelves::Page::shelf_contents() * Extracted shelf items processing from shelfpage() into a separate sub shelf_contents() in order to be able to test it * Added tests for shelf_contents() with XSLT
Created attachment 29561 [details] [review] bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL
Created attachment 29562 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 29563 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 29564 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
(In reply to Bernardo Gonzalez Kriegel from comment #35) > (In reply to M. Tompsett from comment #34) > > I'd start at comment 25. :) * GetItemsInfo() is getting called in t/db_dependent/Items.t: bug_11213: GetItemsInfo() test * searchResults() is already being called from t/db_dependent/Search.t; XSLTResultsDisplay pref handling in the test should make sure the code is visited: bug_11213: Include XSLT processing for searchResults() test * shelfpage() did not have a test, and I can add one * XSLTParse4Display() is being explicitly called in t/db_dependent/Items.t > > Mmmm, I can finish right now. > > My friends, what I expect is a clear list of what needs to be tested, > or run, and what to expect as result. In that way it's easier. > In it's current state is discouraging. Clear list of what is expected is impossible, at least for me, to give. I cannot even imagine in what different ways particular configs can affect XSLT display. The best I can do is: * Create tests that should pass. In this case we have (in t/db_dependent): Items.t Search.t VirtualShelves_Page.t (will add shelfpage() call) * Ask for XSLT display on OPAC and Intranet related pages to be checked, which includes: - search results - individual biblio + items display - virtual shelves > > Anyway, > 1) In it's current state patch doesn't apply. Some patches have been pushed > that gave conflicts. I solved all but I prefer the author have the last word > on this Rebased > > 2) prove t/db_dependent/VirtualShelves_Page.t > t/db_dependent/VirtualShelves_Page.t .. 1/3 > # Failed test 'opac items XSLT' > # at t/db_dependent/VirtualShelves_Page.t line 48. > Please fix and resubmit. That one worked for me after rebase. Can you please try again.
Created attachment 38584 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 38585 [details] [review] bug_11213: whitespace correction
Created attachment 38586 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 38587 [details] [review] bug_11213: C4::VirtualShelves::Page::shelf_contents() * Extracted shelf items processing from shelfpage() into a separate sub shelf_contents() in order to be able to test it * Added tests for shelf_contents() with XSLT
Created attachment 38588 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 38589 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 38590 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
Created attachment 39023 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 39024 [details] [review] bug_11213: whitespace correction
Created attachment 39025 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 39026 [details] [review] bug_11213: C4::VirtualShelves::Page::shelf_contents() * Extracted shelf items processing from shelfpage() into a separate sub shelf_contents() in order to be able to test it * Added tests for shelf_contents() with XSLT
Created attachment 39027 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 39028 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 39029 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
Applying: bug_11213: C4::VirtualShelves::Page::shelf_contents() fatal: sha1 information is lacking or useless (C4/VirtualShelves/Page.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
(In reply to Marcel de Rooy from comment #59) > Applying: bug_11213: C4::VirtualShelves::Page::shelf_contents() > fatal: sha1 information is lacking or useless (C4/VirtualShelves/Page.pm). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. Not sure why. Applying: bug_11213: C4::VirtualShelves::Page::shelf_contents() Using index info to reconstruct a base tree... M C4/VirtualShelves/Page.pm Falling back to patching base and 3-way merge... Auto-merging C4/VirtualShelves/Page.pm etc Can you please give it another go?
Repeated on fresh master: Applying: bug_11213: C4::VirtualShelves::Page::shelf_contents() fatal: sha1 information is lacking or useless (C4/VirtualShelves/Page.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 bug_11213: C4::VirtualShelves::Page::shelf_contents() Since our git version may be different, please apply them again with your git version and resubmit.
(In reply to Marcel de Rooy from comment #61) > Repeated on fresh master: > Applying: bug_11213: C4::VirtualShelves::Page::shelf_contents() > fatal: sha1 information is lacking or useless (C4/VirtualShelves/Page.pm). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 bug_11213: C4::VirtualShelves::Page::shelf_contents() > > Since our git version may be different, please apply them again with your > git version and resubmit. mtompset@debian:~/kohaclone$ git checkout master Switched to branch 'master' Your branch is up-to-date with 'origin/master'. mtompset@debian:~/kohaclone$ git branch -D bug_11213_restart Deleted branch bug_11213_restart (was 113c548). mtompset@debian:~/kohaclone$ git checkout master Already on 'master' Your branch is up-to-date with 'origin/master'. mtompset@debian:~/kohaclone$ git pull Already up-to-date. mtompset@debian:~/kohaclone$ git remote update -p Fetching origin mtompset@debian:~/kohaclone$ git checkout -b bug_11213_restart origin/master Branch bug_11213_restart set up to track remote branch master from origin. Switched to a new branch 'bug_11213_restart' mtompset@debian:~/kohaclone$ git bz apply 11213 Bug 11213 - GetItemsInfo() called twice 39023 - bug_11213: Changed XSLTParse4Display() interface 39024 - bug_11213: whitespace correction 39025 - bug_11213: Include XSLT processing for searchResults() test 39026 - bug_11213: C4::VirtualShelves::Page::shelf_contents() 39027 - bug_11213: GetItemsInfo() test 39028 - bug_11213: Added XSLTParse4Display() to Items test 39029 - bug_11213: Check for $item->{itype} presence to avoid warning Apply? [(y)es, (n)o, (i)nteractive] y Applying: bug_11213: Changed XSLTParse4Display() interface Applying: bug_11213: whitespace correction Applying: bug_11213: Include XSLT processing for searchResults() test Applying: bug_11213: C4::VirtualShelves::Page::shelf_contents() Using index info to reconstruct a base tree... M C4/VirtualShelves/Page.pm Falling back to patching base and 3-way merge... Auto-merging C4/VirtualShelves/Page.pm Applying: bug_11213: GetItemsInfo() test Applying: bug_11213: Added XSLTParse4Display() to Items test Using index info to reconstruct a base tree... M t/db_dependent/Items.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Items.t Applying: bug_11213: Check for $item->{itype} presence to avoid warning mtompset@debian:~/kohaclone$ It works fine for me with Debian Jessie. Did you check if your 'git bz' is up to date?
(In reply to M. Tompsett from comment #62) > It works fine for me with Debian Jessie. > Did you check if your 'git bz' is up to date? If you submit what you applied here, we probably tackled it..
Created attachment 42234 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 42235 [details] [review] bug_11213: whitespace correction
Created attachment 42236 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 42237 [details] [review] bug_11213: C4::VirtualShelves::Page::shelf_contents() * Extracted shelf items processing from shelfpage() into a separate sub shelf_contents() in order to be able to test it * Added tests for shelf_contents() with XSLT
Created attachment 42238 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 42239 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 42240 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
(In reply to Marcel de Rooy from comment #63) > If you submit what you applied here, we probably tackled it.. Hope this helps. I left the status as is, since you were the one to change it last.
(In reply to M. Tompsett from comment #71) > (In reply to Marcel de Rooy from comment #63) > > If you submit what you applied here, we probably tackled it.. > > Hope this helps. I left the status as is, since you were the one to change > it last. Yes it does. Applies like a train now. (Converted expression..) Thanks.
ok 5 - GetItemsInfo tests 1..2 DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1339. DBIx::Class::ResultSet::create(): DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1339. at t/db_dependent/Items.t line 222 # Child (Test Koha::Database->schema()->resultset('Item')->itemtype()) exited without calling finalize() not ok 6 - Test Koha::Database->schema()->resultset('Item')->itemtype()
Created attachment 42425 [details] [review] Bug 11213: Fix mixing Koha::Database and C4::Context->dbh in test
I believe it is very wrong to mix KOha::Database and C4::Context->dbh based tests. Those should be separated.
I'm wondering if perhaps it should be something more like: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14334#c0
(In reply to M. Tompsett from comment #76) > I'm wondering if perhaps it should be something more like: > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14334#c0 In general I second the idea. I would have done it in a slightly different way (global $schema and only one txn_begin), but that's not important. Would that guarantee safe mixing with modules that use Koha::Database?
Created attachment 49750 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - removed GetItemsLocationInfo() - added sort_by input param to GetItemsInfo() - VirtualShelves::Page::shelfpage() - replaced GetItemsLocationInfo() call with GetItemsInfo() call, passing order_by "cn_sort" * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 49751 [details] [review] bug_11213: whitespace correction
Created attachment 49752 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 49753 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 49754 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 49755 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
(In reply to Srdjan Jankovic from comment #78) > - virtualshelves/shelves.pl > - opac-shelves.pl On these pages I get an error: Undefined subroutine &main::GetItemsLocationInfo called at virtualshelves/shelves.pl line 249. On the other pages I tested it was hard for me to tell the speed difference with and without the patch (based on the Net panel in Firebug), with the exception of OPAC search results which were noticeably improved.
Created attachment 50163 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - added sort_by input param to GetItemsInfo() * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 50164 [details] [review] bug_11213: whitespace correction
Created attachment 50165 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 50166 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 50167 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 50168 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
(In reply to Owen Leonard from comment #84) > (In reply to Srdjan Jankovic from comment #78) > > - virtualshelves/shelves.pl > > - opac-shelves.pl > > On these pages I get an error: > > Undefined subroutine &main::GetItemsLocationInfo called at > virtualshelves/shelves.pl line 249. > Ah that call was probably moved from C4::VirtualShelves::Page::shelfpage(). I've put GetItemsLocationInfo back in.
I'm having a problem with item information on two pages: opac-shelves.pl and opac-search.pl. With these patches applied, opac-shelves.pl always shows "No items available" when viewing the contents of a list. This prompted me to look again at item results on opac-search.pl and I noticed that they look different compared to master. Some items are not showing up on the "Availability" line. I don't see an obvious reason why the missing items are missing.
(In reply to Owen Leonard from comment #92) > I'm having a problem with item information on two pages: opac-shelves.pl and > opac-search.pl. > > With these patches applied, opac-shelves.pl always shows "No items > available" when viewing the contents of a list. On kc/master I get the list, with all entries "No title ". If that is the same as you are getting, then it could be a bug in master. What happens when you try it on master please?
(In reply to Srdjan Jankovic from comment #93) > On kc/master I get the list, with all entries "No title ". I don't have the problem of entries appearing with "No title" in master.
(In reply to Owen Leonard from comment #92) > I'm having a problem with item information on two pages: opac-shelves.pl and > opac-search.pl. I have no probs with either of them. Changes in searchResults() are minor, the main change is in buildKohaItemsNamespace(). I've tried opac-search.pl with both XSLT on and off. Can we please ask someone to check this?
(In reply to Owen Leonard from comment #94) > (In reply to Srdjan Jankovic from comment #93) > > > On kc/master I get the list, with all entries "No title ". > > I don't have the problem of entries appearing with "No title" in master. There was someone on IRC reporting this issue not so long ago - we were not trying to identify the cause. He had 2 system himself, one where it was happening and one where everything was fine.
(In reply to Katrin Fischer from comment #96) > > There was someone on IRC reporting this issue not so long ago - we were not > trying to identify the cause. He had 2 system himself, one where it was > happening and one where everything was fine. Yes, I have that happening in both master and with the patch, so I'm not worried too mush about it. Katrin, since you jumped in, can I please ask you to check whether you have any problems on the Opac search results screen with these patches, as in comment 95. Much obliged
Srdjan, I am sorry, I didn't get back to this earlier :( Now patches don't apply and can't retest as you asked: ... CONFLICT (content): Merge conflict in opac/opac-detail.pl Auto-merging catalogue/detail.pl CONFLICT (content): Merge conflict in catalogue/detail.pl Auto-merging C4/XSLT.pm CONFLICT (content): Merge conflict in C4/XSLT.pm Auto-merging C4/Search.pm CONFLICT (content): Merge conflict in C4/Search.pm Auto-merging C4/Items.pm ...
Created attachment 54447 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - added sort_by input param to GetItemsInfo() * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 54448 [details] [review] bug_11213: whitespace correction
Created attachment 54449 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 54450 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 54451 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 54452 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
Sorry for being slow on the draw.
Created attachment 57578 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - added sort_by input param to GetItemsInfo() * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 57579 [details] [review] bug_11213: whitespace correction
Created attachment 57580 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 57581 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 57582 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 57583 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
How might this interact with bug 17527?
I guess whoever comes second will have to resolve a minor conflict.
Created attachment 62434 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - added sort_by input param to GetItemsInfo() * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 62435 [details] [review] bug_11213: whitespace correction
Created attachment 62436 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 62437 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 62438 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 62439 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
Created attachment 68646 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
Needs adaptation at least because of bug 17248
Created attachment 68818 [details] [review] bug_11213: Changed XSLTParse4Display() interface The list of biblio items is passed on now, instead of GetItemsInfo() being called. This is because the callers already have the list ready, so the GetItemsInfo() call is being duplicated unnecessarily. Search::searchResults() builds items list from XML, and that one is passed instead. * XSLT::XSLTParse4Display() - supply the items list as input param - removed hidden items list param - hidden should not be in the items list - changed buildKohaItemsNamespace() accordingly * Items - added sort_by input param to GetItemsInfo() * catalogue/detail.pl, opac/opac-detail.pl, shelfpage() - added items list to the XSLTParse4Display() call * Search::searchResults() - include all available info when building items lists - added combined items list (available, on loan, other) to the XSLTParse4Display() call To test: This change is a noop, so following screens need to be checked against any changes: * Intranet: - catalogue/search.pl (results) - catalogue/detail.pl - virtualshelves/shelves.pl * Opac - opac-search.pl (results, hidelostitems syspref on and off) - opac-detail.pl - opac-shelves.pl The display should stay the same before and after patch. The speed should increase though.
Created attachment 68819 [details] [review] bug_11213: whitespace correction
Created attachment 68820 [details] [review] bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up
Created attachment 68821 [details] [review] bug_11213: GetItemsInfo() test
Created attachment 68822 [details] [review] bug_11213: Added XSLTParse4Display() to Items test
Created attachment 68823 [details] [review] bug_11213: Check for $item->{itype} presence to avoid warning
Hi Srdjan, I did test these patches now, but I am sorry: The test Search.t is failing after this patchset... # Failed test 'Warning is raised correctly for invalid tags in MARC::Record' # at t/db_dependent/Search.t line 681. there are also so many warnings about preferences missing in mock virtualshelves on intranet and opac goes to ISE: DBIx::Class::ResultSet::_construct_results(): Unable to properly collapse has_many results in iterator mode due to order criteria - performed an eager cursor slurp underneath. Consider using ->all() instead at /home/vagrant/kohaclone/Koha/Objects.pm line 209 Can't use string ("1") as an ARRAY ref while "strict refs" in use at /home/vagrant/kohaclone/C4/XSLT.pm line 290.
Comment on attachment 68820 [details] [review] bug_11213: Include XSLT processing for searchResults() test Not required
(In reply to Josef Moravec from comment #128) > Hi Srdjan, > I did test these patches now, but I am sorry: > > The test Search.t is failing after this patchset... Don't be sorry, thanks for testing. I've decided that Search.t is broken, and there's no use changing it at this stage, so I dropped that patch. > virtualshelves on intranet and opac goes to ISE: > > DBIx::Class::ResultSet::_construct_results(): Unable to properly collapse > has_many results in iterator mode due to order criteria - performed an eager > cursor slurp underneath. Consider using ->all() instead at > /home/vagrant/kohaclone/Koha/Objects.pm line 209 > Can't use string ("1") as an ARRAY ref while "strict refs" in use at > /home/vagrant/kohaclone/C4/XSLT.pm line 290. Can you please tell me the steps to get that. Thanks.
No longer valid.