Bug 15527

Summary: OAI-PMH should have a stylesheet to aid usability
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Web servicesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: brendan, dcook, f.demians, hector.hecaxmmx, jonathan.druart, kyle
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16194
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 28742, 16174    
Attachments: Original style
Proposed style
Bug 15527: Add a stylesheet for OAI-PMH to aid usability
Bug 15527: (followup) Allow using custom stylesheets
Bug 15527: (followup) Avoid replacing spaces for non-breaking spaces
Bug 15527: Add a stylesheet for OAI-PMH to aid usability
Bug 15527 Custom stylesheet in extended mode
[SIGNED-OFF]Bug 15527: Custom stylesheet in extended mode
Bug 15527: (qa followup) Add DSpace license to the about page
Bug 15527: Add a stylesheet for OAI-PMH to aid usability
Bug 15527: (qa followup) Add DSpace license to the about page

Description Tomás Cohen Arazi 2016-01-08 13:39:50 UTC
Created attachment 46429 [details]
Original style

Both EPrints and DSpace projects ship XSLT stylesheets that are included in the OAI response so using a web browser to navigate the repository. We should add one ourselves.

See the attached screenshots for better understanding this enhancement proposal.
Comment 1 Tomás Cohen Arazi 2016-01-08 13:40:13 UTC
Created attachment 46430 [details]
Proposed style
Comment 2 Tomás Cohen Arazi 2016-01-08 14:29:54 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2016-01-08 14:30:03 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2016-01-08 15:39:09 UTC Comment hidden (obsolete)
Comment 5 Frédéric Demians 2016-01-08 17:57:14 UTC
It doesn't work for me when your stylesheet is in opac-tmpl/xsl/. I have to put it in opac-tmpl directly, and modify Repository.pm file to make it works. Could you try on your side? I've seen in your screenshot you're working with 'localhost' as your server URL. It may come from there.

