Bug 9052 - YUI assets installed in the wrong place by packages
Summary: YUI assets installed in the wrong place by packages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Robin Sheat
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-10 15:24 UTC by Jared Camins-Esakov
Modified: 2019-06-27 09:24 UTC (History)
9 users (show)

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


Attachments
Bug 9052: 2 yui files installed in the wrong place by the packages (1.00 KB, patch)
2012-11-28 06:45 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9052 - rewrite the YUI links to use the system library (3.26 KB, patch)
2012-12-14 03:11 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9052 - rewrite the YUI links to use the system library (3.46 KB, patch)
2012-12-14 12:43 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 9052 - rewrite the YUI links to use the system library (3.77 KB, patch)
2012-12-15 21:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9052 - followup: fix the YUI CSS locations for SCO (2.41 KB, patch)
2012-12-18 04:51 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9052 - followup: fix the YUI CSS locations for SCO (2.47 KB, patch)
2012-12-18 12:14 UTC, Mirko Tietgen
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9052 - followup: fix the YUI CSS locations for SCO (3.35 KB, patch)
2012-12-30 13:52 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-11-10 15:24:12 UTC
Even though YUI has been moved to opac-tmpl/lib/yui, the packages were installing some of the CSS assets to opac-tmpl/prog/en/yui
Comment 1 Magnus Enger 2012-11-27 08:57:27 UTC
Some more details:

The problem is that these two files are not where they are supposed to be:

/opac-tmpl/lib/yui/reset-fonts-grids.css
/opac-tmpl/lib/yui/skin.css
Comment 2 Chris Cormack 2012-11-28 06:45:20 UTC Comment hidden (obsolete)
Comment 3 vimal kumar 2012-11-28 08:51:16 UTC
(In reply to comment #1)
> Some more details:
> 
> The problem is that these two files are not where they are supposed to be:
> 
> /opac-tmpl/lib/yui/reset-fonts-grids.css
> /opac-tmpl/lib/yui/skin.css

Please guide me, which files I have to move to which location for proper working?
Comment 4 MJ Ray (software.coop) 2012-12-05 19:51:12 UTC
rangi reports in IRC that the patch doesn't work. Moving back to in discussion.
Comment 5 MJ Ray (software.coop) 2012-12-06 12:09:03 UTC
This is a very dirty (messing in /usr/share is not good if you are not the package manager) fix for it:
  sudo cp -al /usr/share/koha/opac/htdocs/opac-tmpl/prog/en/css/{reset-fonts-grids,skin}.css lib/yui/
Comment 6 MJ Ray (software.coop) 2012-12-06 12:15:47 UTC
I think Bug 8623 was what exposed this bug.

Why aren't we depending on Package libjs-yui anyway?
Comment 7 Robin Sheat 2012-12-14 02:33:13 UTC
We are linking in the system yui libraries, however Koha isn't referencing them correctly (e.g. skin.css and reset-fonts-grids.css aren't in the same directory:

robin@debmaker32:/usr/share/koha/opac/htdocs$ find -L -name skin.css -or -name reset-fonts-grids.css
./opac-tmpl/lib/yui/assets/skins/sam/skin.css
./opac-tmpl/lib/yui/reset-fonts-grids/reset-fonts-grids.css
./opac-tmpl/prog/en/css/skin.css
./opac-tmpl/prog/en/css/reset-fonts-grids.css

The first two are symlinked from libjs-yui, the second two the ones that Koha bundles.

The Koha version is quite a bit older than the debian version. This may be a problem, we I'll try it anyway.

Koha is also referencing it badly:

robin@zarathud:~/catalyst/koha/koha-tmpl/opac-tmpl/prog/en$ grep -r skin.css *
includes/doc-head-close.inc:<link rel="stylesheet" type="text/css" href="/opac-tmpl/lib/yui/skin.css" />
modules/sco/help.tt:<link rel="stylesheet" type="text/css" href="[% yuipath %]/skin.css" />
modules/sco/sco-main.tt:<link rel="stylesheet" type="text/css" href="[% yuipath %]/skin.css" />

I think we should be referencing it from the opac.css, that way it's centralised and it can be re-written as part of the package building process.
Comment 8 Robin Sheat 2012-12-14 03:11:24 UTC Comment hidden (obsolete)
Comment 9 Robin Sheat 2012-12-14 03:14:11 UTC
The patch attached hasn't had a lot of testing by me, just enough to indicate that it looks like it works. If someone else could give it a good looking at, that would be appreciated.

It deliberately doesn't modify the behaviour for the staff client or the SCO-related templates. I haven't established if there are issues there or not yet, I think the OPAC is more critical.
Comment 10 Mirko Tietgen 2012-12-14 12:43:20 UTC Comment hidden (obsolete)
Comment 11 Mirko Tietgen 2012-12-14 12:57:21 UTC
I have not had time do extensive testing, I mainly checked that OPAC pages look OK and Firebug does not show any errors. Given that packages are the recommended way of installation I'd like to see this pushed soon as it makes 3.10 packages usable without manual tweaking -- even if that means less extensive testing. If somebody with more time at hand would do a second signoff that would of course be appreciated.

On a sidenote, I had a problem with

> libdigest-jhash-perl: 0.07-1koha1 Depends: perlapi-5.10.1 but it is not installable

and had to manually install an older version of libdigest-jhash-perl in the pbuilder environment. It happened when i tried to build master+9052, which does not work anyway. I am new to the roll-your-own-packages-business so I'm not sure if it is supposed to or not, probably an error on my side. 3.10+master works and is what I based my signoff on.
Comment 12 Mirko Tietgen 2012-12-14 12:59:24 UTC
(In reply to comment #11)
> 3.10+master works and is what I based my signoff on.

That line is supposed to read
'3.10+9052' works and is what I based my signoff on'
Comment 13 Robin Sheat 2012-12-14 19:06:32 UTC
Thanks for testing!

The libjhash thing is completely unrelated, basically it's an annoyingly problematic package. We're going to fix that by removing that dependency, as it's optional anyway.
Comment 14 Katrin Fischer 2012-12-15 21:21:13 UTC Comment hidden (obsolete)
Comment 15 Robin Sheat 2012-12-15 22:49:15 UTC
I didn't think to put this in the testing notes, however it would be worth checking that these changes don't break a git or tarball install, as I have changed things that shouldn't-but-might affect that, in particular how the CSS is loaded on all OPAC pages.
Comment 16 Chris Cormack 2012-12-16 19:43:29 UTC
(In reply to comment #15)
> I didn't think to put this in the testing notes, however it would be worth
> checking that these changes don't break a git or tarball install, as I have
> changed things that shouldn't-but-might affect that, in particular how the
> CSS is loaded on all OPAC pages.

I think Katrin tested this for a git install right ?
Comment 17 Robin Sheat 2012-12-16 19:47:59 UTC
(In reply to comment #16)
> I think Katrin tested this for a git install right ?

I didn't read it that way initially, but on re-read you might be right.
Comment 18 Katrin Fischer 2012-12-16 19:58:46 UTC
Yes, I did a git install and took a look at the OPAC there before creating the packages from that changed branch.
Comment 19 Mirko Tietgen 2012-12-16 20:37:16 UTC
Really, we are not going to lose anything, but win the "package installation is our recommended way" back. I think it is hardly possible that it's worse than before.
Comment 20 Jared Camins-Esakov 2012-12-17 14:05:25 UTC
This patch has been pushed to master.
Comment 21 Chris Cormack 2012-12-17 21:41:20 UTC
Pushed to 3.10.x will be in 3.10.1
Comment 22 Robin Sheat 2012-12-18 04:34:51 UTC
The staff client needs no work: it seems to keep the .css files in the language specific directory (that said, it might be nice to remove them and uses the system ones anyway, but it's not critical.)
Comment 23 Robin Sheat 2012-12-18 04:39:50 UTC
It _is_ an issue for the self-checkout module. I'll patch this in the same way.
Comment 24 Robin Sheat 2012-12-18 04:51:06 UTC Comment hidden (obsolete)
Comment 25 Robin Sheat 2012-12-18 04:51:53 UTC
Attached patch is a whole lot simpler, but the same testing process should apply as for the previous one.
Comment 26 Mirko Tietgen 2012-12-18 12:14:46 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2012-12-29 10:29:56 UTC
Working on this.
Comment 28 Katrin Fischer 2012-12-30 13:52:02 UTC
Created attachment 14329 [details] [review]
[SIGNED-OFF] Bug 9052 - followup: fix the YUI CSS locations for SCO

This is followup to the previous YUI-fixing patch, and simply causes the
self-checkout page to load the YUI files via the sco CSS file,
and that gets re-written at package build time to work.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested on a package installation using packages built by
Mirko Tietgen and a git dev installation.

To test:
1) Enable WebBasedSelfCheck system preference
2) Enter login date for staff user in AutoSelfCheckAllowed... prefernces
3) Go to the self checkout page:
   .../cgi-bin/koha/sco/sco-main.pl
4) Verfiy all pages have the usual look, CSS, Javascript and images
   look normal
5) Verify the same is true for translated templates

Manual: http://manual.koha-community.org/3.10/en/selfcheckout.html
Comment 29 Jared Camins-Esakov 2012-12-30 23:38:33 UTC
This patch has been pushed to master.
Comment 30 Chris Cormack 2013-04-27 05:28:19 UTC
Doesn't apply to 3.10.x please rebase and send a new patch
Comment 31 Robin Sheat 2013-04-28 23:23:40 UTC
This patch is already in 3.10.x.