Fix missing check for empty URLs

Missing check for empty URLs vector is added and
some code beautifying.

11 files changed, 77 insertions, 40 deletions

diff --git a/src/components/application_manager/include/application_manager/policies/policy_handler.h b/src/components/application_manager/include/application_manager/policies/policy_handler.h
index 31b3487a6f..9d8c225362 100644
--- a/src/components/application_manager/include/application_manager/policies/policy_handler.h
+++ b/src/components/application_manager/include/application_manager/policies/policy_handler.h
@@ -573,11 +573,13 @@ class PolicyHandler : public PolicyHandlerInterface,
   /**
    * @brief Checks if the application with the given policy
    * application id is registered or it is default id
-   * @param policy_app_id Application policy id
-   * @return TRUE if the application with given id is registered or
-   * it is default id, otherwise FALSE
+   * @param app_idx Application idx from EndpointUrls vector
+   * @param urls EndpointUrls vector
+   * @return TRUE if the vector with URLs with given idx is not empty
+   * and is related to a registered application or these are default URLs,
+   * otherwise FALSE
    */
-  bool IsUrlAppIdValid(const std::string& policy_app_id) const;
+  bool IsUrlAppIdValid(const int app_idx, const EndpointUrls& urls) const;
 
   DISALLOW_COPY_AND_ASSIGN(PolicyHandler);
 };
diff --git a/src/components/application_manager/src/policies/policy_handler.cc b/src/components/application_manager/src/policies/policy_handler.cc
index e315643666..29c38fb213 100644
--- a/src/components/application_manager/src/policies/policy_handler.cc
+++ b/src/components/application_manager/src/policies/policy_handler.cc
@@ -1367,10 +1367,11 @@ void PolicyHandler::OnSnapshotCreated(const BinaryMessage& pt_string) {
   }
 
   AppIdURL app_url = policy_manager_->GetNextUpdateUrl(urls);
-  while (!IsUrlAppIdValid(app_url.first)) {
+  while (!IsUrlAppIdValid(app_url.first, urls)) {
     app_url = policy_manager_->GetNextUpdateUrl(urls);
   }
-  SendMessageToSDK(pt_string, app_url.second);
+  const std::string& url = urls[app_url.first].url[app_url.second];
+  SendMessageToSDK(pt_string, url);
 #endif  // PROPRIETARY_MODE
   // reset update required false
   OnUpdateRequestSentToMobile();
@@ -1798,13 +1799,17 @@ void PolicyHandler::Add(const std::string& app_id,
   policy_manager_->Add(app_id, type, timespan_seconds);
 }
 
-bool PolicyHandler::IsUrlAppIdValid(const std::string& policy_app_id) const {
-  ApplicationSharedPtr app =
-      application_manager_.application_by_policy_id(policy_app_id);
+bool PolicyHandler::IsUrlAppIdValid(const int app_idx,
+                                    const EndpointUrls& urls) const {
+  const EndpointData& app_data = urls[app_idx];
+  const std::vector<std::string> app_urls = app_data.url;
+  const ApplicationSharedPtr app =
+      application_manager_.application_by_policy_id(app_data.app_id);
 
   const bool is_registered = (app && (app->IsRegistered()));
-  const bool is_default = (policy_app_id == policy::kDefaultId);
+  const bool is_default = (app_data.app_id == policy::kDefaultId);
+  const bool is_empty_urls = app_urls.empty();
 
-  return (is_registered || is_default);
+  return ((is_registered && !is_empty_urls) || is_default);
 }
 }  //  namespace policy
diff --git a/src/components/application_manager/test/policy_handler_test.cc b/src/components/application_manager/test/policy_handler_test.cc
index 93fc17a330..b746f5f7a3 100644
--- a/src/components/application_manager/test/policy_handler_test.cc
+++ b/src/components/application_manager/test/policy_handler_test.cc
@@ -1353,7 +1353,7 @@ TEST_F(PolicyHandlerTest, OnSnapshotCreated_UrlAdded) {
   EndpointUrls test_data;
   EndpointData data(kDummyData);
   test_data.push_back(data);
-  AppIdURL next_app_url = std::make_pair(kDefaultId, kDummyData);
+  AppIdURL next_app_url = std::make_pair(0, 0);
   ApplicationSharedPtr mock_app;
 
 #ifdef PROPRIETARY_MODE
diff --git a/src/components/include/policy/policy_external/policy/policy_manager.h b/src/components/include/policy/policy_external/policy/policy_manager.h
index 5fd25d2461..6928133775 100644
--- a/src/components/include/policy/policy_external/policy/policy_manager.h
+++ b/src/components/include/policy/policy_external/policy/policy_manager.h
@@ -484,7 +484,8 @@ class PolicyManager : public usage_statistics::StatisticsManager {
   /**
    * @brief Finds the next URL that must be sent on OnSystemRequest retry
    * @param urls vector of vectors that contain urls for each application
-   * @return Pair of policy application id and application url id
+   * @return Pair of policy application id and application url id from the
+   * urls vector
    */
   virtual AppIdURL GetNextUpdateUrl(const EndpointUrls& urls) = 0;
  protected:
diff --git a/src/components/include/policy/policy_regular/policy/policy_types.h b/src/components/include/policy/policy_regular/policy/policy_types.h
index b13e6bab87..3405cf30d6 100644
--- a/src/components/include/policy/policy_regular/policy/policy_types.h
+++ b/src/components/include/policy/policy_regular/policy/policy_types.h
@@ -313,6 +313,11 @@ struct MetaInfo {
   std::string language;
 };
 
+/**
+ * @brief The index of the application and the index of its URL
+ * from the Endpoints vector that will be sent on the next
+ * OnSystemRequest retry sequence
+ */
 struct RetrySequenceURL {
   int app_idx_;
   int url_idx_;
@@ -323,7 +328,7 @@ struct RetrySequenceURL {
   }
 };
 
-typedef std::pair<std::string, std::string> AppIdURL;
+typedef std::pair<int, int> AppIdURL;
 
 }  //  namespace policy
 
diff --git a/src/components/include/test/policy/policy_external/policy/mock_policy_manager.h b/src/components/include/test/policy/policy_external/policy/mock_policy_manager.h
index 8c5a51a55d..30c1496495 100644
--- a/src/components/include/test/policy/policy_external/policy/mock_policy_manager.h
+++ b/src/components/include/test/policy/policy_external/policy/mock_policy_manager.h
@@ -183,6 +183,9 @@ class MockPolicyManager : public PolicyManager {
   MOCK_CONST_METHOD0(get_settings, const PolicySettings&());
   MOCK_METHOD1(set_settings, void(const PolicySettings* get_settings));
   MOCK_METHOD1(GetNextUpdateUrl, AppIdURL(const EndpointUrls& urls));
+  MOCK_CONST_METHOD2(RetrySequenceUrl,
+                     AppIdURL(const struct RetrySequenceURL&,
+                              const EndpointUrls& urls));
 };
 }  // namespace policy_manager_test
 }  // namespace components
