I would definitely consider this a bug due to the fact that when not using XSLT the results page shows the holding library, but when using XSLT the results display the home library. I would consider that default ( non-XSLT ) results to be canonical and thus the XSLT results should be default replicate the standard results.

This is going to have to be a syspref - people are going to want to choose which to display on the OPAC. 

NEKLS, for example, would not want to see the holding branch. They would want to see the home branch.

(In reply to comment #4)
> This is going to have to be a syspref - people are going to want to choose
> which to display on the OPAC. 
> 
> NEKLS, for example, would not want to see the holding branch. They would
> want to see the home branch.

The idea behind using XSLT is to add the ability to customize the results and details, rather than displaying Koha's default way of doing so. You would still be able to customize your xslt so that the home branch is displayed. The idea behind this patch is simply to bring the default xslt view in line with the default non-xslt view. It seems rather counter-intuitive for the default non-xslt to display the holding library, but for the default xslt to display the home library instead.

Kyle, I agree this is definitely a bug, but the problem in my eyes is with the "regular" view not the XSLT view. I think a syspref would be a good choice for this, because the patch as it stands eliminates functionality in the system for the sake of consistency (not that there's anything wrong with consistency, but I'd rather a slightly inconsistent interface that works for more people than a consistent interface that works for fewer).

I think we can't make a change like that without making it configurable and leaving and defaulting to the current display. For me home branch is the logical choice for display. Holding branch probably makes sense when you have floating collections, but for most scenarios I can think of displaying the home branch would be what is expected.

Just to add my voice to chorus of fix the default view with a syspref.

My third followup to add a syspref causes XSLT errors. If anyone can help me out I would greatly appreciate it.

compilation error: file /home/koha/kohaclone/koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl element if
element if only allowed within a template, variable or param
compilation error: file /home/koha/kohaclone/koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl element key
element key only allowed as child of stylesheet
compilation error: file /home/koha/kohaclone/koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl element if
element if only allowed within a template, variable or param
compilation error: file /home/koha/kohaclone/koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl element key
element key only allowed as child of stylesheet
compilation error: file /home/koha/kohaclone/koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl element if
xsltParseStylesheetTop: unknown if element
compilation error: file /home/koha/kohaclone/koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl element if
xsltParseStylesheetTop: unknown if element
 at /home/koha/kohaclone/C4/XSLT.pm line 219

You should be using xsl:choose / xsl:when for those kinds of conditionals I think. xsl:if tends to be used for doing things like assigning a value based on a choice. I guess they're semantically different (I expect that xsl:if shortcuts, but I haven't finished reading the book on XSLT.)

From reading the error messages I suspect it's this part:

<xsl:key name="item-by-status" match="items:item" use="items:status"/>
-    <xsl:key name="item-by-status-and-branch" match="items:item" use="concat(items:status, ' ', items:holdingbranch)"/>
+    <xsl:if test="$OPACResultsBranchXSLT='holdingbranch'">
+        <xsl:key name="item-by-status-and-branch" match="items:item" use="concat(items:status, ' ', items:holdingbranch)"/>
+    </xsl:if>
+    <xsl:if test="$OPACResultsBranchXSLT='homebranch'">
+        <xsl:key name="item-by-status-and-branch" match="items:item" use="concat(items:status, ' ', items:homebranch)"/>
+    </xsl:if>

Yeah, ignore what I said, it's not right :)

I recommend using "items:$OPACResultsByBranchXSLT" in your key (eliminating the if). Also, don't forget to declare your variables, and note that you will need to declare the variable prior to its use. Also, remember to use <choose> instead of if when choosing between multiple options (in the "Copies available for loan" stanza).

I think I've got everything taken care of now. This new patch is a squashed version of the previous three patches, along with a fixed version of the xslt file.

I have the value 'default' in the preference systeme OPACXSLTResultsDisplay
I have the value 'holdingbranch' in the in the preference systeme OPACResultsBranchXSLT 

but in the opac results the homebranch is display instead of the holdingbranch

Is this the right test?...

This no longer applies for me.

merge marker in the updatedatabase.pl file

+<<<<<<< HEAD

Why add a preference which only affects the XSLT view? Why not have it affect both?

Test Plan:
1) Apply all the patches
2) Run updatedatabase.pl
4) Run a search that will show items with different home and holding branches
3) Test both settings of OPACResultsBranch with OPACXSLTResultsDisplay disabled and also set to 'default'

Tried to test, but first patch doesn't apply
 
>Test Plan:
>1) Apply all the patches

~/kohaclone$ git bz apply 7441
...
Patch failed at 0001 Bug 7441 - search results showing wrong branch

