Bug 20538 - Remove the need of writing [% KOHA_VERSION %] everywhere
Summary: Remove the need of writing [% KOHA_VERSION %] everywhere
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Katrin Fischer
URL:
Keywords:
: 20066 (view as bug list)
Depends on:
Blocks: 20761 21583
  Show dependency treegraph
 
Reported: 2018-04-06 16:59 UTC by Julian Maurice
Modified: 2019-10-14 19:55 UTC (History)
6 users (show)

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


Attachments
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere (296.91 KB, patch)
2018-04-06 17:00 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere (296.99 KB, patch)
2018-04-08 10:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere (295.60 KB, patch)
2018-04-12 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20538: (follow-up) Update discharges request page with Asset plugin (1.13 KB, patch)
2018-04-12 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20538: Fix copyright notice in Asset.pm (753 bytes, patch)
2018-04-12 08:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20538: Fix 2 JS paths (1.59 KB, patch)
2018-04-12 08:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20538: Move categories.js out of language directory (735 bytes, patch)
2018-04-12 09:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20538: Add POD for Asset.pm (2.49 KB, patch)
2018-04-12 09:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere (295.66 KB, patch)
2018-04-12 09:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20538: (follow-up) Update discharges request page with Asset plugin (1.19 KB, patch)
2018-04-12 09:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20538: Fix copyright notice in Asset.pm (803 bytes, patch)
2018-04-12 09:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20538: Fix 2 JS paths (1.63 KB, patch)
2018-04-12 09:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20538: Move categories.js out of language directory (792 bytes, patch)
2018-04-12 09:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20538: Add POD for Asset.pm (2.54 KB, patch)
2018-04-12 09:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20538: ->tag is private - tell it explicitly (1.27 KB, patch)
2018-04-13 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20538: Prevent warnings in xt/author/valid-templates.t (1.12 KB, patch)
2018-04-25 07:22 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2018-04-06 16:59:44 UTC
Having to write [% KOHA_VERSION %] for each url is bad because:
- It's easily forgettable when adding new <script> or <link>
- It prevents grep'ing for the full filename
- It violates the DRY principle
- If at some point we want to change the "force js and css reload" mechanism, it will be tedious
Comment 1 Julian Maurice 2018-04-06 17:00:30 UTC
Created attachment 73791 [details] [review]
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere

Having to write [% KOHA_VERSION %] for each url is bad because:
- It's easily forgettable when adding new <script> or <link>
- It prevents grep'ing for the full filename
- It violates the DRY principle
- If at some point we want to change the "force js and css reload"
  mechanism, it will be tedious

This patch:
- adds a Template::Toolkit plugin that generates <script> and
  <link> tags for JS and CSS files, and inserts automatically the Koha
  version in the filename
- use the new plugin to remove all occurences of [% KOHA_VERSION %]
- remove the code that was adding KOHA_VERSION as a template variable

Test plan:
1. Apply patch
2. Go to several different pages in Koha (opac and intranet) while
   checking your browser's dev tools (there should be no 404 for JS and
   CSS files, and the Koha version should appear in filenames) and the
   server logs (there should be no "File not found")
3. `git grep KOHA_VERSION` should return nothing
4. prove t/db_dependent/Koha/Template/Plugin/Asset.t
Comment 2 Roch D'Amour 2018-04-06 18:56:30 UTC
Patch doesn't apply for me on master, sorry
Comment 3 Julian Maurice 2018-04-08 10:52:20 UTC Comment hidden (obsolete)
Comment 4 Julian Maurice 2018-04-08 10:52:41 UTC
Patch rebased on master
Comment 5 Julian Maurice 2018-04-08 10:57:16 UTC
Just in case, the sed commands I used to modify (almost) all templates:

sed -i -E 's/<(script|link) .*(href|src)=["'"'"']((\[%.*?%\])\/)*([^%]*)_\[% KOHA_VERSION %\]\.(js|css)["'"'"'].*(\/?>|<\/\1>)/[% Asset.\6("\5\.\6") %]/' **/*.inc **/*.tt

git status --porcelain | grep '^ M' | awk '{print $2}' | xargs -- sed -i '1i[% USE Asset %]'
Comment 6 Josef Moravec 2018-04-12 07:17:44 UTC Comment hidden (obsolete)
Comment 7 Josef Moravec 2018-04-12 07:17:47 UTC Comment hidden (obsolete)
Comment 8 Josef Moravec 2018-04-12 07:20:18 UTC
I started to write a rule for coding guidelines here: https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML8:_use_Asset_TT_plugin_for_linking_javascript_and_css_files

feel free to update it and we can discuss it on the next meeting
Comment 9 Katrin Fischer 2018-04-12 08:20:31 UTC
There are some FAILs in the QA script, but I assume they are just side effects/false positives for this patch. Maybe something to look into later:

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-MARCdetail.tt
   FAIL	  forbidden patterns
		forbidden pattern: opac-tmpl should certainly replaced with [% interface %] (line 67)
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-detail.tt
   FAIL	  forbidden patterns
		forbidden pattern: opac-tmpl should certainly replaced with [% interface %] (line 132)
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template
...

