map-per/organicmaps
1
Fork 0
forked from organicmaps/organicmaps
Code Pull requests Activity

[booking] review fixes

Browse source
This commit is contained in:
Arsentiy Milchakov 2017-11-13 15:38:51 +03:00 • committed by Yuri Gorshenin
parent e7040c7b57
commit c011846c94
13 changed files with 134 additions and 85 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
generator/generator_tests_support/test_feature.cpp
View file

@ -40,11 +40,13 @@ uint64_t GenUniqueId()
TestFeature::TestFeature(string const & name, string const & lang)
: m_id(GenUniqueId()), m_center(0, 0), m_type(Type::Unknown), m_name(name), m_lang(lang)
{
Init();
}
TestFeature::TestFeature(m2::PointD const & center, string const & name, string const & lang)
: m_id(GenUniqueId()), m_center(center), m_type(Type::Point), m_name(name), m_lang(lang)
{
Init();
}
TestFeature::TestFeature(vector<m2::PointD> const & boundary, string const & name,
@ -52,6 +54,12 @@ TestFeature::TestFeature(vector<m2::PointD> const & boundary, string const & nam
: m_id(GenUniqueId()), m_boundary(boundary), m_type(Type::Area), m_name(name), m_lang(lang)
{
ASSERT(!m_boundary.empty(), ());
Init();
}
void TestFeature::Init()
{
m_metadata.Set(feature::Metadata::FMD_TEST_ID, strings::to_string(m_id));
}
bool TestFeature::Matches(FeatureType const & feature) const
@ -64,8 +72,18 @@ bool TestFeature::Matches(FeatureType const & feature) const
void TestFeature::Serialize(FeatureBuilder1 & fb) const
{
auto & metadata = fb.GetMetadataForTesting();
metadata.Set(feature::Metadata::FMD_TEST_ID, strings::to_string(m_id));
using feature::Metadata;
size_t i = static_cast<size_t>(Metadata::EType::FMD_CUISINE);
size_t const count = static_cast<size_t>(Metadata::EType::FMD_COUNT);
for (; i < count; ++i)
{
auto const type = static_cast<Metadata::EType>(i);
if (m_metadata.Has(type))
{
auto const value = m_metadata.Get(type);
fb.GetMetadataForTesting().Set(type, value);
}
}
switch (m_type)
{
@ -231,8 +249,6 @@ void TestPOI::Serialize(FeatureBuilder1 & fb) const
fb.AddHouseNumber(m_houseNumber);
if (!m_streetName.empty())
fb.AddStreet(m_streetName);
if (!m_metadata.Empty())
fb.GetMetadataForTesting() = m_metadata;
}
string TestPOI::ToString() const

7
generator/generator_tests_support/test_feature.hpp
View file

@ -32,6 +32,7 @@ public:
inline void SetPostcode(std::string const & postcode) { m_postcode = postcode; }
inline uint64_t GetId() const { return m_id; }
inline std::string const & GetName() const { return m_name; }
inline feature::Metadata & GetMetadata() { return m_metadata; }
virtual void Serialize(FeatureBuilder1 & fb) const;
virtual std::string ToString() const = 0;
@ -56,6 +57,10 @@ protected:
std::string const m_name;
std::string const m_lang;
std::string m_postcode;
feature::Metadata m_metadata;
private:
void Init();
};
class TestCountry : public TestFeature
@ -125,13 +130,11 @@ public:
inline void SetHouseNumber(std::string const & houseNumber) { m_houseNumber = houseNumber; }
inline void SetStreet(TestStreet const & street) { m_streetName = street.GetName(); }
inline void SetTypes(std::vector<std::vector<std::string>> const & types) { m_types = types; }
inline void SetMetadata(feature::Metadata const & metadata) { m_metadata = metadata; }
private:
std::string m_houseNumber;
std::string m_streetName;
std::vector<std::vector<std::string>> m_types;
feature::Metadata m_metadata;
};
class TestBuilding : public TestFeature

2
indexer/indexer_tests_support/test_mwm_environment.hpp
View file

@ -80,4 +80,4 @@ private:
MwmList m_mwmFiles;
};
} // namespace tests_support
} // namespace indexer
} // namespace indexer

103
map/booking_filter.cpp
View file

