fix: parse error message properly for manual installs #541
Conversation
|
🚀 Deployed on https://pr-541--dhis2-app-management.netlify.app |
kabaros
force-pushed
the
DHIS2-13883/fix-manual-install-error-handling
branch
from
581cc18
to
f40f287
Compare
Mohammer5
requested review from
stian-sandvold and
tomzemp
and removed request for
stian-sandvold and
tomzemp
| @@ -10,9 +11,18 @@ class Api { | |||
| method, | |||
| body, | |||
| credentials: 'include', | |||
| }).then((res) => { | |||
| redirect: 'manual', | |||
There was a problem hiding this comment.
Not sure we need this: If we're explicitly using res.json(), shouldn't we also explicity set the Content-Type header to application/json?
There was a problem hiding this comment.
Not sure we need this:
you mean the redirect: manual - we need that, because the API is not returning 401, it just redirects to login.action, and if you don't specify manual then it returns 200 (from the login.action) - I added a link for the three possible modes for fetch (the other option could be to throw an error on redirect, but we can't keep the default)
it defaults to that application/json
There was a problem hiding this comment.
I meant the sentence following the colon. But if this defaults to application/json, then we're fine (although I don't see where the default is applied?)
src/api.js
Outdated
| if (res.status < 200 || res.status >= 300) { | ||
| throw new Error(res.statusText) | ||
| const errorBody = await res.json() |
There was a problem hiding this comment.
Is it guaranteed that we'll get a json response for non-200 responses? I'd expect 404 to be text/plain.
In that case this threw an error, the error message would be about not being able to parse json rather than the actual issue.
There was a problem hiding this comment.
so if we don't get JSON, then we're just back to where we started, this will throw an error that's handled by the consumer and then we just display an empty error message like before.
I am not happy with the solution in general as well, but this looked like the least intrusive one without doing a big refactor
…od more resilient
There was a problem hiding this comment.
Looks good 🚀 (I did the QA review)
When testing against my local instance, I didn't actually get this modified error when non-authenticated (I got Failed to install app: Failed to fetch), but I did get the Your session has expired error... when testing against debug.dhis2.org/dev
## [100.2.30](v100.2.29...v100.2.30) (2023-12-13) ### Bug Fixes * parse error message properly for manual installs ([#541](#541)) ([ea2c62f](ea2c62f))
|
🎉 This PR is included in version 100.2.30 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes DHIS2-13883 and DHIS2-15304.
res.json()to get their content. This fixes error handling there so that the error details are surfaced to the consumers.manualthen the response we get back doesn't have a 302 but a property we can check - more info here