dhis2-devs team mailing list archive

[noreply@xxxxxxxxxxxxx]

------------------------------------------------------------
revno: 21684
committer: jimgrace@xxxxxxxxx
branch nick: dhis2
timestamp: Sun 2016-01-10 22:14:37 -0500
message:
  Performance enhancement to HibernateDataApprovalStore.getDataApprovals
modified:
  dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApproval.java
  dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalStatus.java
  dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DataApprovalPermissionsEvaluator.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/main/java/org/hisp/dhis/dataapproval/hibernate/HibernateDataApprovalStore.java
  dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceCategoryOptionGroupTest.java
  dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java
  dhis-2/dhis-web/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/DataApprovalController.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/DataApproval.java'
--- dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApproval.java	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApproval.java	2016-01-11 03:14:37 +0000
@@ -74,12 +74,12 @@
     private DataApprovalWorkflow workflow;
 
     /**
-     * The Period of the DataSet values being approved (required).
+     * The Period of the approval (required).
      */
     private Period period;
 
     /**
-     * The OrganisationUnit of the DataSet values being approved (required).
+     * The OrganisationUnit of the approval (required).
      */
     private OrganisationUnit organisationUnit;
     
@@ -95,13 +95,12 @@
     private boolean accepted;
 
     /**
-     * The Date (including time) when the DataSet values were approved
-     * (required).
+     * The Date (including time) when this approval was made (required).
      */
     private Date created;
 
     /**
-     * The User who approved the DataSet values (required).
+     * The User who made this approval (required).
      */
     private User creator;
 

=== modified file 'dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalStatus.java'
--- dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalStatus.java	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalStatus.java	2016-01-11 03:14:37 +0000
@@ -28,6 +28,10 @@
  * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+import org.hisp.dhis.user.User;
+
+import java.util.Date;
+
 /**
  * Current status of data approval for a given selection of data from a
  * data set. Returns the approval state and, if approved for this particular
@@ -43,24 +47,52 @@
     private DataApprovalState state;
 
     /**
-     * If the selection of data is approved, the data approval object.
-     * If the selection is approved at more than one level, this is the
-     * approval object for the highest level of approval.
-     */
-    private DataApproval dataApproval;
-
-    /**
      * If the selection of data is approved, the data approval level object
      * at which it is approved. If the selection is approved at more than
      * one level, this is for the highest level of approval.
      */
-    private DataApprovalLevel dataApprovalLevel;
-    
+    private DataApprovalLevel approvedLevel;
+
+    /**
+     * If the selection of data is approved, the approval level (same as above)
+     * but if the selection is not approved, the level for this orgUnit at
+     * which it could be approved (if any).
+     */
+    private DataApprovalLevel actionLevel;
+
+    /*
+     * If the selection is approved, the OrganisationUnit UID.
+     */
+    private String organisationUnitUid;
+
+    /**
+     * If the selection is approved, the attribute category option combo UID.
+     */
+    private String attributeOptionComboUid;
+
+    /**
+     * If the selection is approved, whether or not it is accepted
+     * at the highest level approved.
+     */
+    private boolean accepted;
+
     /**
      * Permissions granted for current user for the this approval state.
      */
     private DataApprovalPermissions permissions;
-    
+
+    /**
+     * If the selection is approved, and if present (not always needed),
+     * the date at which the highest level of approval was created.
+     */
+    private Date created;
+
+    /**
+     * If the selection is approved, and if present (not always needed),
+     * The user who made this approval.
+     */
+    private User creator;
+
     // -------------------------------------------------------------------------
     // Constructors
     // -------------------------------------------------------------------------
@@ -69,12 +101,22 @@
     {
     }
 
