Bug 7441 - Search results showing wrong branch
Summary: Search results showing wrong branch
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Ian Walls
URL:
Keywords:
Depends on: 10206
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-12 19:08 UTC by Nicole C. Engard
Modified: 2017-12-07 22:15 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7441 - search results showing wrong branch - Part 1 - Add holdingbranch to xml (1.16 KB, patch)
2012-03-27 19:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch - Part 2 - Modify OPAC xsl file to display holding branch instead of home branch (4.07 KB, patch)
2012-03-27 19:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - Followup - Control display of branch via system preference (11.57 KB, patch)
2012-05-22 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch - Part 3 - Control display of branch via system preference (11.61 KB, patch)
2012-05-22 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch - Part 3 - Control display of branch via system preference (11.85 KB, patch)
2012-09-27 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch - Part 3 - Control display of branch via system preference (13.67 KB, patch)
2012-09-27 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch - Part 1 - Add holdingbranch to xml (1.20 KB, patch)
2012-09-27 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (15.26 KB, patch)
2012-09-27 13:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (15.78 KB, patch)
2012-12-03 19:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (15.31 KB, patch)
2012-12-04 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441: Followup search results showing wrong branch [UNIMARC] (9.21 KB, patch)
2012-12-21 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (18.63 KB, patch)
2013-01-02 15:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (18.42 KB, patch)
2013-01-02 15:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7441 - search results showing wrong branch (15.33 KB, patch)
2013-01-31 15:43 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7441: Followup search results showing wrong branch [UNIMARC] (9.28 KB, patch)
2013-01-31 15:43 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7441 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (18.65 KB, patch)
2013-01-31 15:43 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (17.12 KB, patch)
2013-05-02 15:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441: Followup search results showing wrong branch [UNIMARC] (9.27 KB, patch)
2013-05-02 15:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (18.56 KB, patch)
2013-05-02 15:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Delete Koha TT Plugin (2.22 KB, patch)
2013-05-06 18:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (17.19 KB, patch)
2013-05-15 13:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441: Followup search results showing wrong branch [UNIMARC] (9.27 KB, patch)
2013-05-15 13:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (18.60 KB, patch)
2013-05-15 13:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Delete Koha TT Plugin (2.22 KB, patch)
2013-05-15 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (16.69 KB, patch)
2013-06-11 19:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441: Followup search results showing wrong branch [UNIMARC] (9.27 KB, patch)
2013-06-11 19:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (16.67 KB, patch)
2013-06-11 19:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Fix XSLT for option "homebranch" + syspref description (3.63 KB, patch)
2013-06-13 15:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (16.78 KB, patch)
2013-06-18 08:02 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7441: Followup search results showing wrong branch [UNIMARC] (9.32 KB, patch)
2013-06-18 08:03 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (16.77 KB, patch)
2013-06-18 08:04 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch (16.81 KB, patch)
2013-07-08 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441: Followup search results showing wrong branch [UNIMARC] (9.32 KB, patch)
2013-07-08 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - Part 1 - search results showing wrong branch (16.82 KB, patch)
2013-07-08 12:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - Part 2 - Followup search results showing wrong branch [UNIMARC] (9.33 KB, patch)
2013-07-08 12:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - Part 3 - search results showing wrong branch? - Followup - Use syspref for XSLT and non-XSLT (16.80 KB, patch)
2013-07-08 12:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - Part 4 - search results showing wrong branch? - Followup - Fix XSLT for option "homebranch" + syspref description (3.64 KB, patch)
2013-07-08 12:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? (24.83 KB, patch)
2016-06-02 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? (24.88 KB, patch)
2016-06-23 16:50 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? (24.94 KB, patch)
2016-06-23 16:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 7441 - search results showing wrong branch? (18.04 KB, patch)
2016-06-24 15:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7441 - Followup search results showing wrong branch [UNIMARC] (7.23 KB, patch)
2016-06-24 15:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7441 - Followup search results showing wrong branch [UNIMARC] (7.49 KB, patch)
2016-08-05 18:12 UTC, Frédéric Demians
Details | Diff | Splinter Review
[PASSED QA] Bug 7441 - search results showing wrong branch? (18.10 KB, patch)
2016-08-07 08:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7441 - Followup search results showing wrong branch [UNIMARC] (7.56 KB, patch)
2016-08-07 08:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7441: QA follow-up - Add note about missing NORMARC support (1.08 KB, patch)
2016-08-10 05:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-01-12 19:08:29 UTC
When you search in the OPAC it shows you the HOME branch on the location in XSLT (see here: http://screencast.com/t/m870Mr8Wm), 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.

Nicole
Comment 1 Kyle M Hall 2012-03-27 19:24:19 UTC
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.
Comment 2 Kyle M Hall 2012-03-27 19:24:48 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-03-27 19:24:59 UTC Comment hidden (obsolete)
Comment 4 Liz Rea 2012-04-09 16:52:58 UTC
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.
Comment 5 Kyle M Hall 2012-04-10 16:34:09 UTC
(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.
Comment 6 Jared Camins-Esakov 2012-05-12 15:11:46 UTC
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).
Comment 7 Katrin Fischer 2012-05-12 16:20:58 UTC
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.
Comment 8 Chris Cormack 2012-05-14 00:01:56 UTC
Just to add my voice to chorus of fix the default view with a syspref.
Comment 9 Kyle M Hall 2012-05-22 13:41:10 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-05-22 13:42:07 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2012-05-22 13:43:09 UTC
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
Comment 12 Robin Sheat 2012-06-27 16:56:31 UTC
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.)
Comment 13 Katrin Fischer 2012-06-27 17:03:00 UTC
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>
Comment 14 Robin Sheat 2012-06-27 17:07:58 UTC
Yeah, ignore what I said, it's not right :)
Comment 15 Jared Camins-Esakov 2012-06-27 17:11:20 UTC
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).
Comment 16 Kyle M Hall 2012-09-27 12:09:39 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2012-09-27 12:56:52 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2012-09-27 12:57:49 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2012-09-27 13:00:52 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2012-09-27 13:03:37 UTC
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.
Comment 21 delaye 2012-09-28 13:43:25 UTC
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?...
Comment 22 Owen Leonard 2012-10-16 13:46:45 UTC
This no longer applies for me.
Comment 23 Kyle M Hall 2012-12-03 19:41:56 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2012-12-04 16:02:19 UTC
merge marker in the updatedatabase.pl file