QA comment:
 * patch does not apply anymore
 * in the 3rd patch, the Koha/Template/Plugin/Koha.pm is improperly named I think, it should be Systempreference.pm or something more meaningful than "Koha". I also think this new plugin is cool and could be in a separate bug, to be clearly visible in the release notes. Because it means we can improve performances by removing 
    # these template parameters are set the same regardless of $in->{'type'}
    $template->param(
            "BiblioDefaultView".C4::Context->preference("BiblioDefaultView")         => 1,
            EnhancedMessagingPreferences => C4::Context->preference('EnhancedMessagingPreferences'),
            GoogleJackets                => C4::Context->preference("GoogleJackets"),
            OpenLibraryCovers            => C4::Context->preference("OpenLibraryCovers"),
            KohaAdminEmailAddress        => "" . C4::Context->preference("KohaAdminEmailAddress"),
            LoginBranchcode              => (C4::Context->userenv?C4::Context->userenv->{"branch"}:"insecure"),
            LoginFirstname               => (C4::Context->userenv?C4::Context->userenv->{"firstname"}:"Bel"),
            LoginSurname                 => C4::Context->userenv?C4::Context->userenv->{"surname"}:"Inconnu",
            emailaddress                 => C4::Context->userenv?C4::Context->userenv->{"emailaddress"}:undef,
            ...
in C4/Auth.pm, in favour of adding [% use Systempreference %] and [% Systempreference.preference('What you need') %] where you need a given systempref. This change will be a long term goal, and deserve an OMNIBUS patch.
At the end of this message, my request for this 2nd point is to create an omnibus to remove all those C4/Auth.pm lines.

>  * in the 3rd patch, the Koha/Template/Plugin/Koha.pm is improperly named I
> think, it should be Systempreference.pm or something more meaningful than
> "Koha".

I think we need a Koha-level TT plugin the same way we have a Koha.pm. It should be a place to put all the once-off plugin logic that does not have multiple functions.

If we have a separate TT plugin for each action like this, we will end up with a dozen or more plugins, each of which will need a USE line at the top of each template. That seems unnecessary, doesn't it? [% Koha.Preference( PrefName ) %] seems more clean and sensible than [% KohaPreferences.Preference( PrefName ) %].

If you disagree with my logic here, just reset the status to Failed QA and I will make the requested modifications.

This patch now depends on bug 10206 for the Koha TT Plugin.

Hi Kyle, 

I had to rebase the first patch, because of a conflict in updatedatabase and found 1 problem:

Third patch contains those lines:
+[% USE Koha %]
+[% USE KohaBranchName %]
But fourth patch deletes the Koha TT plugin without removing them.

I think as a result of that the QA script notes a problem with opac-results.tt:

 * koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt                      FAIL
	forbidden patterns          OK
	tt_valid                    OK
	valid_template              FAIL
		Koha: plugin not found

Please fix and switch back to signed off if the only changes are the rebase and fixing the plugin problem.

Katrin, I split off the plugin to a separate bug and added it as a dependency on this bug. I'll still take are of the rebasing though.

Kyle

(In reply to comment #42)
> Hi Kyle, 
> 
> I had to rebase the first patch, because of a conflict in updatedatabase and
> found 1 problem:
> 
> Third patch contains those lines:
> +[% USE Koha %]
> +[% USE KohaBranchName %]
> But fourth patch deletes the Koha TT plugin without removing them.
> 
> I think as a result of that the QA script notes a problem with
> opac-results.tt:
> 
>  * koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt                     
> FAIL
> 	forbidden patterns          OK
> 	tt_valid                    OK
> 	valid_template              FAIL
> 		Koha: plugin not found
> 
> Please fix and switch back to signed off if the only changes are the rebase
> and fixing the plugin problem.

I'm Chris Rohde from Roseville Public Library, Roseville, California. I have tested this patch, both with and without XSLT, using the test plan in Comment 30. I would like to sign it off in my name.

Hey, thanks Chris C for adding Chris R's sign off.  I had that on my list to do today, and I am pleased to find that you've already taken care of it.  Yay! :)

Melia

Patch doesn't apply there is a conflict in 
koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl

That I don't know how to fix

At some point the patches got out of order. I've reuploaded them in the corrected order and added numbering to the patch titles.

I think if we have UNIMARC and MARC21, we should not forget about NORMARC. Kyle, could you add a follow up for NORMARC please? I will continue testing these.

There is a problem in the non-XSLT display:

Availability: Copies available: Demo Library (1), Demo Library (1),

Items look like this:

home library:holding library

demo library:springfield
demo library:demo library

It works nicely for the XSLT view.

Does this patch have to work for Non-XSLT? Can't we say it's an XSLT Only thing?  Can we get this moving again?

Hi Nicole, we already have some features that work only with the XSLT view, so I think this probably should not block the feature. But I am not sure if my tests showed a problem in the non-XSLT views that was caused by the patches. I think we need to retest.

Patch no longer applies, I also see this problem in master so updating both status and version

Still valid in current master

I have extracted the UNIMARC XSLT changes from the patch (as it was before the last patchset).
So if I understand everything correctly, we still need a signoff from an UNIMARC user and the changes to the NORMARC XSLT, that's it?

I am wondering if it would not be easier to use something like items.$pref_value instead of the xsl:choose blocks which copy/paste code.

(In reply to Jonathan Druart from comment #76)
> I have extracted the UNIMARC XSLT changes from the patch (as it was before
> the last patchset).
> So if I understand everything correctly, we still need a signoff from an
> UNIMARC user and the changes to the NORMARC XSLT, that's it?

Yes, it would be good to have a unimarc s/o.

> I am wondering if it would not be easier to use something like
> items.$pref_value instead of the xsl:choose blocks which copy/paste code.

I'm not sure what you mean, but if you want to submit a followup to make that change I'd be happy to test and sign off on it!

(In reply to Kyle M Hall from comment #77)
> > I am wondering if it would not be easier to use something like
> > items.$pref_value instead of the xsl:choose blocks which copy/paste code.
> 
> I'm not sure what you mean, but if you want to submit a followup to make
> that change I'd be happy to test and sign off on it!

Sorry but I really don't want to write xslt code.

Waiting for a unimarc signoff!

Jonathan, would you be ok with me testing with a UNIMARC sample database (from the sandboxes I think)?

(In reply to Katrin Fischer from comment #79)
> Jonathan, would you be ok with me testing with a UNIMARC sample database
> (from the sandboxes I think)?

Yes, of course!

When executing atomicupdate/bug_7441.sql, I get this error:

Bareword found where operator expected at ./installer/data/mysql/atomicupdate/bug_7441.sql line 1, near ") VALUES"
        (Missing operator before VALUES?)
syntax error at ./installer/data/mysql/atomicupdate/bug_7441.sql line 2, near ") VALUES
"

(In reply to Frédéric Demians from comment #81)
> When executing atomicupdate/bug_7441.sql, I get this error:
> 
> Bareword found where operator expected at
> ./installer/data/mysql/atomicupdate/bug_7441.sql line 1, near ") VALUES"
>         (Missing operator before VALUES?)
> syntax error at ./installer/data/mysql/atomicupdate/bug_7441.sql line 2,
> near ") VALUES
> "

Forget this! The SQL statement works properly.

We got MARC21 and UNIMARC - but we are missing NORMARC still.

Created attachment 54101 [details] [review]
[PASSED QA] Bug 7441 - search results showing wrong branch?

When you search in the OPAC it shows you the HOME branch on the location
in XSLT, but if you click through to the detail page it shows you the
CURRENT BRANCH in the holdings table which is very confusing to patrons.
I don't know what's the right solution - home or holding branch, but they
should be the same in both places for the patron's sake. If you do the same
search in the staff client you see the right branch info on the search results
and on the detail page.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Search the catalog, you search should include results with items
   that have different home and holding libraries.
4) The results should look the same as before the patch
5) Change the system preference OPACResultsLibrary to "current location"
6) Refresh your page of search results
7) The results show now show the holding library instead of the home library

Signed-off-by: Barbara Walters <bwalters@ncrl.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54102 [details] [review]
[PASSED QA] Bug 7441 - Followup search results showing wrong branch [UNIMARC]

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
  Playing with OPACResultsLibrary syspref, biblio with items having different
  home/holding library are displayed properly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

This shouldn't break things for NORMARC, but I'd like Magnus to take a look. We should support NORMARC or note that we don't (I'd prefer the former).

