------------------------------------------------------------
revno: 16297
committer: jimgr...@gmail.com
branch nick: dhis2
timestamp: Sun 2014-08-03 18:03:34 -0400
message:
Fix data approval acceptance permissions bug.
modified:
dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java
dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java
dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java
dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java
--
lp:dhis2
https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk
Your team DHIS 2 developers is subscribed to branch lp:dhis2.
To unsubscribe from this branch go to
https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk/+edit-subscription
=== modified file 'dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java'
--- dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java 2014-04-28 10:17:37 +0000
+++ dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalLevelService.java 2014-08-03 22:03:34 +0000
@@ -63,6 +63,14 @@
DataApprovalLevel getDataApprovalLevelByName( String name );
/**
+ * Gets the data approval level with the given level number.
+ *
+ * @param levelNumber number of the level to return.
+ * @return a data approval level.
+ */
+ DataApprovalLevel getDataApprovalLevelByLevelNumber( int levelNumber );
+
+ /**
* Gets a list of all data approval levels.
*
* @return List of all data approval levels, ordered from 1 to n.
=== modified file 'dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java'
--- dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java 2014-07-18 11:51:12 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalLevelService.java 2014-08-03 22:03:34 +0000
@@ -101,6 +101,20 @@
return dataApprovalLevelStore.getByName( name );
}
+ public DataApprovalLevel getDataApprovalLevelByLevelNumber( int levelNumber )
+ {
+ List<DataApprovalLevel> dataApprovalLevels = getAllDataApprovalLevels();
+
+ if ( levelNumber < 1 || levelNumber > dataApprovalLevels.size() )
+ {
+ return null;
+ }
+ else
+ {
+ return dataApprovalLevels.get( levelNumber - 1 );
+ }
+ }
+
public List<DataApprovalLevel> getAllDataApprovalLevels()
{
List<DataApprovalLevel> dataApprovalLevels = dataApprovalLevelStore.getAllDataApprovalLevels();
=== modified file 'dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java'
--- dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java 2014-06-10 20:46:05 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java 2014-08-03 22:03:34 +0000
@@ -146,7 +146,7 @@
public void deleteDataApproval( DataApproval dataApproval )
{
if ( ( dataApproval.getCategoryOptionGroup() == null || securityService.canRead( dataApproval.getCategoryOptionGroup() ) )
- && mayUnapprove( dataApproval.getOrganisationUnit(), dataApproval.isAccepted() ) )
+ && mayUnapprove( dataApproval ) )
{
PeriodType selectionPeriodType = dataApproval.getPeriod().getPeriodType();
PeriodType dataSetPeriodType = dataApproval.getDataSet().getPeriodType();
@@ -178,7 +178,7 @@
}
else
{
- warnNotPermitted( dataApproval, "unapprove", mayUnapprove( dataApproval.getOrganisationUnit(), dataApproval.isAccepted() ) );
+ warnNotPermitted( dataApproval, "unapprove", mayUnapprove( dataApproval ) );
}
}
@@ -230,20 +230,21 @@
&& ( dataApprovalLevel.getCategoryOptionGroupSet() == null || securityService.canRead( dataApprovalLevel.getCategoryOptionGroupSet() ))
&& canReadOneCategoryOptionGroup( categoryOptionGroups ) )
{
- boolean unacceptPermissionNeededToUnapprove = false;
-
switch ( status.getDataApprovalState() )
{
case PARTIALLY_ACCEPTED_HERE:
case ACCEPTED_HERE:
- unacceptPermissionNeededToUnapprove = true;
case PARTIALLY_APPROVED_HERE:
case APPROVED_HERE:
+ permissions.setMayApprove( mayApprove( organisationUnit ) );
+ permissions.setMayUnapprove( mayUnapprove( status.getDataApproval() ) );
+ permissions.setMayAccept( mayAcceptOrUnaccept( status.getDataApproval() ) );
+ permissions.setMayUnaccept( permissions.isMayAccept() );
+ break;
+
case UNAPPROVED_READY:
permissions.setMayApprove( mayApprove( organisationUnit ) );
- permissions.setMayUnapprove( mayUnapprove( organisationUnit, unacceptPermissionNeededToUnapprove ) );
- permissions.setMayAccept( mayAcceptOrUnaccept( organisationUnit ) );
- permissions.setMayUnaccept( permissions.isMayAccept() );
+ permissions.setMayUnapprove( isAuthorizedToUnapprove( organisationUnit ) );
break;
}
}
@@ -281,7 +282,7 @@
public void acceptOrUnaccept ( DataApproval dataApproval, boolean accepted )
{
if ( ( dataApproval.getCategoryOptionGroup() == null || securityService.canRead( dataApproval.getCategoryOptionGroup() ) )
- && mayAcceptOrUnaccept( dataApproval.getOrganisationUnit() ) )
+ && mayAcceptOrUnaccept( dataApproval ) )
{
PeriodType selectionPeriodType = dataApproval.getPeriod().getPeriodType();
PeriodType dataSetPeriodType = dataApproval.getDataSet().getPeriodType();
@@ -300,7 +301,7 @@
}
} else
{
- warnNotPermitted( dataApproval, accepted ? "accept" : "unaccept", mayAcceptOrUnaccept( dataApproval.getOrganisationUnit() ) );
+ warnNotPermitted( dataApproval, accepted ? "accept" : "unaccept", mayAcceptOrUnaccept( dataApproval ) );
}
}
@@ -401,7 +402,7 @@
* @param categoryOptionGroups option groups (if any) for data selection
* @return true if at most 1 option group and user can read, else false
*/
- boolean canReadOneCategoryOptionGroup( Set<CategoryOptionGroup> categoryOptionGroups )
+ boolean canReadOneCategoryOptionGroup( Collection<CategoryOptionGroup> categoryOptionGroups )
{
if ( categoryOptionGroups == null || categoryOptionGroups.size() == 0 )
{
@@ -417,6 +418,30 @@
}
/**
+ * Return true if there are no category option groups, or if the user
+ * can read any category option group from the collection.
+ *
+ * @param categoryOptionGroups option groups (if any) for data selection
+ * @return true if at most 1 option group and user can read, else false
+ */
+ boolean canReadSomeCategoryOptionGroup( Collection<CategoryOptionGroup> categoryOptionGroups )
+ {
+ if ( categoryOptionGroups == null )
+ {
+ return true;
+ }
+
+ for ( CategoryOptionGroup cog : categoryOptionGroups )
+ {
+ if ( securityService.canRead( cog ) )
+ {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
* Checks to see whether a user may approve data for a given
* organisation unit.
*
@@ -457,7 +482,7 @@
}
/**
- * Checks to see whether a user may unapprove for a given organisation unit.
+ * Checks to see whether a user may unapprove a data approval.
* <p>
* A user may unapprove data for organisation unit A if they have the
* authority to approve data for organisation unit B, and B is an
@@ -471,56 +496,72 @@
* has been approved already at a higher level for the same period and
* data set, and the user is not authorized to remove that approval as well.
*
- * @param organisationUnit The data approval status to check for permission.
- * @param unacceptPermissionNeededToUnapprove Whether *unaccept* permission
- * is also needed to unapprove this data.
+ * @param dataApproval the data approval object for the attempted operation.
* @return true if the user may unapprove, otherwise false
*/
- private boolean mayUnapprove( OrganisationUnit organisationUnit, boolean unacceptPermissionNeededToUnapprove )
+ private boolean mayUnapprove( DataApproval dataApproval )
{
- if ( isAuthorizedToUnapprove( organisationUnit ) )
+ if ( isAuthorizedToUnapprove( dataApproval.getOrganisationUnit() ) )
{
- if ( !unacceptPermissionNeededToUnapprove || mayAcceptOrUnaccept( organisationUnit ) )
+ if ( !dataApproval.isAccepted() || mayAcceptOrUnaccept( dataApproval ) )
{
- log.debug( "mayUnapprove = true for organisation unit " + organisationUnit.getName() );
-
return true;
}
}
- log.debug( "mayUnapprove = false for organisation unit " + organisationUnit.getName() );
-
return false;
}
/**
- * Checks to see whether a user may accept or unaccept for a given
- * organisation unit.
+ * Checks to see whether a user may accept or unaccept an approval.
*
- * @param organisationUnit The organisation unit to check for permission.
+ * @param dataApproval The approval to check for permission.
* @return true if the user may accept or unaccept, otherwise false.
*/
- private boolean mayAcceptOrUnaccept ( OrganisationUnit organisationUnit )
+ private boolean mayAcceptOrUnaccept ( DataApproval dataApproval )
{
+ OrganisationUnit organisationUnit = null;
+
User user = currentUserService.getCurrentUser();
- if ( user != null )
+ if ( dataApproval != null && user != null )
{
boolean mayAcceptAtLowerLevels = user.getUserCredentials().isAuthorized( DataApproval.AUTH_ACCEPT_LOWER_LEVELS );
- if ( mayAcceptAtLowerLevels && CollectionUtils.containsAny( user.getOrganisationUnits(),
- organisationUnit.getAncestors() ) )
+ organisationUnit = dataApproval.getOrganisationUnit();
+
+ DataApprovalLevel dataApprovalLevel = dataApproval.getDataApprovalLevel();
+
+ if ( mayAcceptAtLowerLevels && organisationUnit != null && dataApprovalLevel != null && dataApprovalLevel.getLevel() > 1 )
{
- log.debug( "User may accept or unaccept for organisation unit " + organisationUnit.getName() );
-
- return true;
+ DataApprovalLevel acceptLevel = dataApprovalLevelService.getDataApprovalLevelByLevelNumber( dataApprovalLevel.getLevel() - 1 );
+
+ if ( securityService.canRead( acceptLevel )
+ && ( acceptLevel.getCategoryOptionGroupSet() == null ||
+ ( securityService.canRead( acceptLevel.getCategoryOptionGroupSet() )
+ && canReadSomeCategoryOptionGroup( acceptLevel.getCategoryOptionGroupSet().getMembers() ) ) ) )
+ {
+ OrganisationUnit acceptOrgUnit = dataApproval.getOrganisationUnit();
+ for ( int i = acceptLevel.getOrgUnitLevel(); i < dataApprovalLevel.getOrgUnitLevel(); i++ )
+ {
+ acceptOrgUnit = acceptOrgUnit.getParent();
+ }
+
+ if ( user.getOrganisationUnits().contains( acceptOrgUnit ) ||
+ CollectionUtils.containsAny( user.getOrganisationUnits(), acceptOrgUnit.getAncestors() ) )
+ {
+ log.debug( "User may accept or unaccept for organisation unit " + organisationUnit.getName() );
+
+ return true;
+ }
+ }
}
}
log.debug( "User with AUTH_ACCEPT_LOWER_LEVELS " + user.getUserCredentials().isAuthorized( DataApproval.AUTH_ACCEPT_LOWER_LEVELS )
+ " with " + user.getOrganisationUnits().size() + " org units"
- + " may not accept or unaccept for organisation unit " + organisationUnit.getName()
- + " with " + organisationUnit.getAncestors().size() + " ancestors." );
+ + " may not accept or unaccept for organisation unit "
+ + ( organisationUnit == null ? "(null)" : organisationUnit.getName() ) );
return false;
}
=== modified file 'dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java'
--- dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java 2014-07-18 11:51:12 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceTest.java 2014-08-03 22:03:34 +0000
@@ -1092,6 +1092,7 @@
@Test
public void testMultiPeriodApproval() throws Exception
{
+ dataApprovalLevelService.addDataApprovalLevel( level1 );
dataApprovalLevelService.addDataApprovalLevel( level2 );
dataSetA.setApproveData( true );
_______________________________________________
Mailing list: https://launchpad.net/~dhis2-devs
Post to : dhis2-devs@lists.launchpad.net
Unsubscribe : https://launchpad.net/~dhis2-devs
More help : https://help.launchpad.net/ListHelp