A remark: you introduce a new syspref OAI-PMH:CustomStylesheet. I think that
this kind of parameter belongs to the 'Extended Mode'. The YAML config file
could be used to link to a local stylesheet.
Comment 6 Tomás Cohen Arazi 2016-01-09 01:59:47 UTC
(In reply to Frédéric Demians from comment #5)
> It doesn't work for me when your stylesheet is in opac-tmpl/xsl/. I have to
> put it in opac-tmpl directly, and modify Repository.pm file to make it
> works. Could you try on your side? I've seen in your screenshot you're
> working with 'localhost' as your server URL. It may come from there.

I'm using kohadevbox:ansible. I think the permissions (apache ones) on a regular dev/normal install have diverged from the packages. I'll take a look.


> A remark: you introduce a new syspref OAI-PMH:CustomStylesheet. I think that
> this kind of parameter belongs to the 'Extended Mode'. The YAML config file
> could be used to link to a local stylesheet.

That's probably a good idea. I'll dig into it.
Comment 7 Héctor Eduardo Castro Avalos 2016-01-09 03:04:36 UTC
Looks very good. Good job, Tomás.
Comment 8 Frédéric Demians 2016-01-09 07:48:59 UTC
(In reply to Tomás Cohen Arazi from comment #6)

> I'm using kohadevbox:ansible. I think the permissions (apache ones) on a
> regular dev/normal install have diverged from the packages. I'll take a look.

This part in apache conf is the culprit:

<DirectoryMatch "__OPAC_WWW_DIR__/.*/(modules|xslt|includes)">
   Order deny,allow
   Deny from all
</DirectoryMatch>

I will propose a patch fixing that, and using OAI server conf file to define custom XSLT for web view.
Comment 9 Frédéric Demians 2016-01-09 07:56:36 UTC Comment hidden (obsolete)
Comment 10 Frédéric Demians 2016-01-09 07:56:48 UTC Comment hidden (obsolete)
Comment 11 Frédéric Demians 2016-01-09 08:01:29 UTC
(In reply to Frédéric Demians from comment #10)
> Created attachment 46460 [details] [review] [review]
> Bug 15527 Custom stylesheet in extended mode
> 
> Add a parameter to YAML config file in order to specify a local XSLT
> used style the XML returned by OAI Server. Some doc.
> 
> Move default XSLT from /opac-tmpl/xslt to
> /opac-tmpl/bootstrap/css/OAI.xslt.

Tomás, I've signed-off your first patch, and obsoleted the 2 others. You should take a look at mine (2nd patch attached) and sign it whether it seems OK to you. Maybe a third eye (Hector) and sign-off would be welcome. Thanks for the good job.
Comment 12 Héctor Eduardo Castro Avalos 2016-01-10 01:43:23 UTC
> 
> Tomás, I've signed-off your first patch, and obsoleted the 2 others. You
> should take a look at mine (2nd patch attached) and sign it whether it seems
> OK to you. Maybe a third eye (Hector) and sign-off would be welcome. Thanks
> for the good job.

For me looks very good I'll signed-off if you don't care Tomás.
Comment 13 Héctor Eduardo Castro Avalos 2016-01-10 01:45:26 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-01-11 10:05:24 UTC
Comment on attachment 46464 [details] [review]
[SIGNED-OFF]Bug 15527: Custom stylesheet in extended mode

Review of attachment 46464 [details] [review]:
-----------------------------------------------------------------

::: Koha/OAI/Server/Repository.pm
@@ +111,5 @@
>  
>      # Load configuration file if defined in OAI-PMH:ConfFile syspref
> +    my $conf = C4::Context->preference("OAI-PMH:ConfFile");
> +    if ( $conf ) {
> +        $self->{ conf } = $conf = LoadFile( $conf );

What's the point of this change? I personally prefer the previous version.
Comment 15 Frédéric Demians 2016-01-11 11:25:05 UTC
(In reply to Jonathan Druart from comment #14)
> Comment on attachment 46464 [details] [review] [review]
> [SIGNED-OFF]Bug 15527: Custom stylesheet in extended mode
> 
> Review of attachment 46464 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/OAI/Server/Repository.pm
> @@ +111,5 @@
> >  
> >      # Load configuration file if defined in OAI-PMH:ConfFile syspref
> > +    my $conf = C4::Context->preference("OAI-PMH:ConfFile");
> > +    if ( $conf ) {
> > +        $self->{ conf } = $conf = LoadFile( $conf );
> 
> What's the point of this change? I personally prefer the previous version.

The $conf variable is used above (line 148) in order to get the new extended 'xslt' parameter:

148     $response->xslt(
149         $conf && $conf->{xslt}
150           ? $conf->{xslt}
151           : "/opac-tmpl/bootstrap/css/OAI.xslt"
152     );
Comment 16 Jonathan Druart 2016-01-19 10:07:33 UTC
QA comments:
1/ Could you please add the DSpace license (linked to the xslt) to the about page?
2/ Frédéric, why did you move the xslt file to the OPAC css dir? Why not koha-tmpl/opac-tmpl/bootstrap/en/xslt/?
Comment 17 Jonathan Druart 2016-01-19 10:09:35 UTC
3/ Should the new file be translatable?
Comment 18 Frédéric Demians 2016-01-19 10:28:59 UTC
> 2/ Frédéric, why did you move the xslt file to the OPAC css dir? Why not
> koha-tmpl/opac-tmpl/bootstrap/en/xslt/?

See comment 8. The XSL file is accessed directly by the web browser by http. Files in koha-tmpl/opac-tmpl/bootstrap/en/xslt/ can only be accessed by Koha web scripts.
Comment 19 Frédéric Demians 2016-01-19 10:30:57 UTC
(In reply to Jonathan Druart from comment #17)
> 3/ Should the new file be translatable?

In my opinion, humble as appropriate, no.
Comment 20 Frédéric Demians 2016-03-03 08:21:26 UTC
What point(s) prevent this patch to return to 'signed-off' status?
Comment 21 Kyle M Hall 2016-03-03 17:29:56 UTC
I think the issue at hand is that this patch revealed a new bug in testing where the non-package version of Koha's apache conf no longer matches the package version in behavior and functionality. While appreciated, I think Frédéric's followup is not needed and we should instead fix the bug for non-package installs of Koha. That should solve the issue and once that and the license followup have been submitted this can move forward.
Comment 22 Frédéric Demians 2016-03-03 18:45:23 UTC
Kyle, my patch was also about adding (In reply to Kyle M Hall from comment #21)
> I think the issue at hand is that this patch revealed a new bug in testing
> where the non-package version of Koha's apache conf no longer matches the
> package version in behavior and functionality. While appreciated, I think
> Frédéric's followup is not needed and we should instead fix the bug for
> non-package installs of Koha. That should solve the issue and once that and
> the license followup have been submitted this can move forward.

I agree with this plan...

Where is located the Apache conf file? Do you already know what is its difference with etc/koha-http.conf?

My followup patch was also introducing a way to specify a local XSLT in extended mode. See this:

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46460&action=diff

But it could be added later when this patch will be in master.
Comment 23 Kyle M Hall 2016-03-07 15:54:41 UTC
(In reply to Frédéric Demians from comment #22)
> Kyle, my patch was also about adding (In reply to Kyle M Hall from comment
> #21)
> > I think the issue at hand is that this patch revealed a new bug in testing
> > where the non-package version of Koha's apache conf no longer matches the
> > package version in behavior and functionality. While appreciated, I think
> > Frédéric's followup is not needed and we should instead fix the bug for
> > non-package installs of Koha. That should solve the issue and once that and
> > the license followup have been submitted this can move forward.
> 
> I agree with this plan...
> 
> Where is located the Apache conf file? Do you already know what is its
> difference with etc/koha-http.conf?
> 
> My followup patch was also introducing a way to specify a local XSLT in
> extended mode. See this:
> 
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46460&action=diff
> 
> But it could be added later when this patch will be in master.

I agree. I think it would be good as a followup on a new bug report. I believe Tomás is going to file the bug and patch for the issue at hand.
Comment 24 Jonathan Druart 2016-03-22 10:51:48 UTC
(In reply to Jonathan Druart from comment #16)
> QA comments:
> 1/ Could you please add the DSpace license (linked to the xslt) to the about
> page?

This is still valid.
Comment 25 Tomás Cohen Arazi 2016-03-22 12:51:58 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-03-23 16:12:15 UTC
Created attachment 49451 [details] [review]
Bug 15527: Add a stylesheet for OAI-PMH to aid usability

This patch introduces an XSLT file grabbed from DSpace and a couple
css files.

To test:

- Point to your dev Koha's oai endpoint. For example:
  http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListMetadataFormats
=> FAIL: Look how ugly it is for humans.
- Apply this patch
- Reload
=> SUCCES: It is styled and provides really useful links to interact with
  the OAI server verbs.
- Sign off :-D

Sponsored-by: Universidad Empresarial Siglo 21

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
  Works as described. Nice looking.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2016-03-23 16:12:21 UTC
Created attachment 49452 [details] [review]
Bug 15527: (qa followup) Add DSpace license to the about page

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Brendan Gallagher 2016-03-24 17:29:18 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!