=== 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 2015-04-15 11:58:48 +0000 +++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/DefaultDataApprovalService.java 2015-04-29 19:13:26 +0000 @@ -141,8 +141,8 @@ da.setDataApprovalLevel( status.getDataApproval().getDataApprovalLevel() ); } } - - if ( status != null && status.getState().isApproved() && + + if ( status != null && status.getState().isApproved() && da.getDataApprovalLevel() != null && da.getDataApprovalLevel().getLevel() >= status.getDataApprovalLevel().getLevel() ) { continue; // Already approved at or above this level @@ -156,6 +156,15 @@ throw new DataMayNotBeApprovedException(); } + if ( organisationUnitService.getLevelOfOrganisationUnit( da.getOrganisationUnit() ) != da.getDataApprovalLevel().getOrgUnitLevel() ) + { + log.warn( "approveData: org unit " + da.getOrganisationUnit().getUid() + " '" + da.getOrganisationUnit().getName() + + "' has wrong org unit level " + organisationUnitService.getLevelOfOrganisationUnit( da.getOrganisationUnit() ) + + " for approval level " + da.getDataApprovalLevel().getLevel()); + + throw new DataMayNotBeApprovedException(); + } + checkedList.add ( da ); } @@ -196,8 +205,8 @@ throw new DataMayNotBeUnapprovedException(); } - if ( !status.getState().isApproved() || - da.getDataApprovalLevel().getLevel() < status.getDataApprovalLevel().getLevel() ) + if ( !status.getState().isApproved() || ( da.getDataApprovalLevel() != null && + da.getDataApprovalLevel().getLevel() < status.getDataApprovalLevel().getLevel() ) ) { continue; // Already unapproved at or below this level } @@ -306,7 +315,7 @@ da.setDataApprovalLevel( status.getDataApproval().getDataApprovalLevel() ); } - if ( status == null || ( !status.getState().isAccepted() && da.getDataApprovalLevel().getLevel() == status.getDataApprovalLevel().getLevel() ) || + if ( status == null || ( !status.getState().isAccepted() && da.getDataApprovalLevel() != null && da.getDataApprovalLevel().getLevel() == status.getDataApprovalLevel().getLevel() ) || da.getDataApprovalLevel().getLevel() < status.getDataApprovalLevel().getLevel() ) { continue; // Already unaccepted at or not approved up to this level === 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 2015-03-31 00:55:04 +0000 +++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/hibernate/HibernateDataApprovalStore.java 2015-04-29 19:13:26 +0000 @@ -275,21 +275,43 @@ categoryComboIds = TextUtils.getCommaDelimitedString( catComboIds ); } - int orgUnitLevel = 1; + List approvalLevels = dataApprovalLevelService.getAllDataApprovalLevels(); + + if ( CollectionUtils.isEmpty( approvalLevels ) ) + { + log.warn( " No approval levels configured." ); + + return new ArrayList<>(); // Unapprovable. + } + + int orgUnitLevel; String orgUnitJoinOn; + String highestApprovedOrgUnitCompare; if ( orgUnit != null ) { orgUnitLevel = organisationUnitService.getLevelOfOrganisationUnit( orgUnit ); orgUnitJoinOn = "o.organisationunitid = " + orgUnit.getId(); + highestApprovedOrgUnitCompare = "da.organisationunitid = o.organisationunitid "; } else { - for ( DataApprovalLevel dal : dataApprovalLevelService.getAllDataApprovalLevels() ) + highestApprovedOrgUnitCompare = ""; + orgUnitLevel = 0; + + for ( DataApprovalLevel dal : approvalLevels ) { - orgUnitLevel = dal.getOrgUnitLevel(); // Get lowest (last level -> greatest number) level. + if ( dal.getOrgUnitLevel() != orgUnitLevel ) + { + orgUnitLevel = dal.getOrgUnitLevel(); // Remember lowest (last level -> greatest number) level. + + highestApprovedOrgUnitCompare += ( highestApprovedOrgUnitCompare.length() == 0 ? "(" : " or" ) + + " da.organisationunitid = o.idlevel" + orgUnitLevel; + } } - + + highestApprovedOrgUnitCompare += ") "; + orgUnitJoinOn = "o.level = " + orgUnitLevel; } @@ -314,17 +336,15 @@ int orgUnitLevelAbove = 0; - List approvalLevels = dataApprovalLevelService.getAllDataApprovalLevels(); - - if ( CollectionUtils.isEmpty( approvalLevels ) ) - { - log.warn( " No approval levels configured." ); - - return new ArrayList<>(); // Unapprovable. - } + int highestApprovalOrgUnitLevel = 99999999; for ( DataApprovalLevel dal : approvalLevels ) { + if ( dal.getOrgUnitLevel() < highestApprovalOrgUnitLevel ) + { + highestApprovalOrgUnitLevel = dal.getOrgUnitLevel(); + } + if ( dal.getOrgUnitLevel() < orgUnitLevel ) { orgUnitLevelAbove = dal.getOrgUnitLevel(); // Keep getting the lowest org unit level above ours. @@ -337,6 +357,16 @@ if ( dal.getOrgUnitLevel() > orgUnitLevel ) // If there is a lower (higher number) approval orgUnit level. { + String coOrgUnitConstraint = ""; // Constrain lower level orgUnit by categoryOption orgUnit(s) (if any). + + if ( !isDefaultCombo ) + { + for ( int i = 1; i <= dal.getOrgUnitLevel(); i++ ) + { + coOrgUnitConstraint += ( i == 1 ? "" : "or " ) + "( o2.level = " + i + " and ous.idlevel" + i + " = c_o.organisationunitid ) "; + } + } + boolean acceptanceRequiredForApproval = (Boolean) systemSettingManager.getSystemSetting( KEY_ACCEPTANCE_REQUIRED_FOR_APPROVAL, false ); readyBelowSubquery = "not exists (select 1 from _orgunitstructure ous " + @@ -348,14 +378,20 @@ ( acceptanceRequiredForApproval ? "and da.accepted " : "" ) + ") " + "and ous.idlevel" + orgUnitLevel + " = o.organisationunitid " + - "and ous.level = " + dal.getOrgUnitLevel() + ") "; + "and ous.level = " + dal.getOrgUnitLevel() + " " + + ( isDefaultCombo ? "" : + "and ( not exists ( select 1 from categoryoption_organisationunits c_o where c_o.categoryoptionid = cocco.categoryoptionid ) " + + "or exists ( select 1 from categoryoption_organisationunits c_o " + + "join _orgunitstructure o2 on o2.organisationunitid = c_o.organisationunitid " + + "where c_o.categoryoptionid = cocco.categoryoptionid and (" + coOrgUnitConstraint + ") ) ) " ) + + ")"; break; } } String approvedAboveSubquery = "false"; // Not approved above if this is the highest (lowest number) approval orgUnit level. - if ( orgUnitLevelAbove > 0 ) + if ( orgUnitLevelAbove > 0 && orgUnit != null ) { approvedAboveSubquery = "exists(select 1 from dataapproval da " + "join dataapprovallevel dal on dal.dataapprovallevelid = da.dataapprovallevelid " + @@ -365,18 +401,12 @@ final String sql = "select cocco.categoryoptioncomboid, o.organisationunitid, " + - "(select min(coalesce(dal.level, 0)) from period p " + - "left join dataapproval da on da.datasetid in (" + dataSetIds + ") and da.periodid = p.periodid " + - "left join dataapprovallevel dal on dal.dataapprovallevelid = da.dataapprovallevelid " + - "where p.periodid in (" + periodIds + ") " + - "and da.attributeoptioncomboid = cocco.categoryoptioncomboid and da.organisationunitid = o.organisationunitid " + - ") as highest_approved_level, " + - "(select substring(min(concat(100000 + coalesce(dal.level, 0), coalesce(da.accepted, FALSE))) from 7) from period p " + - "left join dataapproval da on da.datasetid in (" + dataSetIds + ") and da.periodid = p.periodid " + - "left join dataapprovallevel dal on dal.dataapprovallevelid = da.dataapprovallevelid " + - "where p.periodid in (" + periodIds + ") " + - "and da.attributeoptioncomboid = cocco.categoryoptioncomboid and da.organisationunitid = o.organisationunitid " + - ") as accepted_at_highest_level, " + + "(select min(coalesce(dal.level + case when da.accepted then .0 else .1 end, 0)) from period p " + + "left join dataapproval da on da.datasetid in (" + dataSetIds + ") and da.periodid = p.periodid " + + "left join dataapprovallevel dal on dal.dataapprovallevelid = da.dataapprovallevelid " + + "where p.periodid in (" + periodIds + ") " + + "and da.attributeoptioncomboid = cocco.categoryoptioncomboid and " + highestApprovedOrgUnitCompare + + ") as highest_approved, " + readyBelowSubquery + " as ready_below, " + approvedAboveSubquery + " as approved_above " + "from categoryoptioncombos_categoryoptions cocco " + @@ -410,14 +440,14 @@ { final Integer aoc = rowSet.getInt( 1 ); final Integer ouId = rowSet.getInt( 2 ); - final Integer level = rowSet.getInt( 3 ); - final String acceptedString = rowSet.getString( 4 ); - final boolean readyBelow = rowSet.getBoolean( 5 ); - final boolean approvedAbove = rowSet.getBoolean( 6 ); - - final boolean accepted = ( acceptedString == null ? false : acceptedString.substring( 0, 1 ).equalsIgnoreCase( "t" ) ); - - DataApprovalLevel statusLevel = ( level == null || level == 0 ? null : levelMap.get( level ) ); // null if not approved + final Double highestApproved = rowSet.getDouble( 3 ); + final boolean readyBelow = rowSet.getBoolean( 4 ); + final boolean approvedAbove = rowSet.getBoolean( 5 ); + + 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, new Callable() @@ -458,9 +488,9 @@ statusList.add( new DataApprovalStatus( state, da, statusLevel, null ) ); log.debug( "Get approval result: level " + level + " dataApprovalLevel " + ( daLevel != null ? daLevel.getLevel() : "[none]" ) - + " approved " + ( statusLevel != null ) - + " readyBelow " + readyBelow + " approvedAbove " + approvedAbove - + " accepted " + accepted + " state " + ( state != null ? state.name() : "[none]" ) + " " + da ); + + " approved " + ( statusLevel != null ) + + " readyBelow " + readyBelow + " approvedAbove " + approvedAbove + + " accepted " + accepted + " state " + ( state != null ? state.name() : "[none]" ) + " " + da ); } } === 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 2015-04-16 00:11:41 +0000 +++ dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/dataapproval/DataApprovalServiceCategoryOptionGroupTest.java 2015-04-29 19:13:26 +0000 @@ -1670,29 +1670,31 @@ // Approve ChinaA1_1 at level 1 // --------------------------------------------------------------------- - assertTrue( approve( superUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertTrue( unapprove( superUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertTrue( approve( globalConsultant, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertTrue( unapprove( globalConsultant, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( approve( globalReadEverything, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( approve( brazilInteragencyUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( chinaInteragencyUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( indiaInteragencyUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( approve( brazilAgencyAUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( chinaAgencyAUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( chinaAgencyBUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( indiaAgencyAUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( approve( brazilPartner1User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( chinaPartner1User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( chinaPartner2User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( approve( indiaPartner1User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertTrue( approve( globalUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); + assertFalse( approve( superUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); // Wrong org unit. + + assertTrue( approve( superUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertTrue( unapprove( superUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertTrue( approve( globalConsultant, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertTrue( unapprove( globalConsultant, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( approve( globalReadEverything, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( approve( brazilInteragencyUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( chinaInteragencyUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( indiaInteragencyUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( approve( brazilAgencyAUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( chinaAgencyAUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( chinaAgencyBUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( indiaAgencyAUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( approve( brazilPartner1User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( chinaPartner1User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( chinaPartner2User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( approve( indiaPartner1User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertTrue( approve( globalUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); // --------------------------------------------------------------------- // ChinaA1_1 is approved at level 1 @@ -1782,29 +1784,29 @@ // Unapprove ChinaA1_1 at level 1 // --------------------------------------------------------------------- - assertTrue( unapprove( superUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertTrue( approve( superUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertTrue( unapprove( globalConsultant, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertTrue( approve( globalConsultant, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( unapprove( globalReadEverything, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( unapprove( brazilInteragencyUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( chinaInteragencyUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( indiaInteragencyUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( unapprove( brazilAgencyAUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( chinaAgencyAUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( chinaAgencyBUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( indiaAgencyAUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertFalse( unapprove( brazilPartner1User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( chinaPartner1User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( chinaPartner2User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - assertFalse( unapprove( indiaPartner1User, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); - - assertTrue( unapprove( globalUser, globalLevel1, dataSetA, periodA, china, chinaA1_1Combo ) ); + assertTrue( unapprove( superUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertTrue( approve( superUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertTrue( unapprove( globalConsultant, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertTrue( approve( globalConsultant, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( unapprove( globalReadEverything, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( unapprove( brazilInteragencyUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( chinaInteragencyUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( indiaInteragencyUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( unapprove( brazilAgencyAUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( chinaAgencyAUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( chinaAgencyBUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( indiaAgencyAUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertFalse( unapprove( brazilPartner1User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( chinaPartner1User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( chinaPartner2User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + assertFalse( unapprove( indiaPartner1User, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); + + assertTrue( unapprove( globalUser, globalLevel1, dataSetA, periodA, global, chinaA1_1Combo ) ); assertArrayEquals( new String[] { "ou=Brazil mechanism=BrazilA1 level=4 UNAPPROVED_READY approve=T unapprove=F accept=F unaccept=F read=T",