diff --git a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/dao/EvaluationDaoImpl.java b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/dao/EvaluationDaoImpl.java index 5d851e5e5..c195bc6a4 100644 --- a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/dao/EvaluationDaoImpl.java +++ b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/dao/EvaluationDaoImpl.java @@ -347,8 +347,7 @@ public List getParticipantsForEval(Long evaluationId, String use @SuppressWarnings("unchecked") public List getEvalsWithoutUserAssignments() { String hql = "select eval from EvalAssignUser eau right join eau.evaluation eval where eau.id is null"; - List evals = (List) executeHqlQuery(hql, new Object[] {}, 0, 0); - return evals; + return (List) executeHqlQuery(hql, new Object[] {}, 0, 0); } /** @@ -1252,10 +1251,7 @@ public Set getResponseUserIds(Long evaluationId, String[] evalGroupIds, for (Object object : results) { responseUsers.add((String) object); } - if (log.isDebugEnabled()) { - log.debug("ResponseUserIds(eval:"+evaluationId+", groups:" - +ArrayUtils.arrayToString(evalGroupIds)+", completed="+completed+"): users="+responseUsers); - } + log.debug("ResponseUserIds(eval:{}, groups:{}, completed={}): users={}", evaluationId, ArrayUtils.arrayToString(evalGroupIds), completed, responseUsers); return responseUsers; } diff --git a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalCommonLogicImpl.java b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalCommonLogicImpl.java index 0eb152062..6f228de76 100644 --- a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalCommonLogicImpl.java +++ b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalCommonLogicImpl.java @@ -569,7 +569,7 @@ else if( EvalConstants.PERM_TAKE_EVALUATION.equals( permission ) ) || EvalConstants.PERM_TAKE_EVALUATION.equals(permission) || EvalConstants.PERM_ASSISTANT_ROLE.equals( permission ) ) { - log.debug( "Using eval groups provider: evalGroupId: " + evalGroupID + ", permission: " + permission ); + log.debug("Using eval groups provider: evalGroupId: {}, permission: {}", evalGroupID, permission); userIDs.addAll( evalGroupsProvider.getUserIdsForEvalGroups( new String[] { evalGroupID }, EvalExternalLogicImpl.translatePermission(permission)) ); } @@ -620,10 +620,8 @@ public boolean isUserAllowedInEvalGroup(String userId, String permission, String if (EvalConstants.PERM_BE_EVALUATED.equals(permission) || EvalConstants.PERM_TAKE_EVALUATION.equals(permission) || EvalConstants.PERM_ASSISTANT_ROLE.equals(permission) ) { - log.debug("Using eval groups provider: userId: " + userId + ", permission: " + permission + ", evalGroupId: " + evalGroupId); - if ( evalGroupsProvider.isUserAllowedInGroup(userId, EvalExternalLogicImpl.translatePermission(permission), evalGroupId) ) { - return true; - } + log.debug("Using eval groups provider: userId: {}, permission: {}, evalGroupId: {}", userId, permission, evalGroupId); + return evalGroupsProvider.isUserAllowedInGroup(userId, EvalExternalLogicImpl.translatePermission(permission), evalGroupId); } } diff --git a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationServiceImpl.java b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationServiceImpl.java index 3be3c1296..b79ddeb48 100644 --- a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationServiceImpl.java +++ b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationServiceImpl.java @@ -327,88 +327,8 @@ public List getParticipantsForEval(Long evaluationId, String use if (evaluationId == null && (userId == null || "".equals(userId)) ) { throw new IllegalArgumentException("At least one of the following must be set: evaluationId, userId"); } - /** - // create the search - Search search = new Search(); - if (evaluationId != null) { - // getEvaluationOrFail(evaluationId); // took out eval fetch for now - search.addRestriction( new Restriction("evaluation.id", evaluationId) ); - } - if (evalStateConstant != null) { - EvalUtils.validateStateConstant(evalStateConstant); - search.addRestriction( new Restriction("evaluation.state", evalStateConstant) ); - } - if (evalGroupIds != null && evalGroupIds.length > 0) { - search.addRestriction( new Restriction("evalGroupId", evalGroupIds) ); - } - if (assignTypeConstant != null - && includeConstant == null) { - EvalAssignUser.validateType(assignTypeConstant); - // only set this if the includeConstant is not set - search.addRestriction( new Restriction("type", assignTypeConstant) ); - } - if (assignStatusConstant == null) { - search.addRestriction( new Restriction("status", EvalAssignUser.STATUS_REMOVED, Restriction.NOT_EQUALS) ); - } else if (STATUS_ANY.equals(assignStatusConstant)) { - // no restriction needed in this case - } else { - EvalAssignUser.validateStatus(assignStatusConstant); - search.addRestriction( new Restriction("status", assignStatusConstant) ); - } - if (userId != null && ! "".equals(userId)) { - search.addRestriction( new Restriction("userId", userId) ); - } - boolean includeFilterUsers = false; - Set userFilter = null; - if (includeConstant != null) { - EvalUtils.validateEmailIncludeConstant(includeConstant); - String[] groupIds = new String[] {}; - if (evalGroupIds != null && evalGroupIds.length > 0) { - groupIds = evalGroupIds; - } - // force the results to only include eval takers - search.addRestriction( new Restriction("type", EvalAssignUser.TYPE_EVALUATOR) ); - // now set up the filter - if (EvalConstants.EVAL_INCLUDE_NONTAKERS.equals(includeConstant)) { - // get all users who have NOT responded - userFilter = dao.getResponseUserIds(evaluationId, groupIds); - includeFilterUsers = false; - } else if (EvalConstants.EVAL_INCLUDE_RESPONDENTS.equals(includeConstant)) { - // get all users who have responded - userFilter = dao.getResponseUserIds(evaluationId, groupIds); - includeFilterUsers = true; - } else if (EvalConstants.EVAL_INCLUDE_ALL.equals(includeConstant)) { - // do nothing - } else { - throw new IllegalArgumentException("Unknown includeConstant: " + includeConstant); - } - } - // get the assignments based on the search - List results = dao.findBySearch(EvalAssignUser.class, search); - List assignments = new ArrayList( results ); - // This code is potentially expensive but there is not really a better way to handle it -AZ - if (userFilter != null && ! userFilter.isEmpty()) { - // filter the results based on the userFilter - for (Iterator iterator = assignments.iterator(); iterator.hasNext();) { - EvalAssignUser evalAssignUser = iterator.next(); - String uid = evalAssignUser.getUserId(); - if (includeFilterUsers) { - // only include users in the filter - if (! userFilter.contains(uid)) { - iterator.remove(); - } - } else { - // exclude all users in the filter - if (userFilter.contains(uid)) { - iterator.remove(); - } - } - } - } - **/ // this is handled in the DAO now - List assignments = dao.getParticipantsForEval(evaluationId, userId, evalGroupIds, assignTypeConstant, assignStatusConstant, includeConstant, evalStateConstant); - return assignments; + return dao.getParticipantsForEval(evaluationId, userId, evalGroupIds, assignTypeConstant, assignStatusConstant, includeConstant, evalStateConstant); } public int countParticipantsForEval(Long evaluationId, String[] evalGroupIds) { @@ -701,17 +621,15 @@ public EvalAssignGroup getAssignGroupById(Long assignGroupId) { } public EvalAssignGroup getAssignGroupByEvalAndGroupId(Long evaluationId, String evalGroupId) { - log.debug("evaluationId: " + evaluationId + ", evalGroupId: " + evalGroupId); - if (evaluationId == null - || evalGroupId == null || "".equals(evalGroupId)) { + log.debug("evaluationId: {}, evalGroupId: {}", evaluationId, evalGroupId); + if (evaluationId == null || evalGroupId == null || evalGroupId.isEmpty()) { throw new IllegalArgumentException("evaluationId and evalGroupId must not be null"); } - EvalAssignGroup assignGroup = dao.findOneBySearch(EvalAssignGroup.class, new Search( + return dao.findOneBySearch(EvalAssignGroup.class, new Search( new Restriction[] { new Restriction("evaluation.id", evaluationId), new Restriction("evalGroupId", evalGroupId) }) ); - return assignGroup; } public List getAssignHierarchyByEval(Long evaluationId) { @@ -732,7 +650,7 @@ public EvalAssignHierarchy getAssignHierarchyById(Long assignHierarchyId) { public Map> getAssignGroupsForEvals(Long[] evaluationIds, boolean includeUnApproved, Boolean includeHierarchyGroups) { - log.debug("evalIds: " + ArrayUtils.arrayToString(evaluationIds) + ", includeUnApproved=" + includeUnApproved); + log.debug("evalIds: {}, includeUnApproved={}", ArrayUtils.arrayToString(evaluationIds), includeUnApproved); Map> evals = new TreeMap<>(); if ( evaluationIds != null && evaluationIds.length > 0){ @@ -765,14 +683,12 @@ public Map> getAssignGroupsForEvals(Long[] evaluatio search.addOrder( new Order("evalGroupId") ); List l = dao.findBySearch(EvalAssignGroup.class, search ); - for (int i=0; i innerList = evals.get(evalId); - innerList.add( eac ); - } + for (EvalAssignGroup eac : l) { + // put stuff in inner list + Long evalId = eac.getEvaluation().getId(); + List innerList = evals.get(evalId); + innerList.add(eac); + } } return evals; } diff --git a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationSetupServiceImpl.java b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationSetupServiceImpl.java index 6fd193c1b..1759a8bcf 100644 --- a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationSetupServiceImpl.java +++ b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationSetupServiceImpl.java @@ -1023,8 +1023,7 @@ public List synchronizeUserAssignments(Long evaluationId, String evalGroup * @return the list of {@link EvalAssignUser} ids changed during the synchronization (created, updated, deleted), * NOTE: deleted {@link EvalAssignUser} will not be able to be retrieved */ - public List synchronizeUserAssignmentsForced(EvalEvaluation evaluation, - String evalGroupId, boolean removeAllowed) { + public List synchronizeUserAssignmentsForced(EvalEvaluation evaluation, String evalGroupId, boolean removeAllowed) { Long evaluationId = evaluation.getId(); String currentUserId = commonLogic.getCurrentUserId(); if (currentUserId == null) { @@ -1099,11 +1098,7 @@ public List synchronizeUserAssignmentsForced(EvalEvaluation evaluation, currentTakers.addAll(currentAssistants); currentTakers.addAll(currentEvaluated); } - - HashSet currentAll = new HashSet<>(); - currentAll.addAll(currentEvaluated); - currentAll.addAll(currentAssistants); - currentAll.addAll(currentTakers); + /* Resolve the current permissions against the existing assignments, * this should only change linked records but should respect unlinked and removed records by not * adding a record where one already exists for the given user/group combo, @@ -1162,12 +1157,9 @@ public List synchronizeUserAssignmentsForced(EvalEvaluation evaluation, if (assignUserToRemove.isEmpty() && assignUserToSave.isEmpty()) { message += ": no changes to the user assignments ("+assignedUsers.size()+")"; } else { - if (removeAllowed - && ! assignUserToRemove.isEmpty()) { - Long[] assignUserToRemoveArray = assignUserToRemove.toArray(new Long[assignUserToRemove.size()]); - if (log.isDebugEnabled()) { - log.debug("Deleting user eval assignment Ids: "+assignUserToRemove); - } + if (removeAllowed && ! assignUserToRemove.isEmpty()) { + Long[] assignUserToRemoveArray = assignUserToRemove.toArray(new Long[0]); + log.debug("Deleting user eval assignment Ids: {}", assignUserToRemove); dao.deleteSet(EvalAssignUser.class, assignUserToRemoveArray); message += ": removed the following assignments: " + assignUserToRemove; changedUserAssignments.addAll( assignUserToRemove ); @@ -1178,9 +1170,7 @@ public List synchronizeUserAssignmentsForced(EvalEvaluation evaluation, } // this is meant to force the assigned users set to be re-calculated assignUserToSave = new HashSet<>(assignUserToSave); - if (log.isDebugEnabled()) { - log.debug("Saving user eval assignments: "+assignUserToSave); - } + log.debug("Saving user eval assignments: {}", assignUserToSave); dao.saveSet(assignUserToSave); message += ": created the following assignments: " + assignUserToSave; for (EvalAssignUser evalAssignUser : assignUserToSave) { @@ -1205,7 +1195,7 @@ public List synchronizeUserAssignmentsForced(EvalEvaluation evaluation, } } if (! orphanedUserAssignments.isEmpty()) { - Long[] orphanedUserAssignmentsArray = orphanedUserAssignments.toArray(new Long[orphanedUserAssignments.size()]); + Long[] orphanedUserAssignmentsArray = orphanedUserAssignments.toArray(new Long[0]); dao.deleteSet(EvalAssignUser.class, orphanedUserAssignmentsArray); message += ": removed the following orphaned user assignments: " + orphanedUserAssignments; changedUserAssignments.addAll( orphanedUserAssignments ); diff --git a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/externals/EvalExternalLogicImpl.java b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/externals/EvalExternalLogicImpl.java index 7bce8084d..cb2664002 100644 --- a/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/externals/EvalExternalLogicImpl.java +++ b/sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/externals/EvalExternalLogicImpl.java @@ -851,10 +851,7 @@ public Set getUserIdsForEvalGroup( String evalGroupID, String permission } azGroups.add( azGroup ); userIDs.addAll( authzGroupService.getUsersIsAllowed( permission, azGroups ) ); - if( userIDs.contains( ADMIN_USER_ID ) ) - { - userIDs.remove( ADMIN_USER_ID ); - } + userIDs.remove( ADMIN_USER_ID ); } // Otherwise, it's section aware but we only need to run the following if the sectin prefix is present in the evalGroupID @@ -900,10 +897,24 @@ else if( groupID.hasSection() ) catch( IdNotFoundException ex ) { log.warn( "Could not find section with ID = " + groupID.getSectionID(), ex ); } } - // Return the user IDs + // Clean the userIDs of RoleViewType users first + userIDs.removeAll( getRoleViewTypeUserIds(userIDs) ); return userIDs; } + /** + * Need to help remove the fake users from Sakai 23+ View Site As + */ + protected Set getRoleViewTypeUserIds(Set userIDs) { + Set roleViewTypeUserIds = new HashSet<>(); + for (String userId : userIDs) { + if (userDirectoryService.isRoleViewType(userId)) { + roleViewTypeUserIds.add(userId); + } + } + return roleViewTypeUserIds; + } + /* (non-Javadoc) * @see org.sakaiproject.evaluation.logic.EvalExternalLogic#isUserAllowedInEvalGroup(java.lang.String, java.lang.String, java.lang.String) */ @@ -917,9 +928,13 @@ public boolean isUserAllowedInEvalGroup(String userId, String permission, String return isUserSakaiAdmin(userId); } - // try checking Sakai - String reference = evalGroupId; - return securityService.unlock(userId, permission, reference); + // Special RoleView user from View Site As + if (userDirectoryService.isRoleViewType(userId)) { + return false; + } + + // try checking Sakai authz permissions last + return securityService.unlock(userId, permission, evalGroupId); } /* (non-Javadoc) diff --git a/sakai-evaluation-tool/src/java/org/sakaiproject/evaluation/tool/producers/EvaluationAssignConfirmProducer.java b/sakai-evaluation-tool/src/java/org/sakaiproject/evaluation/tool/producers/EvaluationAssignConfirmProducer.java index bfe381246..6901800e1 100644 --- a/sakai-evaluation-tool/src/java/org/sakaiproject/evaluation/tool/producers/EvaluationAssignConfirmProducer.java +++ b/sakai-evaluation-tool/src/java/org/sakaiproject/evaluation/tool/producers/EvaluationAssignConfirmProducer.java @@ -304,7 +304,7 @@ private int getEnrollmentCount(HashMap> groupIdToEA if (groupIdToEAUList.get(evalGroupId) == null) { //enrollmentCount = 0; // Since no users have been added to the eval YET, get the number of members that WILL be added - // this number is more intuative for the instructor than 0. + // this number is more intuitive for the instructor than 0. enrollmentCount = commonLogic.countUserIdsForEvalGroup(evalGroupId, EvalConstants.PERM_TAKE_EVALUATION, evaluation.getSectionAwareness()); } else { enrollmentCount = groupIdToEAUList.get(evalGroupId).size();