-    public DataApprovalStatus( DataApprovalState state, DataApproval dataApproval,
-        DataApprovalLevel dataApprovalLevel, DataApprovalPermissions permissions )
-    {
-        this.state = state;
-        this.dataApproval = dataApproval;
-        this.dataApprovalLevel = dataApprovalLevel;
+    public DataApprovalStatus( DataApprovalState state )
+    {
+        this.state = state;
+    }
+
+    public DataApprovalStatus( DataApprovalState state,
+        DataApprovalLevel approvedLevel, DataApprovalLevel actionLevel,
+        String organisationUnitUid, String attributeOptionComboUid,
+        boolean accepted, DataApprovalPermissions permissions )
+    {
+        this.state = state;
+        this.approvedLevel = approvedLevel;
+        this.actionLevel = actionLevel;
+        this.organisationUnitUid = organisationUnitUid;
+        this.attributeOptionComboUid = attributeOptionComboUid;
+        this.accepted = accepted;
         this.permissions = permissions;
     }
 
@@ -82,11 +124,6 @@
     // Getters and setters
     // -------------------------------------------------------------------------
 
-    public DataApproval getDataApproval()
-    {
-        return dataApproval;
-    }
-
     public DataApprovalState getState()
     {
         return state;
@@ -97,19 +134,54 @@
         this.state = state;
     }
 
-    public void setDataApproval( DataApproval dataApproval )
-    {
-        this.dataApproval = dataApproval;
-    }
-
-    public DataApprovalLevel getDataApprovalLevel()
-    {
-        return dataApprovalLevel;
-    }
-
-    public void setDataApprovalLevel( DataApprovalLevel dataApprovalLevel )
-    {
-        this.dataApprovalLevel = dataApprovalLevel;
+    public DataApprovalLevel getApprovedLevel()
+    {
+        return approvedLevel;
+    }
+
+    public void setApprovedLevel( DataApprovalLevel approvedLevel )
+    {
+        this.approvedLevel = approvedLevel;
+    }
+
+    public DataApprovalLevel getActionLevel()
+    {
+        return actionLevel;
+    }
+
+    public void setActionLevel( DataApprovalLevel actionLevel )
+    {
+        this.actionLevel = actionLevel;
+    }
+
+    public String getOrganisationUnitUid()
+    {
+        return organisationUnitUid;
+    }
+
+    public void setOrganisationUnitUid( String organisationUnitUid )
+    {
+        this.organisationUnitUid = organisationUnitUid;
+    }
+
+    public String getAttributeOptionComboUid()
+    {
+        return attributeOptionComboUid;
+    }
+
+    public void setAttributeOptionComboUid( String attributeOptionComboUid )
+    {
+        this.attributeOptionComboUid = attributeOptionComboUid;
+    }
+
+    public boolean isAccepted()
+    {
+        return accepted;
+    }
+
+    public void setAccepted( boolean accepted )
+    {
+        this.accepted = accepted;
     }
 
     public DataApprovalPermissions getPermissions()
@@ -121,4 +193,24 @@
     {
         this.permissions = permissions;
     }
+
+    public Date getCreated()
+    {
+        return created;
+    }
+
+    public void setCreated( Date created )
+    {
+        this.created = created;
+    }
+
+    public User getCreator()
+    {
+        return creator;
+    }
+
+    public void setCreator( User creator )
+    {
+        this.creator = creator;
+    }
 }

=== modified file 'dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DataApprovalPermissionsEvaluator.java'
--- dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DataApprovalPermissionsEvaluator.java	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DataApprovalPermissionsEvaluator.java	2016-01-11 03:14:37 +0000
@@ -82,6 +82,14 @@
         .maximumSize( 50000 ).build();
 
     /**
+     * Clears the user approval level cache, for unit testing when the same
+     * user ID may have different approval levels in quick succession.
+     */
+    public static void invalidateCache() {
+        USER_APPROVAL_LEVEL_CACHE.invalidateAll();
+    }
+
+    /**
      * Allocates and populates the context for determining user permissions
      * on one or more DataApproval objects.
      *
@@ -127,6 +135,9 @@
      * the context of system settings and user information.
      * <p>
      * If there is a data permissions state, also takes this into account.
+     * <p>
+     * It is assumed that the org units have been filtered already for only
+     * the org units that a user may see (read).
      *
      * @param status the data approval status (if any)
      * @param orgUnit the organisation unit being looked at
@@ -135,35 +146,33 @@
      */
     public DataApprovalPermissions getPermissions( DataApprovalStatus status, OrganisationUnit orgUnit, DataApprovalWorkflow workflow )
     {
-        DataApproval da = status.getDataApproval();
-
         DataApprovalState s = status.getState();
 
         DataApprovalPermissions permissions = new DataApprovalPermissions();
 
-        if ( da == null || da.getOrganisationUnit() == null )
+        if ( status.getOrganisationUnitUid() == null )
         {
-            log.debug( "getPermissions da " + da + ( da != null ? ( " " + da.getOrganisationUnit() ) : "" ) );
+            log.debug( "getPermissions organisationUnitUid null for user " + ( user == null ? "(null)" : user.getUsername() ) + " orgUnit " + ( orgUnit == null ? "[null]" : orgUnit.getName() ) );
 
-            permissions.setMayReadData( organisationUnitService.isInUserHierarchy( orgUnit ) );
+            permissions.setMayReadData( true );
 
             return permissions; // No approval permissions set.
         }
 
-        DataApprovalLevel userApprovalLevel = getUserApprovalLevelWithCache( da, workflow );
+        DataApprovalLevel userApprovalLevel = getUserApprovalLevelWithCache( status.getOrganisationUnitUid(), workflow );
 
         if ( userApprovalLevel == null )
         {
-            log.debug( "getPermissions userApprovalLevel null for user " + ( user == null ? "(null)" : user.getUsername() ) + " orgUnit " + da.getOrganisationUnit().getName() );
+            log.debug( "getPermissions userApprovalLevel null for user " + ( user == null ? "(null)" : user.getUsername() ) + " orgUnit " + ( orgUnit == null ? "[null]" : orgUnit.getName() ) );
 
-            permissions.setMayReadData( organisationUnitService.isInUserHierarchy( orgUnit ) );
+            permissions.setMayReadData( true );
 
             return permissions; // Can't find user approval level, so no approval permissions are set.
         }
 
         int userLevel = userApprovalLevel.getLevel();
 
-        DataApprovalLevel dal = da.getDataApprovalLevel();
+        DataApprovalLevel dal = status.getActionLevel();
         int dataLevel = ( dal != null ? dal.getLevel() : maxApprovalLevel );
 
         boolean approvableAtNextHigherLevel = s.isApproved() && dal != null && dataLevel > 1;
@@ -198,12 +207,11 @@
         }
 
         boolean mayReadData = mayApprove || mayUnapprove || mayAccept || mayUnaccept ||
-                ( ( userLevel >= dataLevel || mayViewLowerLevelUnapprovedData )
-                    && organisationUnitService.isInUserHierarchy( da.getOrganisationUnit() ) );
+                ( userLevel >= dataLevel || mayViewLowerLevelUnapprovedData );
 
-        log.debug( "getPermissions orgUnit " + ( da.getOrganisationUnit() == null ? "(null)" : da.getOrganisationUnit().getName() )
+        log.debug( "getPermissions orgUnit " + ( orgUnit == null ? "[null]" : orgUnit.getName() )
             + " workflow " + workflow.getName()
-            + " combo " + da.getAttributeOptionCombo().getName() + " state " + s.name()
+            + " comboUid " + status.getAttributeOptionComboUid() + " state " + s.name()
             + " isApproved " + s.isApproved() + " isApprovable " + s.isApprovable() + " isUnapprovable " + s.isUnapprovable()
             + " isAccepted " + s.isAccepted() + " isAcceptable " + s.isAcceptable() + " isUnacceptable " + s.isUnacceptable()
             + " userLevel " + userLevel + " dataLevel " + dataLevel
@@ -220,18 +228,20 @@
         return permissions;
     }
 
-    private DataApprovalLevel getUserApprovalLevelWithCache( DataApproval da, DataApprovalWorkflow workflow )
+    private DataApprovalLevel getUserApprovalLevelWithCache( String orgUnitUid, DataApprovalWorkflow workflow )
     {
         DataApprovalLevel userApprovalLevel = null;
 
-        final DataApproval dataApproval = da;
+        final String organisationUnitUid = orgUnitUid;
 
         final DataApprovalWorkflow dataApprovalWorkflow = workflow;
 
         try
         {
-            userApprovalLevel = USER_APPROVAL_LEVEL_CACHE.get( user.getId() + "-" + da.getOrganisationUnit().getId(),
-                () -> dataApprovalLevelService.getUserApprovalLevel( user, dataApproval.getOrganisationUnit(), dataApprovalWorkflow.getSortedLevels() ) );
+            userApprovalLevel = USER_APPROVAL_LEVEL_CACHE.get( user.getId() + "-" + organisationUnitUid,
+                () -> dataApprovalLevelService.getUserApprovalLevel( user,
+                    organisationUnitService.getOrganisationUnit( organisationUnitUid ),
+                    dataApprovalWorkflow.getSortedLevels() ) );
         }
         catch ( CacheLoader.InvalidCacheLoadException ex )
         {

=== 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	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java	2016-01-11 03:14:37 +0000
@@ -135,16 +135,16 @@
                 if ( status.getState().isApproved() ) // If approved already, approve at next level up (lower level number)
                 {
                     da.setDataApprovalLevel( dataApprovalLevelService.getDataApprovalLevelByLevelNumber(
-                        status.getDataApproval().getDataApprovalLevel().getLevel() - 1 ) );
+                        status.getActionLevel().getLevel() - 1 ) );
                 }
                 else
                 {
-                    da.setDataApprovalLevel( status.getDataApproval().getDataApprovalLevel() );
+                    da.setDataApprovalLevel( status.getActionLevel() );
                 }
             }
             
             if ( status != null && status.getState().isApproved() && da.getDataApprovalLevel() != null &&
-                da.getDataApprovalLevel().getLevel() >= status.getDataApprovalLevel().getLevel() )
+                da.getDataApprovalLevel().getLevel() >= status.getApprovedLevel().getLevel() )
             {
                 continue; // Already approved at or above this level
             }
@@ -196,7 +196,7 @@
 
             if ( da.getDataApprovalLevel() == null )
             {
-                da.setDataApprovalLevel( status.getDataApproval().getDataApprovalLevel() );
+                da.setDataApprovalLevel( status.getActionLevel() );
             }
 
             if ( status == null || !status.getPermissions().isMayUnapprove() )
