Bug 25002 - JS Includes should be wrapped with template comments
Summary: JS Includes should be wrapped with template comments
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25003
  Show dependency treegraph
 
Reported: 2020-03-27 15:11 UTC by Martin Renvoize
Modified: 2020-11-30 21:49 UTC (History)
3 users (show)

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


Attachments
Bug 25002: JS Includes should be wrapped with template comments (22.28 KB, patch)
2020-03-28 16:43 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25002: JS Includes should be wrapped with template comments (22.41 KB, patch)
2020-04-21 13:44 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25002: JS Includes should be wrapped with template comments (22.34 KB, patch)
2020-04-28 10:13 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 25002: JS Includes should be wrapped with template comments (22.60 KB, patch)
2020-04-28 18:44 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 Martin Renvoize 2020-03-27 15:11:50 UTC
When debugging, we are often faced with a nameless <script> area when it comes to inspecting the page.. it would be helpful to have an HTML comment wrapping these script in our includes so you can quickly identify where the JS is coming from.
Comment 1 Martin Renvoize 2020-03-27 15:15:28 UTC
See first few minutes of the `Koha Virtual Hackfest - Code Spaghetti` live stream recording for further context.
Comment 2 Andrew Fuerste-Henry 2020-03-27 15:52:10 UTC
*** Bug 25003 has been marked as a duplicate of this bug. ***
Comment 3 Andrew Fuerste-Henry 2020-03-27 15:52:28 UTC
Martin beat me to it :)
Comment 4 Owen Leonard 2020-03-28 16:43:54 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2020-03-28 16:45:38 UTC
I hope this patch is what you had in mind.

Going through the includes highlights the need to update and/or remove several of them now that we have proper JS i18n, but I didn't think it hurt to update them anyway in the meantime.
Comment 6 David Roberts 2020-03-29 22:55:27 UTC
I've been through this, and have a few comments.

Firstly, I can't see where the additem.js.inc displays - it doesn't seem to be on the same page as all the other "Cataloging -> Add/Edit item:" comments that you've added.

Secondly, <!-- / password_check.inc --> and <!-- password_check.inc --> seem to render the wrong way round - i.e. the closing tag displays first.

Thirdly, str/tinymce_i18n.inc doesn't seem to have a closing comment.

I haven't been able to test the web installer pages yet, and I can't get to the source on the Patrons -> Patron -> Print summary screen because the printer dialogue blocks access to it.

Everything else seems perfect though :-)
Comment 7 Owen Leonard 2020-04-21 13:44:29 UTC
Created attachment 103374 [details] [review]
Bug 25002: JS Includes should be wrapped with template comments

This patch adds HTML comments to Template::Toolkit include files which
contain <script> tags so that it is clear where the embedded scripts can
be found in the code.

To test, apply the patch and view source on the following pages to
verify the presence of the comments:

Acquisitions home page:
  - acquisitions-toolbar.inc
  - validtor-strings.inc
  - js_includes.inc
  - format-price.inc
Acquisitions -> Add order from new record,
Acquisitions -> Receive order:
  - additem.js.inc
Cataloging -> Add/Edit item:
  - columns_settings.inc
  - strings.inc
  - select2.inc
  - calendar.inc
  - str/cataloging_additem.inc
Authorities home page:
  - authorities_js.inc
Bibliographic detail page:
  - catalog-strings.inc
Cataloging -> Advanced editor:
  - cateditor-ui.inc
  - cateditor-widgets-marc21.inc
Administration -> Item types:
  - greybox.inc
ILL requests:
  - ill-list-table-strings.inc
Web installer
  - installer-intranet-bottom.inc
Web installer -> Onboarding
  - installer-strings.inc
Lists -> List contents -> Merge records
  - merge-record-strings.inc
Patrons -> Patron -> Change password
  - password_check.inc
  - str/members-menu.inc
Patrons -> Patron -> Print summary
  - slip-print.inc