@ -16,23 +16,21 @@ namespace
{
struct HotelToResult
{
HotelToResult(std::string const & id, search::Result const & result,
availability::Cache::HotelStatus status)
: m_hotelId(id), m_result(result), m_cacheStatus(status)
{
}
HotelToResult(search::Result const & result) : m_result(result) {}
std::string m_hotelId;
search::Result m_result;
std::string m_hotelId;
availability::Cache::HotelStatus m_cacheStatus;
};
using HotelToResults = std::vector<HotelToResult>;
void FillAvailability(HotelToResult & hotelToResult, std::vector<std::string> const & hotelIds,
availability::Cache & cache, search::Results & results)
{
using availability::Cache;
if (hotelToResult.m_cacheStatus == Cache::HotelStatus::UnAvailable)
if (hotelToResult.m_cacheStatus == Cache::HotelStatus::Unavailable)
return;
if (hotelToResult.m_cacheStatus == Cache::HotelStatus::Available)
@ -62,10 +60,69 @@ void FillAvailability(HotelToResult & hotelToResult, std::vector<std::string> co
}
else
{
cache.Insert(hotelToResult.m_hotelId, Cache::HotelStatus::UnAvailable);
cache.Insert(hotelToResult.m_hotelId, Cache::HotelStatus::Unavailable);
}
}
}
void PrepareData(Index const & index, search::Results const & results,
HotelToResults & hotelToResults, availability::Cache & cache,
booking::AvailabilityParams & p)
{
std::vector<FeatureID> features;
for (auto const & r : results)
{
if (!r.m_metadata.m_isSponsoredHotel ||
r.GetResultType() != search::Result::ResultType::RESULT_FEATURE)
{
continue;
}
features.push_back(r.GetFeatureID());
hotelToResults.emplace_back(r);
}
std::sort(features.begin(), features.end());
MwmSet::MwmId currentMwmId;
std::unique_ptr<Index::FeaturesLoaderGuard> guard;
for (auto const & featureId : features)
{
if (currentMwmId != featureId.m_mwmId)
{
guard = my::make_unique<Index::FeaturesLoaderGuard>(index, featureId.m_mwmId);
currentMwmId = featureId.m_mwmId;
}
auto it = std::find_if(hotelToResults.begin(), hotelToResults.end(),
[&featureId](HotelToResult const & item)
{
return item.m_result.GetFeatureID() == featureId;
});
ASSERT(it != hotelToResults.cend(), ());
FeatureType ft;
if (!guard->GetFeatureByIndex(featureId.m_index, ft))
{
hotelToResults.erase(it);
LOG(LERROR, ("Feature can't be loaded:", featureId));
continue;
}
std::string hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID);
auto const status = cache.Get(hotelId);
it->m_hotelId = hotelId;
it->m_cacheStatus = status;
if (status != availability::Cache::HotelStatus::Absent)
continue;
cache.Reserve(hotelId);
p.m_hotelIds.push_back(std::move(hotelId));
}
}
} // namespace
namespace booking
@ -75,40 +132,16 @@ namespace filter
Filter::Filter(Index const & index, booking::Api const & api) : m_index(index), m_api(api) {}
void Filter::Availability(search::Results const & results,
availability::internal::Params const & params) const
availability::internal::Params && params) const
{
auto & p = params.m_params;
auto const & cb = params.m_callback;
ASSERT(p.m_hotelIds.empty(), ());
std::vector<HotelToResult> hotelToResults;
// Fill hotel ids.
for (auto const & r : results)
{
if (!r.m_metadata.m_isSponsoredHotel ||
r.GetResultType() != search::Result::ResultType::RESULT_FEATURE)
continue;
HotelToResults hotelToResults;
auto const & id = r.GetFeatureID();
Index::FeaturesLoaderGuard const guard(m_index, id.m_mwmId);
FeatureType ft;
if (!guard.GetFeatureByIndex(id.m_index, ft))
{
LOG(LERROR, ("Feature can't be loaded:", id));
continue;
}
std::string hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID);
auto const status = m_availabilityCache.Get(hotelId);
hotelToResults.emplace_back(hotelId, r, status);
if (status != availability::Cache::HotelStatus::Absent)
continue;
m_availabilityCache.Reserve(hotelId);
p.m_hotelIds.push_back(std::move(hotelId));
}
PrepareData(m_index, results, hotelToResults, m_availabilityCache, p);
auto const apiCallback = [this, cb, hotelToResults](std::vector<std::string> hotelIds) mutable
{

7
map/booking_filter.hpp
View file

@ -3,13 +3,13 @@
#include "map/booking_filter_availability_params.hpp"
#include "map/booking_filter_cache.hpp"
class Index;
namespace search
{
class Results;
}
class Index;
namespace booking
{
class Api;
@ -20,8 +20,9 @@ class Filter
{
public:
Filter(Index const & index, booking::Api const & api);
void Availability(search::Results const & results,
availability::internal::Params const & params) const;
availability::internal::Params && params) const;
private:
Index const & m_index;

7
map/booking_filter_cache.cpp
View file

@ -29,8 +29,7 @@ void Cache::Reserve(std::string const & hotelId)
{
std::lock_guard<std::mutex> lock(m_mutex);
Item item;
m_hotelToResult.emplace(hotelId, std::move(item));
m_hotelToResult.emplace(hotelId, HotelStatus::NotReady);
}
void Cache::Insert(std::string const & hotelId, HotelStatus const s)
@ -43,9 +42,7 @@ void Cache::Insert(std::string const & hotelId, HotelStatus const s)
m_hotelToResult[hotelId] = std::move(item);
if (!m_agingInProgress)
{
RemoveOutdated();
}
}
void Cache::RemoveOutdated()
@ -95,7 +92,7 @@ std::string DebugPrint(Cache::HotelStatus status)
{
case Cache::HotelStatus::Absent: return "Absent";
case Cache::HotelStatus::NotReady: return "NotReady";
case Cache::HotelStatus::UnAvailable: return "UnAvailable";
case Cache::HotelStatus::Unavailable: return "Unavailable";
case Cache::HotelStatus::Available: return "Available";
}
}

