Skip to content

Conversation

@kkolotyuk
Copy link
Contributor

No description provided.

@sharipov-ru
Copy link
Contributor

sharipov-ru commented Nov 24, 2016

Originally, we started considering :check_in_too_near, :check_in_too_far, :stay_too_short error codes as whitelisted because we can't handle them on the roomorama side without significant efforts like changing calendar + api to correctly prevent users from getting these errors. (we are not supporting behaviours like these). That's why we want just to show a message to user and to consider these errors as "known" errors which we shouldn't care about.

Having said that, in my point :property_not_instant_bookable and :max_guests_exceeded are bit different: if we got these errors, then something really went wrong: we either show properties which we shouldn't show or we have wrong property's max_guests data. So here, it's better to be posted about errors instead of whitelisting them.

Am I missing something here or maybe you already had discussion about that?

@keang @kkolotyuk

@kkolotyuk
Copy link
Contributor Author

@sharipov-ru I agree with you about :property_not_instant_bookable, but as well as Roomorama has guests number field in quote form:
guests_field

it sounds reasonable to show :max_guests_exceeded error message to user. Right?

@kkolotyuk
Copy link
Contributor Author

Ahhh Guests is not simple input but select. Then may be you are right. Actually I don't see anything bad in these messages for users. We will see them in concierge, so let's wait @keang thoughts.

@sharipov-ru
Copy link
Contributor

sharipov-ru commented Nov 24, 2016

Hm, yes, from the user's perspective, it's an improvement: he is starting to receive more descriptive error message instead of general one.

But we still should be notified about these cases, since I was not in the loop I don't remember whether we still creating errors in rollbar for those which are whitelisted, do you remember that?

@keang
Copy link
Contributor

keang commented Nov 24, 2016

Yup we do create external-errors still: https://rollbar.com/Roomorama/Concierge/items/499/
I agree with this PR, it's just that we need to patch the response wrapping hack for roomorama in apps/api/middlewares/roomorama_webhook.rb:

# apps/api/middlewares/roomorama_webhook.rb:188
          if response["status"] == "ok" && !response["available"]
            payload["inquiry"]["errors"] = { "base" => "room unavailable" }
          end

We need to add it to payload["inquiry"]["errors"]["base"], and patch Roomorama to expect 200 response with these additional ["errors"]["base"].. probably can delay to next week since there're more pressing things to finish..

@kkolotyuk
Copy link
Contributor Author

I'm sure we call announce_error for this cases. Don't remember about rollbar.

@kkolotyuk
Copy link
Contributor Author

@keang please manage this PR further

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.

4 participants