Summary: | JS Includes should be wrapped with template comments | ||
---|---|---|---|
Product: | Koha | Reporter: | Martin Renvoize <martin.renvoize> |
Component: | Templates | Assignee: | Owen Leonard <oleonard> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | 1joynelson, andrewfh, david.roberts |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
20.05.00
|
|
Bug Depends on: | |||
Bug Blocks: | 25003 | ||
Attachments: |
Bug 25002: JS Includes should be wrapped with template comments
Bug 25002: JS Includes should be wrapped with template comments Bug 25002: JS Includes should be wrapped with template comments Bug 25002: JS Includes should be wrapped with template comments |
Description
Martin Renvoize
2020-03-27 15:11:50 UTC
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 |