Bug 15527 - OAI-PMH should have a stylesheet to aid usability
Summary: OAI-PMH should have a stylesheet to aid usability
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16174
  Show dependency treegraph
 
Reported: 2016-01-08 13:39 UTC by Tomás Cohen Arazi
Modified: 2016-12-05 21:27 UTC (History)
6 users (show)

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


Attachments
Original style (143.84 KB, image/png)
2016-01-08 13:39 UTC, Tomás Cohen Arazi
Details
Proposed style (73.88 KB, image/png)
2016-01-08 13:40 UTC, Tomás Cohen Arazi
Details
Bug 15527: Add a stylesheet for OAI-PMH to aid usability (199.62 KB, patch)
2016-01-08 14:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15527: (followup) Allow using custom stylesheets (4.64 KB, patch)
2016-01-08 14:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15527: (followup) Avoid replacing spaces for non-breaking spaces (1.09 KB, patch)
2016-01-08 15:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15527: Add a stylesheet for OAI-PMH to aid usability (199.79 KB, patch)
2016-01-09 07:56 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15527 Custom stylesheet in extended mode (3.67 KB, patch)
2016-01-09 07:56 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 15527: Custom stylesheet in extended mode (3.75 KB, patch)
2016-01-10 01:45 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 15527: (qa followup) Add DSpace license to the about page (1.19 KB, patch)
2016-03-22 12:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15527: Add a stylesheet for OAI-PMH to aid usability (199.87 KB, patch)
2016-03-23 16:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15527: (qa followup) Add DSpace license to the about page (1.26 KB, patch)
2016-03-23 16:12 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!