Created attachment 5590 [details] [review] Proposed patch This patch implements intranet XSLTResultsDisplay. I've added very very small xslt stylesheets for MARC21 and UNIMARC.
Created attachment 5592 [details] Screenshot after applying patch This isn't working for me. I see no title information in my results display.
After applying this patch all the title information disappears from the search results.
Created attachment 5595 [details] no titles after patch this is marc21 if that matters.
Created attachment 5596 [details] before patch titles show
oops, i said the same thing owen did :)
Created attachment 5602 [details] [review] Replacement patch Indeed, MARC21 xslt was wrong. Now it should be ok. This patch aims to implement intranet XSLTResultsDisplay not to improve shipping xslt stylesheets. So, they are very light.
Still no titles? http://screencast.com/t/kAuKVMLD (MARC21)
Created attachment 5607 [details] [review] Replacement patch i'm tired ...
Sorry, still not titles
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.
Liz & Nicole : the UNIMARC version is better. If you want a great MARC21 version, please give Alex the MARC21 fields to display.
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.
Created attachment 5774 [details] [review] bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt
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).
It works well for Unimarc. Signed off. Waiting for someone to test Marc21 xslt.
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.
Updating Version : This ENH will be for Koha 3.8
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)
*** Bug 4813 has been marked as a duplicate of this bug. ***
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.
comments 17 and 21 note some problems with the patch. I am changing to failed qa.
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
Created attachment 7453 [details] [review] proposed patch if 4032 is apply after 6919 proposed patch if 4032 is apply after 6919
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
Created attachment 7507 [details] [review] [SIGNED-OFF] bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 7508 [details] [review] [PATCH] 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.
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>
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>
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.
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?
Created attachment 7898 [details] [review] Follow up patch
To test, apply: bug 6919 - implements XSLTResultsDisplay and adding both unimarc and marc21 xslt Bug 6919 - Follow-up - Implementing XSLTResultsDisplay Follow up patch
small follow-up for QA acceptance, I think it can be switched to "signed-off" directly
When I apply 7542, 7543 and then the followup 7898, the last one fails. Please check..
Using "git bz apply" I have no problems applying all three patches.
Forget my comment: vim problems..
Created attachment 7958 [details] Picture of staff results Follows..
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.
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.