@@ -207,7 +207,7 @@
             }
 
             if ( !status.getState().isApproved() || ( da.getDataApprovalLevel() != null &&
-                da.getDataApprovalLevel().getLevel() < status.getDataApprovalLevel().getLevel() ) )
+                da.getDataApprovalLevel().getLevel() < status.getApprovedLevel().getLevel() ) )
             {
                 continue; // Already unapproved at or below this level
             }
@@ -251,12 +251,12 @@
 
             if ( da.getDataApprovalLevel() == null )
             {
-                da.setDataApprovalLevel( status.getDataApproval().getDataApprovalLevel() );
+                da.setDataApprovalLevel( status.getActionLevel() );
             }
 
-            if ( status != null && status.getState() != null && status.getDataApprovalLevel() != null &&
-                ( status.getState().isAccepted() && da.getDataApprovalLevel().getLevel() == status.getDataApprovalLevel().getLevel() ||
-                da.getDataApprovalLevel().getLevel() > status.getDataApprovalLevel().getLevel() ) )
+            if ( status != null && status.getState() != null && status.getApprovedLevel() != null &&
+                ( status.getState().isAccepted() && da.getDataApprovalLevel().getLevel() == status.getApprovedLevel().getLevel() ||
+                da.getDataApprovalLevel().getLevel() > status.getApprovedLevel().getLevel() ) )
             {
                 continue; // Already accepted at, or approved above, this level
             }
@@ -311,11 +311,11 @@
 
             if ( da.getDataApprovalLevel() == null )
             {
-                da.setDataApprovalLevel( status.getDataApproval().getDataApprovalLevel() );
+                da.setDataApprovalLevel( status.getActionLevel() );
             }
 
-            if ( status == null || ( !status.getState().isAccepted() && da.getDataApprovalLevel() != null && da.getDataApprovalLevel().getLevel() == status.getDataApprovalLevel().getLevel() ) ||
-                da.getDataApprovalLevel().getLevel() < status.getDataApprovalLevel().getLevel() )
+            if ( status == null || ( !status.getState().isAccepted() && da.getDataApprovalLevel() != null && da.getDataApprovalLevel().getLevel() == status.getApprovedLevel().getLevel() ) ||
+                da.getDataApprovalLevel().getLevel() < status.getApprovedLevel().getLevel() )
             {
                 continue; // Already unaccepted at or not approved up to this level
             }
@@ -368,19 +368,19 @@
         {
             DataApprovalStatus status = statuses.get( 0 );
 
-            DataApproval da = status.getDataApproval();
-
-            da = dataApprovalStore.getDataApproval( da );
+            DataApproval da = dataApprovalStore.getDataApproval( status.getActionLevel(),
+                workflow, period, organisationUnit, attributeOptionCombo );
 
             if ( da != null )
             {
-                status.setDataApproval( da ); // Includes created and creator from database
+                status.setCreated( da.getCreated() );
+                status.setCreator( da.getCreator() );
             }
 
             return status;
         }
 
-        return new DataApprovalStatus( DataApprovalState.UNAPPROVABLE, null, null, null );
+        return new DataApprovalStatus( DataApprovalState.UNAPPROVABLE );
     }
 
     @Override
@@ -479,7 +479,7 @@
             {
                 status.setPermissions( evaluator.getPermissions( status, da0.getOrganisationUnit(), da0.getWorkflow() ) );
 
-                statusMap.put( daKey( status.getDataApproval() ), status );
+                statusMap.put( daKey( da0 ), status );
             }
         }
 

=== modified file 'dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/hibernate/HibernateDataApprovalStore.java'
--- dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/hibernate/HibernateDataApprovalStore.java	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/hibernate/HibernateDataApprovalStore.java	2016-01-11 03:14:37 +0000
@@ -48,7 +48,6 @@
 import org.hibernate.Criteria;
 import org.hibernate.criterion.Restrictions;
 import org.hisp.dhis.common.IdentifiableObjectUtils;
-import org.hisp.dhis.commons.collection.CachingMap;
 import org.hisp.dhis.dataapproval.DataApproval;
 import org.hisp.dhis.dataapproval.DataApprovalLevel;
 import org.hisp.dhis.dataapproval.DataApprovalLevelService;
@@ -193,11 +192,10 @@
         Period period, OrganisationUnit orgUnit, DataElementCategoryCombo attributeCombo,
         Set<DataElementCategoryOptionCombo> attributeOptionCombos )
     {
-        final CachingMap<Integer, DataElementCategoryOptionCombo> optionComboCache = new CachingMap<>();
-        final CachingMap<Integer, OrganisationUnit> orgUnitCache = new CachingMap<>();
-        
         final User user = currentUserService.getCurrentUser();
 
+        boolean isSuperUser = currentUserService.currentUserIsSuper();
+
         final String startDate = DateUtils.getMediumDateString( period.getStartDate() );
         final String endDate = DateUtils.getMediumDateString( period.getEndDate() );
 
@@ -228,8 +226,17 @@
         String orgUnitJoinOn = null;
         String highestApprovedOrgUnitCompare = null;
 
+        Set<OrganisationUnit> userOrgUnits = user.getDataViewOrganisationUnitsWithFallback();
+
         if ( orgUnit != null )
         {
+            if ( !orgUnit.isDescendant( userOrgUnits ) )
+            {
+                log.debug( "User " + user.getUsername() + " can't see orgUnit " + orgUnit.getName() );
+
+                return new ArrayList<>(); // Unapprovable.
+            }
+
             orgUnitLevel = orgUnit.getLevel();
             orgUnitJoinOn = "o.organisationunitid = " + orgUnit.getId();
             highestApprovedOrgUnitCompare = "da.organisationunitid = o.organisationunitid ";
@@ -253,10 +260,21 @@
             highestApprovedOrgUnitCompare += ") ";
 
             orgUnitJoinOn = "o.level = " + orgUnitLevel;
+
+            if ( !userOrgUnits.isEmpty() )
+            {
+                String restrict = "";
+
+                for ( OrganisationUnit ou : userOrgUnits )
+                {
+                    restrict += ( restrict.length() == 0 ? " and ( " : " or " )
+                        + "o.idlevel" + ou.getLevel() + " = " + ou.getId();
+                }
+
+                orgUnitJoinOn += restrict + ")";
+            }
         }
 
-        boolean isSuperUser = currentUserService.currentUserIsSuper();
-
         DataApprovalLevel lowestApprovalLevelForOrgUnit = null;
 
         String joinAncestors = StringUtils.EMPTY;
@@ -309,9 +327,9 @@
                     "where not exists (select 1 from dataapproval da " +
                         "join period p on p.periodid = da.periodid " +
                         "where da.organisationunitid = ous.organisationunitid " +
-                        "and da.dataapprovallevelid = " + dal.getId() +
+                        "and da.dataapprovallevelid = " + dal.getId() + " " +
                         "and '" + endDate + "' >= p.startdate and '" + endDate + "' <= p.enddate " +
-                        "and da.workflowid = " + workflow.getId() +
+                        "and da.workflowid = " + workflow.getId() + " " +
                         "and da.attributeoptioncomboid = cocco.categoryoptioncomboid " +
                         ( acceptanceRequiredForApproval ? "and da.accepted " : "" ) +
                     ") " +
@@ -339,19 +357,24 @@
                 "and da.workflowid = " + workflow.getId() + " and da.attributeoptioncomboid = cocco.categoryoptioncomboid)";
         }
 
