Bug 6919 - Implementing XSLTResultsDisplay
Summary: Implementing XSLTResultsDisplay
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: 3.6
Hardware: All All
: P1 - high major (vote)
Assignee: Alex Arnaud
QA Contact: Marcel de Rooy
URL:
Keywords:
: 4813 (view as bug list)
Depends on:
Blocks: 4032 7696
  Show dependency treegraph
 
Reported: 2011-09-26 13:34 UTC by Alex Arnaud
Modified: 2012-10-25 23:10 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (8.56 KB, patch)
2011-09-26 14:42 UTC, Alex Arnaud
Details | Diff | Splinter Review
Screenshot after applying patch (25.50 KB, image/png)
2011-09-26 16:35 UTC, Owen Leonard
Details
no titles after patch (108.89 KB, image/png)
2011-09-26 17:52 UTC, Nicole C. Engard
Details
before patch titles show (178.62 KB, image/png)
2011-09-26 17:53 UTC, Nicole C. Engard
Details
Replacement patch (8.57 KB, patch)
2011-09-27 08:29 UTC, Alex Arnaud
Details | Diff | Splinter Review
Replacement patch (8.63 KB, patch)
2011-09-27 15:53 UTC, Alex Arnaud
Details | Diff | Splinter Review
bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt (57.19 KB, patch)
2011-10-07 15:03 UTC, Alex Arnaud
Details | Diff | Splinter Review
Screenshot showing duplicate availability information (19.50 KB, image/png)
2011-10-19 13:41 UTC, Owen Leonard
Details
proposed patch if 4032 is apply after 6919 (1.18 KB, patch)
2012-02-03 16:08 UTC, Stéphane Delaune
Details | Diff | Splinter Review
[SIGNED-OFF] bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt (57.09 KB, patch)
2012-02-08 16:29 UTC, Owen Leonard
Details | Diff | Splinter Review
[PATCH] Bug 6919 - Follow-up - Implementing XSLTResultsDisplay (18.52 KB, patch)
2012-02-08 16:31 UTC, Owen Leonard
Details | Diff | Splinter Review
bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt (57.14 KB, patch)
2012-02-09 20:40 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6919 - Follow-up - Implementing XSLTResultsDisplay (19.60 KB, patch)
2012-02-09 20:41 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Follow up patch (2.05 KB, patch)
2012-02-27 10:51 UTC, Alex Arnaud
Details | Diff | Splinter Review
Picture of staff results (335.60 KB, image/jpeg)
2012-03-01 09:27 UTC, Marcel de Rooy
Details
[SIGNED_OFF] Follow up patch (2.56 KB, patch)
2012-03-01 09:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2011-09-26 13:34:26 UTC

    
Comment 1 Alex Arnaud 2011-09-26 14:42:28 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2011-09-26 16:35:57 UTC Comment hidden (obsolete)
Comment 3 Nicole C. Engard 2011-09-26 17:52:17 UTC
After applying this patch all the title information disappears from the search results.
Comment 4 Nicole C. Engard 2011-09-26 17:52:49 UTC Comment hidden (obsolete)
Comment 5 Nicole C. Engard 2011-09-26 17:53:06 UTC Comment hidden (obsolete)
Comment 6 Nicole C. Engard 2011-09-26 17:53:37 UTC
oops, i said the same thing owen did :)
Comment 7 Alex Arnaud 2011-09-27 08:29:11 UTC Comment hidden (obsolete)
Comment 8 Liz Rea 2011-09-27 14:56:28 UTC
Still no titles? http://screencast.com/t/kAuKVMLD (MARC21)
Comment 9 Alex Arnaud 2011-09-27 15:53:26 UTC Comment hidden (obsolete)
Comment 10 Nicole C. Engard 2011-09-27 20:54:36 UTC
Sorry, still not titles
Comment 11 Liz Rea 2011-09-27 21:34:34 UTC
Interesting, there were titles when I tested this earlier. But I still don't think I can pass it - it would be extremely confusing for libraries who have the intranetxsltresults pref - they would be expecting an *improvement* in the display, and as it was when I tested it, it was title only and none of the other things librarians would be expecting. 

This is a great patch to build off of, so I'm grateful to Alex for submitting it, but it will have to be built up before it can be included, imo.
Comment 12 Paul Poulain 2011-09-28 11:29:36 UTC
Liz & Nicole : the UNIMARC version is better. If you want a great MARC21 version, please give Alex the MARC21 fields to display.
Comment 13 Liz Rea 2011-09-28 19:51:41 UTC
I might suggest you split this into marc21 and unimarc patches, so that unimarc, if it passes a unimarc user's tests,  can be passed on it's own.
Comment 14 Alex Arnaud 2011-10-07 15:03:22 UTC Comment hidden (obsolete)
Comment 15 Alex Arnaud 2011-10-07 15:07:36 UTC
Just sent a new patch. It's the same than last one but there's more things in the MARC21 xslt (It comes from opac).
Comment 16 Julian Maurice 2011-10-07 15:20:56 UTC
It works well for Unimarc.
Signed off.
Waiting for someone to test Marc21 xslt.
Comment 17 Owen Leonard 2011-10-19 13:41:18 UTC
Created attachment 5983 [details]
Screenshot showing duplicate availability information

This is much improved, but I don't think we're there yet. There are a couple of things missing:

- linking the author name to an author search
- publication information, "Washington, D.C. : Smithsonian Folkways, 1990"
- physical description: "1 sound disc (32:15 min.) : ; 4 3/4 in."

The other issue I see is that the availability information is now duplicated: It's shown once in the XSL output and again in the Location table column. It could be that people who enable the XSL view would like to have availability show only in the XSL and we could get rid of the Location column. If so I think we probably need to make the status information more verbose as it is in the Location column.

