[Bug 11213] New: GetItemsInfo() called twice
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Bug ID: 11213 Summary: GetItemsInfo() called twice Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: OPAC Assignee: oleonard@myacpl.org Reporter: srdjan@catalyst.net.nz QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oleonard@myacpl.org |srdjan@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #1 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 22800 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22800&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P3 --- Comment #2 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 mathieu saby <mathieu.saby@univ-rennes2.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mathieu.saby@univ-rennes2.f | |r --- Comment #3 from mathieu saby <mathieu.saby@univ-rennes2.fr> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #4 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- I believe that's exactly where it shows most benefits. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #5 from mathieu saby <mathieu.saby@univ-rennes2.fr> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #6 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #7 from mathieu saby <mathieu.saby@univ-rennes2.fr> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #8 from mathieu saby <mathieu.saby@univ-rennes2.fr> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- Why did you remove the hidden stuff? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #10 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #12 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (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) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 22800 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22800 bug_11213: Changed XSLTParse4Display() interface Review of attachment 22800: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=11213&attachment=22800) ----------------------------------------------------------------- ::: 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->...? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22800|0 |1 is obsolete| | --- Comment #14 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26394 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26394&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #15 from M. Tompsett <mtompset@hotmail.com> --- Sorry, but could you put the whitespace fixes in a separate patch? It makes reading the actual code changes easier. :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26394|0 |1 is obsolete| | --- Comment #16 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26430 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26430&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #17 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26431 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26431&action=edit bug_11213: whitespace correction -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #18 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26430|0 |1 is obsolete| | --- Comment #19 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 26499 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26499&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26431|0 |1 is obsolete| | --- Comment #20 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 26500 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26500&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #21 from M. 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #22 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26695 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26695&action=edit bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #23 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26696 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26696&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26696|0 |1 is obsolete| | --- Comment #24 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26713 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26713&action=edit 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 are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #25 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #26 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26718 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26718&action=edit bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #27 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26719 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26719&action=edit bug_11213: GetItemsInfo() test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #28 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26720 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26720&action=edit bug_11213: Added XSLTParse4Display() to Items test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #29 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 26721 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26721&action=edit bug_11213: Check for $item->{itype} presence to avoid warning -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #30 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 26718 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26718 bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL Review of attachment 26718: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=11213&attachment=26718) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #31 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (In reply to M. Tompsett from comment #30)
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.
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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bgkriegel@gmail.com --- Comment #32 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- First two are signed. Test plan for the remaining? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #33 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #34 from M. Tompsett <mtompset@hotmail.com> --- I'd start at comment 25. :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #35 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26499|0 |1 is obsolete| | --- Comment #36 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29557 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29557&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26500|0 |1 is obsolete| | --- Comment #37 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29558 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29558&action=edit bug_11213: whitespace correction -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26695|0 |1 is obsolete| | --- Comment #38 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29559 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29559&action=edit bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26713|0 |1 is obsolete| | --- Comment #39 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29560 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29560&action=edit 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 are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26718|0 |1 is obsolete| | --- Comment #40 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29561 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29561&action=edit bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26719|0 |1 is obsolete| | --- Comment #41 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29562 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29562&action=edit bug_11213: GetItemsInfo() test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26720|0 |1 is obsolete| | --- Comment #42 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29563 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29563&action=edit bug_11213: Added XSLTParse4Display() to Items test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26721|0 |1 is obsolete| | --- Comment #43 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 29564 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29564&action=edit bug_11213: Check for $item->{itype} presence to avoid warning -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #44 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29557|0 |1 is obsolete| | --- Comment #45 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 38584 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38584&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29558|0 |1 is obsolete| | --- Comment #46 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 38585 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38585&action=edit bug_11213: whitespace correction -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29559|0 |1 is obsolete| | --- Comment #47 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 38586 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38586&action=edit bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29560|0 |1 is obsolete| | --- Comment #48 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 38587 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38587&action=edit 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 are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29561|0 |1 is obsolete| | Attachment #29562|0 |1 is obsolete| | --- Comment #49 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 38588 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38588&action=edit bug_11213: GetItemsInfo() test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29563|0 |1 is obsolete| | --- Comment #50 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 38589 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38589&action=edit bug_11213: Added XSLTParse4Display() to Items test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29564|0 |1 is obsolete| | --- Comment #51 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 38590 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38590&action=edit bug_11213: Check for $item->{itype} presence to avoid warning -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38584|0 |1 is obsolete| | --- Comment #52 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 39023 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39023&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38585|0 |1 is obsolete| | --- Comment #53 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 39024 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39024&action=edit bug_11213: whitespace correction -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38586|0 |1 is obsolete| | --- Comment #54 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 39025 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39025&action=edit bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38587|0 |1 is obsolete| | --- Comment #55 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 39026 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39026&action=edit 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 are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38588|0 |1 is obsolete| | --- Comment #56 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 39027 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39027&action=edit bug_11213: GetItemsInfo() test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38589|0 |1 is obsolete| | --- Comment #57 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 39028 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39028&action=edit bug_11213: Added XSLTParse4Display() to Items test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38590|0 |1 is obsolete| | --- Comment #58 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 39029 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39029&action=edit bug_11213: Check for $item->{itype} presence to avoid warning -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|Needs Signoff |Patch doesn't apply --- Comment #59 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #60 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #61 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #62 from M. Tompsett <mtompset@hotmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #63 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39023|0 |1 is obsolete| | --- Comment #64 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 42234 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42234&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39024|0 |1 is obsolete| | --- Comment #65 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 42235 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42235&action=edit bug_11213: whitespace correction -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39025|0 |1 is obsolete| | --- Comment #66 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 42236 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42236&action=edit bug_11213: Include XSLT processing for searchResults() test * Added template paths to temp test dir, so XSLT templates can be picked up -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39026|0 |1 is obsolete| | --- Comment #67 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 42237 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42237&action=edit 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 are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39027|0 |1 is obsolete| | --- Comment #68 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 42238 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42238&action=edit bug_11213: GetItemsInfo() test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39028|0 |1 is obsolete| | --- Comment #69 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 42239 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42239&action=edit bug_11213: Added XSLTParse4Display() to Items test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39029|0 |1 is obsolete| | --- Comment #70 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 42240 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42240&action=edit bug_11213: Check for $item->{itype} presence to avoid warning -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #71 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #72 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #73 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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() -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #74 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 42425 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42425&action=edit Bug 11213: Fix mixing Koha::Database and C4::Context->dbh in test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #75 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- I believe it is very wrong to mix KOha::Database and C4::Context->dbh based tests. Those should be separated. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #76 from M. Tompsett <mtompset@hotmail.com> --- I'm wondering if perhaps it should be something more like: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14334#c0 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213 --- Comment #77 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (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? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org