+        int workflowPeriodId = getWorkflowPeriodId( workflow, endDate );
+
         final String sql =
-            "select cocco.categoryoptioncomboid, o.organisationunitid, " +
-            "(select min(coalesce(dal.level + case when da.accepted then .0 else .1 end, 0)) from period p " +
-                "left join dataapproval da on da.workflowid = " + workflow.getId() + " and da.periodid = p.periodid " +
-                "left join dataapprovallevel dal on dal.dataapprovallevelid = da.dataapprovallevelid " +
-                "where '" + endDate + "' >= p.startdate and '" + endDate + "' <= p.enddate " +
-                "and da.attributeoptioncomboid = cocco.categoryoptioncomboid and " + highestApprovedOrgUnitCompare +
+            "select coc.uid as categoryoptioncombouid, o.organisationunituid, " +
+            "(select min(dal.level + case when da.accepted then .0 else .1 end) " +
+                "from dataapproval da " +
+                "join dataapprovallevel dal on dal.dataapprovallevelid = da.dataapprovallevelid " +
+                "where da.workflowid = " + workflow.getId() + " " +
+                "and da.periodid = " + getWorkflowPeriodId( workflow, endDate ) + " " +
+                "and da.attributeoptioncomboid = cocco.categoryoptioncomboid " +
+                "and " + highestApprovedOrgUnitCompare +
             ") as highest_approved, " +
             readyBelowSubquery + " as ready_below, " +
             approvedAboveSubquery + " as approved_above " +
-            "from categoryoptioncombos_categoryoptions cocco " +
+            "from categoryoptioncombo coc " +
+            "join categoryoptioncombos_categoryoptions cocco on cocco.categoryoptioncomboid = coc.categoryoptioncomboid " +
                 ( attributeCombo == null ? "" : "join categorycombos_optioncombos ccoc on ccoc.categoryoptioncomboid = cocco.categoryoptioncomboid " +
-                    " and ccoc.categorycomboid = " + attributeCombo.getId() + " " ) +
+                    "and ccoc.categorycomboid = " + attributeCombo.getId() + " " ) +
                 "join dataelementcategoryoption co on co.categoryoptionid = cocco.categoryoptionid " +
                     "and (co.startdate is null or co.startdate <= '" + endDate + "') and (co.enddate is null or co.enddate >= '" + startDate + "') " +
                 "join _orgunitstructure o on " + orgUnitJoinOn + " " +
@@ -378,8 +401,8 @@
 
         while ( rowSet.next() )
         {
-            final Integer aoc = rowSet.getInt( 1 );
-            final Integer ouId = rowSet.getInt( 2 );
+            final String aocUid = rowSet.getString( 1 );
+            final String ouUid = rowSet.getString( 2 );
             final Double highestApproved = rowSet.getDouble( 3 );
             final boolean readyBelow = rowSet.getBoolean( 4 );
             final boolean approvedAbove = rowSet.getBoolean( 5 );
@@ -387,21 +410,15 @@
             final int level = highestApproved == null ? 0 : highestApproved.intValue();
             final boolean accepted = ( highestApproved == level );
 
-            DataApprovalLevel statusLevel = ( level == 0 ? null : levelMap.get( level ) ); // null if not approved
-            DataApprovalLevel daLevel = ( statusLevel == null ? lowestApprovalLevelForOrgUnit : statusLevel );
-
-            DataElementCategoryOptionCombo optionCombo = aoc == null || aoc == 0 ? null : optionComboCache.get( aoc, () -> categoryService.getDataElementCategoryOptionCombo( aoc ) );
-
-            OrganisationUnit ou = orgUnit != null ? orgUnit : orgUnitCache.get( ouId, () -> organisationUnitService.getOrganisationUnit( ouId ) );
-
-            if ( ou != null )
+            DataApprovalLevel approvedLevel = ( level == 0 ? null : levelMap.get( level ) ); // null if not approved
+            DataApprovalLevel actionLevel = ( approvedLevel == null ? lowestApprovalLevelForOrgUnit : approvedLevel );
+
+            if ( ouUid != null )
             {
-                DataApproval da = new DataApproval( daLevel, workflow, period, ou, optionCombo, accepted, null, null );
-
                 DataApprovalState state = (
                     approvedAbove ?
                         APPROVED_ABOVE :
-                        statusLevel == null ?
+                        approvedLevel == null ?
                             lowestApprovalLevelForOrgUnit == null ?
                                 orgUnitLevelAbove == 0 ?
                                     UNAPPROVABLE :
@@ -413,15 +430,48 @@
                                 ACCEPTED_HERE :
                                 APPROVED_HERE );
     
-                statusList.add( new DataApprovalStatus( state, da, statusLevel, null ) );
+                statusList.add( new DataApprovalStatus( state, approvedLevel, actionLevel, ouUid, aocUid, accepted, null ) );
     
-                log.debug( "Get approval result: level " + level + " dataApprovalLevel " + ( daLevel != null ? daLevel.getLevel() : "[none]" )
-                    + " approved " + ( statusLevel != null )
+                log.debug( "Get approval result: level " + level + " dataApprovalLevel " + ( actionLevel != null ? actionLevel.getLevel() : "[none]" )
+                    + " approved " + ( approvedLevel != null )
                     + " readyBelow " + readyBelow + " approvedAbove " + approvedAbove
-                    + " accepted " + accepted + " state " + ( state != null ? state.name() : "[none]" ) + " " + da );
+                    + " accepted " + accepted + " state " + ( state != null ? state.name() : "[none]" )
+                    + " orgUnitUid " + ouUid
+                    + " aocUid " + aocUid );
             }
         }
 
         return statusList;
     }
+
+    /**
+     * Get the id for the workflow period that spans the given end date.
+     * The workflow period may or may not be the same as the period for which
+     * we are checking data validity. The workflow period will have a period
+     * type that matches the workflow period type, and it will contain the
+     * end date of the period for which we are checking data validity.
+     *
+     * Returns zero if there is no such workflow period.
+     *
+     * It turns out that this is much faster done as a separate query in
+     * postgresql than imbedding this as a subquery in the larger query above.
+     *
+     * @param workflow workflow we are checking
+     * @param endDate end date of the period we are checking approval for,
+     *                formatted as a string for a SQL query.
+     * @return id of the workflow period which overlaps with the endDate
+     */
+    private int getWorkflowPeriodId( DataApprovalWorkflow workflow, String endDate )
+    {
+        final String sql = "select periodid from period where '" + endDate + "' >= startdate and '" + endDate + "' <= enddate and periodtypeid = " + workflow.getPeriodType().getId();
+
+        SqlRowSet rowSet = jdbcTemplate.queryForRowSet( sql );
+
+        if ( rowSet.next() )
+        {
+            return rowSet.getInt( 1 );
+        }
+
+        return 0;
+    }
 }

=== modified file 'dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceCategoryOptionGroupTest.java'
--- dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceCategoryOptionGroupTest.java	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceCategoryOptionGroupTest.java	2016-01-11 03:14:37 +0000
@@ -317,10 +317,18 @@
         int chinaId = china.getId();
         int indiaId = india.getId();
 
