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

Latest changes for SAW integration #333

Merged
merged 18 commits into from
Sep 21, 2016
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions apps/workers/suppliers/saw.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def perform
return
end

result = importer.fetch_available_properties_by_countries(countries)
result = importer.fetch_properties_by_countries(countries)

if result.success?
properties = result.value
Expand All @@ -36,15 +36,13 @@ def perform
if result.success?
all_unit_rates = result.value
else
message = "Failed to perform the `#fetch_rates_for_properties` operation"
message = "Failed to perform the `#fetch_all_unit_rates_for_properties` operation"
announce_error(message, result)
return
end

properties.each do |property|
synchronisation.start(property.internal_id) do
Concierge.context.disable!

unit_rates = find_rates(property.internal_id, all_unit_rates)
fetch_details_and_build_property(property, unit_rates)
end
Expand All @@ -53,7 +51,7 @@ def perform
synchronisation.finish!
end

def fetch_details_and_build_property(property, rates)
def fetch_details_and_build_property(property, unit_rates)
result = importer.fetch_detailed_property(property.internal_id)

if result.success?
Expand All @@ -62,7 +60,7 @@ def fetch_details_and_build_property(property, rates)
roomorama_property = ::SAW::Mappers::RoomoramaProperty.build(
property,
detailed_property,
rates
unit_rates
)

Result.new(roomorama_property)
Expand Down
3 changes: 2 additions & 1 deletion lib/concierge/suppliers/saw/commands/base_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ class BaseFetcher
#
# Whitelisted SAW API errors:
# 1007 - No properties are available for the given search parameters
VALID_RESULT_ERROR_CODES = ['1007']
# 3031 - Rates are not available for this property
VALID_RESULT_ERROR_CODES = ['1007', '3031']

attr_reader :credentials

Expand Down
6 changes: 3 additions & 3 deletions lib/concierge/suppliers/saw/commands/bulk_rates_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def build_rates(rates_hash)
Array(rates).map do |rate|
safe_hash = Concierge::SafeAccessHash.new(rate)
SAW::Mappers::PropertyRate.build(safe_hash)
end
end.compact
end

def build_payload(ids)
Expand All @@ -59,11 +59,11 @@ def build_payload(ids)
end

def check_in
(today + 30 * one_day).strftime("%d/%m/%Y")
(today + 90 * one_day).strftime("%d/%m/%Y")
end

def check_out
(today + 31 * one_day).strftime("%d/%m/%Y")
(today + 92 * one_day).strftime("%d/%m/%Y")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my ignorance, but previously we were quoting the price for 1 day (30 days from now to 31 days from now), and now we are quoting the price for 2 days (90 to 92 days from now). Won't that cause the returned price to account for 2 nights?

What am I missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YES 💥 Looks like I didn't changed the second part which uses these prices 💭 💭

Moreover, now I don't like what my PropertyRate mapper / entity do, I need to refactor them & fix this bug.

@keang please hold on with the merge

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

end

def one_day
Expand Down
12 changes: 6 additions & 6 deletions lib/concierge/suppliers/saw/commands/price_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,15 @@ def call(params)
result_hash = response_parser.to_hash(result.value.body)

if valid_result?(result_hash)
property_rate = build_property_rate(result_hash)
property_rate_res = build_property_rate(result_hash)

quotation = if property_rate.has_unit?(params[:unit_id])
SAW::Mappers::Quotation.build(params, property_rate)
if property_rate_res.success?
Result.new SAW::Mappers::Quotation.build(params, property_rate_res.value)
elsif property_rate_res.error.code == :empty_unit_rates
Result.new SAW::Mappers::Quotation.build_unavailable(params)
else
SAW::Mappers::Quotation.build_unavailable(params)
property_rate_res
end

Result.new(quotation)
else
error_result(result_hash)
end
Expand Down
39 changes: 1 addition & 38 deletions lib/concierge/suppliers/saw/importer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,6 @@ def fetch_properties_by_country(country)
properties_fetcher.call(country)
end

