Bug 4394 - opaccolorstylesheet syspref won't allow an external URL
Summary: opaccolorstylesheet syspref won't allow an external URL
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.4
Hardware: PC All
: PATCH-Sent (DO NOT USE) enhancement (vote)
Assignee: Jane Wagner
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-21 12:21 UTC by Jane Wagner
Modified: 2013-12-05 20:05 UTC (History)
4 users (show)

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


Attachments
patch file against current head (1.67 KB, patch)
2010-04-21 12:48 UTC, Chris Cormack
Details | Diff | Splinter Review
revised patch file against current head (13.43 KB, patch)
2010-05-04 12:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Additional change required (1.42 KB, patch)
2011-01-10 16:46 UTC, Owen Leonard
Details | Diff | Splinter Review
revised patch without conflicts on sysprefs.sql (13.36 KB, patch)
2011-04-05 14:21 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:26:40 UTC


---- Reported by jwagner@ptfs.com 2010-04-21 12:21:14 ----

The opaccolorstylesheet syspref, according to the manual, is now used "to specify a secondary stylesheet that can override the default one in as many ways as the author wishes."  However, while the opacstylesheet syspref can point to a complete replacement stylesheet on a different server (full URL), the opaccolorstylesheet syspref won't obey an external URL.  It requires the auxiliary stylesheet to be on the same server.  For sites without easy access to the server level, this is a problem.

The code seems to indicate that originally this setting was designed to be part of the the themes feature:

<link rel="stylesheet" type="text/css" href="<!-- TMPL_VAR NAME="themelang" -->/css/<!-- TMPL_VAR NAME="opaccolorstylesheet" -->" />


However, that feature seems never to have be implemented.  I've modified this line to match the syntax of the call to opacstylesheet:

<link rel="stylesheet" type="text/css" href="<!-- TMPL_VAR NAME="opaccolorstylesheet" -->" />

and the opaccolorstylesheet can now use a stylesheet stored on a remote server.



---- Additional Comments From jwagner@ptfs.com 2010-04-21 12:48:46 ----

Created an attachment
patch file against current head

Patch sent April 21, 2010 making the proposed change to doc-head-close.inc.  However, some discusson on the IRC indicates possible concerns with languages and themes (themes apparently worked in 2.x but have been partially broken in 3.x).  May need to review the proposed change to see if it causes any problems for other areas.




---- Additional Comments From dbavousett@ptfs.com 2010-04-21 14:33:29 ----

There are a couple of problems with this patch as-is:

a) As HDL pointed out in the Patches list, this would immediately break existing values of the syspref, unless the user remembered to tweak the existing syspref as soon as they deployed it. 

b) Sites that are using themes/language variants wouldn't be able to use different stylesheets in the variations; this may or may not be problematic.  It would be a better solution, again concurring with HDL, to check and see if the syspref contains http:// or https:// (or slashes, to pick off a local file that isn't in the standard places) and if so, pass it through untouched, otherwise to insert the theme/lang/css value as in the existing code.  Might need to back up a step or two in the code, and do that in C4::Context::preference, then make darn sure that this same change is applied everywhere relevant.  (That would add to this proposed patch, not replace it.)



---- Additional Comments From jwagner@ptfs.com 2010-04-23 14:54:21 ----

It sounded from the IRC discussion that this might be a problem.  Using the "check for http" approach would probably be better.  I'm not able to take that one on right now.  If anyone else wants to, feel free :-)  Otherwise, we'll work on it when we have a chance.





---- Additional Comments From jwagner@ptfs.com 2010-05-04 12:21:40 ----

Created an attachment
revised patch file against current head

Revised fix so that instead of just changing the path, the code in Auth.pm looks for the presence of http in the opaccolorstylesheet syspref.  If it's found, it sets up a new variable opacexternalsheet.  The template file then checks to see if that variable exists.  If it does, it points to the external stylesheet.  If not, it retains the css directory structure.





---- Additional Comments From jwagner@ptfs.com 2010-05-04 12:23:00 ----

The revised patch also updates the installer description of the opaccolorstylesheet syspref to read:

Define an auxiliary stylesheet for OPAC use, to override specified settings from the primary opac.css stylesheet. Enter the filename (if the file is in the server's css directory) or a complete URL beginning with http (if the file lives on a remote server).

The previous description was outdated and not relevant to the way the syspref is now used.





--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:26 UTC  ---

This bug was previously known as _bug_ 4394 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4394
Imported an attachment (id=2066)
Imported an attachment (id=2067)

Unknown operating system Windows 2000. Setting to default OS "All".
Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was jwagner@ptfs.com.
The original assignee of this bug does not have
   an account here. Reassigning to the default assignee
   for the component, chris@bigballofwax.co.nz.
   Previous assignee was jwagner@ptfs.com.
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.
Bug reassigned, setting status to "NEW".
   Previous status was "ASSIGNED".
The original submitter of attachment 2066 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 2067 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Owen Leonard 2011-01-10 16:46:00 UTC
Created attachment 2968 [details] [review]
Additional change required

jwagner's patch works for me, but will require this additional change now that system preference descriptions are in .pref files.
Comment 2 Matthias Meusburger 2011-04-05 12:38:44 UTC
Changed status to failed QA until a patch is attached to the bug.
Comment 3 Matthias Meusburger 2011-04-05 12:41:45 UTC
Please forget my last comment. This comment was not for this bug. Wrong manipulation.
Comment 4 claire.hernandez@biblibre.com 2011-04-05 14:21:18 UTC
Created attachment 3696 [details] [review]
revised patch without conflicts on sysprefs.sql

Second patch does not apply (opaccolorsyspref already exists)

CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/sysprefs.sql

so I submitted a new patch with a little escape "\'" in sysprefs.sql and resolved conflict.
Comment 5 Chris Cormack 2011-04-08 23:44:31 UTC
Pushed patches
Comment 6 Jared Camins-Esakov 2012-12-31 00:26:31 UTC
There have been no further reports of problems so I am marking this bug resolved.