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!
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 => {}
(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.
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 => {}
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>
All things (but Preservation) pass, the changes are simple and make sense. QA script happy as well. Straight QA.
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.05
Nice work everyone! Pushed to oldstable for 22.11.x