+        String globalUid = global.getUid();
+        String americasUid = americas.getUid();
+        String asiaUid = asia.getUid();
+        String brazilUid = brazil.getUid();
+        String chinaUid = china.getUid();
+        String indiaUid = india.getUid();
+
         jdbcTemplate.execute(
                 "CREATE TABLE _orgunitstructure "+
                         "(" +
                         "  organisationunitid integer NOT NULL, " +
+                        "  organisationunituid character(11) NOT NULL, " +
                         "  level integer, " +
                         "  idlevel1 integer, " +
                         "  idlevel2 integer, " +
@@ -328,12 +336,12 @@
                         "  CONSTRAINT _orgunitstructure_pkey PRIMARY KEY (organisationunitid)" +
                         ");" );
 
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + globalId + ", 1, " + globalId + ", null, null);" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + americasId + ", 2, " + globalId + ", " + americasId + ", null);" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + asiaId + ", 2, " + globalId + ", " + asiaId + ", null);" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + brazilId + ", 3, " + globalId + ", " + americasId + ", " + brazilId + ");" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + chinaId + ", 3, " + globalId + ", " + asiaId + ", " + chinaId + ");" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + indiaId + ", 3, " + globalId + ", " + asiaId + ", " + indiaId + ");" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + globalId + ", '" + globalUid + "', 1, " + globalId + ", null, null);" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + americasId + ", '" + americasUid + "', 2, " + globalId + ", " + americasId + ", null);" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + asiaId + ", '" + asiaUid + "', 2, " + globalId + ", " + asiaId + ", null);" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + brazilId + ", '" + brazilUid + "', 3, " + globalId + ", " + americasId + ", " + brazilId + ");" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + chinaId + ", '" + chinaUid + "', 3, " + globalId + ", " + asiaId + ", " + chinaId + ");" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + indiaId + ", '" + indiaUid + "', 3, " + globalId + ", " + asiaId + ", " + indiaId + ");" );
 
         userA = createUser( 'A' );
         userService.addUser( userA );
@@ -539,6 +547,8 @@
 
         systemSettingManager.saveSystemSetting( SettingKey.HIDE_UNAPPROVED_DATA_IN_ANALYTICS, false );
         systemSettingManager.saveSystemSetting( SettingKey.ACCEPTANCE_REQUIRED_FOR_APPROVAL, false );
+
+        DataApprovalPermissionsEvaluator.invalidateCache();
     }
 
     // -------------------------------------------------------------------------
@@ -561,11 +571,11 @@
 
     private String getStatusString( DataApprovalStatus status )
     {
-        DataApproval da = status.getDataApproval();
-        String approval = da == null ? "approval=null" :
-                "ou=" + ( da.getOrganisationUnit() == null ? "(null)" : da.getOrganisationUnit().getName() )
-                        + " mechanism=" + ( da.getAttributeOptionCombo() == null ? "(null)" : da.getAttributeOptionCombo().getName() )
-                        + " level=" + ( da.getDataApprovalLevel() == null ? "(null)" : da.getDataApprovalLevel().getLevel() );
+        DataApprovalLevel dal = status.getActionLevel();
+        String approval = dal == null ? "approval=null" :
+                "ou=" + ( status.getOrganisationUnitUid() == null ? "(null)" : organisationUnitService.getOrganisationUnit( status.getOrganisationUnitUid() ).getName() )
+                        + " mechanism=" + ( status.getAttributeOptionComboUid() == null ? "(null)" : categoryService.getDataElementCategoryOptionCombo( status.getAttributeOptionComboUid() ).getName() )
+                        + " level=" + ( dal == null ? "(null)" : dal.getLevel() );
 
         DataApprovalPermissions p = status.getPermissions();
 

=== 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	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java	2016-01-11 03:14:37 +0000
@@ -303,17 +303,25 @@
 
         defaultCombo = categoryService.getDefaultDataElementCategoryOptionCombo();
 
-        int orgA = organisationUnitA.getId();
-        int orgB = organisationUnitB.getId();
-        int orgC = organisationUnitC.getId();
-        int orgD = organisationUnitD.getId();
-        int orgE = organisationUnitE.getId();
-        int orgF = organisationUnitF.getId();
+        int idA = organisationUnitA.getId();
+        int idB = organisationUnitB.getId();
+        int idC = organisationUnitC.getId();
+        int idD = organisationUnitD.getId();
+        int idE = organisationUnitE.getId();
+        int idF = organisationUnitF.getId();
+
+        String uidA = organisationUnitA.getUid();
+        String uidB = organisationUnitB.getUid();
+        String uidC = organisationUnitC.getUid();
+        String uidD = organisationUnitD.getUid();
+        String uidE = organisationUnitE.getUid();
+        String uidF = organisationUnitF.getUid();
 
         jdbcTemplate.execute(
                 "CREATE TABLE _orgunitstructure "+
                 "(" +
                 "  organisationunitid integer NOT NULL, " +
+                "  organisationunituid character(11) NOT NULL, " +
                 "  level integer, " +
                 "  idlevel1 integer, " +
                 "  idlevel2 integer, " +
@@ -322,12 +330,12 @@
                 "  CONSTRAINT _orgunitstructure_pkey PRIMARY KEY (organisationunitid)" +
                 ");" );
 
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + orgA + ", 1, " + orgA + ", null, null, null);" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + orgB + ", 2, " + orgA + ", " + orgB + ", null, null);" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + orgC + ", 3, " + orgA + ", " + orgB + ", " + orgC + ", null);" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + orgD + ", 4, " + orgA + ", " + orgB + ", " + orgC + ", " + orgD + ");" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + orgE + ", 3, " + orgA + ", " + orgB + ", " + orgE + ", null);" );
-        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + orgF + ", 4, " + orgA + ", " + orgB + ", " + orgE + ", " + orgF + ");" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + idA + ", '" + uidA + "', 1, " + idA + ", null, null, null);" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + idB + ", '" + uidB + "', 2, " + idA + ", " + idB + ", null, null);" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + idC + ", '" + uidC + "', 3, " + idA + ", " + idB + ", " + idC + ", null);" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + idD + ", '" + uidD + "', 4, " + idA + ", " + idB + ", " + idC + ", " + idD + ");" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + idE + ", '" + uidE + "', 3, " + idA + ", " + idB + ", " + idE + ", null);" );
+        jdbcTemplate.execute( "INSERT INTO _orgunitstructure VALUES (" + idF + ", '" + uidF + "', 4, " + idA + ", " + idB + ", " + idE + ", " + idF + ");" );
         
         systemSettingManager.invalidateCache();
     }
@@ -345,6 +353,8 @@
         setDependency( dataApprovalLevelService, "currentUserService", currentUserService, CurrentUserService.class );
         setDependency( organisationUnitService, "currentUserService", currentUserService, CurrentUserService.class );
         setDependency( dataApprovalStore, "currentUserService", currentUserService, CurrentUserService.class );
+
+        DataApprovalPermissionsEvaluator.invalidateCache();
     }
 
     private void setCurrentUserServiceDependencies( CurrentUserService mockCurrentUserService )