+<<<<<<< HEAD
Comment 25 Kyle M Hall 2012-12-04 18:50:09 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2012-12-21 14:38:49 UTC Comment hidden (obsolete)
Comment 27 Owen Leonard 2012-12-31 15:30:37 UTC
Why add a preference which only affects the XSLT view? Why not have it affect both?
Comment 28 Kyle M Hall 2013-01-02 15:06:49 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-01-02 15:08:09 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2013-01-02 15:11:17 UTC
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'
Comment 31 Bernardo Gonzalez Kriegel 2013-01-21 23:49:01 UTC
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
Comment 32 Bernardo Gonzalez Kriegel 2013-01-31 15:43:18 UTC Comment hidden (obsolete)
Comment 33 Bernardo Gonzalez Kriegel 2013-01-31 15:43:32 UTC Comment hidden (obsolete)
Comment 34 Bernardo Gonzalez Kriegel 2013-01-31 15:43:42 UTC Comment hidden (obsolete)
Comment 35 Paul Poulain 2013-03-18 16:56:57 UTC
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.
Comment 36 Kyle M Hall 2013-05-02 15:10:06 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2013-05-02 15:10:15 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2013-05-02 15:10:29 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2013-05-02 15:15:58 UTC
>  * 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.
Comment 40 Kyle M Hall 2013-05-06 18:01:56 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2013-05-06 18:30:58 UTC
This patch now depends on bug 10206 for the Koha TT Plugin.
Comment 42 Katrin Fischer 2013-05-12 10:08:53 UTC
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.
Comment 43 Kyle M Hall 2013-05-13 12:35:53 UTC
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.
Comment 44 Kyle M Hall 2013-05-15 13:34:55 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2013-05-15 13:35:32 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2013-05-15 13:35:43 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2013-05-15 13:36:09 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2013-06-11 19:14:43 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2013-06-11 19:14:53 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2013-06-11 19:15:01 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2013-06-13 15:25:58 UTC Comment hidden (obsolete)
Comment 52 Chris Rohde 2013-06-17 16:29:45 UTC
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.
Comment 53 Chris Cormack 2013-06-18 08:02:01 UTC Comment hidden (obsolete)
Comment 54 Chris Cormack 2013-06-18 08:03:10 UTC Comment hidden (obsolete)
Comment 55 Chris Cormack 2013-06-18 08:04:12 UTC Comment hidden (obsolete)
Comment 56 Melia Meggs 2013-06-18 21:19:10 UTC
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
Comment 57 Chris Cormack 2013-07-07 04:15:31 UTC
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
Comment 58 Kyle M Hall 2013-07-08 12:15:43 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2013-07-08 12:15:59 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2013-07-08 12:17:40 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2013-07-08 12:17:52 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2013-07-08 12:18:04 UTC Comment hidden (obsolete)
Comment 63 Kyle M Hall 2013-07-08 12:18:14 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2013-07-08 12:19:00 UTC
At some point the patches got out of order. I've reuploaded them in the corrected order and added numbering to the patch titles.
Comment 65 Katrin Fischer 2013-08-03 19:53:05 UTC
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.
Comment 66 Katrin Fischer 2013-08-03 20:42:09 UTC
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.
Comment 67 Nicole C. Engard 2014-06-06 16:39:14 UTC
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?
Comment 68 Katrin Fischer 2014-06-29 20:24:01 UTC
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.
Comment 69 Nick Clemens 2014-07-03 14:49:19 UTC
Patch no longer applies, I also see this problem in master so updating both status and version
Comment 70 Nick Clemens 2016-05-12 21:25:11 UTC
Still valid in current master
Comment 71 Kyle M Hall 2016-06-02 14:05:51 UTC Comment hidden (obsolete)
Comment 72 Nicole C. Engard 2016-06-23 16:50:34 UTC Comment hidden (obsolete)
Comment 73 Nick Clemens 2016-06-23 16:53:54 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2016-06-24 15:09:14 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2016-06-24 15:09:28 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2016-06-24 15:12:31 UTC
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.
Comment 77 Kyle M Hall 2016-06-27 13:53:53 UTC
(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!
Comment 78 Jonathan Druart 2016-08-02 09:11:20 UTC
(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!
Comment 79 Katrin Fischer 2016-08-02 21:56:16 UTC
Jonathan, would you be ok with me testing with a UNIMARC sample database (from the sandboxes I think)?
Comment 80 Jonathan Druart 2016-08-03 07:56:03 UTC
(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!
Comment 81 Frédéric Demians 2016-08-05 17:58:33 UTC
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
"
Comment 82 Frédéric Demians 2016-08-05 18:12:14 UTC Comment hidden (obsolete)
Comment 83 Frédéric Demians 2016-08-05 18:22:24 UTC
(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.
Comment 84 Katrin Fischer 2016-08-07 08:34:49 UTC
We got MARC21 and UNIMARC - but we are missing NORMARC still.
Comment 85 Katrin Fischer 2016-08-07 08:51:21 UTC
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>
Comment 86 Katrin Fischer 2016-08-07 08:52:00 UTC
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>
Comment 87 Katrin Fischer 2016-08-07 08:54:39 UTC
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).
Comment 88 Jonathan Druart 2016-08-08 10:48:33 UTC
(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.
Comment 89 Magnus Enger 2016-08-08 10:53:47 UTC
(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.
Comment 90 Katrin Fischer 2016-08-08 11:13:51 UTC
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.
Comment 91 Katrin Fischer 2016-08-10 05:16:46 UTC
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.
Comment 92 Kyle M Hall 2016-08-10 13:14:48 UTC
Pushed to master for 16.11, thanks Jonathan, Katrin!
Comment 93 Frédéric Demians 2016-08-23 09:27:27 UTC
Pushed in 16.05. Will be in 16.05.03.
Comment 94 Julian Maurice 2016-08-23 14:19:41 UTC
Pushed to 3.22.x, will be in 3.22.10
Comment 95 Jonathan Druart 2016-08-31 08:37:49 UTC
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.