The other option of course is to remove the availability information from the XSL output and keep the Location column. This would probably be a good topic to bring up on the Koha list.

Whether this means this patch can't be approved I don't know. Since it's controlled by a system preference it's not going to hurt anyone to have the option (especially if the UNIMARC version is better). But I hate to offer people a new feature which isn't more polished.
Comment 18 Paul Poulain 2011-10-24 11:38:11 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 19 Paul Poulain 2011-10-25 15:05:45 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 20 Owen Leonard 2011-12-08 14:09:34 UTC
*** Bug 4813 has been marked as a duplicate of this bug. ***
Comment 21 Doug Dearden 2011-12-08 21:10:54 UTC
On this line:

+    <xsl:variable name="Show856uAsImage" select="marc:sysprefs/marc:syspref[@name='OPACDisplay856uAsImage']"/>

OPACDisplay856uAsImage should be changed to Display856uAsImage so it picks up the correct system preference from the Staff Client area instead of the OPAC area.
Comment 22 Katrin Fischer 2011-12-21 20:58:11 UTC
comments 17 and 21 note some problems with the patch. I am changing to failed qa.
Comment 23 Paul Poulain 2012-02-03 15:25:19 UTC
Reviving this bug:
  * it's not an Enhancement, it's a bugfix = the syspref exist and does nothing at the moment !
  * The proposed MARC21 XSLT is derived from the OPAC one. If there is something wrong, please, propose a follow-up. If there is something missing I think that's for another bug entry. Because atm, activating the syspref results in nothing, so the most important is to have the syspref work !

switching back to needs signoff

Also note that Stéphane submit an enhancement to let libraries define the XSLT url, switching the 4 sysprefs from yes/no to full URL, see bug 4032
Comment 24 Stéphane Delaune 2012-02-03 16:08:41 UTC Comment hidden (obsolete)
Comment 25 Stéphane Delaune 2012-02-03 16:14:04 UTC
If the 4032 is apply after this, you'll need to apply
the "proposed patch if 4032 is apply after 6919 " to make compatibility
between them
Comment 26 Owen Leonard 2012-02-08 16:29:27 UTC Comment hidden (obsolete)
Comment 27 Owen Leonard 2012-02-08 16:31:43 UTC Comment hidden (obsolete)
Comment 28 Jared Camins-Esakov 2012-02-09 20:40:09 UTC
Created attachment 7542 [details] [review]
bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 29 Jared Camins-Esakov 2012-02-09 20:41:16 UTC
Created attachment 7543 [details] [review]
Bug 6919 - Follow-up - Implementing XSLTResultsDisplay

- Adding search result numbering to match non-XSLT
  results display.
- Copying author handling (with link to author search) from
  OPAC detail XSL.
- Copying physical description output from OPAC detail XSL.
- Copying ISBN output from OPAC detail XSL.
- Removing availability output (which had been copied from
  the OPAC) because it is redundant in the staff client.
- Removing duplicate inclusion of JS highlight plugin on
  results page.
- Updating highlight JS to make it compatible with XSLT
  results.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 30 Paul Poulain 2012-02-14 21:48:57 UTC
QA comment:
* The patch from Stephane should not be applied, obsoleting it. The 4032 should be applicable only
* translatable-template.t OK
* valid-templates.t OK
* works as expected in Unimarc with Owen follow-up

I propose passed QA status.
It's a BibLibre patch, and a quite large one, so won't mark passed QA, but ask Marcel.
Comment 31 Marcel de Rooy 2012-02-20 13:11:19 UTC
Code looks good in general. One point that blocks this patch:

The call to XSLTParse4Display is now made twice in Search.pm. (Line 1785 and line 1852)
For the staff client case the function is called once due to the context check around the second call. For opac the function is called both times.

Could you make an adjustment that combines both calls in one section of this script?
Comment 32 Alex Arnaud 2012-02-27 10:51:18 UTC Comment hidden (obsolete)
Comment 33 Alex Arnaud 2012-02-27 10:52:43 UTC
To test, apply:
bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt
Bug 6919 - Follow-up - Implementing XSLTResultsDisplay 
Follow up patch
Comment 34 Paul Poulain 2012-02-27 12:52:17 UTC
small follow-up for QA acceptance, I think it can be switched to "signed-off" directly
Comment 35 Marcel de Rooy 2012-02-29 14:42:34 UTC
When I apply 7542, 7543 and then the followup 7898, the last one fails. Please check..
Comment 36 Owen Leonard 2012-02-29 15:07:47 UTC
Using "git bz apply" I have no problems applying all three patches.
Comment 37 Marcel de Rooy 2012-02-29 15:41:29 UTC
Forget my comment: vim problems..
Comment 38 Marcel de Rooy 2012-03-01 09:27:41 UTC
Created attachment 7958 [details]
Picture of staff results

Follows..
Comment 39 Marcel de Rooy 2012-03-01 09:37:49 UTC
Created attachment 7959 [details] [review]
[SIGNED_OFF] Follow up patch

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signing off on this one with the following note: You have moved the call to XSLTParse4Display from around line 1775 to around 1842, as compared to the situation before the three 6919 patches. I probably would have left it at its original location, but while examining the code between these two spots, I do not see any real problems with this move. Tested it, works okay.
Comment 40 Marcel de Rooy 2012-03-01 09:41:51 UTC
Additional QA comments:
1) These patches are a welcome contribution! Resolving the missing XSLT display in staff for quite some time.
2) Please note my comment on the signoff of the last followup. (Thx for making the change BTW)
3) See my picture. Look at the first result. If there is no author, the first line is very close under the title. This is just a very minor design issue. But if you could send another followup making it look better, you are welcome.

Marking these three patches now as Passed QA.