Bug 34448 - ERM should be able to display error messages coming from the API
Summary: ERM should be able to display error messages coming from the API
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 34587 35199
  Show dependency treegraph
 
Reported: 2023-07-31 14:54 UTC by Pedro Amorim
Modified: 2023-10-31 13:55 UTC (History)
6 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.04,22.11.10


Attachments
Bug 34448: Update the way we handle response in http-client.js (2.48 KB, patch)
2023-07-31 14:57 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34448: Update cypress tests (41.67 KB, patch)
2023-07-31 14:57 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34448: Update cypress tests (6.06 KB, patch)
2023-07-31 15:04 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34448: Update cypress tests (6.06 KB, patch)
2023-08-01 08:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34448: Update the way we handle response in http-client.js (2.59 KB, patch)
2023-08-01 08:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34448: Update cypress tests (6.14 KB, patch)
2023-08-04 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34448: Update the way we handle response in http-client.js (2.67 KB, patch)
2023-08-04 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-07-31 14:54:31 UTC

    
Comment 1 Pedro Amorim 2023-07-31 14:57:35 UTC
Created attachment 154097 [details] [review]
Bug 34448: Update the way we handle response in http-client.js

Test plan:
Before patch:
- Visit a non-existent ID i.e. /cgi-bin/koha/erm/agreements/999
- Visit a char ID i.e. /cgi-bin/koha/erm/agreements/abc

Apply patch:
Repeat above steps.

Run cypress tests
Comment 2 Pedro Amorim 2023-07-31 14:57:38 UTC
Created attachment 154098 [details] [review]
Bug 34448: Update cypress tests
Comment 3 Pedro Amorim 2023-07-31 15:04:27 UTC
Created attachment 154099 [details] [review]
Bug 34448: Update cypress tests
Comment 4 Jonathan Druart 2023-08-01 06:50:13 UTC
I would keep the console.log with the whole 'response' object.

Looking at js/datatables.js, 

https://git.koha-community.org/Koha-community/Koha/src/branch/master/koha-tmpl/intranet-tmpl/prog/js/datatables.js#L493

And wondering if we should not split on several lines instead of separating with a space.
Comment 5 Pedro Amorim 2023-08-01 08:36:11 UTC
Created attachment 154123 [details] [review]
Bug 34448: Update cypress tests
Comment 6 Pedro Amorim 2023-08-01 08:36:40 UTC
Created attachment 154124 [details] [review]
Bug 34448: Update the way we handle response in http-client.js

Test plan:
Before patch:
- Visit a non-existent ID i.e. /cgi-bin/koha/erm/agreements/999
- Visit a char ID i.e. /cgi-bin/koha/erm/agreements/abc

Apply patch:
Repeat above steps.

Run cypress tests
Comment 7 Pedro Amorim 2023-08-01 08:41:27 UTC
(In reply to Jonathan Druart from comment #4)
> I would keep the console.log with the whole 'response' object.
> 
> Looking at js/datatables.js, 
> 
> https://git.koha-community.org/Koha-community/Koha/src/branch/master/koha-
> tmpl/intranet-tmpl/prog/js/datatables.js#L493
> 
> And wondering if we should not split on several lines instead of separating
> with a space.

Thank you very much Jonathan, I've squashed that into the patch!
Comment 8 Jonathan Druart 2023-08-04 12:03:46 UTC
Before:
Something went wrong: Error: Error: Internal Server Error (500)

After:
Something went wrong: Error: Error: Something went wrong, check Koha logs for details.

Before:
Something went wrong: Error: Not Found (404)
After:
Something went wrong: Error: Agreement not found

Before:
Something went wrong: Error: Bad Request (400)
After:
Something went wrong: Error: Expected integer - got string.

Note for myself:
"""The ok read-only property of the Response interface contains a Boolean stating whether the response was successful (status in the range 200-299) or not."""

Only wondering if we should not keep the HTTP status code.
Comment 9 Jonathan Druart 2023-08-04 12:04:40 UTC
Created attachment 154259 [details] [review]
Bug 34448: Update cypress tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2023-08-04 12:04:43 UTC
Created attachment 154260 [details] [review]
Bug 34448: Update the way we handle response in http-client.js

Test plan:
Before patch:
- Visit a non-existent ID i.e. /cgi-bin/koha/erm/agreements/999
- Visit a char ID i.e. /cgi-bin/koha/erm/agreements/abc

Apply patch:
Repeat above steps.

Run cypress tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Tomás Cohen Arazi 2023-09-01 14:25:01 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 12 Fridolin Somers 2023-09-08 05:52:30 UTC
Can we backport to 23.05.x ?
Comment 13 Pedro Amorim 2023-09-08 08:28:12 UTC
(In reply to Fridolin Somers from comment #12)
> Can we backport to 23.05.x ?

I believe so, if it applies and the test plan checks out?
It's technically an enhancement, though. Up to you really!
Comment 14 Fridolin Somers 2023-09-14 17:54:12 UTC
Pushed to 23.05.x for 23.05.04
Comment 15 Matt Blenkinsop 2023-09-15 10:08:51 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 16 Jonathan Druart 2023-10-31 11:13:04 UTC
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!