diff --git a/src/components/include/test/policy/policy_regular/policy/mock_policy_manager.h b/src/components/include/test/policy/policy_regular/policy/mock_policy_manager.h
index c6c02fac55..16e647dfba 100644
--- a/src/components/include/test/policy/policy_regular/policy/mock_policy_manager.h
+++ b/src/components/include/test/policy/policy_regular/policy/mock_policy_manager.h
@@ -185,6 +185,9 @@ class MockPolicyManager : public PolicyManager {
   MOCK_METHOD1(set_settings, void(const PolicySettings* get_settings));
   MOCK_CONST_METHOD0(GetLockScreenIconUrl, std::string());
   MOCK_METHOD1(GetNextUpdateUrl, AppIdURL(const EndpointUrls& urls));
+  MOCK_CONST_METHOD2(RetrySequenceUrl,
+                     AppIdURL(const struct RetrySequenceURL&,
+                              const EndpointUrls& urls));
 };
 
 }  // namespace policy_manager_test
diff --git a/src/components/policy/policy_external/include/policy/policy_types.h b/src/components/policy/policy_external/include/policy/policy_types.h
index 2ba2800778..94898b2556 100644
--- a/src/components/policy/policy_external/include/policy/policy_types.h
+++ b/src/components/policy/policy_external/include/policy/policy_types.h
@@ -364,6 +364,11 @@ struct MetaInfo {
   std::string language;
 };
 
+/**
+ * @brief The index of the application and the index of its URL
+ * from the Endpoints vector that will be sent on the next
+ * OnSystemRequest retry sequence
+ */
 struct RetrySequenceURL {
   int app_idx_;
   int url_idx_;
@@ -374,7 +379,7 @@ struct RetrySequenceURL {
   }
 }
 
-typedef std::pair<std::string, std::string> AppIdURL;
+typedef std::pair<int, int> AppIdURL;
 
 }  //  namespace policy
 
