Fix code review discussions

components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.create/public/js/policy-create.js

@@ -594,7 +594,6 @@ $(document).ready(function () {
         }
     });
 
-    //todo
     $('input[type=radio][name=policy-type-radio-btn]').change(function() {
         if ($(this).val() === "CORRECTIVE") {
             $("#select-general-policy-type").addClass("hidden");

components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.view/view.js

@@ -45,7 +45,5 @@ function onRequest(context) {
     var devicemgtProps = require("/app/modules/conf-reader/main.js")["conf"];
     page["isCloud"] = devicemgtProps.isCloud;
     
-    /*page["correctivePolicies"] = JSON.stringify(policyModule.getAllPoliciesByType("CORRECTIVE")["content"]);*/
-    
     return page;
 }

components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/dao/impl/PolicyDAOImpl.java

@@ -306,16 +306,12 @@ public class PolicyDAOImpl implements PolicyDAO {
     public List<CorrectiveAction> getCorrectiveActionsOfPolicy(int policyId) throws PolicyManagerDAOException {
         try {
             Connection conn = this.getConnection();
-            String query = "SELECT * " +
+            String query = "SELECT ACTION_TYPE, CORRECTIVE_POLICY_ID, FEATURE_ID, POLICY_ID " +
                     "FROM DM_POLICY_CORRECTIVE_ACTION " +
                     "WHERE POLICY_ID = ?";
             try (PreparedStatement selectStmt = conn.prepareStatement(query)) {
-                List<CorrectiveAction> correctiveActions = new ArrayList<>();
                 selectStmt.setInt(1, policyId);
-                try (ResultSet rs = selectStmt.executeQuery()) {
+                return extractCorrectivePolicies(selectStmt);
-                    extractCorrectivePolicies(selectStmt, correctiveActions);
-                }
-                return correctiveActions;
             }
         } catch (SQLException e) {
             String msg = "Error occurred while retrieving corrective actions of policy ID " + policyId;
@@ -328,12 +324,11 @@ public class PolicyDAOImpl implements PolicyDAO {
     public List<CorrectiveAction> getAllCorrectiveActions() throws PolicyManagerDAOException {
         try {
             Connection conn = this.getConnection();
-            String query = "SELECT * " +
+            String query = "SELECT ACTION_TYPE, CORRECTIVE_POLICY_ID, FEATURE_ID, POLICY_ID " +
                     "FROM DM_POLICY_CORRECTIVE_ACTION ";
             try (PreparedStatement stmt = conn.prepareStatement(query)) {
                 List<CorrectiveAction> correctiveActions = new ArrayList<>();
-                extractCorrectivePolicies(stmt, correctiveActions);
+                return extractCorrectivePolicies(stmt);
-                return correctiveActions;
             }
         } catch (SQLException e) {
             String msg = "Error occurred while retrieving all corrective actions";
@@ -342,8 +337,14 @@ public class PolicyDAOImpl implements PolicyDAO {
         }
     }
 
-    private void extractCorrectivePolicies(PreparedStatement stmt, List<CorrectiveAction>
+    /**
-            correctiveActions) throws SQLException {
+     * Extract corrective policies from DB query result
+     * @param stmt DB Query statement
+     * @return List of corrective actions queries
+     * @throws SQLException when a DB related issue occurs
+     */
+    private List<CorrectiveAction> extractCorrectivePolicies(PreparedStatement stmt) throws SQLException {
+        List<CorrectiveAction> correctiveActions = new ArrayList<>();
         try (ResultSet rs = stmt.executeQuery()) {
             CorrectiveAction correctiveAction;
             while (rs.next()) {
@@ -355,6 +356,7 @@ public class PolicyDAOImpl implements PolicyDAO {
                 correctiveActions.add(correctiveAction);
             }
         }
+        return correctiveActions;
     }
 
     @Override
@@ -362,14 +364,12 @@ public class PolicyDAOImpl implements PolicyDAO {
                                                 int policyId, int featureId)
             throws PolicyManagerDAOException {
         try {
-            boolean isFeatureIdContains = false;
             Connection conn = this.getConnection();
             String query = "UPDATE DM_POLICY_CORRECTIVE_ACTION " +
                            "SET CORRECTIVE_POLICY_ID = ? " +
                            "WHERE ACTION_TYPE = ? " +
                            "AND POLICY_ID = ? ";
             if (featureId != -1) {
-                isFeatureIdContains = true;
                 query = query.concat("AND FEATURE_ID = ?");
             }
             try (PreparedStatement updateStmt = conn.prepareStatement(query)) {
@@ -377,7 +377,7 @@ public class PolicyDAOImpl implements PolicyDAO {
                     updateStmt.setInt(1, correctiveAction.getPolicyId());
                     updateStmt.setString(2, correctiveAction.getActionType());
                     updateStmt.setInt(3, policyId);
-                    if (isFeatureIdContains) {
+                    if (featureId != -1) {
                         updateStmt.setInt(4, featureId);
                     }
                     updateStmt.addBatch();
@@ -400,16 +400,14 @@ public class PolicyDAOImpl implements PolicyDAO {
             String query = "DELETE FROM DM_POLICY_CORRECTIVE_ACTION " +
                            "WHERE ACTION_TYPE = ? " +
                            "AND POLICY_ID = ? ";
-            boolean isFeatueIdContains = false;
             if (featureId != -1) {
-                isFeatueIdContains = true;
                 query = query.concat("AND FEATURE_ID = ?");
             }
             try (PreparedStatement deleteStmt = conn.prepareStatement(query)) {
                 for (CorrectiveAction correctiveAction : correctiveActions) {
                     deleteStmt.setString(1, correctiveAction.getActionType());
                     deleteStmt.setInt(2, policyId);
-                    if (isFeatueIdContains) {
+                    if (featureId != -1) {
                         deleteStmt.setInt(3, featureId);
                     }
                     deleteStmt.addBatch();

components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/mgt/impl/PolicyManagerImpl.java

@@ -170,14 +170,6 @@ public class PolicyManagerImpl implements PolicyManager {
                 policyDAO.addPolicyCriteriaProperties(policy.getPolicyCriterias());
             }
 
-            /*if (policy.getCorrectiveActions() != null && !policy.getCorrectiveActions().isEmpty()) {
-                if (log.isDebugEnabled()) {
-                    log.debug("Adding corrective actions for policy " + policy.getPolicyName() +
-                            " having policy id " + policy.getId());
-                }
-                policyDAO.addCorrectiveActionsOfPolicy(policy.getCorrectiveActions(), policy.getId());
-            }*/
-
             if (policy.isActive()) {
                 policyDAO.activatePolicy(policy.getId());
             }
@@ -358,6 +350,13 @@ public class PolicyManagerImpl implements PolicyManager {
         return policy;
     }
 
+    /**
+     * Using for update old type of corrective policies which has single corrective policy
+     * per single general policy
+     * @param policy updating new corrective policy
+     * @param previousPolicy previous corrective policy
+     * @throws PolicyManagerDAOException for errors occur while updating corrective actions
+     */
     private void updateSingleCorrectiveActionList(Policy policy, Policy previousPolicy)
             throws PolicyManagerDAOException {
         List<CorrectiveAction> updatedCorrectiveActions = policy.getCorrectiveActions();
@@ -400,42 +399,30 @@ public class PolicyManagerImpl implements PolicyManager {
         }
 
         if (!correctiveActionsToUpdate.isEmpty()) {
-            try {
             policyDAO.updateCorrectiveActionsOfPolicy(correctiveActionsToUpdate,
                     previousPolicy.getId(), -1);
-            } catch (PolicyManagerDAOException e) {
-                String msg = "Error occurred while updating corrective policies of the policy" +
-                        " "+ previousPolicy.getId();
-                log.error(msg, e);
-                throw new PolicyManagerDAOException(msg, e);
-            }
         }
 
         if (!correctiveActionsToAdd.isEmpty()) {
-            try {
             policyDAO.addCorrectiveActionsOfPolicy(correctiveActionsToAdd,
                     previousPolicy.getId(), -1);
-            } catch (PolicyManagerDAOException e) {
-                String msg = "Error occurred while adding new corrective policies to the " +
-                        "policy the policy " + previousPolicy.getId();
-                log.error(msg, e);
-                throw new PolicyManagerDAOException(msg, e);
-            }
         }
 
         if (!correctiveActionsToDelete.isEmpty()) {
-            try {
             policyDAO.deleteCorrectiveActionsOfPolicy(correctiveActionsToDelete,
                     previousPolicy.getId(), -1);
-            } catch (PolicyManagerDAOException e) {
-                String msg = "Error occurred while deleting corrective actions from the " +
-                        "policy " + previousPolicy.getId();
-                log.error(msg, e);
-                throw new PolicyManagerDAOException(msg, e);
-            }
         }
     }
 
+    /**
+     * Using for update new type of corrective policies which has multiple corrective policies
+     * per single general policy
+     * @param updatedCorrectiveActionsMap updated corrective actions <FeatureId, CorrectiveActionList>
+     * @param existingCorrectiveActionsMap existing corrective actions <FeatureId, CorrectiveActionList>
+     * @param policy updating policy
+     * @param previousPolicy for errors occur while updating corrective actions
+     * @throws PolicyManagerDAOException
+     */
     private void updateMultipleCorrectiveActions(
             Map<Integer, List<CorrectiveAction>> updatedCorrectiveActionsMap,
             Map<Integer, List<CorrectiveAction>> existingCorrectiveActionsMap,
@@ -496,7 +483,6 @@ public class PolicyManagerImpl implements PolicyManager {
             }
         }
 
-
         for (Integer featureId : existingCorrectiveActionsMap.keySet()) {
             List<CorrectiveAction> existingCorrectiveActions = existingCorrectiveActionsMap
                     .get(featureId);
@@ -521,52 +507,30 @@ public class PolicyManagerImpl implements PolicyManager {
         }
 
         if (!correctiveActionsToUpdate.isEmpty()) {
-            try {
             for (Integer featureId : correctiveActionsToUpdate.keySet()) {
                 List<CorrectiveAction> correctiveActions = correctiveActionsToUpdate
                         .get(featureId);
                 policyDAO.updateCorrectiveActionsOfPolicy(correctiveActions,
                         previousPolicy.getId(), featureId);
             }
-            } catch (PolicyManagerDAOException e) {
-                String msg = "Error occurred while updating corrective policies of the policy" +
-                        " "+ previousPolicy.getId();
-                log.error(msg, e);
-                throw new PolicyManagerDAOException(msg, e);
-            }
         }
 
         if (!correctiveActionsToAdd.isEmpty()) {
-            try {
             for (Integer featureId : correctiveActionsToAdd.keySet()) {
                 List<CorrectiveAction> correctiveActions = correctiveActionsToAdd
                         .get(featureId);
                 policyDAO.addCorrectiveActionsOfPolicy(correctiveActions,
                         previousPolicy.getId(), featureId);
             }
-
-            } catch (PolicyManagerDAOException e) {
-                String msg = "Error occurred while adding new corrective policies to the " +
-                        "policy the policy " + previousPolicy.getId();
-                log.error(msg, e);
-                throw new PolicyManagerDAOException(msg, e);
-            }
         }
 
         if (!correctiveActionsToDelete.isEmpty()) {
-            try {
             for (Integer featureId : correctiveActionsToDelete.keySet()) {
                 List<CorrectiveAction> correctiveActions = correctiveActionsToDelete
                         .get(featureId);
                 policyDAO.deleteCorrectiveActionsOfPolicy(correctiveActions,
                         previousPolicy.getId(), featureId);
             }
-            } catch (PolicyManagerDAOException e) {
-                String msg = "Error occurred while deleting corrective actions from the " +
-                        "policy " + previousPolicy.getId();
-                log.error(msg, e);
-                throw new PolicyManagerDAOException(msg, e);
-            }
         }
     }
 
@@ -908,6 +872,12 @@ public class PolicyManagerImpl implements PolicyManager {
         try {
             Profile profile = profileManager.getProfile(policy.getProfileId());
             policy.setProfile(profile);
+        } catch (ProfileManagementException e) {
+            throw new PolicyManagementException("Error occurred while getting the profile related to policy ID (" +
+                    policyId + ")", e);
+        }
+
+        try {
             PolicyManagementDAOFactory.openConnection();
             List<CorrectiveAction> correctiveActionsOfPolicy = policyDAO
                     .getCorrectiveActionsOfPolicy(policyId);
@@ -922,13 +892,6 @@ public class PolicyManagerImpl implements PolicyManager {
                 policy.setCorrectiveActions(getSingleCorrectiveAction
                         (correctiveActionsOfPolicy, policyId));
             }
-        } catch (ProfileManagementException e) {
-            throw new PolicyManagementException("Error occurred while getting the profile related to policy ID (" +
-                    policyId + ")", e);
-//        } catch (SQLException e) {
-//            throw new PolicyManagementException("Error occurred while opening a connection to the data source", e);
-//        } finally {
-//            PolicyManagementDAOFactory.closeConnection();
         } catch (PolicyManagerDAOException e) {
             String msg = "Error occurred while getting the corrective actions related to policy " +
                     "ID (" + policyId + ")";
@@ -941,8 +904,6 @@ public class PolicyManagerImpl implements PolicyManager {
         } finally {
             PolicyManagementDAOFactory.closeConnection();
         }
-
-
         return policy;
     }
 
@@ -1525,8 +1486,15 @@ public class PolicyManagerImpl implements PolicyManager {
         return policyList;
     }
 
+    /**
+     * Build the list of policies which are included new and old types of corrective actions
+     * @param policyList queried policy list
+     * @param profileList queried profile list
+     * @throws PolicyManagerDAOException when failed to read policies from DB
+     * @throws GroupManagementException when failed to read policy groups from DB
+     */
     private void buildPolicyList(List<Policy> policyList, List<Profile> profileList)
-            throws PolicyManagerDAOException, GroupManagementException, PolicyManagementException {
+            throws PolicyManagerDAOException, GroupManagementException {
         List<CorrectiveAction> allCorrectiveActions = policyDAO.getAllCorrectiveActions();
         for (Policy policy : policyList) {
             String policyPayloadVersion = policy.getPolicyPayloadVersion();
@@ -1594,6 +1562,10 @@ public class PolicyManagerImpl implements PolicyManager {
         }
     }
 
+    /**
+     * Clear corrective action metadata values to avoid sending in payload
+     * @param correctiveAction list of corrective actions
+     */
     private void clearMetaDataValues(CorrectiveAction correctiveAction) {
         correctiveAction.setAssociatedGeneralPolicyId(null); //avoiding send in payload
         correctiveAction.setFeatureId(null); //avoiding send in payload

components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/util/PolicyManagerUtil.java

@@ -150,7 +150,7 @@ public class PolicyManagerUtil {
                 PolicyManagementConstants.GENERAL_POLICY_TYPE.equals(policy.getPolicyType())) {
             String policyPayloadVersion = policy.getPolicyPayloadVersion();
             float payloadVersion = 0f;
-            if (policyPayloadVersion != null && !StringUtils.isEmpty(policyPayloadVersion)) {
+            if (!StringUtils.isEmpty(policyPayloadVersion)) {
                 payloadVersion = Float.parseFloat(policyPayloadVersion);
             }
             if (payloadVersion >= 2.0f) {
@@ -229,8 +229,9 @@ public class PolicyManagerUtil {
         correctiveProfileOperation.setCode(PolicyManagementConstants.POLICY_ACTIONS);
         Set<Integer> correctivePolicyIdSet = new HashSet<>();
         try {
+            List<CorrectiveAction> correctiveActions;
             for (ProfileFeature feature : features) {
-                List<CorrectiveAction> correctiveActions = feature.getCorrectiveActions();
+                correctiveActions = feature.getCorrectiveActions();
                 for (CorrectiveAction correctiveAction : correctiveActions) {
                     if (PolicyManagementConstants.POLICY_CORRECTIVE_ACTION_TYPE
                             .equals(correctiveAction.getActionType())) {
@@ -242,13 +243,12 @@ public class PolicyManagerUtil {
             PolicyAdministratorPoint pap = new PolicyAdministratorPointImpl();
             List<Policy> allCorrectivePolicies = pap
                     .getPolicies(PolicyManagementConstants.CORRECTIVE_POLICY_TYPE);
-            idLoop:
             for (Integer policyId : correctivePolicyIdSet) {
                 for (Policy correctivePolicy : allCorrectivePolicies) {
                     if (policyId == correctivePolicy.getId()) {
                         createCorrectiveProfileOperations(correctivePolicy, correctiveProfileOperation);
                         policyOperation.getProfileOperations().add(correctiveProfileOperation);
-                        continue idLoop;
+                        break;
                     }
                 }
             }
@@ -287,7 +287,11 @@ public class PolicyManagerUtil {
         correctiveOperationList.setPayLoad(payLoad);
     }
 
-
+    /**
+     * Create list of profile operations
+     * @param effectiveFeatures effective features of the policy
+     * @return List of ProfileOperation
+     */
     public static List<ProfileOperation> createProfileOperations(List<ProfileFeature> effectiveFeatures) {
         List<ProfileOperation> profileOperations = new ArrayList<>();
         for (ProfileFeature feature : effectiveFeatures) {

components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/test/resources/sql/CreateH2TestDB.sql

@@ -206,20 +206,16 @@ CREATE TABLE IF NOT EXISTS DM_POLICY_CORRECTIVE_ACTION (
   ACTION_TYPE VARCHAR(45) NOT NULL,
   CORRECTIVE_POLICY_ID INT(11) DEFAULT NULL,
   POLICY_ID INT(11) NOT NULL,
-  FEATURE_ID INT(11) NOT NULL,
+  FEATURE_ID INT(11) DEFAULT NULL,
   PRIMARY KEY (ID),
   CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION
     FOREIGN KEY (POLICY_ID)
     REFERENCES DM_POLICY (ID)
     ON DELETE NO ACTION
-    ON UPDATE NO ACTION,
-  CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION
-    FOREIGN KEY (FEATURE_ID)
-    REFERENCES DM_PROFILE_FEATURES (ID)
-    ON DELETE NO ACTION
     ON UPDATE NO ACTION
 );
 
+
 DROP TABLE IF EXISTS DM_DEVICE_POLICY;
 CREATE  TABLE IF NOT EXISTS DM_DEVICE_POLICY (
   ID INT(11) NOT NULL AUTO_INCREMENT ,

features/device-mgt/org.wso2.carbon.device.mgt.basics.feature/src/main/resources/dbscripts/cdm/postgresql.sql

@@ -243,19 +243,15 @@ CREATE TABLE IF NOT EXISTS DM_POLICY_CORRECTIVE_ACTION (
   ACTION_TYPE VARCHAR(45) NOT NULL,
   CORRECTIVE_POLICY_ID INTEGER DEFAULT NULL,
   POLICY_ID INTEGER NOT NULL,
-  FEATURE_ID INTEGER NOT NULL,
+  FEATURE_ID INTEGER DEFAULT NULL,
   CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION
     FOREIGN KEY (POLICY_ID)
     REFERENCES DM_POLICY (ID)
     ON DELETE NO ACTION
-    ON UPDATE NO ACTION,
-  CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION
-    FOREIGN KEY (FEATURE_ID)
-    REFERENCES DM_PROFILE_FEATURES (ID)
-    ON DELETE NO ACTION
     ON UPDATE NO ACTION
 );
 
+
 CREATE  TABLE IF NOT EXISTS DM_DEVICE_POLICY (
     ID INTEGER NOT NULL DEFAULT NEXTVAL ('DM_DEVICE_POLICY_seq') ,
     DEVICE_ID INTEGER NOT NULL ,