# Retrieves the list of available properties in a given country
# Available means those which doesn't have `on_request` attribute
#
# Returns a +Result+ wrapping +Array+ of +SAW::Entities::BasicProperty+
# when operation succeeds
# Returns a +Result+ with +Result::Error+ when operation fails
def fetch_available_properties_by_country(country)
result = fetch_properties_by_country(country)

if result.success?
all_properties = result.value
available_properties = all_properties.reject { |p| p.on_request? }
Result.new(available_properties)
else
result
end
end

# Retrieves the list of properties in given countries
# In case if request to one of the countries fails, method stops its
# execution and returns a result with a failure.
Expand All @@ -93,25 +75,6 @@ def fetch_properties_by_countries(countries)
Result.new(properties)
end

# Retrieves the list of available properties in given countries
# In case if request to one of the countries fails, method stops its
# execution and returns a result with a failure.
#
# Returns a +Result+ wrapping +Array+ of +SAW::Entities::BasicProperty+
# when operation succeeds
# Returns a +Result+ with +Result::Error+ when operation fails
def fetch_available_properties_by_countries(countries)
properties = countries.map do |country|
result = fetch_available_properties_by_country(country)

return result unless result.success?

result.value
end.flatten

Result.new(properties)
end

# Retrieves property with extended information.
#
# Returns a +Result+ wrapping +SAW::Entities::DetailedProperty+ object
Expand All @@ -132,7 +95,7 @@ def fetch_unit_rates_by_property_ids(ids)
bulk_rates_fetcher.call(ids)
end

# Retrieves rates for properties
# Retrieves rates for units of all given properties.
#
# It groups properties by currency because SAW API doesn't support
# fetching property rates when multiple currencies are given.
Expand Down
20 changes: 14 additions & 6 deletions lib/concierge/suppliers/saw/mappers/property_rate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ class << self
# * +hash+ [Concierge::SafeAccessHash] property rate object
# attributes
#
# Returns [SAW::Entities::PropertyRate]
# Returns [Result] wrapping [SAW::Entities::PropertyRate]
def build(hash)
Entities::PropertyRate.new(
return Result.error(:empty_unit_rates) if empty_unit_rates?(hash)

Result.new(Entities::PropertyRate.new(
id: hash.get("@id"),
units: build_units(hash),
currency: parse_currency(hash)
)
))
end

private
Expand All @@ -28,10 +30,16 @@ def parse_currency(hash)
hash.get("currency_code")
end

def empty_unit_rates?(hash)
units_hash(hash).nil?
end

def units_hash(hash)
hash.get("apartments.accommodation_type.property_accommodation")
end

def build_units(hash)
units = hash.get(
"apartments.accommodation_type.property_accommodation"
)
units = units_hash(hash)

Array(units).map do |unit_hash|
safe_hash = Concierge::SafeAccessHash.new(unit_hash)
Expand Down
24 changes: 14 additions & 10 deletions lib/concierge/suppliers/saw/mappers/quotation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,20 @@ class Quotation
def self.build(params, property_rate)
requested_unit = property_rate.find_unit(params[:unit_id])

::Quotation.new(
property_id: params[:property_id],
unit_id: params[:unit_id],
check_in: params[:check_in].to_s,
check_out: params[:check_out].to_s,
guests: params[:guests],
currency: property_rate.currency,
total: requested_unit.price,
available: requested_unit.available
)
if requested_unit
::Quotation.new(
property_id: params[:property_id],
unit_id: params[:unit_id],
check_in: params[:check_in].to_s,
check_out: params[:check_out].to_s,
guests: params[:guests],
currency: property_rate.currency,
total: requested_unit.price,
available: requested_unit.available
)
else
self.build_unavailable(params)
end
end

# Builds unavailable quotation.
Expand Down
1 change: 0 additions & 1 deletion lib/concierge/suppliers/saw/payload_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ def propertysearch_request(country:, property_id: nil)
#{build_username_and_password}
<countryid>#{country}</countryid>
<number_of_guests>-1</number_of_guests>
<flag_free_sale>Y</flag_free_sale>
#{property_id ? build_property_container(property_id) : nil }
</request>
}
Expand Down
12 changes: 10 additions & 2 deletions spec/api/controllers/saw/quote_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,17 @@ def provoke_failure!