10
map/booking_filter_cache.hpp
View file

@ -17,9 +17,13 @@ class Cache
public:
enum class HotelStatus
{
// The hotel is absent in cache.
Absent,
// Information about the hotel was requested, but request is not ready yet.
NotReady,
UnAvailable,
// The hotel is unavailable for booking.
Unavailable,
// The hotel is available for booking.
Available,
};
@ -40,8 +44,6 @@ public:
void Reserve(std::string const & hotelId);
void Insert(std::string const & hotelId, HotelStatus const s);
DISALLOW_COPY_AND_MOVE(Cache);
private:
void RemoveOutdated();
void RemoveExtra();
@ -54,6 +56,8 @@ private:
size_t const m_maxCount = 1000;
// Aging process is disabled when |m_expiryPeriodSeconds| is equal to zero.
size_t const m_expiryPeriodSeconds = 60;
DISALLOW_COPY_AND_MOVE(Cache);
};
std::string DebugPrint(Cache::HotelStatus status);

8
map/framework.cpp
View file

@ -3306,13 +3306,13 @@ ugc::Reviews Framework::FilterUGCReviews(ugc::Reviews const & reviews) const
return result;
}
void Framework::FilterSearchResults(booking::filter::availability::Params const & params,
search::Results const & results, bool inViewport)
void Framework::FilterSearchResultsOnBooking(booking::filter::availability::Params const & params,
search::Results const & results, bool inViewport)
{
using namespace booking::filter;
auto const & cb = params.m_callback;
availability::internal::Params const paramsInternal
availability::internal::Params paramsInternal
{
params.m_params,
[this, cb, inViewport](search::Results const & results)
@ -3340,5 +3340,5 @@ void Framework::FilterSearchResults(booking::filter::availability::Params const
}
};
m_bookingFilter.Availability(results, paramsInternal);
m_bookingFilter.Availability(results, std::move(paramsInternal));
}

4
map/framework.hpp
View file

@ -825,6 +825,6 @@ private:
ugc::Reviews FilterUGCReviews(ugc::Reviews const & reviews) const;
public:
void FilterSearchResults(booking::filter::availability::Params const & params,
search::Results const & results, bool inViewport) override;
void FilterSearchResultsOnBooking(booking::filter::availability::Params const & params,
search::Results const & results, bool inViewport) override;
};

10
map/map_tests/booking_availability_cache_test.cpp
View file

@ -12,7 +12,7 @@ namespace
{
UNIT_TEST(AvailabilityCache_Smoke)
{
Cache cache(2, 0);
Cache cache(2 /* maxCount */, 0 /* expiryPeriodSeconds */);
std::string kHotelId = "0";
@ -26,14 +26,14 @@ UNIT_TEST(AvailabilityCache_Smoke)
TEST_EQUAL(cache.Get(kHotelId), Cache::HotelStatus::Available, ());
cache.Insert(kHotelId, Cache::HotelStatus::UnAvailable);
cache.Insert(kHotelId, Cache::HotelStatus::Unavailable);
TEST_EQUAL(cache.Get(kHotelId), Cache::HotelStatus::UnAvailable, ());
TEST_EQUAL(cache.Get(kHotelId), Cache::HotelStatus::Unavailable, ());
}
UNIT_TEST(AvailabilityCache_RemoveExtra)
{
Cache cache(3, 0);
Cache cache(3 /* maxCount */, 0 /* expiryPeriodSeconds */);
std::vector<std::string> const kHotelIds = {"1", "2", "3"};
for (auto const & id : kHotelIds)
@ -52,4 +52,4 @@ UNIT_TEST(AvailabilityCache_RemoveExtra)
TEST_EQUAL(cache.Get("4"), Cache::HotelStatus::Available, ());
}
} // namespace
} // namespace

