Bug 4437

Summary: acq.js uses XMLHttpRequest() directly; should use jQuery
Product: Koha Reporter: Galen Charlton <gmcharlt>
Component: AcquisitionsAssignee: Bugs List <koha-bugs>
Status: Failed QA --- QA Contact: Bugs List <koha-bugs>
Severity: minor    
Priority: P5 - low CC: hugh, mtompset, paul.derscheid, veron
Version: master   
Hardware: All   
OS: All   
URL: ./koha-tmpl/intranet-tmpl/prog/en/js/acq.js
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 9412    
Attachments: bug 4437 Change XMLHttpRequest calls to jQuery.ajax
bug 4437 Change XMLHttpRequest calls to jQuery.ajax
Bug 4437: acq.js uses XMLHttpRequest() directly; should use jQuery
Bug 4437: acq.js uses XMLHttpRequest() directly; should use jQuery

Description Chris Cormack 2010-05-21 01:27:10 UTC


---- 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.

Comment 1 Owen Leonard 2013-01-09 14:10:09 UTC
Issue still exists in master.
Comment 2 Hugh Davenport 2013-01-17 01:08:02 UTC
I'm working on this now
Comment 3 Hugh Davenport 2013-01-17 03:15:42 UTC Comment hidden (obsolete)
Comment 4 Hugh Davenport 2013-01-17 03:17:57 UTC
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
Comment 5 Hugh Davenport 2013-01-17 04:43:22 UTC
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>
Comment 6 Owen Leonard 2013-02-05 17:37:42 UTC
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.
Comment 7 Hugh Davenport 2013-07-05 21:12:14 UTC
Hey Owen,

What would the format of a test plan be?

Cheers,

Hugh
Comment 8 Owen Leonard 2013-08-08 13:25:37 UTC
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.
Comment 9 Marc Véron 2016-08-22 08:38:58 UTC
Sill valid?
Comment 10 Mark Tompsett 2017-05-27 02:55:45 UTC
(In reply to Marc Véron from comment #9)
> Sill valid?

Yes.
$ git grep -l XMLHttpRequest | grep acq
Comment 11 Mark Tompsett 2017-05-27 03:00:57 UTC
Doesn't apply because fetchSortDropbox no longer exists.
The other three functional changes should still be possible.
Attempting resurrection.
Comment 12 Mark Tompsett 2017-05-27 03:27:24 UTC
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.
Comment 13 Mark Tompsett 2017-05-27 03:37:42 UTC
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.
Comment 14 Marc Véron 2017-05-27 08:23:43 UTC
> 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)
Comment 15 Katrin Fischer 2023-01-07 22:06:21 UTC
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.
Comment 16 Paul Derscheid 2023-06-29 10:27:55 UTC
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.
Comment 17 Owen Leonard 2023-06-29 11:22:25 UTC
> 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
Comment 18 Paul Derscheid 2023-06-29 12:37:39 UTC
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.
Comment 19 Paul Derscheid 2023-06-29 13:21:43 UTC
Hey oleonard, do you understand what the getAuthValueDropbox fn is even supposed to do?
Comment 20 Paul Derscheid 2023-06-29 13:28:18 UTC
> 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