1) Is the copyright statement correct?
+package Koha::Template::Plugin::Asset;
+
+# Copyright Marc Véron / marc veron ag, Switzerland

2) Not blocker, but a note: Need to update qa tools!

Can you quickly check 1) please?
Comment 10 Julian Maurice 2018-04-12 08:31:37 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2018-04-12 08:31:41 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2018-04-12 08:33:26 UTC
(In reply to Katrin Fischer from comment #9)
> There are some FAILs in the QA script, but I assume they are just side
> effects/false positives for this patch. Maybe something to look into later:
> 
>  FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-MARCdetail.tt
>    FAIL	  forbidden patterns
> 		forbidden pattern: opac-tmpl should certainly replaced with [% interface
> %] (line 67)
>    OK	  git manipulation
>    OK	  spelling
>    OK	  tt_valid
>    OK	  valid_template
> 
>  FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-detail.tt
>    FAIL	  forbidden patterns
> 		forbidden pattern: opac-tmpl should certainly replaced with [% interface
> %] (line 132)
>    OK	  git manipulation
>    OK	  spelling
>    OK	  tt_valid
>    OK	  valid_template
> ...
These ones are true positives. Fixed in the last patch

> 
> 1) Is the copyright statement correct?
> +package Koha::Template::Plugin::Asset;
> +
> +# Copyright Marc Véron / marc veron ag, Switzerland
> 
> 2) Not blocker, but a note: Need to update qa tools!
> 
> Can you quickly check 1) please?
Done. Thanks for catching that.
Comment 13 Julian Maurice 2018-04-12 09:37:42 UTC Comment hidden (obsolete)
Comment 14 Julian Maurice 2018-04-12 09:37:46 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2018-04-12 09:39:59 UTC
I wrote a little patch for qa-test-tools to avoid all the 'File not found' warnings. You can found it here : https://github.com/jajm/koha-qa-test-tools/tree/bug_20538
Comment 16 Katrin Fischer 2018-04-12 09:51:06 UTC
Created attachment 74058 [details] [review]
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere

Having to write [% KOHA_VERSION %] for each url is bad because:
- It's easily forgettable when adding new <script> or <link>
- It prevents grep'ing for the full filename
- It violates the DRY principle
- If at some point we want to change the "force js and css reload"
  mechanism, it will be tedious

This patch:
- adds a Template::Toolkit plugin that generates <script> and
  <link> tags for JS and CSS files, and inserts automatically the Koha
  version in the filename
- use the new plugin to remove all occurences of [% KOHA_VERSION %]
- remove the code that was adding KOHA_VERSION as a template variable

Test plan:
1. Apply patch
2. Go to several different pages in Koha (opac and intranet) while
   checking your browser's dev tools (there should be no 404 for JS and
   CSS files, and the Koha version should appear in filenames) and the
   server logs (there should be no "File not found")
3. `git grep KOHA_VERSION` should return nothing
4. prove t/db_dependent/Koha/Template/Plugin/Asset.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2018-04-12 09:51:10 UTC
Created attachment 74059 [details] [review]
Bug 20538: (follow-up) Update discharges request page with Asset plugin

This is follow-up of bug 20524

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2018-04-12 09:51:13 UTC
Created attachment 74060 [details] [review]
Bug 20538: Fix copyright notice in Asset.pm

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2018-04-12 09:51:17 UTC
Created attachment 74061 [details] [review]
Bug 20538: Fix 2 JS paths

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2018-04-12 09:51:21 UTC
Created attachment 74062 [details] [review]
Bug 20538: Move categories.js out of language directory

This was the last js/css file inside a language directory

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2018-04-12 09:51:24 UTC
Created attachment 74063 [details] [review]
Bug 20538: Add POD for Asset.pm

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Jonathan Druart 2018-04-13 15:08:23 UTC
Created attachment 74171 [details] [review]
Bug 20538: ->tag is private - tell it explicitly
Comment 23 Jonathan Druart 2018-04-13 16:42:01 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 24 Jonathan Druart 2018-04-24 16:12:00 UTC
Could you take a look at xt/author/valid-templates.t ?
There are too many errors:

File not found : lib/emoji-picker/js/emoji-picker.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83.
File not found : js/basket.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83.
File not found : js/tags.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83.
File not found : js/datatables.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83.

[...]
Comment 25 Julian Maurice 2018-04-25 07:22:28 UTC
Created attachment 74835 [details] [review]
Bug 20538: Prevent warnings in xt/author/valid-templates.t
Comment 26 Jonathan Druart 2018-04-25 14:51:08 UTC
(In reply to Julian Maurice from comment #25)
> Created attachment 74835 [details] [review] [review]
> Bug 20538: Prevent warnings in xt/author/valid-templates.t

Pushed to master, thanks Julian!
Comment 27 Nick Clemens 2018-05-21 17:36:35 UTC
Enhancement, not backported for 17.11
Comment 28 Mark Tompsett 2018-09-06 18:23:18 UTC
*** Bug 20066 has been marked as a duplicate of this bug. ***