From 903a958783d49f099fe7233d85623e597e264a38 Mon Sep 17 00:00:00 2001 From: Nayan Ambali Date: Fri, 15 Mar 2013 15:32:02 +0530 Subject: [PATCH] review comments incorporated --- .../22-group_name_modified_to_not_null.sql | 6 +- .../GroupCommandFromApiJsonDeserializer.java | 25 +++--- .../GroupLevelReadPlatformServiceImpl.java | 5 +- .../service/GroupReadPlatformServiceImpl.java | 17 ++-- ...WritePlatformServiceJpaRepositoryImpl.java | 80 ++++++++++++------- 5 files changed, 80 insertions(+), 53 deletions(-) diff --git a/mifosng-db/patches/22-group_name_modified_to_not_null.sql b/mifosng-db/patches/22-group_name_modified_to_not_null.sql index 72eae9345..dcf9185b2 100644 --- a/mifosng-db/patches/22-group_name_modified_to_not_null.sql +++ b/mifosng-db/patches/22-group_name_modified_to_not_null.sql @@ -1 +1,5 @@ -ALTER TABLE m_group MODIFY name VARCHAR(100) NOT NULL; \ No newline at end of file +ALTER TABLE m_group MODIFY name VARCHAR(100) NOT NULL; +ALTER TABLE m_group DROP INDEX name; +ALTER TABLE m_group DROP INDEX external_id; +ALTER TABLE m_group ADD UNIQUE INDEX name (name, level_Id); +ALTER TABLE m_group ADD UNIQUE INDEX external_id (external_id, level_Id); \ No newline at end of file diff --git a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/serialization/GroupCommandFromApiJsonDeserializer.java b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/serialization/GroupCommandFromApiJsonDeserializer.java index ce4dc8673..b2ec935c8 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/serialization/GroupCommandFromApiJsonDeserializer.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/serialization/GroupCommandFromApiJsonDeserializer.java @@ -95,23 +95,23 @@ public final class GroupCommandFromApiJsonDeserializer extends AbstractFromApiJs } /* - * OfficeId is mandatory field + * Either OfficeId or ParentId is mandatory to have value */ - final String officeIdParameterName = "officeId"; - final Long officeId = this.fromApiJsonHelper.extractLongNamed(officeIdParameterName, element); - baseDataValidator.reset().parameter(officeIdParameterName).value(officeId).notNull().integerGreaterThanZero(); - final String parentIdParameterName = "parentId"; if (this.fromApiJsonHelper.parameterExists(parentIdParameterName, element)) { final Long parentId = this.fromApiJsonHelper.extractLongNamed(parentIdParameterName, element); baseDataValidator.reset().parameter(parentIdParameterName).value(parentId).notNull().integerGreaterThanZero(); + }else{ + final String officeIdParameterName = "officeId"; + final Long officeId = this.fromApiJsonHelper.extractLongNamed(officeIdParameterName, element); + baseDataValidator.reset().parameter(officeIdParameterName).value(officeId).notNull().integerGreaterThanZero(); } final String staffIdParameterName = "staffId"; if (this.fromApiJsonHelper.parameterExists(staffIdParameterName, element)) { final Long staffId = this.fromApiJsonHelper.extractLongNamed(staffIdParameterName, element); - baseDataValidator.reset().parameter(staffIdParameterName).value(staffId).notNull().integerGreaterThanZero(); + baseDataValidator.reset().parameter(staffIdParameterName).value(staffId).ignoreIfNull().integerGreaterThanZero(); } if (!dataValidationErrors.isEmpty()) { throw new PlatformApiDataValidationException("validation.msg.validation.errors.exist", @@ -146,13 +146,13 @@ public final class GroupCommandFromApiJsonDeserializer extends AbstractFromApiJs final String parentIdParameterName = "parentId"; if (this.fromApiJsonHelper.parameterExists(parentIdParameterName, element)) { final Long parentId = this.fromApiJsonHelper.extractLongNamed(parentIdParameterName, element); - baseDataValidator.reset().parameter(parentIdParameterName).value(parentId).notNull().integerGreaterThanZero(); + baseDataValidator.reset().parameter(parentIdParameterName).value(parentId).ignoreIfNull().integerGreaterThanZero(); } final String staffIdParameterName = "staffId"; if (this.fromApiJsonHelper.parameterExists(staffIdParameterName, element)) { final Long staffId = this.fromApiJsonHelper.extractLongNamed(staffIdParameterName, element); - baseDataValidator.reset().parameter(staffIdParameterName).value(staffId).notNull().integerGreaterThanZero(); + baseDataValidator.reset().parameter(staffIdParameterName).value(staffId).ignoreIfNull().integerGreaterThanZero(); } if (!dataValidationErrors.isEmpty()) { throw new PlatformApiDataValidationException("validation.msg.validation.errors.exist", @@ -164,9 +164,6 @@ public final class GroupCommandFromApiJsonDeserializer extends AbstractFromApiJs final Type typeOfMap = new TypeToken>() {}.getType(); - /* - * Office updated is not supported - */ final Set supportedParametersUnassignStaff = new HashSet(Arrays.asList("staffId")); this.fromApiJsonHelper.checkForUnsupportedParameters(typeOfMap, json, supportedParametersUnassignStaff); @@ -177,10 +174,8 @@ public final class GroupCommandFromApiJsonDeserializer extends AbstractFromApiJs final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("group"); final String staffIdParameterName = "staffId"; - if (this.fromApiJsonHelper.parameterExists(staffIdParameterName, element)) { - final Long staffId = this.fromApiJsonHelper.extractLongNamed(staffIdParameterName, element); - baseDataValidator.reset().parameter(staffIdParameterName).value(staffId).notNull().integerGreaterThanZero(); - } + final Long staffId = this.fromApiJsonHelper.extractLongNamed(staffIdParameterName, element); + baseDataValidator.reset().parameter(staffIdParameterName).value(staffId).notNull().integerGreaterThanZero(); if (!dataValidationErrors.isEmpty()) { throw new PlatformApiDataValidationException("validation.msg.validation.errors.exist", "Validation errors exist.", dataValidationErrors); } diff --git a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupLevelReadPlatformServiceImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupLevelReadPlatformServiceImpl.java index 8cb88b206..f1ef9d3da 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupLevelReadPlatformServiceImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupLevelReadPlatformServiceImpl.java @@ -9,6 +9,7 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.util.Collection; +import org.mifosplatform.infrastructure.core.domain.JdbcSupport; import org.mifosplatform.infrastructure.core.service.TenantAwareRoutingDataSource; import org.mifosplatform.infrastructure.security.service.PlatformSecurityContext; import org.mifosplatform.portfolio.group.data.GroupLevelData; @@ -54,9 +55,9 @@ public class GroupLevelReadPlatformServiceImpl implements GroupLevelReadPlatform final Long levelId = rs.getLong("id"); final String levelName = rs.getString("levelName"); - final Long parentLevelId = rs.getLong("parentLevelId"); + final Long parentLevelId = JdbcSupport.getLong(rs, "parentLevelId"); final String parentLevelName = rs.getString("parentName"); - final Long childLevelId = rs.getLong("childLevelId"); + final Long childLevelId = JdbcSupport.getLong(rs, "childLevelId"); final String childLevelName = rs.getString("childLevelName"); final boolean superParent = rs.getBoolean("superParent"); final boolean recursable = rs.getBoolean("recursable"); diff --git a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupReadPlatformServiceImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupReadPlatformServiceImpl.java index d3645cbb7..a35d8d434 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupReadPlatformServiceImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupReadPlatformServiceImpl.java @@ -34,6 +34,7 @@ import org.mifosplatform.portfolio.group.domain.Group; import org.mifosplatform.portfolio.group.domain.GroupRepository; import org.mifosplatform.portfolio.group.exception.GroupLevelNotFoundException; import org.mifosplatform.portfolio.group.exception.GroupNotFoundException; +import org.mifosplatform.useradministration.domain.AppUser; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.dao.EmptyResultDataAccessException; import org.springframework.jdbc.core.JdbcTemplate; @@ -64,16 +65,20 @@ public class GroupReadPlatformServiceImpl implements GroupReadPlatformService { public Collection retrieveAllGroups(final String extraCriteria) { this.context.authenticatedUser(); + + AppUser currentUser = context.authenticatedUser(); + String hierarchy = currentUser.getOffice().getHierarchy(); + String hierarchySearchString = hierarchy + "%"; final GroupDataMapper rm = new GroupDataMapper(); - String sql = "select " + rm.groupSchema() + " where g.is_deleted=0"; - + String sql = "select " + rm.groupSchema() + " where o.hierarchy like ? and g.is_deleted=0 "; + if (StringUtils.isNotBlank(extraCriteria)) { sql += " and (" + extraCriteria + ")"; } - return this.jdbcTemplate.query(sql, rm, new Object[] {}); + return this.jdbcTemplate.query(sql, rm, new Object[] {hierarchySearchString}); } @Override @@ -334,8 +339,8 @@ public class GroupReadPlatformServiceImpl implements GroupReadPlatformService { public String groupSchema() { return "g.office_id as officeId, g.parent_id as parentId , o.name as officeName," + " g.id as id, g.external_id as externalId, g.name as name , s.display_name as staffName , pg.name as" - + " parentName , g.staff_id as staffId , g.hierarchy as hierarchy from m_group g join m_office o on o.id = g.office_id left join " - + "m_staff s on s.id = g.staff_id left join m_group pg on pg.id = g.parent_id"; + + " parentName , g.staff_id as staffId , g.hierarchy as hierarchy from m_group g join m_office o on o.id = g.office_id left join" + + " m_staff s on s.id = g.staff_id left join m_group pg on pg.id = g.parent_id"; } @Override @@ -346,7 +351,7 @@ public class GroupReadPlatformServiceImpl implements GroupReadPlatformService { final String externalId = rs.getString("externalId"); final Long officeId = rs.getLong("officeId"); final String officeName = rs.getString("officeName"); - final Long parentId = rs.getLong("parentId"); + final Long parentId = JdbcSupport.getLong(rs, "parentId"); final String parentName = rs.getString("parentName"); final Long staffId = JdbcSupport.getLong(rs, "staffId"); final String staffName = rs.getString("staffName"); diff --git a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupWritePlatformServiceJpaRepositoryImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupWritePlatformServiceJpaRepositoryImpl.java index 37a531ca4..ed83e635d 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupWritePlatformServiceJpaRepositoryImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/portfolio/group/service/GroupWritePlatformServiceJpaRepositoryImpl.java @@ -73,6 +73,8 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla @Transactional @Override public CommandProcessingResult createGroup(final JsonCommand command) { + + GroupLevel groupLevel = null; try { this.context.authenticatedUser(); @@ -84,7 +86,7 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla final Long parentId = command.longValueOfParameterNamed("parentId"); Long officeId = null; - GroupLevel groupLevel = null; + Group parentGroup = null; if (parentId == null) { @@ -152,7 +154,7 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla .build(); } catch (final DataIntegrityViolationException dve) { - handleGroupDataIntegrityIssues(command, dve); + handleGroupDataIntegrityIssues(command, dve , groupLevel ); return new CommandProcessingResult(Long.valueOf(-1)); } } @@ -161,6 +163,8 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla @Override public CommandProcessingResult updateGroup(final Long grouptId, final JsonCommand command) { + GroupLevel groupLevel = null; + try { this.context.authenticatedUser(); @@ -197,12 +201,16 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla if (command.isChangeInLongParameterNamed(staffIdParamName, presentStaffId)) { final Long newValue = command.longValueOfParameterNamed(staffIdParamName); actualChanges.put(staffIdParamName, newValue); - final Staff newStaff = this.staffRepository.findByOffice(newValue, officeId); - if (newStaff == null) { throw new StaffNotFoundException(newValue); } + + Staff newStaff = null; + if(newValue != null){ + newStaff = this.staffRepository.findByOffice(newValue, officeId); + if (newStaff == null) { throw new StaffNotFoundException(newValue); } + } groupForUpdate.setStaff(newStaff); } - final GroupLevel groupLevel = this.groupLevelRepository.findOne(groupForUpdate.getGroupLevel().getId()); + groupLevel = this.groupLevelRepository.findOne(groupForUpdate.getGroupLevel().getId()); /* * Ignoring parentId param, if group for update is super parent. @@ -216,27 +224,28 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla final Long newValue = command.longValueOfParameterNamed(parentIdParamName); actualChanges.put(parentIdParamName, newValue); - final Group newParentGroup = this.groupRepository.findOne(newValue); + Group newParentGroup = null; + if (newValue != null) { + newParentGroup = this.groupRepository.findOne(newValue); + if (newParentGroup == null || newParentGroup.isDeleted()) { throw new StaffNotFoundException(newValue); } - if (newParentGroup == null || newParentGroup.isDeleted()) { throw new StaffNotFoundException(newValue); } + if (!newParentGroup.isOfficeIdentifiedBy(officeId)) { + final String errorMessage = "Group and parent group must have the same office"; + throw new InvalidOfficeException("group", "attach.to.parent.group", errorMessage); + } + /* + * If Group is not super parent then validate group + * level's parent level is same as group parent's level + * this check makes sure new group is added at immediate + * next level in hierarchy + */ - if (!newParentGroup.isOfficeIdentifiedBy(officeId)) { - final String errorMessage = "Group and parent group must have the same office"; - throw new InvalidOfficeException("group", "attach.to.parent.group", errorMessage); + if (!groupForUpdate.getGroupLevel().isIdentifiedByParentId(newParentGroup.getGroupLevel().getId())) { + final String errorMessage = "Parent group's level is not equal to child level's parent level "; + throw new InvalidGroupLevelException("add", "invalid.level", errorMessage); + } } - - /* - * If Group is not super parent then validate group level's - * parent level is same as group parent's level this check - * makes sure new group is added at immediate next level in - * hierarchy - */ - - if (!groupForUpdate.getGroupLevel().isIdentifiedByParentId(newParentGroup.getGroupLevel().getId())) { - final String errorMessage = "Parent group's level is not equal to child level's parent level "; - throw new InvalidGroupLevelException("add", "invalid.level", errorMessage); - } - + groupForUpdate.setParent(newParentGroup); } } @@ -262,7 +271,7 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla .build(); } catch (final DataIntegrityViolationException dve) { - handleGroupDataIntegrityIssues(command, dve); + handleGroupDataIntegrityIssues(command, dve , groupLevel); return new CommandProcessingResult(Long.valueOf(-1)); } } @@ -378,14 +387,27 @@ public class GroupWritePlatformServiceJpaRepositoryImpl implements GroupWritePla * Guaranteed to throw an exception no matter what the data integrity issue * is. */ - private void handleGroupDataIntegrityIssues(final JsonCommand command, final DataIntegrityViolationException dve) { + private void handleGroupDataIntegrityIssues(final JsonCommand command, final DataIntegrityViolationException dve, GroupLevel groupLevel) { final Throwable realCause = dve.getMostSpecificCause(); + String errorMessageForUser = null; + String errorMessageForMachine = null; + if (realCause.getMessage().contains("external_id")) { - throw new PlatformDataIntegrityException("error.msg.group.duplicate.externalId", "Group with externalId {0} already exists", - "externalId", command.stringValueOfParameterNamed("externalId")); - } else if (realCause.getMessage().contains("name")) { throw new PlatformDataIntegrityException("error.msg.group.duplicate.name", - "Group with name {0} already exists", "name", command.stringValueOfParameterNamed("name")); } + + errorMessageForUser = groupLevel.getLevelName() + " with externalId {0} already exists"; + errorMessageForMachine = "error.msg." + groupLevel.getLevelName().toLowerCase() + ".duplicate.externalId"; + throw new PlatformDataIntegrityException(errorMessageForMachine, errorMessageForUser, "externalId", + command.stringValueOfParameterNamed("externalId")); + + } else if (realCause.getMessage().contains("name")) { + + errorMessageForUser = groupLevel.getLevelName() + " with name {0} already exists"; + errorMessageForMachine = "error.msg." + groupLevel.getLevelName().toLowerCase() + ".duplicate.name"; + throw new PlatformDataIntegrityException(errorMessageForMachine, errorMessageForUser, "name", + command.stringValueOfParameterNamed("name")); + + } logger.error(dve.getMessage(), dve); throw new PlatformDataIntegrityException("error.msg.group.unknown.data.integrity.issue",