diff --git a/src/components/policy/policy_external/src/policy_manager_impl.cc b/src/components/policy/policy_external/src/policy_manager_impl.cc
index 76b35945d7..3e1b28f5be 100644
--- a/src/components/policy/policy_external/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_external/src/policy_manager_impl.cc
@@ -1195,21 +1195,26 @@ void PolicyManagerImpl::SetDecryptedCertificate(
 AppIdURL PolicyManagerImpl::GetNextUpdateUrl(const EndpointUrls& urls) {
   LOG4CXX_AUTO_TRACE(logger_);
 
-  uint32_t current_url_idx = retry_sequence_url_.url_idx_;
-  uint32_t current_app_idx = retry_sequence_url_.app_idx_;
+  const AppIdURL next_app_url = RetrySequenceUrl(retry_sequence_url_, urls);
 
-  if (current_url_idx >= urls[current_app_idx].url.size()) {
-    current_url_idx = 0;
-    if (++current_app_idx >= urls.size()) {
-      current_app_idx = 0;
+  retry_sequence_url_.url_idx_ = next_app_url.second + 1;
+  retry_sequence_url_.app_idx_ = next_app_url.first;
+
+  return next_app_url;
+}
+
+AppIdURL PolicyManagerImpl::RetrySequenceUrl(const struct RetrySequenceURL& rs,
+                                             const EndpointUrls& urls) const {
+  uint32_t url_idx = rs.url_idx_;
+  uint32_t app_idx = rs.app_idx_;
+
+  if (url_idx >= urls[app_idx].url.size()) {
+    url_idx = 0;
+    if (++app_idx >= urls.size()) {
+      app_idx = 0;
     }
   }
-
-  const std::string app = urls[current_app_idx].app_id;
-  const std::string url = urls[current_app_idx].url[current_url_idx];
-  const AppIdURL next_app_url = std::make_pair(app, url);
-  retry_sequence_url_.url_idx_ = current_url_idx + 1;
-  retry_sequence_url_.app_idx_ = current_app_idx;
+  const AppIdURL next_app_url = std::make_pair(app_idx, url_idx);
 
   return next_app_url;
 }
diff --git a/src/components/policy/policy_regular/include/policy/policy_manager_impl.h b/src/components/policy/policy_regular/include/policy/policy_manager_impl.h
index e25140327c..f7946e84a0 100644
--- a/src/components/policy/policy_regular/include/policy/policy_manager_impl.h
+++ b/src/components/policy/policy_regular/include/policy/policy_manager_impl.h
@@ -213,7 +213,10 @@ class PolicyManagerImpl : public PolicyManager {
 
   const PolicySettings& get_settings() const OVERRIDE;
 
-  virtual AppIdURL GetNextUpdateUrl(const EndpointUrls& urls) OVERRIDE;
+  AppIdURL GetNextUpdateUrl(const EndpointUrls& urls) OVERRIDE;
+
+  AppIdURL RetrySequenceUrl(const struct RetrySequenceURL& rs,
+                            const EndpointUrls& urls) const OVERRIDE;
 
  private:
   void CheckTriggers();
diff --git a/src/components/policy/policy_regular/src/policy_manager_impl.cc b/src/components/policy/policy_regular/src/policy_manager_impl.cc
index 790cc3b269..2dc768dda0 100644
--- a/src/components/policy/policy_regular/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_regular/src/policy_manager_impl.cc
@@ -903,21 +903,26 @@ std::string PolicyManagerImpl::RetrieveCertificate() const {
 AppIdURL PolicyManagerImpl::GetNextUpdateUrl(const EndpointUrls& urls) {
   LOG4CXX_AUTO_TRACE(logger_);
 
-  uint32_t current_url_idx = retry_sequence_url_.url_idx_;
-  uint32_t current_app_idx = retry_sequence_url_.app_idx_;
+  const AppIdURL next_app_url = RetrySequenceUrl(retry_sequence_url_, urls);
 
-  if (current_url_idx >= urls[current_app_idx].url.size()) {
-    current_url_idx = 0;
-    if (++current_app_idx >= urls.size()) {
-      current_app_idx = 0;
+  retry_sequence_url_.url_idx_ = next_app_url.second + 1;
+  retry_sequence_url_.app_idx_ = next_app_url.first;
+
+  return next_app_url;
+}
+
+AppIdURL PolicyManagerImpl::RetrySequenceUrl(const struct RetrySequenceURL& rs,
+                                             const EndpointUrls& urls) const {
+  uint32_t url_idx = rs.url_idx_;
+  uint32_t app_idx = rs.app_idx_;
+
+  if (url_idx >= urls[app_idx].url.size()) {
+    url_idx = 0;
+    if (++app_idx >= urls.size()) {
+      app_idx = 0;
     }
   }
-
-  const std::string app = urls[current_app_idx].app_id;
-  const std::string url = urls[current_app_idx].url[current_url_idx];
-  const AppIdURL next_app_url = std::make_pair(app, url);
-  retry_sequence_url_.url_idx_ = current_url_idx + 1;
-  retry_sequence_url_.app_idx_ = current_app_idx;
+  const AppIdURL next_app_url = std::make_pair(app_idx, url_idx);
 
   return next_app_url;
 }