Skip to content

Conversation

@dennispaagman
Copy link
Contributor

@dennispaagman dennispaagman commented Sep 22, 2016

We sometimes send borked data to Help Scout, leading to validation errors on their side. They return these nicely, unfortunately the gem currently assumes success (such an optimist) and gives a non descriptive:

"undefined method `match' for nil:NilClass"

Because of a missing location header. This adds a check on the status code of the response and raises a HelpScout::ValidationError exception with the actual error message returned from HS.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also define this inside the HelpScout class, I think I've seen it be done like that more often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thought that was only possible inside a Module, but seems to be working 👍

Copy link
Contributor

@bittersweet bittersweet left a comment

Choose a reason for hiding this comment

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

Looks good, great improvement. There are some stylistic comments you might want to take a look at implementing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice for readability to extract these status codes into constants, I always forget what they are, or at least I have to think a bit for non 200/403/404 etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about raising a catch all type thing here at the end if the response is not one of 201 or 400? I'm not super sure what kind of statuses they return at the moment though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we normally do raise ErrorClass.new(message) but fine either way.

@dennispaagman dennispaagman force-pushed the raise_error_on_validation_fail branch from 331fb38 to ce6c17a Compare September 22, 2016 10:18
@bittersweet
Copy link
Contributor

🚢

@dennispaagman dennispaagman merged commit cc3386f into master Sep 22, 2016
@dennispaagman dennispaagman deleted the raise_error_on_validation_fail branch September 22, 2016 11:50
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.

3 participants