@@ -484,208 +494,195 @@
         DataApproval dataApprovalC = new DataApproval( level2, workflow12A, periodB, organisationUnitB, defaultCombo, NOT_ACCEPTED, date, userA );
         DataApproval dataApprovalD = new DataApproval( level2, workflow12B, periodA, organisationUnitB, defaultCombo, NOT_ACCEPTED, date, userA );
 
-        dataApprovalService.approveData( newArrayList(dataApprovalB, dataApprovalC, dataApprovalD) ); // Must be approved before A.
-        dataApprovalService.approveData( newArrayList(dataApprovalA) );
+        dataApprovalService.approveData( newArrayList( dataApprovalB, dataApprovalC, dataApprovalD ) ); // Must be approved before A.
+        dataApprovalService.approveData( newArrayList( dataApprovalA ) );
 
         DataApprovalStatus status = dataApprovalService.getDataApprovalStatus( workflow12A, periodA, organisationUnitA, defaultCombo );
         assertEquals( DataApprovalState.APPROVED_HERE, status.getState() );
-        DataApproval da = status.getDataApproval();
-        assertNotNull( da );
-        assertEquals( workflow12A, da.getWorkflow() );
-        assertEquals( periodA, da.getPeriod() );
-        assertEquals( organisationUnitA.getId(), da.getOrganisationUnit().getId() );
-        assertEquals( date, da.getCreated() );
-        assertEquals( userA.getId(), da.getCreator().getId() );
-        DataApprovalLevel level = status.getDataApprovalLevel();
+        assertNotNull( status );
+        assertEquals( organisationUnitA.getUid(), status.getOrganisationUnitUid() );
+        assertEquals( date, status.getCreated() );
+        assertEquals( userA.getId(), status.getCreator().getId() );
+        DataApprovalLevel level = status.getApprovedLevel();
         assertNotNull( level );
         assertEquals( level1, level );
 
         status = dataApprovalService.getDataApprovalStatus( workflow12A, periodA, organisationUnitB, defaultCombo );
         assertEquals( DataApprovalState.APPROVED_ABOVE, status.getState() );
-        da = status.getDataApproval();
-        assertNotNull( da );
-        assertEquals( workflow12A.getId(), da.getWorkflow().getId() );
-        assertEquals( periodA, da.getPeriod() );
-        assertEquals( organisationUnitB.getId(), da.getOrganisationUnit().getId() );
-        assertEquals( date, da.getCreated() );
-        assertEquals( userA.getId(), da.getCreator().getId() );
-        level = status.getDataApprovalLevel();
+        assertNotNull( status );
+        assertEquals( organisationUnitB.getUid(), status.getOrganisationUnitUid() );
+        assertEquals( date, status.getCreated() );
+        assertEquals( userA.getId(), status.getCreator().getId() );
+        level = status.getApprovedLevel();
         assertNotNull( level );
         assertEquals( level2, level );
 
         status = dataApprovalService.getDataApprovalStatus( workflow12A, periodB, organisationUnitB, defaultCombo );
         assertEquals( DataApprovalState.APPROVED_HERE, status.getState() );
-        da = status.getDataApproval();
-        assertNotNull( da );
-        assertEquals( workflow12A.getId(), da.getWorkflow().getId() );
-        assertEquals( periodB, da.getPeriod() );
-        assertEquals( organisationUnitB.getId(), da.getOrganisationUnit().getId() );
-        assertEquals( date, da.getCreated() );
-        assertEquals( userA.getId(), da.getCreator().getId() );
-        level = status.getDataApprovalLevel();
+        assertNotNull( status );
+        assertEquals( organisationUnitB.getUid(), status.getOrganisationUnitUid() );
+        assertEquals( date, status.getCreated() );
+        assertEquals( userA.getId(), status.getCreator().getId() );
+        level = status.getApprovedLevel();
         assertNotNull( level );
         assertEquals( level2, level );
 
         status = dataApprovalService.getDataApprovalStatus( workflow12B, periodA, organisationUnitB, defaultCombo );
         assertEquals( DataApprovalState.APPROVED_HERE, status.getState() );
-        da = status.getDataApproval();
-        assertNotNull( da );
-        assertEquals( workflow12B.getId(), da.getWorkflow().getId() );
-        assertEquals( periodA, da.getPeriod() );
-        assertEquals( organisationUnitB.getId(), da.getOrganisationUnit().getId() );
-        assertEquals( date, da.getCreated() );
-        assertEquals( userA.getId(), da.getCreator().getId() );
-        level = status.getDataApprovalLevel();
+        assertNotNull( status );
+        assertEquals( organisationUnitB.getUid(), status.getOrganisationUnitUid() );
+        assertEquals( date, status.getCreated() );
+        assertEquals( userA.getId(), status.getCreator().getId() );
+        level = status.getApprovedLevel();
         assertNotNull( level );
         assertEquals( level2, level );
 
         status = dataApprovalService.getDataApprovalStatus( workflow12B, periodB, organisationUnitB, defaultCombo );
         assertEquals( DataApprovalState.UNAPPROVED_READY, status.getState() );
-        assertNotNull( status.getDataApproval() );
-        level = status.getDataApprovalLevel();
+        level = status.getApprovedLevel();
         assertNull( level );
     }
 
