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

Add forgotten quote errors to success response list #556

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

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.

3 participants