Bug 35199

Summary: Fix error handling in http-client.js
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to oldstable --- QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: major    
Priority: P5 - low CC: fridolin.somers, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.05,22.11.12
Bug Depends on: 34448    
Bug Blocks: 35201    
Attachments: Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js

Description Jonathan Druart 2023-10-31 13:55:03 UTC
Coming from bug 34448.

386         // Submit the form, get 500
387         cy.intercept("POST", "/api/v1/erm/agreements", {
388             statusCode: 500,
389             error: "Something went wrong",
390         });
391         cy.get("#agreements_add").contains("Submit").click();
392         cy.get("main div[class='dialog alert']").contains(
393             "Something went wrong: SyntaxError: Unexpected end of JSON input"
394         );

This is totally wrong. We are now showing a JS error instead of the expected 500: internal server error!
Comment 1 Jonathan Druart 2023-10-31 14:02:09 UTC
Created attachment 158121 [details] [review]
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js

From bug bug 34448.

386         // Submit the form, get 500
387         cy.intercept("POST", "/api/v1/erm/agreements", {
388             statusCode: 500,
389             error: "Something went wrong",
390         });
391         cy.get("#agreements_add").contains("Submit").click();
392         cy.get("main div[class='dialog alert']").contains(
393             "Something went wrong: SyntaxError: Unexpected end of JSON input"
394         );

This is wrong: we are now showing a JS error (SyntaxError) instead of the expected 500: internal server error!

The problem was that a regular 500 does not have anything in the body,
and _fetchJSON didn't handle that ( JSON.parse(text) ).
If the body of the response does not contain anything we need to get the
text from statusText (which contains "Internal Server Error" in case of
500).

Test plan:
1. Make sure all cypress tests pass
2. Confirm the above:
Raise an exception from a given route (/agreements for instance) and
confirm that the error displayed on the interface is correct (ie. not
SyntaxError, but "Error: Internal Server Error")

For QA:
* This change is covered properly in Dialog_spec.ts, no need to redo it in
every other test files.
* Without the following change in count, we see:
"Something went wrong: Error: Error: Internal Server Error" because
setError is called twice. We don't need to set the error from count, it
has been set from _fetchJSON already.
-            error => {
-                setError(error.toString());
-            }
+            error => {}
Comment 2 Jonathan Druart 2023-10-31 14:19:06 UTC
(In reply to Jonathan Druart from comment #1)
> 1. Make sure all cypress tests pass

Note that "Preservation" tests will be fixed on bug 35201.
Comment 3 Jonathan Druart 2023-10-31 14:31:48 UTC
Created attachment 158125 [details] [review]
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js

From bug bug 34448.

386         // Submit the form, get 500
387         cy.intercept("POST", "/api/v1/erm/agreements", {
388             statusCode: 500,
389             error: "Something went wrong",
390         });
391         cy.get("#agreements_add").contains("Submit").click();
392         cy.get("main div[class='dialog alert']").contains(
393             "Something went wrong: SyntaxError: Unexpected end of JSON input"
394         );

This is wrong: we are now showing a JS error (SyntaxError) instead of the expected 500: internal server error!

The problem was that a regular 500 does not have anything in the body,
and _fetchJSON didn't handle that ( JSON.parse(text) ).
If the body of the response does not contain anything we need to get the
text from statusText (which contains "Internal Server Error" in case of
500).

Test plan:
1. Make sure all cypress tests pass
2. Confirm the above:
Raise an exception from a given route (/agreements for instance) and
confirm that the error displayed on the interface is correct (ie. not
SyntaxError, but "Error: Internal Server Error")

For QA:
* This change is covered properly in Dialog_spec.ts, no need to redo it in
every other test files.
* Without the following change in count, we see:
"Something went wrong: Error: Error: Internal Server Error" because
setError is called twice. We don't need to set the error from count, it
has been set from _fetchJSON already.
-            error => {
-                setError(error.toString());
-            }
+            error => {}
Comment 4 Tomás Cohen Arazi 2023-10-31 14:43:03 UTC
Created attachment 158129 [details] [review]
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js

From bug bug 34448.

386         // Submit the form, get 500
387         cy.intercept("POST", "/api/v1/erm/agreements", {
388             statusCode: 500,
389             error: "Something went wrong",
390         });
391         cy.get("#agreements_add").contains("Submit").click();
392         cy.get("main div[class='dialog alert']").contains(
393             "Something went wrong: SyntaxError: Unexpected end of JSON input"
394         );

This is wrong: we are now showing a JS error (SyntaxError) instead of the expected 500: internal server error!

The problem was that a regular 500 does not have anything in the body,
and _fetchJSON didn't handle that ( JSON.parse(text) ).
If the body of the response does not contain anything we need to get the
text from statusText (which contains "Internal Server Error" in case of
500).

Test plan:
1. Make sure all cypress tests pass
2. Confirm the above:
Raise an exception from a given route (/agreements for instance) and
confirm that the error displayed on the interface is correct (ie. not
SyntaxError, but "Error: Internal Server Error")

For QA:
* This change is covered properly in Dialog_spec.ts, no need to redo it in
every other test files.
* Without the following change in count, we see:
"Something went wrong: Error: Error: Internal Server Error" because
setError is called twice. We don't need to set the error from count, it
has been set from _fetchJSON already.
-            error => {
-                setError(error.toString());
-            }
+            error => {}

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2023-10-31 14:45:54 UTC
All things (but Preservation) pass, the changes are simple and make sense. QA script happy as well. Straight QA.
Comment 6 Tomás Cohen Arazi 2023-10-31 19:50:15 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 7 Fridolin Somers 2023-11-02 20:47:44 UTC
Pushed to 23.05.x for 23.05.05
Comment 8 Matt Blenkinsop 2023-11-13 15:11:48 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x