result = controller.quote_price(params)

expect(result.success?).to be false
expect(result.success?).to be true
expect(result).to be_kind_of(Result)
expect(result.value).to be nil

quotation = result.value
expect(quotation).to be_kind_of(Quotation)
expect(quotation.property_id).to eq(params[:property_id])
expect(quotation.unit_id).to eq(params[:unit_id])
expect(quotation.check_in).to eq(params[:check_in])
expect(quotation.check_out).to eq(params[:check_out])
expect(quotation.guests).to eq(params[:guests])
expect(quotation.available).to be false
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
rates = result.value
expect(rates).to be_kind_of(Array)
expect(rates.size).to eq(2)
expect(rates).to all(be_kind_of(SAW::Entities::PropertyRate))
expect(rates).to all(be_kind_of(Result))
rates.each do |r|
expect(r).to be_success
end
end

it "returns rates for single property id" do
Expand All @@ -31,7 +34,22 @@
rates = result.value
expect(rates).to be_kind_of(Array)
expect(rates.size).to eq(1)
expect(rates).to all(be_kind_of(SAW::Entities::PropertyRate))
expect(rates).to all(be_kind_of(Result))
rates.each do |r|
expect(r).to be_success
end
end

it "returns a result with an empty array if all rates are unavailable" do
mock_request(:propertyrates, :rates_not_available)

result = subject.call(property_ids)
expect(result).to be_success

rates = result.value

expect(rates).to be_kind_of(Array)
expect(rates.size).to eq(0)
end

it "returns result with error after error" do
Expand Down
36 changes: 0 additions & 36 deletions spec/lib/concierge/suppliers/saw/importer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,42 +45,6 @@
end
end

describe "fetch_available_properties_by_country" do
it "returns full set if there wasn't any property with on request flag" do
mock_request(:country, :one)
mock_request(:propertysearch, :success)

countries_result = subject.fetch_countries
current_country = countries_result.value.first

properties_result = subject.fetch_available_properties_by_country(
current_country
)
expect(properties_result.success?).to be true
expect(properties_result.value.size).to eq(5)
expect(properties_result.value).to all(
be_kind_of(SAW::Entities::BasicProperty)
)
end

it "filters out properties with on request flag" do
mock_request(:country, :one)
mock_request(:propertysearch, :success_with_on_request)

countries_result = subject.fetch_countries
current_country = countries_result.value.first

properties_result = subject.fetch_available_properties_by_country(
current_country
)
expect(properties_result.success?).to be true
expect(properties_result.value.size).to eq(4)
expect(properties_result.value).to all(
be_kind_of(SAW::Entities::BasicProperty)
)
end
end

describe "fetch_properties_by_countries" do
it "returns a result with an empty array when all requests are empty" do
mock_request(:country, :multiple)
Expand Down
22 changes: 21 additions & 1 deletion spec/lib/concierge/suppliers/saw/mappers/property_rate_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ module SAW
let(:hash) { Concierge::SafeAccessHash.new(attributes) }

it "builds available property rate object" do
property_rate = described_class.build(hash)
property_rate = described_class.build(hash).value
expect(property_rate).to be_kind_of(SAW::Entities::PropertyRate)
expect(property_rate.id).to eq("2596")
expect(property_rate.currency).to eq("EUR")
Expand All @@ -76,5 +76,25 @@ module SAW
expect(unit.available).to eq(true)
expect(unit.max_guests).to eq(4)
end

describe "when rates are not available" do
let(:attributes) do
{
"name"=>"Adagio Access Avignon",
"check_in"=>"01/09/2016",
"check_out"=>"02/09/2016",
"currency_code"=>"EUR",
"default_currency_code"=>"EUR",
"apartments"=> {}
}
end

it "returns Result with error object" do
property_rate = described_class.build(hash)

expect(property_rate).to be_a Result
expect(property_rate.error.code).to eq :empty_unit_rates
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@
rates
)

expect(property.units).not_to eq([])
expect(property.units.size).to eq(6)
expect(property.units).to all(be_kind_of(Roomorama::Unit))
end
end
Expand Down