The form-submit JS is required for submitting stateful forms in some situations, as of JS16. However, it's difficult to use in .inc files, because all scripts need to be included at the end of the template (JS12), and includes are usually processed earlier in the template. This means that if an include needs the form-submit JS, the JS needs to be separately added to each template that uses the original include. Moving the form-submit JS into the global js would make the above situation a lot cleaner, and it makes sense to make it available to all templates anyway given its inclusion in the coding guidelines.
Created attachment 180669 [details] [review] Bug 39567: Move form-submit JS into global js To test: 1. Apply patch 2. Clear browser cache 3. git grep "form-submit.js" --> Confirm that all imports of the form-submit.js asset have been removed, and the only results are the comment in global.js and staff-global.js 4. Test some of the following buttons and confirm they work as expected (all buttons except the first are in the staff interface): - OPAC > Account page > Renew a checkout - Bib record > Holds > Revert transit/waiting status on a hold - Acquisitions > Basket > Create EDIFACT order - Acquisitions > Invoices > detail for a specific invoice > Delete - Acquisitions > Invoices > perform actions on invoices using the action drop-down - Acquisitions > Invoices > open/close invoices by selecting the checkboxes - Administration > Additional fields > Delete an additional field - Administration > Background jobs > Cancel a background job - Edit an item > scroll up > Actions drop-down > Print label - Cataloging > Label creator > Manage label batches > Edit a batch > Delete batch - Cataloging > Label creator > Manage label batches > Delete a batch - Patron account > holds > delete a hold - Tools > Patron lists > delete a list from the Actions drop-down - Reports > Saved reports > Preview SQL on a report > Delete - Reports > Saved reports > Delete a report from the actions drop-down - Rotating collections > Delete a collection from the actions drop-down - Stock rotations > move item to the next stage - Stock rotations > remove item from a rota - Stock rotations > Add/remove "In demand" - More > Lists > edit a specific list > Edit drop-down > Delete
Created attachment 180687 [details] [review] Bug 39567: [POC] Keep form-submit code into a separate file but include it from js_includes.inc
I don't think moving the code to staff-global.js is a good idea. We should actually do the reverse and split it into small files. I won't have much time to follow-up here but I wanted to provide an alternative approach, for discussion (note that it's not ready for inclusion as it's missing the changes for OPAC). Feel free to drop it.
I think the idea is great since this bit is so widely used. Have to agree with Jonathan, smaller, more focused files are better for maintainability and also prevent bloat of the global js. The suggestion to move it into js_includes.inc and have a separate file for the handler is also great. Would you be willing to make that change Emily? Then I would happily SO.
Sounds good to me! I'll upload an updated patchset shortly. Thank you for the feedback!
Created attachment 180983 [details] [review] Bug 39567: Include form-submit code from js_includes.inc (staff interface) Import the form-submit.js script in the global includes file for the staff interface, so that all pages have access to it without needing to import it directly.
Created attachment 180984 [details] [review] Bug 39567: Include form-submit code from js_includes.inc (OPAC) Import the form-submit.js script in the global includes file for the OPAC, so that all pages have access to it without needing to import it directly. This patch also aligns the OPAC copy of form-submit.js to the staff interface copy, as the OPAC copy is missing 3 lines to support opening the link in a new tab (added to the staff interface copy in bug 37192).
Created attachment 180985 [details] [review] Bug 39567: Remove form-submit includes from tt files Now that the form-submit code has been added to the global js-includes files for both the staff interface and OPAC, it no longer needs to be included directly in each template file. To test: 1. Apply patch 2. Clear browser cache 3. git grep "form-submit.js" --> Confirm that all imports of the form-submit.js asset have been removed, and the only results are the comment in global.js and staff-global.js 4. Test OPAC > Account page > Renew a checkout to confirm the link submission still works as expected in the OPAC i. Check out an item to a patron ii. Set a known password for that patron iii. Log into that patron's account in the OPAC iv. Click the "Renew" link next to that checkout in the checkouts table on the patron's OPAC account page --> Confirm the item is renewed as expected 5. Test some of the following buttons to confirm the link submission still works as expected in the staff interface: - Bib record > Holds > Revert transit/waiting status on a hold - Acquisitions > Basket > Create EDIFACT order - Acquisitions > Invoices > detail for a specific invoice > Delete - Acquisitions > Invoices > perform actions on invoices using the action drop-down - Acquisitions > Invoices > open/close invoices by selecting the checkboxes - Administration > Additional fields > Delete an additional field - Administration > Background jobs > Cancel a background job - Edit an item > scroll up > Actions drop-down > Print label - Cataloging > Label creator > Manage label batches > Edit a batch > Delete batch - Cataloging > Label creator > Manage label batches > Delete a batch - Patron account > holds > delete a hold - Tools > Patron lists > delete a list from the Actions drop-down - Reports > Saved reports > Preview SQL on a report > Delete - Reports > Saved reports > Delete a report from the actions drop-down - Rotating collections > Delete a collection from the actions drop-down - Stock rotations > move item to the next stage - Stock rotations > remove item from a rota - Stock rotations > Add/remove "In demand" - More > Lists > edit a specific list > Edit drop-down > Delete
Created attachment 180986 [details] [review] Bug 39567: Include form-submit code from js_includes.inc (staff interface) Import the form-submit.js script in the global includes file for the staff interface, so that all pages have access to it without needing to import it directly.
Created attachment 180987 [details] [review] Bug 39567: Include form-submit code from opac-bottom.inc (OPAC) Import the form-submit.js script in the global includes file for the OPAC, so that all pages have access to it without needing to import it directly. This patch also aligns the OPAC copy of form-submit.js to the staff interface copy, as the OPAC copy is missing 3 lines to support opening the link in a new tab (added to the staff interface copy in bug 37192).
Created attachment 180988 [details] [review] Bug 39567: Remove form-submit includes from tt files Now that the form-submit code has been added to the global js-includes files for both the staff interface and OPAC, it no longer needs to be included directly in each template file. To test: 1. Apply patch 2. Clear browser cache 3. git grep "form-submit.js" --> Confirm that all imports of the form-submit.js asset have been removed, and the only results are the comment in global.js and staff-global.js 4. Test OPAC > Account page > Renew a checkout to confirm the link submission still works as expected in the OPAC i. Check out an item to a patron ii. Set a known password for that patron iii. Log into that patron's account in the OPAC iv. Click the "Renew" link next to that checkout in the checkouts table on the patron's OPAC account page --> Confirm the item is renewed as expected 5. Test some of the following buttons to confirm the link submission still works as expected in the staff interface: - Bib record > Holds > Revert transit/waiting status on a hold - Acquisitions > Basket > Create EDIFACT order - Acquisitions > Invoices > detail for a specific invoice > Delete - Acquisitions > Invoices > perform actions on invoices using the action drop-down - Acquisitions > Invoices > open/close invoices by selecting the checkboxes - Administration > Additional fields > Delete an additional field - Administration > Background jobs > Cancel a background job - Edit an item > scroll up > Actions drop-down > Print label - Cataloging > Label creator > Manage label batches > Edit a batch > Delete batch - Cataloging > Label creator > Manage label batches > Delete a batch - Patron account > holds > delete a hold - Tools > Patron lists > delete a list from the Actions drop-down - Reports > Saved reports > Preview SQL on a report > Delete - Reports > Saved reports > Delete a report from the actions drop-down - Rotating collections > Delete a collection from the actions drop-down - Stock rotations > move item to the next stage - Stock rotations > remove item from a rota - Stock rotations > Add/remove "In demand" - More > Lists > edit a specific list > Edit drop-down > Delete
Ready for testing! Note for QA team: the changes to form-submit.js (in both staff interface and OPAC) are mostly indentation changes (due to replacing two nested function bindings with a single function declaration). If you're not already in the habit of viewing diffs with one of the ignore-whitespace flags, would definitely recommend it for those files in these patches.
I'm digging the move to using an ES module. However, as per JS1 from the coding guidelines, I think we should keep all the relevant JS code in a separate file. You can do that with the following: [% Asset.js("js/form-submit.js",{'type' => 'module'}) | $raw %] Then rather than exporting the function, you can just run it in koha-tmpl/intranet-tmpl/prog/js/form-submit.js Alternatively, if we think we do want to export it for whatever reason, then I'd suggest we make something like koha-tmpl/intranet-tmpl/prog/js/esm/form-submit.js to store the module and have koha-tmpl/intranet-tmpl/prog/js/form-submit.js stay as the initiator script which invokes the function at load time. -- Also, could we get a name change on this bug report to reflect its current nature?
Thank you for the feedback! I'm afraid I'm not completely following it, though - I'm not very familiar with JS modularization, to be honest! If there's a discussion on best practices to be had, I'm going to need some help on this one.
Posted https://chat.koha-community.org/koha-community/pl/wp1sk3une3rjzkju1kwxqfw39r for discussion and consensus
You could just replace this: <script type="module"> import { submit_form } from "/intranet-tmpl/prog/js/form-submit.js"; $("body").on("click", ".submit-form-link", function (e) { e.preventDefault(); submit_form(this); }); </script> with this: [% Asset.js("js/form-submit.js", { 'type' => 'module' }) | $raw %] And then call it within form-submit.js: $("body").on("click", ".submit-form-link", function (e) { e.preventDefault(); submit_form(this); }); ... rest of the code That way we still have this module but no inline code is required in js_includes.inc for example. David's suggestion regarding an esm path and a caller makes sense, but I think just for this one function, introducing that is not necessary. If we decided not to export the function, you could also just wrap it with (() => { <content of form-submit.js> })(); and have it as an IIFE rather than a module. Which would then also shorten the Asset.js call to: [% Asset.js("js/form-submit.js") | $raw %] But that really depends on whether the exported function is in use outside of the context of form-submit.js itself. Maybe an IIFE is more fitting and can later be replaced by an exported function if the need arises?
Created attachment 181235 [details] [review] Bug 39567: Run JS change in separate file ESM as per JS1 As per JS1 in the coding guidelines, I've moved the inline JS from the template into a separate file.
I've gone ahead and attached my suggested changes as an additional patch. Hopefully that makes things clearer?
(In reply to Paul Derscheid from comment #16) > If we decided not to export the function, you could also just wrap it with > (() => { > <content of form-submit.js> > })(); > and have it as an IIFE rather than a module. Which would then also shorten > the Asset.js call to: > [% Asset.js("js/form-submit.js") | $raw %] > > But that really depends on whether the exported function is in use outside > of the context of form-submit.js itself. Maybe an IIFE is more fitting and > can later be replaced by an exported function if the need arises? That's also true. There's nothing about this code currently that necessarily requires the use of a module. We don't need to export the function, and we're not importing anything from a different module. I suppose the handy thing with the IIFE or the module is the scoping. With either option, we don't have to worry about the function "submit_form" conflicting with another function of the same name defined somewhere out there. I think both options are equal in this case. That said, I think you've already done all this work. I've just tweaked it to fit JS1. But since I've added a patch, I'll leave it to QA to determine what the best way to go is.
Speaking of JS guidelines, I suppose we'll need to update JS16... I'm not sure if JS15 is getting enforced... (Just noticed JS9 kind of gives some conflicting advice with JS1 with its example...) -- In any case, I think we're in a transitional time regarding best practices for organising Javascript code and implementing its execution (apart from using separate files).
Thanks for the input, Paul and David! (In reply to David Cook from comment #18) > I've gone ahead and attached my suggested changes as an additional patch. > Hopefully that makes things clearer? That definitely helps! I'm fine with moving forward with this patch - it doesn't sound like there are objections, and I don't want to hold up the dependent patch too long looking for a perfect solution. > Speaking of JS guidelines, I suppose we'll need to update JS16... Good catch, thanks! Adding it to my to-do list for when this patch gets pushed.
s/dependent patch/dependent bug
The patches no longer apply for me 8-(... git bz apply 39567 Bug 39567 - Move form-submit js into js includes files 180986 - Bug 39567: Include form-submit code from js_includes.inc (staff interface) 180987 - Bug 39567: Include form-submit code from opac-bottom.inc (OPAC) 180988 - Bug 39567: Remove form-submit includes from tt files 181235 - Bug 39567: Run JS change in separate file ESM as per JS1 Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 39567: Include form-submit code from js_includes.inc (staff interface) Applying: Bug 39567: Include form-submit code from opac-bottom.inc (OPAC) Applying: Bug 39567: Remove form-submit includes from tt files Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt M koha-tmpl/intranet-tmpl/prog/en/modules/admin/additional-fields.tt M koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt M koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt M koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt M koha-tmpl/intranet-tmpl/prog/en/modules/labels/label-edit-batch.tt M koha-tmpl/intranet-tmpl/prog/en/modules/labels/label-manage.tt M koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt M koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt M koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt M koha-tmpl/intranet-tmpl/prog/en/modules/tools/stockrotation.tt M koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/stockrotation.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/labels/label-manage.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/labels/label-edit-batch.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/additional-fields.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 39567: Remove form-submit includes from tt files
Created attachment 182168 [details] [review] Bug 39567: Include form-submit code from js_includes.inc (staff interface) Import the form-submit.js script in the global includes file for the staff interface, so that all pages have access to it without needing to import it directly.
Created attachment 182169 [details] [review] Bug 39567: Include form-submit code from opac-bottom.inc (OPAC) Import the form-submit.js script in the global includes file for the OPAC, so that all pages have access to it without needing to import it directly. This patch also aligns the OPAC copy of form-submit.js to the staff interface copy, as the OPAC copy is missing 3 lines to support opening the link in a new tab (added to the staff interface copy in bug 37192).
Created attachment 182170 [details] [review] Bug 39567: Remove form-submit includes from tt files Now that the form-submit code has been added to the global js-includes files for both the staff interface and OPAC, it no longer needs to be included directly in each template file. To test: 1. Apply patch 2. Clear browser cache 3. git grep "form-submit.js" --> Confirm that all imports of the form-submit.js asset have been removed, and the only results are the comment in global.js and staff-global.js 4. Test OPAC > Account page > Renew a checkout to confirm the link submission still works as expected in the OPAC i. Check out an item to a patron ii. Set a known password for that patron iii. Log into that patron's account in the OPAC iv. Click the "Renew" link next to that checkout in the checkouts table on the patron's OPAC account page --> Confirm the item is renewed as expected 5. Test some of the following buttons to confirm the link submission still works as expected in the staff interface: - Bib record > Holds > Revert transit/waiting status on a hold - Acquisitions > Basket > Create EDIFACT order - Acquisitions > Invoices > detail for a specific invoice > Delete - Acquisitions > Invoices > perform actions on invoices using the action drop-down - Acquisitions > Invoices > open/close invoices by selecting the checkboxes - Administration > Additional fields > Delete an additional field - Administration > Background jobs > Cancel a background job - Edit an item > scroll up > Actions drop-down > Print label - Cataloging > Label creator > Manage label batches > Edit a batch > Delete batch - Cataloging > Label creator > Manage label batches > Delete a batch - Patron account > holds > delete a hold - Tools > Patron lists > delete a list from the Actions drop-down - Reports > Saved reports > Preview SQL on a report > Delete - Reports > Saved reports > Delete a report from the actions drop-down - Rotating collections > Delete a collection from the actions drop-down - Stock rotations > move item to the next stage - Stock rotations > remove item from a rota - Stock rotations > Add/remove "In demand" - More > Lists > edit a specific list > Edit drop-down > Delete
Created attachment 182171 [details] [review] Bug 39567: Run JS change in separate file ESM as per JS1 As per JS1 in the coding guidelines, I've moved the inline JS from the template into a separate file.
Rebased for the current main