Bug 35199 - Fix error handling in http-client.js
Summary: Fix error handling in http-client.js
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 34448
Blocks: 35201
  Show dependency treegraph
 
Reported: 2023-10-31 13:55 UTC by Jonathan Druart
Modified: 2023-11-13 15:11 UTC (History)
2 users (show)

See Also:
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


Attachments
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js (12.59 KB, patch)
2023-10-31 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js (14.52 KB, patch)
2023-10-31 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35199: (bug 34448 follow-up) Fix error handling in http-client.js (14.58 KB, patch)
2023-10-31 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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