Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Deserialize error objects with different value types other than string/ #88

Closed
wants to merge 1 commit into from

Conversation

mikevmil
Copy link
Contributor

The current code assumes that the error property always is a string, but the error object returned by Moneybird can have different value types. In order to get at least some useful information within the exception, we need to check the value type and then try to extract the information out if it.

Currently, it would only correctly extract the error response:

{
  "error": "Contact is required",
  "symbolic": {
    "contact": "required"
  }
}

But other error responses such as the following:

{
  "error": {
    "delivery_method": [
      "The sender address must contain an email address"
    ]
  }
}

and

{
  "error": {
    "firstname": [
      "is required"
    ],
    "lastname": [
      "is required"
    ],
    "company_name": [
      "is required"
    ]
  }
}

will not be extracted correctly.

This change will make sure that these messages will be shown.

@VincentVrijburg
Copy link
Owner

Thanks for your contribution! Would you be able to add unit tests to this? I don't think we test the HttpRequesterBase right now but maybe you could make a start or move this piece of code to an extension class/method so we can unit test the logic which retrieves the error code.

Let me know if you're able to do it.

@mikevmil
Copy link
Contributor Author

Unfortunately, I am currently unable to write this, but if existing unit tests have already been created or are being created, I am willing to expand on them.

@VincentVrijburg
Copy link
Owner

I have used your suggestion to create a pr with error deserialization improvements as well as unit tests: #108
Will be included in the next minor release.

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

Successfully merging this pull request may close these issues.

2 participants