← Back to team overview

dhis2-devs team mailing list archive

[Branch ~dhis2-devs-core/dhis2/trunk] Rev 16297: Fix data approval acceptance permissions bug.

 

------------------------------------------------------------
revno: 16297
committer: jimgrace@xxxxxxxxx
branch nick: dhis2
timestamp: Sun 2014-08-03 18:03:34 -0400
message:
  Fix data approval acceptance permissions bug.
modified:
  dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java
  dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java
  dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java
  dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java


--
lp:dhis2
https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk

Your team DHIS 2 developers is subscribed to branch lp:dhis2.
To unsubscribe from this branch go to https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk/+edit-subscription
=== modified file 'dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java'
--- dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java	2014-04-28 10:17:37 +0000
+++ dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java	2014-08-03 22:03:34 +0000
@@ -63,6 +63,14 @@
     DataApprovalLevel getDataApprovalLevelByName( String name );
 
     /**
+     * Gets the data approval level with the given level number.
+     *
+     * @param levelNumber number of the level to return.
+     * @return a data approval level.
+     */
+    DataApprovalLevel getDataApprovalLevelByLevelNumber( int levelNumber );
+
+    /**
      * Gets a list of all data approval levels.
      *
      * @return List of all data approval levels, ordered from 1 to n.

=== modified file 'dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java'
--- dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java	2014-07-18 11:51:12 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java	2014-08-03 22:03:34 +0000
@@ -101,6 +101,20 @@
         return dataApprovalLevelStore.getByName( name );
     }
 
+    public DataApprovalLevel getDataApprovalLevelByLevelNumber( int levelNumber )
+    {
+        List<DataApprovalLevel> dataApprovalLevels = getAllDataApprovalLevels();
+
+        if ( levelNumber < 1 || levelNumber > dataApprovalLevels.size() )
+        {
+            return null;
+        }
+        else
+        {
+            return dataApprovalLevels.get( levelNumber - 1 );
+        }
+    }
+
     public List<DataApprovalLevel> getAllDataApprovalLevels()
     {
         List<DataApprovalLevel> dataApprovalLevels = dataApprovalLevelStore.getAllDataApprovalLevels();

=== modified file 'dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java'
--- dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java	2014-06-10 20:46:05 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java	2014-08-03 22:03:34 +0000
@@ -146,7 +146,7 @@
     public void deleteDataApproval( DataApproval dataApproval )
     {
         if ( ( dataApproval.getCategoryOptionGroup() == null || securityService.canRead( dataApproval.getCategoryOptionGroup() ) )
-            && mayUnapprove( dataApproval.getOrganisationUnit(), dataApproval.isAccepted() ) )
+            && mayUnapprove( dataApproval ) )
         {
             PeriodType selectionPeriodType = dataApproval.getPeriod().getPeriodType();
             PeriodType dataSetPeriodType = dataApproval.getDataSet().getPeriodType();
@@ -178,7 +178,7 @@
         }
         else
         {
-            warnNotPermitted( dataApproval, "unapprove", mayUnapprove( dataApproval.getOrganisationUnit(), dataApproval.isAccepted() ) );
+            warnNotPermitted( dataApproval, "unapprove", mayUnapprove( dataApproval ) );
         }
     }
 
@@ -230,20 +230,21 @@
             && ( dataApprovalLevel.getCategoryOptionGroupSet() == null || securityService.canRead( dataApprovalLevel.getCategoryOptionGroupSet() ))
             && canReadOneCategoryOptionGroup( categoryOptionGroups ) )
         {
-            boolean unacceptPermissionNeededToUnapprove = false;
-
             switch ( status.getDataApprovalState() )
             {
                 case PARTIALLY_ACCEPTED_HERE:
                 case ACCEPTED_HERE:
-                    unacceptPermissionNeededToUnapprove = true;
                 case PARTIALLY_APPROVED_HERE:
                 case APPROVED_HERE:
+                    permissions.setMayApprove( mayApprove( organisationUnit ) );
+                    permissions.setMayUnapprove( mayUnapprove( status.getDataApproval() ) );
+                    permissions.setMayAccept( mayAcceptOrUnaccept( status.getDataApproval() ) );
+                    permissions.setMayUnaccept( permissions.isMayAccept() );
+                    break;
+
                 case UNAPPROVED_READY:
                     permissions.setMayApprove( mayApprove( organisationUnit ) );
-                    permissions.setMayUnapprove( mayUnapprove( organisationUnit, unacceptPermissionNeededToUnapprove ) );
-                    permissions.setMayAccept( mayAcceptOrUnaccept( organisationUnit ) );
-                    permissions.setMayUnaccept( permissions.isMayAccept() );
+                    permissions.setMayUnapprove( isAuthorizedToUnapprove( organisationUnit ) );
                     break;
             }
         }
@@ -281,7 +282,7 @@
     public void acceptOrUnaccept ( DataApproval dataApproval, boolean accepted )
     {
         if ( ( dataApproval.getCategoryOptionGroup() == null || securityService.canRead( dataApproval.getCategoryOptionGroup() ) )
-                && mayAcceptOrUnaccept( dataApproval.getOrganisationUnit() ) )
+                && mayAcceptOrUnaccept( dataApproval ) )
         {
             PeriodType selectionPeriodType = dataApproval.getPeriod().getPeriodType();
             PeriodType dataSetPeriodType = dataApproval.getDataSet().getPeriodType();
@@ -300,7 +301,7 @@
             }
         } else
         {
-            warnNotPermitted( dataApproval, accepted ? "accept" : "unaccept", mayAcceptOrUnaccept( dataApproval.getOrganisationUnit() ) );
+            warnNotPermitted( dataApproval, accepted ? "accept" : "unaccept", mayAcceptOrUnaccept( dataApproval ) );
         }
     }
 
@@ -401,7 +402,7 @@
      * @param categoryOptionGroups option groups (if any) for data selection
      * @return true if at most 1 option group and user can read, else false
      */