Circulation -> Check out
  - timepicker.inc
Administration -> System preferences:
  - str/tinymce_i18n.inc
  - wysiwyg-systempreferences.inc
Cataloging -> Z39.50 Search:
  - z3950_search.inc
Comment 8 Owen Leonard 2020-04-21 13:45:59 UTC
Thanks for testing, David. And don't hesitate to mark something "Failed QA" when you see problems. If anything it gets my attention!
Comment 9 Frédéric Demians 2020-04-28 10:13:55 UTC
Created attachment 103841 [details] [review]
Bug 25002: JS Includes should be wrapped with template comments

This patch adds HTML comments to Template::Toolkit include files which
contain <script> tags so that it is clear where the embedded scripts can
be found in the code.

To test, apply the patch and view source on the following pages to
verify the presence of the comments:

Acquisitions home page:
  - acquisitions-toolbar.inc
  - validtor-strings.inc
  - js_includes.inc
  - format-price.inc
Acquisitions -> Add order from new record,
Acquisitions -> Receive order:
  - additem.js.inc
Cataloging -> Add/Edit item:
  - columns_settings.inc
  - strings.inc
  - select2.inc
  - calendar.inc
  - str/cataloging_additem.inc
Authorities home page:
  - authorities_js.inc
Bibliographic detail page:
  - catalog-strings.inc
Cataloging -> Advanced editor:
  - cateditor-ui.inc
  - cateditor-widgets-marc21.inc
Administration -> Item types:
  - greybox.inc
ILL requests:
  - ill-list-table-strings.inc
Web installer
  - installer-intranet-bottom.inc
Web installer -> Onboarding
  - installer-strings.inc
Lists -> List contents -> Merge records
  - merge-record-strings.inc
Patrons -> Patron -> Change password
  - password_check.inc
  - str/members-menu.inc
Patrons -> Patron -> Print summary
  - slip-print.inc
Circulation -> Check out
  - timepicker.inc
Administration -> System preferences:
  - str/tinymce_i18n.inc
  - wysiwyg-systempreferences.inc
Cataloging -> Z39.50 Search:
  - z3950_search.inc

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Comment 10 Katrin Fischer 2020-04-28 18:44:06 UTC
Created attachment 103877 [details] [review]
Bug 25002: JS Includes should be wrapped with template comments

This patch adds HTML comments to Template::Toolkit include files which
contain <script> tags so that it is clear where the embedded scripts can
be found in the code.

To test, apply the patch and view source on the following pages to
verify the presence of the comments:

Acquisitions home page:
  - acquisitions-toolbar.inc
  - validtor-strings.inc
  - js_includes.inc
  - format-price.inc
Acquisitions -> Add order from new record,
Acquisitions -> Receive order:
  - additem.js.inc
Cataloging -> Add/Edit item:
  - columns_settings.inc
  - strings.inc
  - select2.inc
  - calendar.inc
  - str/cataloging_additem.inc
Authorities home page:
  - authorities_js.inc
Bibliographic detail page:
  - catalog-strings.inc
Cataloging -> Advanced editor:
  - cateditor-ui.inc
  - cateditor-widgets-marc21.inc
Administration -> Item types:
  - greybox.inc
ILL requests:
  - ill-list-table-strings.inc
Web installer
  - installer-intranet-bottom.inc
Web installer -> Onboarding
  - installer-strings.inc
Lists -> List contents -> Merge records
  - merge-record-strings.inc
Patrons -> Patron -> Change password
  - password_check.inc
  - str/members-menu.inc
Patrons -> Patron -> Print summary
  - slip-print.inc
Circulation -> Check out
  - timepicker.inc
Administration -> System preferences:
  - str/tinymce_i18n.inc
  - wysiwyg-systempreferences.inc
Cataloging -> Z39.50 Search:
  - z3950_search.inc

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Martin Renvoize 2020-04-29 16:36:04 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-05-08 22:39:37 UTC
enhancement not backported to 19.11