-    @Ignore //TODO Fails with DataMayNotBeApproved
-    @Test
-    public void testAddDuplicateDataApproval()
-    {
-        Set<OrganisationUnit> units = newHashSet( organisationUnitA );
-
-        CurrentUserService currentUserService = new MockCurrentUserService( units, null, DataApproval.AUTH_APPROVE, DataApproval.AUTH_APPROVE_LOWER_LEVELS );
-        setCurrentUserServiceDependencies( currentUserService );
-
-        Date date = new Date();
-        DataApproval dataApprovalA = new DataApproval( level1, workflow12, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA );
-        DataApproval dataApprovalB = new DataApproval( level1, workflow12, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA ); // Same
-
-        dataApprovalService.approveData( newArrayList( dataApprovalA ) );
-        dataApprovalService.approveData( newArrayList( dataApprovalB ) ); // Redundant, so call is ignored.
-    }
-
-    @Test
-    @Ignore
-    public void testDeleteDataApproval()
-    {
-        Set<OrganisationUnit> units = newHashSet( organisationUnitA );
-
-        CurrentUserService currentUserService = new MockCurrentUserService( units, null, DataApproval.AUTH_APPROVE, DataApproval.AUTH_APPROVE_LOWER_LEVELS );
-        setCurrentUserServiceDependencies( currentUserService );
-
-        Date date = new Date();
-        DataApproval dataApprovalA = new DataApproval( level1, workflow12, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA );
-        DataApproval dataApprovalB = new DataApproval( level2, workflow12, periodA, organisationUnitB, defaultCombo, NOT_ACCEPTED, date, userB );
-
-        dataApprovalService.approveData( newArrayList( dataApprovalB ) );
-        dataApprovalService.approveData( newArrayList( dataApprovalA ) );
-
-        assertTrue( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitA, defaultCombo ).getState().isApproved() );
-        assertTrue( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitB, defaultCombo ).getState().isApproved() );
-
-        dataApprovalService.unapproveData( newArrayList( dataApprovalA ) ); // Only A should be deleted.
-
-        assertFalse( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitA, defaultCombo ).getState().isApproved() );
-        assertTrue( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitB, defaultCombo ).getState().isApproved() );
-
-        dataApprovalService.unapproveData( newArrayList( dataApprovalB ) );
-
-        assertFalse( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitA, defaultCombo ).getState().isApproved() );
-        assertFalse( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitB, defaultCombo ).getState().isApproved() );
-    }
-
-    @Test
-    public void testGetDataApprovalState()
-    {
-        Set<OrganisationUnit> units = newHashSet( organisationUnitA );
-
-        CurrentUserService currentUserService = new MockCurrentUserService( units, null, DataApproval.AUTH_APPROVE, DataApproval.AUTH_APPROVE_LOWER_LEVELS );
-        setCurrentUserServiceDependencies( currentUserService );
-
-        // No levels defined.
-        assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitF, defaultCombo ).getState() );
-
-        // Levels defined.
-
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
-
-        Date date = new Date();
-
-        // Approved for organisation unit F
-        DataApproval dataApprovalF = new DataApproval( level4, workflow1234, periodA, organisationUnitF, defaultCombo, NOT_ACCEPTED, date, userA );
-        dataApprovalService.approveData( newArrayList( dataApprovalF ) );
-
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
-
-        // Also approved also for organisation unit E
-        DataApproval dataApprovalE = new DataApproval( level3, workflow1234, periodA, organisationUnitE, defaultCombo, NOT_ACCEPTED, date, userA );
-        dataApprovalService.approveData( newArrayList( dataApprovalE ) );
-
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
-
-        // Also approved for organisation unit D
-        DataApproval dataApprovalD = new DataApproval( level4, workflow1234, periodA, organisationUnitD, defaultCombo, NOT_ACCEPTED, date, userA );
-        dataApprovalService.approveData( newArrayList( dataApprovalD ) );
-
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
-
-        // Also approved for organisation unit C
-        DataApproval dataApprovalC = new DataApproval( level3, workflow1234, periodA, organisationUnitC, defaultCombo, NOT_ACCEPTED, date, userA );
-        dataApprovalService.approveData( newArrayList( dataApprovalC ) );
-
-        assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
-
-        // Also approved for organisation unit B
-        DataApproval dataApprovalB = new DataApproval( level2, workflow1234, periodA, organisationUnitB, defaultCombo, NOT_ACCEPTED, date, userA );
-        dataApprovalService.approveData( newArrayList( dataApprovalB ) );
-
-        assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
-
-        // Also approved for organisation unit A
-        DataApproval dataApprovalA = new DataApproval( level1, workflow1234, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA );
-        dataApprovalService.approveData( newArrayList( dataApprovalA ) );
-
-        assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
-        assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
-    }
+        @Ignore //TODO Fails with DataMayNotBeApproved
+        @Test
+        public void testAddDuplicateDataApproval()
+        {
+            Set<OrganisationUnit> units = newHashSet( organisationUnitA );
+
+            CurrentUserService currentUserService = new MockCurrentUserService( units, null, DataApproval.AUTH_APPROVE, DataApproval.AUTH_APPROVE_LOWER_LEVELS );
+            setCurrentUserServiceDependencies( currentUserService );
+
+            Date date = new Date();
+            DataApproval dataApprovalA = new DataApproval( level1, workflow12, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA );
+            DataApproval dataApprovalB = new DataApproval( level1, workflow12, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA ); // Same
+
+            dataApprovalService.approveData( newArrayList( dataApprovalA ) );
+            dataApprovalService.approveData( newArrayList( dataApprovalB ) ); // Redundant, so call is ignored.
+        }
+
+        @Test
+        @Ignore
+        public void testDeleteDataApproval()
+        {
+            Set<OrganisationUnit> units = newHashSet( organisationUnitA );
+
+            CurrentUserService currentUserService = new MockCurrentUserService( units, null, DataApproval.AUTH_APPROVE, DataApproval.AUTH_APPROVE_LOWER_LEVELS );
+            setCurrentUserServiceDependencies( currentUserService );
+
+            Date date = new Date();
+            DataApproval dataApprovalA = new DataApproval( level1, workflow12, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA );
+            DataApproval dataApprovalB = new DataApproval( level2, workflow12, periodA, organisationUnitB, defaultCombo, NOT_ACCEPTED, date, userB );
+
+            dataApprovalService.approveData( newArrayList( dataApprovalB ) );
+            dataApprovalService.approveData( newArrayList( dataApprovalA ) );
+
+            assertTrue( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitA, defaultCombo ).getState().isApproved() );
+            assertTrue( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitB, defaultCombo ).getState().isApproved() );
+
+            dataApprovalService.unapproveData( newArrayList( dataApprovalA ) ); // Only A should be deleted.
+
+            assertFalse( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitA, defaultCombo ).getState().isApproved() );
+            assertTrue( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitB, defaultCombo ).getState().isApproved() );
+
+            dataApprovalService.unapproveData( newArrayList( dataApprovalB ) );
+
+            assertFalse( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitA, defaultCombo ).getState().isApproved() );
+            assertFalse( dataApprovalService.getDataApprovalStatus( workflow12, periodA, organisationUnitB, defaultCombo ).getState().isApproved() );
+        }
+
+        @Test
+        public void testGetDataApprovalState()
+        {
+            Set<OrganisationUnit> units = newHashSet( organisationUnitA );
+
+            CurrentUserService currentUserService = new MockCurrentUserService( units, null, DataApproval.AUTH_APPROVE, DataApproval.AUTH_APPROVE_LOWER_LEVELS );
+            setCurrentUserServiceDependencies( currentUserService );
+
+            // No levels defined.
+            assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVABLE, dataApprovalService.getDataApprovalStatus( workflow0, periodA, organisationUnitF, defaultCombo ).getState() );
+
+            // Levels defined.
+
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
+
+            Date date = new Date();
+
+            // Approved for organisation unit F
+            DataApproval dataApprovalF = new DataApproval( level4, workflow1234, periodA, organisationUnitF, defaultCombo, NOT_ACCEPTED, date, userA );
+            dataApprovalService.approveData( newArrayList( dataApprovalF ) );
+
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
+
+            // Also approved also for organisation unit E
+            DataApproval dataApprovalE = new DataApproval( level3, workflow1234, periodA, organisationUnitE, defaultCombo, NOT_ACCEPTED, date, userA );
+            dataApprovalService.approveData( newArrayList( dataApprovalE ) );
+
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
+
+            // Also approved for organisation unit D
+            DataApproval dataApprovalD = new DataApproval( level4, workflow1234, periodA, organisationUnitD, defaultCombo, NOT_ACCEPTED, date, userA );
+            dataApprovalService.approveData( newArrayList( dataApprovalD ) );
+
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
+
+            // Also approved for organisation unit C
+            DataApproval dataApprovalC = new DataApproval( level3, workflow1234, periodA, organisationUnitC, defaultCombo, NOT_ACCEPTED, date, userA );
+            dataApprovalService.approveData( newArrayList( dataApprovalC ) );
+
+            assertEquals( DataApprovalState.UNAPPROVED_WAITING, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
+
+            // Also approved for organisation unit B
+            DataApproval dataApprovalB = new DataApproval( level2, workflow1234, periodA, organisationUnitB, defaultCombo, NOT_ACCEPTED, date, userA );
+            dataApprovalService.approveData( newArrayList( dataApprovalB ) );
+
+            assertEquals( DataApprovalState.UNAPPROVED_READY, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
+
+            // Also approved for organisation unit A
+            DataApproval dataApprovalA = new DataApproval( level1, workflow1234, periodA, organisationUnitA, defaultCombo, NOT_ACCEPTED, date, userA );
+            dataApprovalService.approveData( newArrayList( dataApprovalA ) );
+
+            assertEquals( DataApprovalState.APPROVED_HERE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitA, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitB, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitC, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitD, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitE, defaultCombo ).getState() );
+            assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflow1234, periodA, organisationUnitF, defaultCombo ).getState() );
+        }
 
     @Test
     public void testGetDataApprovalStateAbove()