(In reply to Katrin Fischer from comment #87)
> This shouldn't break things for NORMARC, but I'd like Magnus to take a look.
> We should support NORMARC or note that we don't (I'd prefer the former).

I am not sure it's a valid to fail QA these patches, or we need someone taking on NORMARC changes.
Especially when the patches are waiting for 4 years.

(In reply to Jonathan Druart from comment #88)
> I am not sure it's a valid to fail QA these patches, or we need someone
> taking on NORMARC changes.
> Especially when the patches are waiting for 4 years.

Please don't wait for me. :-) I'll have a look and do a followup later for NORMARC.

I didn't intend to block - just to look into it. Failed is to strong.. In Discussion has a bad reputation :)

Can we just add a little note to the pref for documentation sakes? I can do so in a follow-up.

Created attachment 54250 [details] [review]
Bug 7441: QA follow-up - Add note about missing NORMARC support

Adds a note about missing support for NORMARC (only supports
MARC21 and UNIMARC) to the system preference text.

Pushed to master for 16.11, thanks Jonathan, Katrin!

Pushed in 16.05. Will be in 16.05.03.

Pushed to 3.22.x, will be in 3.22.10

This has been backported to 3.22.x and 16.05.x but without the IGNORE keyword in the INSERT pref statement: it will lead to error on upgrade.