Bug 11213 - GetItemsInfo() called twice
Summary: GetItemsInfo() called twice
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---
Assignee: Srdjan Jankovic
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15342
  Show dependency treegraph
 
Reported: 2013-11-07 07:32 UTC by Srdjan Jankovic
Modified: 2017-11-07 01:48 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
bug_11213: Changed XSLTParse4Display() interface (18.49 KB, patch)
2013-11-08 03:16 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (19.09 KB, patch)
2014-03-17 03:06 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (17.62 KB, patch)
2014-03-18 00:07 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.97 KB, patch)
2014-03-18 00:07 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11213: Changed XSLTParse4Display() interface (18.88 KB, patch)
2014-03-21 03:14 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11213: whitespace correction (4.11 KB, patch)
2014-03-21 03:14 UTC, M. Tompsett
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.75 KB, patch)
2014-03-31 03:14 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: C4::VirtualShelves::Page::shelf_contents() (9.93 KB, patch)
2014-03-31 03:55 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: C4::VirtualShelves::Page::shelf_contents() (10.07 KB, patch)
2014-04-01 00:25 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL (2.36 KB, patch)
2014-04-01 05:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.29 KB, patch)
2014-04-01 05:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.71 KB, patch)
2014-04-01 06:17 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2014-04-01 06:19 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (17.60 KB, patch)
2014-07-08 04:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.97 KB, patch)
2014-07-08 04:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.79 KB, patch)
2014-07-08 04:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: C4::VirtualShelves::Page::shelf_contents() (10.06 KB, patch)
2014-07-08 04:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Use branch codes from the database rather than hardcoded CPL and MPL (2.24 KB, patch)
2014-07-08 04:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.29 KB, patch)
2014-07-08 04:13 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.72 KB, patch)
2014-07-08 04:13 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2014-07-08 04:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (17.37 KB, patch)
2015-04-28 04:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.97 KB, patch)
2015-04-28 04:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.79 KB, patch)
2015-04-28 04:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: C4::VirtualShelves::Page::shelf_contents() (10.88 KB, patch)
2015-04-28 04:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.29 KB, patch)
2015-04-28 04:16 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.72 KB, patch)
2015-04-28 04:17 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2015-04-28 04:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (17.34 KB, patch)
2015-05-11 00:08 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.21 KB, patch)
2015-05-11 00:08 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.79 KB, patch)
2015-05-11 00:08 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: C4::VirtualShelves::Page::shelf_contents() (10.88 KB, patch)
2015-05-11 00:08 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.29 KB, patch)
2015-05-11 00:08 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.72 KB, patch)
2015-05-11 00:09 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2015-05-11 00:09 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (17.34 KB, patch)
2015-09-02 16:24 UTC, M. Tompsett
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.21 KB, patch)
2015-09-02 16:24 UTC, M. Tompsett
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.79 KB, patch)
2015-09-02 16:24 UTC, M. Tompsett
Details | Diff | Splinter Review
bug_11213: C4::VirtualShelves::Page::shelf_contents() (10.87 KB, patch)
2015-09-02 16:24 UTC, M. Tompsett
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.29 KB, patch)
2015-09-02 16:25 UTC, M. Tompsett
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.72 KB, patch)
2015-09-02 16:25 UTC, M. Tompsett
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2015-09-02 16:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11213: Fix mixing Koha::Database and C4::Context->dbh in test (2.79 KB, patch)
2015-09-07 06:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (15.11 KB, patch)
2016-03-31 23:45 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.25 KB, patch)
2016-03-31 23:46 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.79 KB, patch)
2016-03-31 23:47 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.26 KB, patch)
2016-03-31 23:47 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.67 KB, patch)
2016-03-31 23:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2016-03-31 23:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (12.84 KB, patch)
2016-04-13 01:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.25 KB, patch)
2016-04-13 01:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.79 KB, patch)
2016-04-13 01:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.26 KB, patch)
2016-04-13 01:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.67 KB, patch)
2016-04-13 01:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2016-04-13 01:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (13.94 KB, patch)
2016-08-15 03:19 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.25 KB, patch)
2016-08-15 03:19 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.79 KB, patch)
2016-08-15 03:19 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.26 KB, patch)
2016-08-15 03:20 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.67 KB, patch)
2016-08-15 03:20 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2016-08-15 03:22 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (14.09 KB, patch)
2016-11-17 05:04 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.25 KB, patch)
2016-11-17 05:04 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.55 KB, patch)
2016-11-17 05:05 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.06 KB, patch)
2016-11-17 05:05 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.67 KB, patch)
2016-11-17 05:05 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2016-11-17 05:06 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (14.00 KB, patch)
2017-04-20 03:35 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.19 KB, patch)
2017-04-20 03:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.55 KB, patch)
2017-04-20 03:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.06 KB, patch)
2017-04-20 03:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.67 KB, patch)
2017-04-20 03:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.39 KB, patch)
2017-04-20 03:37 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.41 KB, patch)
2017-10-26 23:23 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Changed XSLTParse4Display() interface (14.10 KB, patch)
2017-10-30 03:17 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: whitespace correction (3.19 KB, patch)
2017-10-30 03:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Include XSLT processing for searchResults() test (1.55 KB, patch)
2017-10-30 03:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: GetItemsInfo() test (1.06 KB, patch)
2017-10-30 03:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Added XSLTParse4Display() to Items test (1.70 KB, patch)
2017-10-30 03:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11213: Check for $item->{itype} presence to avoid warning (1.41 KB, patch)
2017-10-30 03:19 UTC, Srdjan Jankovic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2013-11-07 07:32:36 UTC
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.
Comment 1 Srdjan Jankovic 2013-11-08 03:16:09 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-11-08 03:33:57 UTC
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.
Comment 3 mathieu saby 2013-11-08 21:43:59 UTC
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
Comment 4 Srdjan Jankovic 2013-11-10 23:08:16 UTC
I believe that's exactly where it shows most benefits.
Comment 5 mathieu saby 2013-11-11 09:23:56 UTC
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
Comment 6 Srdjan Jankovic 2013-11-12 00:11:14 UTC
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.
Comment 7 mathieu saby 2013-11-12 08:26:44 UTC
(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
Comment 8 mathieu saby 2013-11-12 13:22:04 UTC
(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
Comment 9 M. Tompsett 2014-03-16 17:03:50 UTC
Why did you remove the hidden stuff?
Comment 10 Srdjan Jankovic 2014-03-16 22:01:49 UTC
(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?
Comment 11 M. Tompsett 2014-03-17 00:19:17 UTC
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.
Comment 12 Srdjan Jankovic 2014-03-17 00:37:36 UTC
(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 13 M. Tompsett 2014-03-17 02:32:09 UTC
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->...?
Comment 14 Srdjan Jankovic 2014-03-17 03:06:44 UTC Comment hidden (obsolete)
Comment 15 M. Tompsett 2014-03-17 03:23:37 UTC
Sorry, but could you put the whitespace fixes in a separate patch? It makes reading the actual code changes easier. :)
Comment 16 Srdjan Jankovic 2014-03-18 00:07:16 UTC Comment hidden (obsolete)
Comment 17 Srdjan Jankovic 2014-03-18 00:07:35 UTC Comment hidden (obsolete)
Comment 18 M. Tompsett 2014-03-18 05:10:25 UTC
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.
Comment 19 M. Tompsett 2014-03-21 03:14:14 UTC Comment hidden (obsolete)
Comment 20 M. Tompsett 2014-03-21 03:14:24 UTC Comment hidden (obsolete)
Comment 21 M. Tompsett 2014-03-21 03:17:09 UTC
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.
Comment 22 Srdjan Jankovic 2014-03-31 03:14:58 UTC Comment hidden (obsolete)
Comment 23 Srdjan Jankovic 2014-03-31 03:55:46 UTC Comment hidden (obsolete)
Comment 24 Srdjan Jankovic 2014-04-01 00:25:18 UTC Comment hidden (obsolete)
Comment 25 M. Tompsett 2014-04-01 00:36:44 UTC
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.
Comment 26 Srdjan Jankovic 2014-04-01 05:48:33 UTC Comment hidden (obsolete)
Comment 27 Srdjan Jankovic 2014-04-01 05:48:43 UTC Comment hidden (obsolete)
Comment 28 Srdjan Jankovic 2014-04-01 06:17:38 UTC Comment hidden (obsolete)
Comment 29 Srdjan Jankovic 2014-04-01 06:19:15 UTC Comment hidden (obsolete)
Comment 30 M. Tompsett 2014-04-01 10:20:44 UTC
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.
Comment 31 Srdjan Jankovic 2014-04-01 23:18:32 UTC
(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.
Comment 32 Bernardo Gonzalez Kriegel 2014-06-23 01:13:12 UTC
First two are signed.
Test plan for the remaining?
Comment 33 Srdjan Jankovic 2014-06-30 01:40:10 UTC
(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?
Comment 34 M. Tompsett 2014-06-30 04:14:26 UTC
I'd start at comment 25. :)
Comment 35 Bernardo Gonzalez Kriegel 2014-07-03 22:47:13 UTC
(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.
Comment 36 Srdjan Jankovic 2014-07-08 04:11:37 UTC Comment hidden (obsolete)
Comment 37 Srdjan Jankovic 2014-07-08 04:12:14 UTC Comment hidden (obsolete)
Comment 38 Srdjan Jankovic 2014-07-08 04:12:29 UTC Comment hidden (obsolete)
Comment 39 Srdjan Jankovic 2014-07-08 04:12:39 UTC Comment hidden (obsolete)
Comment 40 Srdjan Jankovic 2014-07-08 04:12:49 UTC Comment hidden (obsolete)
Comment 41 Srdjan Jankovic 2014-07-08 04:13:03 UTC Comment hidden (obsolete)
Comment 42 Srdjan Jankovic 2014-07-08 04:13:18 UTC Comment hidden (obsolete)
Comment 43 Srdjan Jankovic 2014-07-08 04:15:35 UTC Comment hidden (obsolete)
Comment 44 Srdjan Jankovic 2014-07-08 04:47:38 UTC
(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.
Comment 45 Srdjan Jankovic 2015-04-28 04:15:08 UTC Comment hidden (obsolete)
Comment 46 Srdjan Jankovic 2015-04-28 04:15:22 UTC Comment hidden (obsolete)
Comment 47 Srdjan Jankovic 2015-04-28 04:15:38 UTC Comment hidden (obsolete)
Comment 48 Srdjan Jankovic 2015-04-28 04:15:52 UTC Comment hidden (obsolete)
Comment 49 Srdjan Jankovic 2015-04-28 04:16:44 UTC Comment hidden (obsolete)
Comment 50 Srdjan Jankovic 2015-04-28 04:17:06 UTC Comment hidden (obsolete)
Comment 51 Srdjan Jankovic 2015-04-28 04:18:01 UTC Comment hidden (obsolete)
Comment 52 Srdjan Jankovic 2015-05-11 00:08:02 UTC Comment hidden (obsolete)
Comment 53 Srdjan Jankovic 2015-05-11 00:08:16 UTC Comment hidden (obsolete)
Comment 54 Srdjan Jankovic 2015-05-11 00:08:28 UTC Comment hidden (obsolete)
Comment 55 Srdjan Jankovic 2015-05-11 00:08:35 UTC Comment hidden (obsolete)
Comment 56 Srdjan Jankovic 2015-05-11 00:08:44 UTC Comment hidden (obsolete)
Comment 57 Srdjan Jankovic 2015-05-11 00:09:08 UTC Comment hidden (obsolete)
Comment 58 Srdjan Jankovic 2015-05-11 00:09:19 UTC Comment hidden (obsolete)
Comment 59 Marcel de Rooy 2015-08-28 09:36:13 UTC
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.
Comment 60 Srdjan Jankovic 2015-08-31 02:18:42 UTC
(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?
Comment 61 Marcel de Rooy 2015-09-01 10:50:02 UTC
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.
Comment 62 M. Tompsett 2015-09-01 18:21:08 UTC
(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?
Comment 63 Marcel de Rooy 2015-09-02 13:01:30 UTC
(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..
Comment 64 M. Tompsett 2015-09-02 16:24:36 UTC Comment hidden (obsolete)
Comment 65 M. Tompsett 2015-09-02 16:24:42 UTC Comment hidden (obsolete)
Comment 66 M. Tompsett 2015-09-02 16:24:49 UTC Comment hidden (obsolete)
Comment 67 M. Tompsett 2015-09-02 16:24:55 UTC Comment hidden (obsolete)
Comment 68 M. Tompsett 2015-09-02 16:25:02 UTC Comment hidden (obsolete)
Comment 69 M. Tompsett 2015-09-02 16:25:08 UTC Comment hidden (obsolete)
Comment 70 M. Tompsett 2015-09-02 16:25:16 UTC Comment hidden (obsolete)
Comment 71 M. Tompsett 2015-09-02 16:28:38 UTC
(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.
Comment 72 Marcel de Rooy 2015-09-03 09:43:29 UTC
(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.
Comment 73 Marcel de Rooy 2015-09-03 10:04:07 UTC
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()
Comment 74 Srdjan Jankovic 2015-09-07 06:15:57 UTC Comment hidden (obsolete)
Comment 75 Srdjan Jankovic 2015-09-07 06:25:51 UTC
I believe it is very wrong to mix KOha::Database and C4::Context->dbh based tests. Those should be separated.
Comment 76 M. Tompsett 2015-09-07 13:17:37 UTC
I'm wondering if perhaps it should be something more like:
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14334#c0
Comment 77 Srdjan Jankovic 2015-09-07 23:32:59 UTC
(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?
Comment 78 Srdjan Jankovic 2016-03-31 23:45:11 UTC Comment hidden (obsolete)
Comment 79 Srdjan Jankovic 2016-03-31 23:46:53 UTC Comment hidden (obsolete)
Comment 80 Srdjan Jankovic 2016-03-31 23:47:17 UTC Comment hidden (obsolete)
Comment 81 Srdjan Jankovic 2016-03-31 23:47:35 UTC Comment hidden (obsolete)
Comment 82 Srdjan Jankovic 2016-03-31 23:48:04 UTC Comment hidden (obsolete)
Comment 83 Srdjan Jankovic 2016-03-31 23:48:38 UTC Comment hidden (obsolete)
Comment 84 Owen Leonard 2016-04-12 16:29:36 UTC
(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.
Comment 85 Srdjan Jankovic 2016-04-13 01:11:12 UTC Comment hidden (obsolete)
Comment 86 Srdjan Jankovic 2016-04-13 01:11:25 UTC Comment hidden (obsolete)
Comment 87 Srdjan Jankovic 2016-04-13 01:11:40 UTC Comment hidden (obsolete)
Comment 88 Srdjan Jankovic 2016-04-13 01:11:50 UTC Comment hidden (obsolete)
Comment 89 Srdjan Jankovic 2016-04-13 01:11:59 UTC Comment hidden (obsolete)
Comment 90 Srdjan Jankovic 2016-04-13 01:12:09 UTC Comment hidden (obsolete)
Comment 91 Srdjan Jankovic 2016-04-13 01:14:57 UTC
(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.
Comment 92 Owen Leonard 2016-04-13 16:51:35 UTC
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.
Comment 93 Srdjan Jankovic 2016-04-14 00:47:39 UTC
(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?
Comment 94 Owen Leonard 2016-04-14 12:13:21 UTC
(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.
Comment 95 Srdjan Jankovic 2016-04-15 03:25:02 UTC
(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?
Comment 96 Katrin Fischer 2016-04-18 21:29:48 UTC
(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.
Comment 97 Srdjan Jankovic 2016-04-19 00:51:43 UTC
(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
Comment 98 Katrin Fischer 2016-08-14 10:42:09 UTC
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
...
Comment 99 Srdjan Jankovic 2016-08-15 03:19:04 UTC Comment hidden (obsolete)
Comment 100 Srdjan Jankovic 2016-08-15 03:19:28 UTC Comment hidden (obsolete)
Comment 101 Srdjan Jankovic 2016-08-15 03:19:46 UTC Comment hidden (obsolete)
Comment 102 Srdjan Jankovic 2016-08-15 03:20:27 UTC Comment hidden (obsolete)
Comment 103 Srdjan Jankovic 2016-08-15 03:20:44 UTC Comment hidden (obsolete)
Comment 104 Srdjan Jankovic 2016-08-15 03:22:05 UTC Comment hidden (obsolete)
Comment 105 M. Tompsett 2016-11-16 22:53:41 UTC
Sorry for being slow on the draw.
Comment 106 Srdjan Jankovic 2016-11-17 05:04:33 UTC Comment hidden (obsolete)
Comment 107 Srdjan Jankovic 2016-11-17 05:04:51 UTC Comment hidden (obsolete)
Comment 108 Srdjan Jankovic 2016-11-17 05:05:05 UTC Comment hidden (obsolete)
Comment 109 Srdjan Jankovic 2016-11-17 05:05:18 UTC Comment hidden (obsolete)
Comment 110 Srdjan Jankovic 2016-11-17 05:05:31 UTC Comment hidden (obsolete)
Comment 111 Srdjan Jankovic 2016-11-17 05:06:36 UTC Comment hidden (obsolete)
Comment 112 M. Tompsett 2016-11-17 05:36:47 UTC
How might this interact with bug 17527?
Comment 113 Srdjan Jankovic 2016-11-17 06:53:57 UTC
I guess whoever comes second will have to resolve a minor conflict.
Comment 114 Srdjan Jankovic 2017-04-20 03:35:54 UTC Comment hidden (obsolete)
Comment 115 Srdjan Jankovic 2017-04-20 03:36:09 UTC Comment hidden (obsolete)
Comment 116 Srdjan Jankovic 2017-04-20 03:36:24 UTC Comment hidden (obsolete)
Comment 117 Srdjan Jankovic 2017-04-20 03:36:39 UTC Comment hidden (obsolete)
Comment 118 Srdjan Jankovic 2017-04-20 03:36:54 UTC Comment hidden (obsolete)
Comment 119 Srdjan Jankovic 2017-04-20 03:37:22 UTC Comment hidden (obsolete)
Comment 120 Srdjan Jankovic 2017-10-26 23:23:17 UTC Comment hidden (obsolete)
Comment 121 Josef Moravec 2017-10-29 19:50:11 UTC
Needs adaptation at least because of bug 17248
Comment 122 Srdjan Jankovic 2017-10-30 03:17:52 UTC
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.
Comment 123 Srdjan Jankovic 2017-10-30 03:18:07 UTC
Created attachment 68819 [details] [review]
bug_11213: whitespace correction
Comment 124 Srdjan Jankovic 2017-10-30 03:18:19 UTC
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
Comment 125 Srdjan Jankovic 2017-10-30 03:18:31 UTC
Created attachment 68821 [details] [review]
bug_11213: GetItemsInfo() test
Comment 126 Srdjan Jankovic 2017-10-30 03:18:43 UTC
Created attachment 68822 [details] [review]
bug_11213: Added XSLTParse4Display() to Items test
Comment 127 Srdjan Jankovic 2017-10-30 03:19:39 UTC
Created attachment 68823 [details] [review]
bug_11213: Check for $item->{itype} presence to avoid warning
Comment 128 Josef Moravec 2017-11-03 17:00:05 UTC
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 129 Srdjan Jankovic 2017-11-07 01:43:27 UTC
Comment on attachment 68820 [details] [review]
bug_11213: Include XSLT processing for searchResults() test

Not required
Comment 130 Srdjan Jankovic 2017-11-07 01:48:47 UTC
(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.