Index: lams_central/src/java/org/lamsfoundation/lams/webservice/SPEnrolmentServlet.java =================================================================== diff -u -rb8c30f48f0925a8b74bd6090c7766f43096877c5 -r3080c61fc08c0d5fdc28fb9ad6c8ab51363693d9 --- lams_central/src/java/org/lamsfoundation/lams/webservice/SPEnrolmentServlet.java (.../SPEnrolmentServlet.java) (revision b8c30f48f0925a8b74bd6090c7766f43096877c5) +++ lams_central/src/java/org/lamsfoundation/lams/webservice/SPEnrolmentServlet.java (.../SPEnrolmentServlet.java) (revision 3080c61fc08c0d5fdc28fb9ad6c8ab51363693d9) @@ -61,9 +61,11 @@ import org.lamsfoundation.lams.logevent.LogEvent; import org.lamsfoundation.lams.logevent.service.ILogEventService; import org.lamsfoundation.lams.usermanagement.Organisation; +import org.lamsfoundation.lams.usermanagement.OrganisationType; import org.lamsfoundation.lams.usermanagement.Role; import org.lamsfoundation.lams.usermanagement.User; import org.lamsfoundation.lams.usermanagement.UserOrganisation; +import org.lamsfoundation.lams.usermanagement.UserOrganisationRole; import org.lamsfoundation.lams.usermanagement.service.IUserManagementService; import org.lamsfoundation.lams.util.Configuration; import org.lamsfoundation.lams.util.ConfigurationKeys; @@ -93,7 +95,7 @@ } private static List getAllRoles() { - return Arrays.asList(LEARNER.getRole(), STAFF.getRole(), MANAGER.getRole()); + return Arrays.asList(MANAGER.getRole(), LEARNER.getRole(), STAFF.getRole()); } }; @@ -155,10 +157,6 @@ throw new ServletException("File is empty"); } - // map of course code (ID) -> course name - ConcurrentMap courses = allLines.parallelStream().unordered().collect( - Collectors.toConcurrentMap(elem -> elem.get(0), elem -> elem.get(1), (elem1, elem2) -> elem1)); - // map of user login -> user ID Map userIDs = new HashMap<>(); @@ -168,30 +166,81 @@ // map of user login -> email, first name // for learner email is login, for staff it is a different ID in email format - ConcurrentMap allUsersMapped = allLines.parallelStream().unordered() + ConcurrentMap allParsedUserMapping = allLines.parallelStream().unordered() .collect(Collectors.toConcurrentMap( elem -> elem.get(6).equals(Mode.STAFF.getRole()) || elem.get(6).equals(Mode.MANAGER.getRole()) ? elem.get(3) : elem.get(5), elem -> new String[] { elem.get(5), elem.get(4) }, (elem1, elem2) -> elem1)); + logger.info("Found " + allParsedUserMapping.size() + " users in the file"); Integer extServerSid = extServer.getSid(); // load all users from DB which are present in the output file // map of user login -> user - Map allExistingUsers = userManagementService - .findByPropertyValues(User.class, "login", allUsersMapped.keySet()).parallelStream() + Map allExistingParsedUsers = userManagementService + .findByPropertyValues(User.class, "login", allParsedUserMapping.keySet()).parallelStream() .collect(Collectors.toConcurrentMap(User::getLogin, u -> u)); + + logger.info(allExistingParsedUsers.size() + " users already exist"); + // load all ext users from DB which are present in the output file // map of user login -> extUser - Map allExistingExtUsers = userManagementService - .findByPropertyValues(ExtUserUseridMap.class, "extUsername", allExistingUsers.keySet()) + Map allExistingParsedExtUsers = userManagementService + .findByPropertyValues(ExtUserUseridMap.class, "extUsername", allExistingParsedUsers.keySet()) .parallelStream().filter(e -> e.getExtServer().getSid().equals(extServerSid)) .collect(Collectors.toConcurrentMap(ExtUserUseridMap::getExtUsername, e -> e)); - // CREATE USERS and ext users - Set allUsersParsed = createUsers(extServer, creatorId, allUsersMapped, userIDs, allExistingUsers, - allExistingExtUsers); - Set allUsersinCourses = new HashSet<>(); + // create users and ext users + Set allParsedUsers = createUsers(extServer, creatorId, allParsedUserMapping, userIDs, + allExistingParsedUsers, allExistingParsedExtUsers); + // map of course code (ID) -> course name + // for all organisations present in the output file + ConcurrentMap allParsedCourseMapping = allLines.parallelStream().unordered().collect( + Collectors.toConcurrentMap(elem -> elem.get(0), elem -> elem.get(1), (elem1, elem2) -> elem1)); + + logger.info("Found " + allParsedCourseMapping.size() + " courses in the file"); + + // load all organisations from DB which are present in the output file, by code + // map of code -> organisation + Map allExistingParsedCourses = userManagementService + .findByPropertyValues(Organisation.class, "code", allParsedCourseMapping.keySet()) + .parallelStream().collect(Collectors.toConcurrentMap(Organisation::getCode, o -> o)); + + logger.info(allExistingParsedCourses.size() + " courses already exist"); + + // prepare codes of all suborganisations disregarding which organisation is their parent + Set allParsedSubcourseMapping = allLines.stream() + .collect(Collectors.mapping(elem -> elem.get(2), Collectors.toSet())); + + logger.info("Found " + allParsedSubcourseMapping.size() + " subcourses in the file"); + + // load all suborganisations from DB which are present in the output file + List allExistingParsedSubcourses = userManagementService + .findByPropertyValues(Organisation.class, "code", allParsedSubcourseMapping); + + logger.info(allParsedSubcourseMapping.size() + " subcourses already exist"); + + // map of course ID -> subcourse code -> subcourse + Map> allExistingParsedCoursesAndSubcourses = allExistingParsedSubcourses + .parallelStream().filter(o -> o.getParentOrganisation() != null) + .collect(Collectors.groupingByConcurrent(o -> o.getParentOrganisation().getOrganisationId(), + Collectors.toConcurrentMap(Organisation::getCode, o -> o))); + + // load all ext courses and subcourses from DB which are present in the output file + Map allExistingParsedExtCourses = userManagementService + .findByPropertyValues(ExtCourseClassMap.class, "classid", Stream + // merge IDs of organisations and suborganisations + .concat(allExistingParsedCourses.values().stream(), + allExistingParsedSubcourses.stream()) + .map(Organisation::getOrganisationId).collect(Collectors.toSet())) + .parallelStream().filter(e -> e.getExtServer().getSid().equals(extServerSid)) + .collect(Collectors.toConcurrentMap(e -> e.getOrganisation().getOrganisationId(), e -> e)); + + Set allExistingUsersFromParsedCourses = Stream + .concat(allExistingParsedCourses.values().stream(), allExistingParsedSubcourses.stream()) + .flatMap(o -> o.getUserOrganisations().stream()) + .collect(Collectors.mapping(UserOrganisation::getUser, Collectors.toSet())); + // map lines into corresponding roles Map>> linesByMode = allLines.stream() .collect(Collectors.groupingByConcurrent(row -> row.get(6))); @@ -215,22 +264,15 @@ } // map of user login -> course ID -> role IDs - // for all users which are present in the output file - Map>> allExistingRoles = userManagementService - .findByPropertyValues(UserOrganisation.class, "user.userId", - allExistingUsers.values().parallelStream() - .collect(Collectors.mapping(User::getUserId, Collectors.toSet()))) - .stream() + // for all organisations which are present in the output file + Map>> allExistingRoles = Stream + .concat(allExistingParsedCourses.values().stream(), allExistingParsedSubcourses.stream()) + .flatMap(o -> o.getUserOrganisations().stream()) .collect(Collectors.groupingBy(uo -> uo.getUser().getLogin(), Collectors.toMap( userOrganisation -> userOrganisation.getOrganisation().getOrganisationId(), userOrganisation -> userOrganisation.getUserOrganisationRoles().stream() .map(userOrganisationRole -> userOrganisationRole.getRole().getRoleId()) .collect(Collectors.toSet())))); - // load all organisations from DB which are present in the output file, by code - // map of code -> organisation - Map allExistingOrganisations = userManagementService - .findByPropertyValues(Organisation.class, "code", courses.keySet()).parallelStream() - .collect(Collectors.toConcurrentMap(Organisation::getCode, o -> o)); // When setting group managers, just process courses, not subcourses and lessons if (mode == Mode.MANAGER) { @@ -240,21 +282,20 @@ Collectors.mapping(elem -> elem.get(3), Collectors.toList()))); AtomicInteger mappingsProcessed = new AtomicInteger(); - logger.info("Processing courses and assigments"); + logger.info("Processing manager courses and assigments"); - for (String courseCode : courses.keySet()) { - Organisation course = allExistingOrganisations.get(courseCode); + for (Entry courseEntry : allParsedCourseMapping.entrySet()) { + Organisation course = getCourse(courseEntry.getKey(), courseEntry.getValue(), extServer, + creatorId, rootOrganisation, allExistingParsedCourses, allExistingParsedExtCourses); - Collection courseUsers = assignManagers(course, creatorId, mappings, allUsersParsed, - userIDs, allExistingRoles, allExistingUsers, mappingsProcessed); + assignManagers(course, creatorId, mappings, allParsedUsers, userIDs, allExistingRoles, + allExistingParsedUsers, mappingsProcessed); - allUsersinCourses.addAll(courseUsers); - logger.info("Processed " + mappingsProcessed.get() + " entries"); } - logger.info("Processing \"" + role + "\" role finished"); + logger.info("Processing manager role finished"); - // END OF GROUP MANAGER PROCESSING! + // END OF GROUP MANAGER PROCESSING continue; } @@ -267,55 +308,30 @@ Collectors.mapping(elem -> mode == Mode.STAFF ? elem.get(3) : elem.get(5), Collectors.toList())))); - // prepare codes of all suborganisations disregarding which organisation is their parent - Set allExistingSubOrganisationCodes = mappings.values().stream() - .flatMap(e -> e.keySet().stream()).collect(Collectors.toSet()); - - // load all suborganisations from DB which are present in the output file - List allExistingSubcourseObjects = userManagementService - .findByPropertyValues(Organisation.class, "code", allExistingSubOrganisationCodes); - - // map of course code -> subcourse code -> subcourse - Map> allExistingSubcourses = allExistingSubcourseObjects - .parallelStream().filter(o -> o.getParentOrganisation() != null) - .collect(Collectors.groupingByConcurrent(o -> o.getParentOrganisation().getOrganisationId(), - Collectors.toConcurrentMap(Organisation::getCode, o -> o))); - - // load all ext courses and subcourses from DB which are present in the output file - Map allExistingExtCourses = userManagementService - .findByPropertyValues(ExtCourseClassMap.class, "classid", Stream - // merge IDs of organisations and suborganisations - .concat(allExistingOrganisations.values().stream() - .map(Organisation::getOrganisationId), - allExistingSubcourseObjects.stream().map(Organisation::getOrganisationId)) - .collect(Collectors.toSet())) - .parallelStream().filter(e -> e.getExtServer().getSid().equals(extServerSid)) - .collect(Collectors.toConcurrentMap(e -> e.getOrganisation().getOrganisationId(), e -> e)); - // go through each course AtomicInteger mappingsProcessed = new AtomicInteger(); logger.info("Processing courses and assigments"); - for (Entry courseEntry : courses.entrySet()) { + + for (Entry courseEntry : allParsedCourseMapping.entrySet()) { String courseCode = courseEntry.getKey(); // create or get existing course Organisation course = getCourse(courseCode, courseEntry.getValue(), extServer, creatorId, - rootOrganisation, allExistingOrganisations, allExistingExtCourses); - // collect learners from all subcourses of this course - Set allUsersInSubcourses = assignLearnersOrStaff(course, mappings, extServer, creatorId, - allUsersParsed, userIDs, allExistingRoles, allExistingSubcourses, allExistingExtCourses, - allExistingUsers, mode == Mode.STAFF, mappingsProcessed); - allUsersinCourses.addAll(allUsersInSubcourses); + rootOrganisation, allExistingParsedCourses, allExistingParsedExtCourses); + // assign learners and staff to subcourses of this course + assignLearnersOrStaff(course, mappings, extServer, creatorId, allParsedUsers, userIDs, + allExistingRoles, allExistingParsedCoursesAndSubcourses, allExistingParsedExtCourses, + allExistingParsedUsers, mode == Mode.STAFF, mappingsProcessed); logger.info("Processed " + mappingsProcessed.get() + " entries"); } - logger.info("Processing \"" + role + "\" role finished"); + logger.info("Processing " + role + " role finished"); } logger.info("Disabling users"); // users who are part of courses but are not in the file anymore are eligible for disabling - allUsersinCourses.removeAll(allUsersParsed); - for (User user : allUsersinCourses) { + allExistingUsersFromParsedCourses.removeAll(allParsedUsers); + for (User user : allExistingUsersFromParsedCourses) { // make a flat set of roles from all subcourses Set roles = userManagementService.getRolesForUser(user.getUserId()).values().stream() .collect(HashSet::new, Set::addAll, Set::addAll); @@ -406,9 +422,9 @@ } private Organisation getCourse(String courseCode, String courseName, ExtServer extServer, Integer creatorId, - Organisation rootOrganisation, Map allExistingOrganisations, - Map allExistingExtCourses) throws UserInfoValidationException { - Organisation course = allExistingOrganisations.get(courseCode); + Organisation rootOrganisation, Map allExistingParsedCourses, + Map allExistingParsedExtCourses) throws UserInfoValidationException { + Organisation course = allExistingParsedCourses.get(courseCode); // create course if (course == null) { String name = courseName; @@ -418,8 +434,8 @@ course.setCode(courseCode); userManagementService.save(course); - allExistingOrganisations.put(courseCode, course); - allExistingExtCourses.put(extOrgMap.getOrganisation().getOrganisationId(), extOrgMap); + allExistingParsedCourses.put(courseCode, course); + allExistingParsedExtCourses.put(extOrgMap.getOrganisation().getOrganisationId(), extOrgMap); String message = "Course created with code \"" + courseCode + "\" and name \"" + name + "\" and ID " + course.getOrganisationId(); @@ -429,15 +445,15 @@ } else { String name = course.getName(); - ExtCourseClassMap extOrgMap = allExistingExtCourses.get(course.getOrganisationId()); + ExtCourseClassMap extOrgMap = allExistingParsedExtCourses.get(course.getOrganisationId()); if (extOrgMap == null) { extOrgMap = new ExtCourseClassMap(); extOrgMap.setCourseid(name); extOrgMap.setExtServer(extServer); extOrgMap.setOrganisation(course); userManagementService.save(extOrgMap); - allExistingExtCourses.put(course.getOrganisationId(), extOrgMap); + allExistingParsedExtCourses.put(course.getOrganisationId(), extOrgMap); String message = "External course created for existing course with code \"" + courseCode + "\" and name \"" + name + "\" and ID " + course.getOrganisationId(); @@ -450,19 +466,19 @@ } @SuppressWarnings("unchecked") - private Set assignLearnersOrStaff(Organisation course, Map>> mappings, + private void assignLearnersOrStaff(Organisation course, Map>> mappings, ExtServer extServer, Integer creatorId, Set allUsersParsed, Map userIDs, Map>> allExistingRoles, - Map> allExistingSubcourses, - Map allExistingExtCourses, Map allExistingUsers, + Map> allExistingParsedCoursesAndSubcourses, + Map allExistingParsedExtCourses, Map allExistingParsedUsers, boolean isStaffMode, AtomicInteger mappingsProcessed) throws UserInfoValidationException { - Set allUsersInSubcourses = new HashSet<>(); String courseCode = course.getCode(); Integer courseId = course.getOrganisationId(); // go through each subcourse for (Entry> subcourseEntry : mappings.get(courseCode).entrySet()) { String subcourseCode = subcourseEntry.getKey(); - ConcurrentMap existingSubcourses = allExistingSubcourses.get(courseId); + ConcurrentMap existingSubcourses = allExistingParsedCoursesAndSubcourses + .get(courseId); Organisation subcourse = existingSubcourses == null ? null : existingSubcourses.get(subcourseCode); // create subcourse @@ -475,10 +491,10 @@ if (existingSubcourses == null) { existingSubcourses = new ConcurrentHashMap<>(); - allExistingSubcourses.put(courseId, existingSubcourses); + allExistingParsedCoursesAndSubcourses.put(courseId, existingSubcourses); } existingSubcourses.put(subcourse.getCode(), subcourse); - allExistingExtCourses.put(extSubOrgMap.getOrganisation().getOrganisationId(), extSubOrgMap); + allExistingParsedExtCourses.put(extSubOrgMap.getOrganisation().getOrganisationId(), extSubOrgMap); String message = "Subcourse created with code and name \"" + courseCode + "\" and ID " + subcourse.getOrganisationId(); @@ -488,7 +504,7 @@ } else { String name = subcourse.getName(); - ExtCourseClassMap extOrgMap = allExistingExtCourses.get(subcourse.getOrganisationId()); + ExtCourseClassMap extOrgMap = allExistingParsedExtCourses.get(subcourse.getOrganisationId()); if (extOrgMap == null) { extOrgMap = new ExtCourseClassMap(); extOrgMap.setCourseid(name); @@ -507,13 +523,23 @@ Integer subcourseId = subcourse.getOrganisationId(); // get existing learners/staff members for given subcourse - Collection subcourseUsers = userManagementService.getUsersFromOrganisationByRole(subcourseId, - isStaffMode ? Role.MONITOR : Role.LEARNER, true); - // add users to set containing all users in any subcourse which is processed - allUsersInSubcourses.addAll(subcourseUsers); + Collection subcourseMonitorsOrLearners = userManagementService + .getUsersFromOrganisationByRole(subcourseId, isStaffMode ? Role.MONITOR : Role.LEARNER, true); + Collection subcourseUsers = new HashSet<>(subcourseMonitorsOrLearners); + if (isStaffMode) { + // make sure that staff has both monitor and author roles in subcourse, + // even if they are course managers in the parent organisations + // and they already have a monitor role in subcourse + Collection authors = userManagementService.getUsersFromOrganisationByRole(subcourseId, + Role.AUTHOR, true); + subcourseUsers.retainAll(authors); + } // go through each user for (String login : subcourseEntry.getValue()) { + + logger.info("Processing \"" + login + "\""); + mappingsProcessed.incrementAndGet(); // check if the user is already a learner/staff member in the subcourse @@ -522,10 +548,12 @@ Integer userId = userIDs.get(login); Iterator subcourseUserIterator = subcourseUsers.iterator(); while (subcourseUserIterator.hasNext()) { - if (userId.equals(subcourseUserIterator.next().getUserId())) { + User user = subcourseUserIterator.next(); + if (userId.equals(user.getUserId())) { // IMPORTANT: if we found a matching existing learner/staff member, we get remove him from this collection // so after this loop he does not get removed from subcourses subcourseUserIterator.remove(); + subcourseMonitorsOrLearners.remove(user); userAlreadyAssigned = true; break; } @@ -547,7 +575,7 @@ } else { existingSubcourseRoles.add(Role.ROLE_LEARNER); } - User user = allExistingUsers.get(login); + User user = allExistingParsedUsers.get(login); userManagementService.setRolesForUserOrganisation(user, subcourse, existingSubcourseRoles.stream().map(String::valueOf).collect(Collectors.toList()), false); @@ -566,30 +594,13 @@ "SPEnrolment: " + message); } - // user is a learner/staff member, but he should not; remove him role from subcourse and lessons - for (User user : subcourseUsers) { - Map> existingSubcoursesRoles = allExistingRoles.get(user.getLogin()); - Set existingSubcourseRoles = existingSubcoursesRoles == null ? null - : existingSubcoursesRoles.get(subcourseId); - if (existingSubcourseRoles != null) { - if (isStaffMode) { - existingSubcourseRoles.remove(Role.ROLE_AUTHOR); - existingSubcourseRoles.remove(Role.ROLE_MONITOR); - } else { - existingSubcourseRoles.remove(Role.ROLE_LEARNER); - } - userManagementService.setRolesForUserOrganisation(user, subcourse, - existingSubcourseRoles.stream().map(String::valueOf).collect(Collectors.toList()), false); + // user is a learner/staff member, but he should not; remove him role from subcourse, course and lessons + for (User user : subcourseMonitorsOrLearners) { - for (Lesson lesson : lessonService.getLessonsByGroup(subcourseId)) { - if (isStaffMode) { - lessonService.removeStaffMember(lesson.getLessonId(), user.getUserId()); - } else { - lessonService.removeLearner(lesson.getLessonId(), user.getUserId()); - } - - } - + boolean removedFromSubcourse = removeFromCourse(subcourse, user, + isStaffMode ? Mode.STAFF : Mode.LEARNER, allExistingParsedCoursesAndSubcourses, + allExistingRoles); + if (removedFromSubcourse) { String message = (isStaffMode ? "Teacher" : "Learner") + " \"" + user.getLogin() + "\" removed from subcourse " + subcourseId + " and its lessons"; logger.info(message); @@ -598,27 +609,83 @@ } } } + } - return allUsersInSubcourses; + private boolean removeFromCourse(Organisation course, User user, Mode mode, + Map> allExistingParsedCoursesAndSubcourses, + Map>> allExistingRoles) { + // no existing roles and the user should be removed - nothing to do + Map> existingCoursesRoles = allExistingRoles.get(user.getLogin()); + Set existingCourseRoles = existingCoursesRoles == null ? null + : existingCoursesRoles.get(course.getOrganisationId()); + if (existingCourseRoles == null || existingCourseRoles.isEmpty()) { + return false; + } + + Organisation parentCourse = course.getOrganisationType().getOrganisationTypeId() + .equals(OrganisationType.CLASS_TYPE) ? course.getParentOrganisation() : null; + + if (mode == Mode.MANAGER) { + existingCourseRoles.remove(Role.ROLE_GROUP_MANAGER); + } else if (mode == Mode.STAFF) { + // managers are always monitors in subcourses + if (parentCourse != null + && userManagementService.hasRoleInOrganisation(user, Role.ROLE_GROUP_MANAGER, parentCourse)) { + return false; + } + + existingCourseRoles.remove(Role.ROLE_MONITOR); + existingCourseRoles.remove(Role.ROLE_AUTHOR); + } else { + existingCourseRoles.remove(Role.ROLE_LEARNER); + } + + userManagementService.setRolesForUserOrganisation(user, course, + existingCourseRoles.stream().map(String::valueOf).collect(Collectors.toList()), false); + + // add learners and staff to lessons + if (mode != Mode.MANAGER) { + for (Lesson lesson : lessonService.getLessonsByGroup(course.getOrganisationId())) { + if (mode == Mode.STAFF) { + lessonService.removeStaffMember(lesson.getLessonId(), user.getUserId()); + } else { + lessonService.removeLearner(lesson.getLessonId(), user.getUserId()); + } + } + + // if learner or staff is not in any subcourse, remove him also from parent course + if (course.getOrganisationType().getOrganisationTypeId().equals(OrganisationType.CLASS_TYPE)) { + for (Organisation subcourse : parentCourse.getChildOrganisations()) { + List rolesInSubcourse = userManagementService + .getUserOrganisationRoles(subcourse.getOrganisationId(), user.getLogin()); + if (!rolesInSubcourse.isEmpty()) { + return true; + } + } + removeFromCourse(parentCourse, user, mode, allExistingParsedCoursesAndSubcourses, allExistingRoles); + } + } + + return true; } @SuppressWarnings("unchecked") - private Collection assignManagers(Organisation course, Integer creatorId, Map> mappings, + private void assignManagers(Organisation course, Integer creatorId, Map> mappings, Set allUsersParsed, Map userIDs, - Map>> allExistingRoles, Map allExistingUsers, + Map>> allExistingRoles, Map allExistingParsedUsers, AtomicInteger mappingsProcessed) throws UserInfoValidationException { String courseCode = course.getCode(); Integer courseId = course.getOrganisationId(); // get existing managers for given course Collection courseUsers = userManagementService.getUsersFromOrganisationByRole(courseId, Role.GROUP_MANAGER, true); - Set initialCourseUsers = new HashSet<>(courseUsers); // go through each user List courseMappings = mappings.get(courseCode); if (courseMappings != null) { for (String login : courseMappings) { + logger.info("Processing manager \"" + login + "\""); mappingsProcessed.incrementAndGet(); // check if the user is already a manager is the course @@ -633,11 +700,9 @@ break; } } - if (userAlreadyAssigned) { - continue; - } - // the user is not a manager yet, so assign him the role + // always set roles for course managers as subcourses could have been added + Map> existingCoursesRoles = allExistingRoles.get(login); Set existingCourseRoles = existingCoursesRoles == null ? null : allExistingRoles.get(login).get(courseId); @@ -646,40 +711,29 @@ } existingCourseRoles.add(Role.ROLE_GROUP_MANAGER); - User user = allExistingUsers.get(login); + User user = allExistingParsedUsers.get(login); userManagementService.setRolesForUserOrganisation(user, course, - existingCourseRoles.stream().map(String::valueOf).collect(Collectors.toList()), false); + existingCourseRoles.stream().map(String::valueOf).collect(Collectors.toList()), true); - String message = "Group manager \"" + login + "\" added to course"; - logger.info(message); - logEventService.logEvent(LogEvent.TYPE_USER_ORG_ADMIN, creatorId, null, null, null, - "SPEnrolment: " + message); + if (!userAlreadyAssigned) { + String message = "Manager \"" + login + "\" added to course"; + logger.info(message); + logEventService.logEvent(LogEvent.TYPE_USER_ORG_ADMIN, creatorId, null, null, null, + "SPEnrolment: " + message); + } } } // user is a group manager, but he should not; remove him role from course for (User user : courseUsers) { - Map> existingCoursesRoles = allExistingRoles.get(user.getLogin()); - Set existingSubcourseRoles = null; - if (existingCoursesRoles == null) { - existingSubcourseRoles = Set.of(); - } else { - existingSubcourseRoles = existingCoursesRoles.get(courseId); - if (existingSubcourseRoles == null) { - existingSubcourseRoles = Set.of(); - } else { - existingSubcourseRoles.remove(Role.ROLE_GROUP_MANAGER); - } + boolean removedFromCourse = removeFromCourse(course, user, Mode.MANAGER, null, allExistingRoles); + if (removedFromCourse) { + String message = "Manager \"" + user.getLogin() + "\" removed from course " + courseId; + logger.info(message); + logEventService.logEvent(LogEvent.TYPE_USER_ORG_ADMIN, creatorId, null, null, null, + "SPEnrolment: " + message); } - userManagementService.setRolesForUserOrganisation(user.getUserId(), courseId, existingSubcourseRoles); - - String message = "Group manager \"" + user.getLogin() + "\" removed from course " + courseId; - logger.info(message); - logEventService.logEvent(LogEvent.TYPE_USER_ORG_ADMIN, creatorId, null, null, null, - "SPEnrolment: " + message); } - - return initialCourseUsers; } @Override Index: lams_common/src/java/org/lamsfoundation/lams/usermanagement/service/UserManagementService.java =================================================================== diff -u -rbbd3fae64f21997ab27690bb74f20e9b6e4af61d -r3080c61fc08c0d5fdc28fb9ad6c8ab51363693d9 --- lams_common/src/java/org/lamsfoundation/lams/usermanagement/service/UserManagementService.java (.../UserManagementService.java) (revision bbd3fae64f21997ab27690bb74f20e9b6e4af61d) +++ lams_common/src/java/org/lamsfoundation/lams/usermanagement/service/UserManagementService.java (.../UserManagementService.java) (revision 3080c61fc08c0d5fdc28fb9ad6c8ab51363693d9) @@ -799,7 +799,7 @@ } if (checkGroupManagerRoles) { - // make sure group managers have monitor and learner in each subgroup + // make sure group managers have monitor in each subgroup checkGroupManager(user, org); } } @@ -829,33 +829,24 @@ uos.add(uo); log.debug("added " + user.getLogin() + " to " + org.getName()); uo = setRoleForUserOrganisation(uo, (Role) findById(Role.class, Role.ROLE_MONITOR)); - uo = setRoleForUserOrganisation(uo, (Role) findById(Role.class, Role.ROLE_LEARNER)); save(uo); - return; + continue; } // iterate through roles and add monitor and learner if don't // already exist Set uors = uo.getUserOrganisationRoles(); if ((uors != null) && !uors.isEmpty()) { boolean isMonitor = false; - boolean isLearner = false; for (UserOrganisationRole uor : uors) { if (uor.getRole().getName().equals(Role.MONITOR)) { isMonitor = true; - } else if (uor.getRole().getName().equals(Role.LEARNER)) { - isLearner = true; - } - if (isMonitor && isLearner) { break; } } if (!isMonitor) { uo = setRoleForUserOrganisation(uo, (Role) findById(Role.class, Role.ROLE_MONITOR)); } - if (!isLearner) { - uo = setRoleForUserOrganisation(uo, (Role) findById(Role.class, Role.ROLE_LEARNER)); - } save(uo); } }