Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

OCLOMRS-958:Errors from the backend should marked the field that caused the error #700

Closed
wants to merge 6 commits into from
Closed

Conversation

jwnasambu
Copy link
Contributor

@jwnasambu jwnasambu marked this pull request as draft May 6, 2021 13:25
Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to shape up quite nicely I think. We still need to modify getPrettyError() here to replace errors["__all__"] with errors["__detail__"] and then test and ensure this works.

Comment on lines 105 to 108
let errorMsg = errorMsgResponse(response);

const errorMessage: string | undefined | {} | [] =
response?.data || response
? STATUS_CODES_TO_MESSAGES[response.status] || errorMsg
: errorMsg;
const errorMsgResponse: {[key: string]: string} = {
"__detail__": "Action could not be completed. Please retry."
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot about these bits. This should be replaced with something like:

let errorMsg = errorMsgResponse(response);

const errorMessage =
  response?.data || response
    ? STATUS_CODES_TO_MESSAGES[response.status] || errorMsg
    : errorMsg["__detail__"];

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright... that was a silly suggestion.

Instead, let's change errorMsgResponse() to look like this:

export function errorMsgResponse(response: AxiosResponse) {
  const errorMsgResponse: {[key: string]: string} = {
    "__detail__": "Action could not be completed. Please retry."
  };

  if (response.data && Object.prototype.hasOwnProperty.call(response.data, "detail")) { 
    errorMsgResponse["__detail__"] = response.data.detail;
  } else if (response.status in STATUS_CODES_TO_MESSAGES) {
    errorMsgResponse["__detail__"] = STATUS_CODES_TO_MESSAGES[response.status];
  }
  
  for (let key in response.data) {
    if (key === "__detail__") continue;
    errorMsgResponse[key] =
        Array.isArray(response.data[key])
          ? response.data[key].join(",")
          : response.data[key]
    } 
    
  return errorMsgResponse;
};

This also means hunting out the places where we call errorMsgResponse() and ensure that we cast the argument we're returning to an AxiosResponse (it should always be this), like so:

const response: AxiosResponse = error.response;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ibacher let me do it right a way

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwnasambu So the second part of this is to just eliminate anything that looks like this:

const errorMessage =
  response?.data || response
    ? STATUS_CODES_TO_MESSAGES[response.status] || errorMsg
    : errorMsg["__detail__"];

From the code base... actually anything that processes the errMsg after it's returned from errorMsgResponse(Response). The idea here is that that processing will be done on the actual page itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibacher thanks for the clear explanation though it seems I had eliminated that part already!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibacher am sorry I have just seen that part and other proposed changes. Kindly let me fix and push the changes right away.

@ibacher
Copy link
Member

ibacher commented May 17, 2021

Already, so, to resolve the test errors we're getting, we need to actually change the tests in src/redux/__test__/utils.test.ts to reflect the new reality (this is fine and an expected part of this work).

Basically, what we need to do is change this line from:

expect(errorMsgResponse(response)).toEqual(errorMsgs[error])

to:

expect(errorMsgResponse(response)["name"]).toEqual(errorMsgs[error])

Array.isArray(response.data[key])
? response.data[key].join(",")
: response.data[key]
);
return errorMsgResponse;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this before... this return errorMsgResponse should be outside of this loop, i.e., the last line of the function before the closing }.

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully those are the last little bits to get this building correctly.

Comment on lines 120 to 122
}
return errorMsgResponse;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
return errorMsgResponse;
};
}
return errorMsgResponse;
};

let errorMsg = errorMsgResponse(response);

const errorMessage: string | undefined | {} | [] =
const errorMessage =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const errorMessage =
const errorMessage: string | undefined =

@jwnasambu
Copy link
Contributor Author

@hadijahkyampeire Kindly open this PR we were still making some changes on it with Ian.

@hadijahkyampeire
Copy link
Collaborator

@jwnasambu I am sorry but I am unable to reopen it, its repository was deleted you will need to open a new PR.

@jwnasambu
Copy link
Contributor Author

@ibacher should I open a new PR or the PR can be re-opened again? Thanks for your help.

@ibacher
Copy link
Member

ibacher commented Jul 16, 2021

@jwnasambu If you have a local copy of your OCLOMRS-958 branch, you should be able to push it to GitHub. Right now, that branch doesn't exist on your GitHub repo, which is why this can't be reopened.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants