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.
See first few minutes of the `Koha Virtual Hackfest - Code Spaghetti` live stream recording for further context.
*** Bug 25003 has been marked as a duplicate of this bug. ***
Martin beat me to it :)
Created attachment 102041 [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 Cataloging -> Add/Edit item: - additem.js.inc - 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
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.
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 :-)
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
Thanks for testing, David. And don't hesitate to mark something "Failed QA" when you see problems. If anything it gets my attention!
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>
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>
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11