29
map/map_tests/booking_filter_test.cpp
View file

@ -33,39 +33,34 @@ UNIT_TEST(BookingFilter_AvailabilitySmoke)
env.BuildMwm("TestMwm", [&kHotelIds](TestMwmBuilder & builder)
{
feature::Metadata metadata;
metadata.Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[0]);
TestPOI hotel1(m2::PointD(1.0, 1.0), "hotel 1", "en");
hotel1.SetMetadata(metadata);
hotel1.GetMetadata().Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[0]);
builder.Add(hotel1);
TestPOI hotel2(m2::PointD(1.1, 1.1), "hotel 2", "en");
metadata.Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[1]);
hotel2.SetMetadata(metadata);
hotel2.GetMetadata().Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[1]);
builder.Add(hotel2);
TestPOI hotel3(m2::PointD(0.9, 0.9), "hotel 3", "en");
metadata.Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[2]);
hotel3.SetMetadata(metadata);
hotel3.GetMetadata().Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[2]);
builder.Add(hotel3);
});
m2::RectD const rect = MercatorBounds::RectByCenterXYAndSizeInMeters(
m2::PointD(1.0, 1.0), 2 / MercatorBounds::degreeInMetres /* rect width */);
m2::RectD const rect(m2::PointD(0.5, 0.5), m2::PointD(1.5, 1.5));
search::Results results;
results.AddResult({"suggest for testing", "suggest for testing"});
search::Results expctedResults;
search::Results expectedResults;
env.GetIndex().ForEachInRect(
[&results, &expctedResults](FeatureType & ft) {
[&results, &expectedResults](FeatureType & ft) {
search::Result::Metadata metadata;
metadata.m_isSponsoredHotel = true;
search::Result result(ft.GetID(), ft.GetCenter(), "", "", "", 0, metadata);
auto copy = result;
results.AddResult(std::move(result));
expctedResults.AddResult(std::move(copy));
expectedResults.AddResult(std::move(copy));
},
rect, scales::GetUpperScale());
availability::ParamsInternal params;
availability::internal::Params params;
search::Results filteredResults;
params.m_callback = [&filteredResults](search::Results const & results)
{
@ -73,16 +68,16 @@ UNIT_TEST(BookingFilter_AvailabilitySmoke)
testing::Notify();
};
filter.Availability(results, params);
filter.Availability(results, std::move(params));
testing::Wait();
TEST_NOT_EQUAL(filteredResults.GetCount(), 0, ());
TEST_EQUAL(filteredResults.GetCount(), expctedResults.GetCount(), ());
TEST_EQUAL(filteredResults.GetCount(), expectedResults.GetCount(), ());
for (size_t i = 0; i < filteredResults.GetCount(); ++i)
{
TEST_EQUAL(filteredResults[i].GetFeatureID(), expctedResults[i].GetFeatureID(), ());
TEST_EQUAL(filteredResults[i].GetFeatureID(), expectedResults[i].GetFeatureID(), ());
}
}
} // namespace
} // namespace

4
map/search_api.cpp
View file

@ -99,7 +99,7 @@ bool SearchAPI::SearchEverywhere(EverywhereSearchParams const & params)
if (results.IsEndMarker() && results.IsEndedNormal() &&
!params.m_bookingFilterParams.IsEmpty())
{
m_delegate.FilterSearchResults(params.m_bookingFilterParams, results, false);
m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, false);
}
});
@ -137,7 +137,7 @@ bool SearchAPI::SearchInViewport(ViewportSearchParams const & params)
if (results.IsEndMarker() && results.IsEndedNormal() &&
!params.m_bookingFilterParams.IsEmpty())
{
m_delegate.FilterSearchResults(params.m_bookingFilterParams, results, true);
m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, true);
}
});

4
map/search_api.hpp
View file

@ -69,8 +69,8 @@ public:
virtual bool IsLocalAdsCustomer(search::Result const & /* result */) const { return false; }
virtual void FilterSearchResults(booking::filter::availability::Params const & params,
search::Results const & results, bool inViewport)
virtual void FilterSearchResultsOnBooking(booking::filter::availability::Params const & params,
search::Results const & results, bool inViewport)
{
}
};