@@ -1397,11 +1394,11 @@
         assertEquals( DataApprovalState.UNAPPROVED_ABOVE, dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getState() );
 
         assertEquals( level1EFGH, dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getState() );
-        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, defaultCombo ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getDataApprovalLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, defaultCombo ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatusAndPermissions( workflowA, periodA, organisationUnitA, null ).getApprovedLevel() );
 
         assertEquals( DataApprovalState.UNAPPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, defaultCombo ).getState() );
         assertEquals( DataApprovalState.UNAPPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getState() );
@@ -1410,12 +1407,12 @@
         assertEquals( DataApprovalState.UNAPPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getState() );
         assertEquals( DataApprovalState.UNAPPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, optionComboAE ).getState() );
 
-        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, optionComboAE ).getDataApprovalLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertNull( dataApprovalService.getDataApprovalStatus( workflowA, periodA, organisationUnitB, optionComboAE ).getApprovedLevel() );
 
         dataApprovalLevelService.addDataApprovalLevel( level1ABCD );
 
@@ -1431,12 +1428,12 @@
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, optionComboAE ).getState() );
 
-        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1EFGH, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, optionComboAE ).getDataApprovalLevel() );
+        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1EFGH, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, optionComboAE ).getApprovedLevel() );
 
         assertEquals( DataApprovalState.UNAPPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, defaultCombo ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getState() );
@@ -1445,12 +1442,12 @@
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, optionComboAE ).getState() );
 
-        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, optionComboAE ).getDataApprovalLevel() );
+        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, optionComboAE ).getApprovedLevel() );
 
         dataApprovalService.unapproveData( newArrayList( dab ) );
 
@@ -1461,12 +1458,12 @@
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, optionComboAE ).getState() );
 
-        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1EFGH, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, optionComboAE ).getDataApprovalLevel() );
+        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1EFGH, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitA, optionComboAE ).getApprovedLevel() );
 
         assertEquals( DataApprovalState.UNAPPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, defaultCombo ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getState() );
@@ -1475,12 +1472,12 @@
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, optionComboAE ).getState() );
 
-        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, optionComboAE ).getDataApprovalLevel() );
+        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( null, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1ABCD, dataApprovalService.getDataApprovalStatus( workflowB, periodA, organisationUnitB, optionComboAE ).getApprovedLevel() );
 
         dataApprovalLevelService.addDataApprovalLevel( level1 );
 
@@ -1496,12 +1493,12 @@
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, optionComboAE ).getState() );
 
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, optionComboAE ).getDataApprovalLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitA, optionComboAE ).getApprovedLevel() );
 
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, defaultCombo ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getState() );
@@ -1510,12 +1507,12 @@
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getState() );
         assertEquals( DataApprovalState.APPROVED_ABOVE, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, optionComboAE ).getState() );
 
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getDataApprovalLevel() );
-        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, optionComboAE ).getDataApprovalLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, null ).getApprovedLevel() );
+        assertEquals( level1, dataApprovalService.getDataApprovalStatus( workflowC, periodA, organisationUnitB, optionComboAE ).getApprovedLevel() );
     }
 
     @Ignore //TODO get this test working
@@ -1659,7 +1656,7 @@
         DataApprovalPermissions permissions = status.getPermissions();
 
         return status.getState().toString()
-                + " level=" + ( status.getDataApprovalLevel() == null ? "null" : status.getDataApprovalLevel().getLevel() )
+                + " level=" + ( status.getApprovedLevel() == null ? "null" : status.getApprovedLevel().getLevel() )
                 + " approve=" + ( permissions.isMayApprove() ? "T" : "F" )
                 + " unapprove=" + ( permissions.isMayUnapprove() ? "T" : "F" )
                 + " accept=" + ( permissions.isMayAccept() ? "T" : "F" )

=== modified file 'dhis-2/dhis-web/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/DataApprovalController.java'
--- dhis-2/dhis-web/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/DataApprovalController.java	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-web/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/DataApprovalController.java	2016-01-11 03:14:37 +0000
@@ -237,9 +237,8 @@
         DataApprovalStatus status = dataApprovalService.getDataApprovalStatusAndPermissions( dataSet.getWorkflow(), period,
             organisationUnit, optionCombo );
 
-        DataApproval dataApproval = status.getDataApproval();
-        Date createdDate = dataApproval == null ? null : dataApproval.getCreated();
-        String createdByUsername = dataApproval == null || dataApproval.getCreator() == null ? null : dataApproval.getCreator().getUsername();
+        Date createdDate = status.getCreated();
+        String createdByUsername = status.getCreator() == null ? null : status.getCreator().getUsername();
 
         String state = status.getState().toString();
 
@@ -265,6 +264,11 @@
 
         OrganisationUnit orgUnit = organisationUnitService.getOrganisationUnit( ou );
 
+        if ( orgUnit != null && orgUnit.isRoot() )
+        {
+            orgUnit = null; // Look for all org units.
+        }
+
         SetMap<DataApprovalWorkflow, DataElementCategoryCombo> workflowCategoryComboMap = new SetMap<>();
 
         for ( DataSet dataSet : dataSets )
@@ -288,26 +292,21 @@
         {
             Map<String, Object> item = new HashMap<>();
 
-            DataApproval approval = status.getDataApproval();
-
             Map<String, String> approvalLevel = new HashMap<>();
 
-            if ( status.getDataApprovalLevel() != null )
-            {
-                approvalLevel.put( "id", status.getDataApprovalLevel().getUid() );
-                approvalLevel.put( "level", String.valueOf( status.getDataApprovalLevel().getLevel() ) );
-            }
-
-            if ( approval != null )
-            {
-                item.put( "id", approval.getAttributeOptionCombo().getUid() );
-                item.put( "level", approvalLevel );
-                item.put( "ou", approval.getOrganisationUnit().getUid() );
-                item.put( "accepted", approval.isAccepted() );
-                item.put( "permissions", status.getPermissions() );
-
-                list.add( item );
-            }
+            if ( status.getApprovedLevel() != null )
+            {
+                approvalLevel.put( "id", status.getApprovedLevel().getUid() );
+                approvalLevel.put( "level", String.valueOf( status.getApprovedLevel().getLevel() ) );
+            }
+
+            item.put( "id", status.getAttributeOptionComboUid() );
+            item.put( "level", approvalLevel );
+            item.put( "ou", status.getOrganisationUnitUid() );
+            item.put( "accepted", status.isAccepted() );
+            item.put( "permissions", status.getPermissions() );
+
+            list.add( item );
         }
 
         renderService.toJson( response.getOutputStream(), list );