Merge pull request #1779 from AKalinich-Luxoft/hotfix/fix_default_app_policies_update_after_ptu

Fix PTU applying for "default" app policies

8 files changed, 136 insertions, 72 deletions

diff --git a/src/components/policy/policy_external/include/policy/policy_helper.h b/src/components/policy/policy_external/include/policy/policy_helper.h
index 42c1ec0b46..6945f45b45 100644
--- a/src/components/policy/policy_external/include/policy/policy_helper.h
+++ b/src/components/policy/policy_external/include/policy/policy_helper.h
@@ -199,6 +199,23 @@ struct CheckAppPolicy {
   CheckAppPolicyResults& out_results_;
 };
 
+/**
+ * @brief Helper struct for filling actions to be done for processed application
+ * using CheckAppPolicyResults data as a source
+ */
+struct FillActionsForAppPolicies {
+  FillActionsForAppPolicies(
+      ApplicationsPoliciesActions& actions,
+      const policy_table::ApplicationPolicies& app_policies)
+      : actions_(actions), app_policies_(app_policies) {}
+
+  void operator()(const policy::CheckAppPolicyResults::value_type& value);
+
+ private:
+  ApplicationsPoliciesActions& actions_;
+  const policy_table::ApplicationPolicies& app_policies_;
+};
+
 /*
  * @brief Fill permissions data with merged rpc permissions for hmi levels and
  * parameters
diff --git a/src/components/policy/policy_external/include/policy/policy_manager_impl.h b/src/components/policy/policy_external/include/policy/policy_manager_impl.h
index f8d226c86a..80ceb06e7e 100644
--- a/src/components/policy/policy_external/include/policy/policy_manager_impl.h
+++ b/src/components/policy/policy_external/include/policy/policy_manager_impl.h
@@ -719,8 +719,9 @@ class PolicyManagerImpl : public PolicyManager {
 
   /**
    * @brief Processes results from policy table update analysis done by
-   * CheckPermissionsChanges() by sending OnPermissionChange and
-   * OnAppPermissionChanged notifications
+   * CheckPermissionsChanges() by filling ApplicationsPoliciesActions struct
+   * with actions which should be done for every application and passes them to
+   * ProcessActionsForAppPolicies()
    * @param results Collection of per-application results
    * @param app_policies Reference to updated application policies section as
    * a data source for generating notifications data
@@ -730,6 +731,19 @@ class PolicyManagerImpl : public PolicyManager {
       const policy_table::ApplicationPolicies& app_policies);
 
   /**
+   * @brief Processes actions filled in ProcessAppPolicyCheckResults() for every
+   * application by sending OnPermissionChange and OnAppPermissionChanged
+   * notifications and by checking consent where it needed
+   * @param actions Reference to map with actions to be done or not for every
+   * application
+   * @param app_policies Reference to updated application policies section as
+   * a data source for generating notifications data
+   */
+  void ProcessActionsForAppPolicies(
+      const ApplicationsPoliciesActions& actions,
+      const policy_table::ApplicationPolicies& app_policies);
+
+  /**
    * @brief Fill structure to be sent with OnPermissionsChanged notification
    *
    * @param Policy table struct, which contains rpc functional groups data
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 9ba1a1cfff..f57919b6f7 100644
--- a/src/components/policy/policy_external/include/policy/policy_types.h
+++ b/src/components/policy/policy_external/include/policy/policy_types.h
@@ -432,6 +432,28 @@ struct ExternalConsentStatusItemSorter {
 };
 
 /**
+ * @brief The ApplicationPolicyActions struct contains actions which should be
+ * done for some application
+ */
+struct ApplicationPolicyActions {
+  ApplicationPolicyActions()
+      : is_notify_system(false)
+      , is_send_permissions_to_app(false)
+      , is_consent_needed(false) {}
+
+  bool is_notify_system;
+  bool is_send_permissions_to_app;
+  bool is_consent_needed;
+};
+
+/**
+ * @brief ApplicationsPoliciesActions map of actions to be done for every
+ * application
+ */
+typedef std::map<std::string, ApplicationPolicyActions>
+    ApplicationsPoliciesActions;
+
+/**
  * @brief Customer connectivity settings status
  */
 typedef std::set<ExternalConsentStatusItem, ExternalConsentStatusItemSorter>
diff --git a/src/components/policy/policy_external/src/cache_manager.cc b/src/components/policy/policy_external/src/cache_manager.cc
index 95b2fda272..3a3f3d06a9 100644
--- a/src/components/policy/policy_external/src/cache_manager.cc
+++ b/src/components/policy/policy_external/src/cache_manager.cc
@@ -633,7 +633,7 @@ void CacheManager::ProcessUpdate(
       *(initial_policy_iter->second.RequestType);
 
   const std::string& app_id = initial_policy_iter->first;
-  RequestTypes merged_pt_request_types;
+  bool update_request_types = true;
 
   if (app_id == kDefaultId || app_id == kPreDataConsentId) {
     if (new_request_types.is_omitted()) {
@@ -641,25 +641,28 @@ void CacheManager::ProcessUpdate(
                    "Application " << app_id
                                   << " has omitted RequestTypes."
                                      " Previous values will be kept.");
-      return;
-    }
-    if (new_request_types.empty()) {
+      update_request_types = false;
+    } else if (new_request_types.empty()) {
       if (new_request_types.is_cleaned_up()) {
         LOG4CXX_INFO(logger_,
                      "Application " << app_id
                                     << " has cleaned up all values."
                                        " Previous values will be kept.");
-        return;
+        update_request_types = false;
+      } else {
+        LOG4CXX_INFO(logger_,
+                     "Application " << app_id
+                                    << " has empty RequestTypes."
+                                       " Any parameter will be allowed.");
       }
-      LOG4CXX_INFO(logger_,
-                   "Application " << app_id
-                                  << " has empty RequestTypes."
-                                     " Any parameter will be allowed.");
     }
-    merged_pt_request_types = new_request_types;
-  } else {
-    merged_pt_request_types = new_request_types;
   }
+
+  const RequestTypes merged_pt_request_types =
+      update_request_types
+          ? new_request_types
+          : *(pt_->policy_table.app_policies_section.apps[app_id].RequestType);
+
   pt_->policy_table.app_policies_section.apps[app_id] =
       initial_policy_iter->second;
   *(pt_->policy_table.app_policies_section.apps[app_id].RequestType) =
@@ -685,15 +688,6 @@ bool CacheManager::ApplyUpdate(const policy_table::Table& update_pt) {
       pt_->policy_table.app_policies_section.apps[iter->first].set_to_null();
       pt_->policy_table.app_policies_section.apps[iter->first].set_to_string(
           "");
-    } else if (policy::kDefaultId == (iter->second).get_string()) {
-      policy_table::ApplicationPolicies::const_iterator iter_default =
-          update_pt.policy_table.app_policies_section.apps.find(kDefaultId);
-      if (update_pt.policy_table.app_policies_section.apps.end() ==
-          iter_default) {
-        LOG4CXX_ERROR(logger_, "The default section was not found in PTU");
-        continue;
-      }
-      ProcessUpdate(iter_default);
     } else {
       ProcessUpdate(iter);
     }
diff --git a/src/components/policy/policy_external/src/policy_helper.cc b/src/components/policy/policy_external/src/policy_helper.cc
index 3041323489..5425777833 100644
--- a/src/components/policy/policy_external/src/policy_helper.cc
+++ b/src/components/policy/policy_external/src/policy_helper.cc
@@ -484,6 +484,41 @@ bool CheckAppPolicy::IsRequestTypeChanged(
   return diff.size();
 }
 
+void FillActionsForAppPolicies::operator()(
+    const policy::CheckAppPolicyResults::value_type& value) {
+  const std::string app_id = value.first;
+  const policy_table::ApplicationPolicies::const_iterator app_policy =
+      app_policies_.find(app_id);
+
+  if (app_policies_.end() == app_policy) {
+    return;
+  }
+
+  if (IsPredefinedApp(*app_policy)) {
+    return;
+  }
+
+  switch (value.second) {
+    case RESULT_APP_REVOKED:
+    case RESULT_NICKNAME_MISMATCH:
+      actions_[app_id].is_notify_system = true;
+      return;
+    case RESULT_CONSENT_NEEDED:
+    case RESULT_PERMISSIONS_REVOKED_AND_CONSENT_NEEDED:
+      actions_[app_id].is_consent_needed = true;
+      break;
+    case RESULT_CONSENT_NOT_REQIURED:
+    case RESULT_PERMISSIONS_REVOKED:
+    case RESULT_REQUEST_TYPE_CHANGED:
+      break;
+    case RESULT_NO_CHANGES:
+    default:
+      return;
+  }
+  actions_[app_id].is_notify_system = true;
+  actions_[app_id].is_send_permissions_to_app = true;
+}
+
 FillNotificationData::FillNotificationData(Permissions& data,
                                            GroupConsent group_state,
                                            GroupConsent undefined_group_consent,
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 8a80e7755b..d50779383a 100644
--- a/src/components/policy/policy_external/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_external/src/policy_manager_impl.cc
@@ -385,53 +385,45 @@ CheckAppPolicyResults PolicyManagerImpl::CheckPermissionsChanges(
 void PolicyManagerImpl::ProcessAppPolicyCheckResults(
     const CheckAppPolicyResults& results,
     const policy_table::ApplicationPolicies& app_policies) {
-  CheckAppPolicyResults::const_iterator it_results = results.begin();
+  ApplicationsPoliciesActions actions_for_apps_policies;
+  FillActionsForAppPolicies filler(actions_for_apps_policies, app_policies);
 
-  for (; results.end() != it_results; ++it_results) {
-    const policy_table::ApplicationPolicies::const_iterator app_policy =
-        app_policies.find(it_results->first);
+  std::for_each(results.begin(), results.end(), filler);
 
+  ProcessActionsForAppPolicies(actions_for_apps_policies, app_policies);
+}
+
+void PolicyManagerImpl::ProcessActionsForAppPolicies(
+    const ApplicationsPoliciesActions& actions,
+    const policy_table::ApplicationPolicies& app_policies) {
+  ApplicationsPoliciesActions::const_iterator it_actions = actions.begin();
+  for (; it_actions != actions.end(); ++it_actions) {
+    policy_table::ApplicationPolicies::const_iterator app_policy =
+        app_policies.find(it_actions->first);
     if (app_policies.end() == app_policy) {
       continue;
     }
 
-    if (IsPredefinedApp(*app_policy)) {
-      continue;
-    }
+    if (it_actions->second.is_consent_needed) {
+      // Post-check after ExternalConsent consent changes
+      const std::string& policy_app_id = app_policy->first;
+      if (!IsConsentNeeded(policy_app_id)) {
+        sync_primitives::AutoLock lock(app_permissions_diff_lock_);
 
-    switch (it_results->second) {
-      case RESULT_NO_CHANGES:
-        continue;
-      case RESULT_APP_REVOKED:
-        NotifySystem(*app_policy);
-        continue;
-      case RESULT_NICKNAME_MISMATCH:
-        NotifySystem(*app_policy);
-        continue;
-      case RESULT_CONSENT_NEEDED:
-      case RESULT_PERMISSIONS_REVOKED_AND_CONSENT_NEEDED: {
-        // Post-check after ExternalConsent consent changes
-        const std::string policy_app_id = app_policy->first;
-        if (!IsConsentNeeded(policy_app_id)) {
-          sync_primitives::AutoLock lock(app_permissions_diff_lock_);
-
-          PendingPermissions::iterator app_id_diff =
-              app_permissions_diff_.find(policy_app_id);
-
-          if (app_permissions_diff_.end() != app_id_diff) {
-            app_id_diff->second.appPermissionsConsentNeeded = false;
-          }
+        PendingPermissions::iterator app_id_diff =
+            app_permissions_diff_.find(policy_app_id);
+
+        if (app_permissions_diff_.end() != app_id_diff) {
+          app_id_diff->second.appPermissionsConsentNeeded = false;
         }
-      } break;
-      case RESULT_CONSENT_NOT_REQIURED:
-      case RESULT_PERMISSIONS_REVOKED:
-      case RESULT_REQUEST_TYPE_CHANGED:
-        break;
-      default:
-        continue;
+      }
+    }
+    if (it_actions->second.is_notify_system) {
+      NotifySystem(*app_policy);
+    }
+    if (it_actions->second.is_send_permissions_to_app) {
+      SendPermissionsToApp(*app_policy);
     }
-    NotifySystem(*app_policy);
-    SendPermissionsToApp(*app_policy);
   }
 }
 
diff --git a/src/components/policy/policy_external/test/policy_manager_impl_ptu_test.cc b/src/components/policy/policy_external/test/policy_manager_impl_ptu_test.cc
index c958f6bcd7..da0cd913b6 100644
--- a/src/components/policy/policy_external/test/policy_manager_impl_ptu_test.cc
+++ b/src/components/policy/policy_external/test/policy_manager_impl_ptu_test.cc
@@ -1344,7 +1344,7 @@ TEST_F(PolicyManagerImplTest2,
   // Add app
   policy_manager_->AddApplication(section_name,
                                   HmiTypes(policy_table::AHT_DEFAULT));
-  EXPECT_CALL(listener_, OnPendingPermissionChange(section_name)).Times(2);
+  EXPECT_CALL(listener_, OnPendingPermissionChange(section_name));
 
   // PTU has single invalid RequestTypes, which must be dropped and replaced
   // with default RT
diff --git a/src/components/policy/policy_regular/src/cache_manager.cc b/src/components/policy/policy_regular/src/cache_manager.cc
index 8b6b9a433e..377278949b 100644
--- a/src/components/policy/policy_regular/src/cache_manager.cc
+++ b/src/components/policy/policy_regular/src/cache_manager.cc
@@ -242,16 +242,6 @@ bool CacheManager::ApplyUpdate(const policy_table::Table& update_pt) {
       pt_->policy_table.app_policies_section.apps[iter->first].set_to_null();
       pt_->policy_table.app_policies_section.apps[iter->first].set_to_string(
           "");
-    } else if (policy::kDefaultId == (iter->second).get_string()) {
-      policy_table::ApplicationPolicies::const_iterator iter_default =
-          update_pt.policy_table.app_policies_section.apps.find(kDefaultId);
-      if (update_pt.policy_table.app_policies_section.apps.end() ==
-          iter_default) {
-        LOG4CXX_ERROR(logger_, "The default section was not found in PTU");
-        continue;
-      }
-      pt_->policy_table.app_policies_section.apps[iter->first] =
-          iter_default->second;
     } else {
       pt_->policy_table.app_policies_section.apps[iter->first] = iter->second;
     }