---- Reported by gmcharlt@gmail.com 2010-05-03 16:13:31 ---- acq.js uses XMLHttpRequest() directly to a web service call, which is more prone to errors and web browser compatibility issues. It should the jQuery AJAX API instead, both to avoid such issues and to be consistent with the other JavaScript in Koha. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:27 UTC --- This bug was previously known as _bug_ 4437 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4437 Actual time not defined. Setting to 0.0 Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one.
Issue still exists in master.
I'm working on this now
Created attachment 14646 [details] [review] bug 4437 Change XMLHttpRequest calls to jQuery.ajax The acq.js file used a few calls to XMLHttpRequest, which have been changed to use jQuery.ajax instead Signed-off-by: Hugh Davenport <hugh@davenport.net.nz>
That file also has a lot of other chunks of code using generic javascript, which should use jQuery when possible, so I reckon this bug should be kept open for that purpose. Cheers, Hugh
Created attachment 14649 [details] [review] bug 4437 Change XMLHttpRequest calls to jQuery.ajax The acq.js file used a few calls to XMLHttpRequest, which have been changed to use jQuery.ajax instead Signed-off-by: Hugh Davenport <hugh@davenport.net.nz>
Can you please add a test plan for this? acq.js is modified in several places, so I assume several different functions are being affected. A test plan will ensure that we test all the changes properly.
Hey Owen, What would the format of a test plan be? Cheers, Hugh
A test plan is a simple description of the steps one would take to confirm that a patch works. Usually this means confirming that a patch fixes a particular problem and doesn't cause other problems. Is there a real-world case where this fixes a browser error? If not, you should describe how to test each affected page/action to confirm that the patch hasn't caused any problems.
Sill valid?
(In reply to Marc Véron from comment #9) > Sill valid? Yes. $ git grep -l XMLHttpRequest | grep acq
Doesn't apply because fetchSortDropbox no longer exists. The other three functional changes should still be possible. Attempting resurrection.
Created attachment 63756 [details] [review] Bug 4437: acq.js uses XMLHttpRequest() directly; should use jQuery The acq.js file used a few calls to XMLHttpRequest, which have been changed to use jQuery.ajax instead. The fetchSortDropbox no longer exists, which is why this patch did not apply. However, the other three functions still had direct XMLHttpRequest calls: - totalExceedsBudget - budgetExceedsParent - checkBudgetParent A test plan would require triggering these three javascript functions.
The totalExceedsBudget is triggered by Check in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt The budgetExceedsParent is triggered by Check in koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgets.tt The checkBudgetParent is triggered by Check in koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgets.tt NOTE: I have not checked this myself, but should someone write a test plan for novice libarian techies, I will check.
> The budgetExceedsParent is triggered by Check in > koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgets.tt Testing budgetExceedsParent (without patch): 1) Go to Home > Administration > Budgets administration 2) Create a Budget 'Testbudget', give it a value of 100, make it active 3) Add a fund 'Testfund 1', give it a value of 90, save 4) Add a fund 'Testfund 2', give it a value of 20 (exceeds budget), save Result OK: Message box ------------------------------------------------------- Form not submitted because of the following problem(s) ------------------------------------------------------- - Fund amount exceeds period allocation ------------------------------------------------------- Testing budgetExceedsParent (with patch): a) Clear browser cache, restart memcached, plack b) Make sure page is reloaded, repeat step 4) above Result: Not OK, no message box, form saves, period allocated is 100, total available is 110 (should not) (Other functions not yet tested)
acq.js appears to be the non-external library that still uses XMLHttpRequest: koha-tmpl/intranet-tmpl/prog/js/acq.js In: totalExceedsBudget() budgetExceedsParent() checkBudgetParent() The others are: koha-tmpl/opac-tmpl/bootstrap/js/Gettext.js koha-tmpl/opac-tmpl/bootstrap/lib/greybox/GreyBox_v5_5/greybox_source/base/AJS.js koha-tmpl/opac-tmpl/bootstrap/lib/jquery/jquery-3.6.0.min.js koha-tmpl/opac-tmpl/lib/verovio/000_acoustic_grand_piano.js koha-tmpl/opac-tmpl/lib/verovio/verovio-toolkit.js I am resetting assignee since this hasn't had an update in several years.
Created attachment 152859 [details] [review] Bug 4437: acq.js uses XMLHttpRequest() directly; should use jQuery I was too lazy to resolve the merge conflicts so I just rewrote the previous patch. The functions should be functionally equivalent to the ones using XMLHttpRequest, so they should still work as intended in their consumers. I deleted the getAuthValueDropbox function as I couldn't find a single reference in the entire codebase. Correct me if I'm wrong. I also ran the configured formatter on the whole file to clean up the mess. Actual changes in implementation are just in these functions - totalExceedsBudget - budgetExceedsParent - checkBudgetParent We could pontentially follow this up by making async requests instead to prevent blocking behaviour. But this would mean that we need to change the consumers as well. To test: Testing budgetExceedsParent (without patch): 1) Go to Home > Administration > Budgets administration 2) Create a Budget 'Testbudget', give it a value of 100, make it active 3) Add a fund 'Testfund 1', give it a value of 90, save 4) Add a fund 'Testfund 2', give it a value of 20 (exceeds budget), save Result OK: Message box ------------------------------------------------------- Form not submitted because of the following problem(s) ------------------------------------------------------- - Fund amount exceeds period allocation ------------------------------------------------------- Testing budgetExceedsParent (with patch): a) Clear browser cache, restart memcached, plack b) Make sure page is reloaded, repeat step 4) above Result: Not OK, no message box, form saves, period allocated is 100, total available is 110 (should not) 5) Test the other consumers in a similar manor. 6) Sign off.
> I deleted the getAuthValueDropbox function It's used in newordertempty.tt, which I don't think is working as it should even if I restore the function. If I create a new order which exceeds the fund shouldn't I get a JS error on the page? Instead I'm redirected to a page with an error message, and if I click the back button the form is empty again. And I don't think I understand how to test checkBudgetParent
You're right! Strange that it didn't show up when I searched for references. I will find out and add to the existing test plan.
Hey oleonard, do you understand what the getAuthValueDropbox fn is even supposed to do?
> If I create a new order which exceeds the fund shouldn't I get a JS error on the page? Did you mean this? https://snipboard.io/KLUjyw.jpg