-    boolean canReadOneCategoryOptionGroup( Set<CategoryOptionGroup> categoryOptionGroups )
+    boolean canReadOneCategoryOptionGroup( Collection<CategoryOptionGroup> categoryOptionGroups )
     {
         if ( categoryOptionGroups == null || categoryOptionGroups.size() == 0 )
         {
@@ -417,6 +418,30 @@
     }
 
     /**
+     * Return true if there are no category option groups, or if the user
+     * can read any category option group from the collection.
+     *
+     * @param categoryOptionGroups option groups (if any) for data selection
+     * @return true if at most 1 option group and user can read, else false
+     */
+    boolean canReadSomeCategoryOptionGroup( Collection<CategoryOptionGroup> categoryOptionGroups )
+    {
+        if ( categoryOptionGroups == null )
+        {
+            return true;
+        }
+
+        for ( CategoryOptionGroup cog : categoryOptionGroups )
+        {
+            if ( securityService.canRead( cog ) )
+            {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
      * Checks to see whether a user may approve data for a given
      * organisation unit.
      *
@@ -457,7 +482,7 @@
     }
 
     /**
-     * Checks to see whether a user may unapprove for a given organisation unit.
+     * Checks to see whether a user may unapprove a data approval.
      * <p>
      * A user may unapprove data for organisation unit A if they have the
      * authority to approve data for organisation unit B, and B is an
@@ -471,56 +496,72 @@
      * has been approved already at a higher level for the same period and
      * data set, and the user is not authorized to remove that approval as well.
      *
-     * @param organisationUnit The data approval status to check for permission.
-     * @param unacceptPermissionNeededToUnapprove Whether *unaccept* permission
-     *                                   is also needed to unapprove this data.
+     * @param dataApproval the data approval object for the attempted operation.
      * @return true if the user may unapprove, otherwise false
      */
-    private boolean mayUnapprove( OrganisationUnit organisationUnit, boolean unacceptPermissionNeededToUnapprove )
+    private boolean mayUnapprove( DataApproval dataApproval )
     {
-        if ( isAuthorizedToUnapprove( organisationUnit ) )
+        if ( isAuthorizedToUnapprove( dataApproval.getOrganisationUnit() ) )
         {
-            if ( !unacceptPermissionNeededToUnapprove || mayAcceptOrUnaccept( organisationUnit ) )
+            if ( !dataApproval.isAccepted() || mayAcceptOrUnaccept( dataApproval ) )
             {
-                log.debug( "mayUnapprove = true for organisation unit " + organisationUnit.getName() );
-
                 return true;
             }
         }
 
-        log.debug( "mayUnapprove = false for organisation unit " + organisationUnit.getName() );
-
         return false;
     }
 
     /**
-     * Checks to see whether a user may accept or unaccept for a given
-     * organisation unit.
+     * Checks to see whether a user may accept or unaccept an approval.
      *
-     * @param organisationUnit The organisation unit to check for permission.
+     * @param dataApproval The approval to check for permission.
      * @return true if the user may accept or unaccept, otherwise false.
      */
-    private boolean mayAcceptOrUnaccept ( OrganisationUnit organisationUnit )
+    private boolean mayAcceptOrUnaccept ( DataApproval dataApproval )
     {
+        OrganisationUnit organisationUnit = null;
+
         User user = currentUserService.getCurrentUser();
 
-        if ( user != null )
+        if ( dataApproval != null && user != null )
         {
             boolean mayAcceptAtLowerLevels = user.getUserCredentials().isAuthorized( DataApproval.AUTH_ACCEPT_LOWER_LEVELS );
 
-            if ( mayAcceptAtLowerLevels && CollectionUtils.containsAny( user.getOrganisationUnits(),
-                organisationUnit.getAncestors() ) )
+            organisationUnit = dataApproval.getOrganisationUnit();
+
+            DataApprovalLevel dataApprovalLevel = dataApproval.getDataApprovalLevel();
+
+            if ( mayAcceptAtLowerLevels && organisationUnit != null && dataApprovalLevel != null && dataApprovalLevel.getLevel() > 1 )
             {
-                log.debug( "User may accept or unaccept for organisation unit " + organisationUnit.getName() );
-
-                return true;
+                DataApprovalLevel acceptLevel = dataApprovalLevelService.getDataApprovalLevelByLevelNumber( dataApprovalLevel.getLevel() - 1 );
+
+                if ( securityService.canRead( acceptLevel )
+                        && ( acceptLevel.getCategoryOptionGroupSet() == null ||
+                        ( securityService.canRead( acceptLevel.getCategoryOptionGroupSet() )
+                                && canReadSomeCategoryOptionGroup( acceptLevel.getCategoryOptionGroupSet().getMembers() ) ) ) )
+                {
+                    OrganisationUnit acceptOrgUnit = dataApproval.getOrganisationUnit();
+                    for ( int i = acceptLevel.getOrgUnitLevel(); i < dataApprovalLevel.getOrgUnitLevel(); i++ )
+                    {
+                        acceptOrgUnit = acceptOrgUnit.getParent();
+                    }
+
+                    if ( user.getOrganisationUnits().contains( acceptOrgUnit ) ||
+                            CollectionUtils.containsAny( user.getOrganisationUnits(), acceptOrgUnit.getAncestors() ) )
+                    {
+                        log.debug( "User may accept or unaccept for organisation unit " + organisationUnit.getName() );
+
+                        return true;
+                    }
+                }
             }
         }
 
         log.debug( "User with AUTH_ACCEPT_LOWER_LEVELS " + user.getUserCredentials().isAuthorized( DataApproval.AUTH_ACCEPT_LOWER_LEVELS )
                 + " with " + user.getOrganisationUnits().size() + " org units"
-                + " may not accept or unaccept for organisation unit " + organisationUnit.getName()
-                + " with " + organisationUnit.getAncestors().size() + " ancestors." );
+                + " may not accept or unaccept for organisation unit "
+                + ( organisationUnit == null ? "(null)" : organisationUnit.getName() ) );
 
         return false;
     }

=== modified file 'dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java'
--- dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java	2014-07-18 11:51:12 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java	2014-08-03 22:03:34 +0000
@@ -1092,6 +1092,7 @@
     @Test
     public void testMultiPeriodApproval() throws Exception
     {
+        dataApprovalLevelService.addDataApprovalLevel( level1 );
         dataApprovalLevelService.addDataApprovalLevel( level2 );
 
         dataSetA.setApproveData( true );