Bug 20538

Summary: Remove the need of writing [% KOHA_VERSION %] everywhere
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Architecture, internals, and plumbingAssignee: Julian Maurice <julian.maurice>
Status: Pushed to Master --- QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, josef.moravec, katrin.fischer, roch.damour
Version: masterKeywords: additional_work_needed
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12904
Whiteboard:
Change sponsored?: --- Patch complexity: ---
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Attachments: Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere
Bug 20538: (follow-up) Update discharges request page with Asset plugin
Bug 20538: Fix copyright notice in Asset.pm
Bug 20538: Fix 2 JS paths
Bug 20538: Move categories.js out of language directory
Bug 20538: Add POD for Asset.pm
Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere
Bug 20538: (follow-up) Update discharges request page with Asset plugin
Bug 20538: Fix copyright notice in Asset.pm
Bug 20538: Fix 2 JS paths
Bug 20538: Move categories.js out of language directory
Bug 20538: Add POD for Asset.pm
Bug 20538: ->tag is private - tell it explicitly
Bug 20538: Prevent warnings in xt/author/valid-templates.t

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!