From 247ae3ea1a39145a5e9e71be27f60ba8b3edac81 Mon Sep 17 00:00:00 2001 From: Kevin Carruth Date: Tue, 19 Oct 2021 12:17:43 -0400 Subject: [PATCH] COL-8459 assignment/edu-services: disable group locking reduced group locks for deletion but enable member changing assignment event observer parse realm.upd events add/remove released grades for gb integration (external only) adding score removal method to gbExtService run sync as admin for 'null' user sessions (authz provider sync) --- .../impl/AssignmentEventObserver.java | 391 ++++++++++++++++++ .../impl/src/webapp/WEB-INF/components.xml | 4 + .../tool/RangeAndGroupsDelegate.java | 22 +- .../GradebookExternalAssessmentService.java | 9 + ...radebookExternalAssessmentServiceImpl.java | 64 +++ 5 files changed, 485 insertions(+), 5 deletions(-) diff --git a/assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentEventObserver.java b/assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentEventObserver.java index 87691398f..721ff66bf 100644 --- a/assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentEventObserver.java +++ b/assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentEventObserver.java @@ -1,34 +1,55 @@ package org.sakaiproject.assignment.impl; +import static org.sakaiproject.assignment.api.AssignmentConstants.*; +import static org.sakaiproject.assignment.api.AssignmentServiceConstants.*; +import static org.sakaiproject.assignment.api.model.Assignment.GradeType.*; + import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; import java.util.Observable; import java.util.Observer; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; import lombok.Setter; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.time.StopWatch; +import org.sakaiproject.assignment.api.AssignmentReferenceReckoner; import org.sakaiproject.assignment.api.AssignmentService; import org.sakaiproject.assignment.api.model.Assignment; import org.sakaiproject.assignment.api.model.AssignmentSubmission; import org.sakaiproject.assignment.api.model.AssignmentSubmissionSubmitter; +import org.sakaiproject.assignment.api.persistence.AssignmentRepository; import org.sakaiproject.event.api.Event; import org.sakaiproject.event.api.EventTrackingService; import org.sakaiproject.exception.IdUnusedException; import org.sakaiproject.exception.PermissionException; import org.sakaiproject.service.gradebook.shared.AssessmentNotFoundException; +import org.sakaiproject.service.gradebook.shared.GradebookExternalAssessmentService; import org.sakaiproject.service.gradebook.shared.GradebookService; +import org.sakaiproject.site.api.Group; +import org.sakaiproject.site.api.Site; +import org.sakaiproject.site.api.SiteService; +import org.sakaiproject.tool.api.Session; +import org.sakaiproject.tool.api.SessionManager; import org.sakaiproject.user.api.User; import org.sakaiproject.user.api.UserDirectoryService; import org.sakaiproject.user.api.UserNotDefinedException; +import org.sakaiproject.util.FormattedText; @Slf4j public class AssignmentEventObserver implements Observer { @Setter private AssignmentService assignmentService; + @Setter private AssignmentRepository assignmentRepository; @Setter private EventTrackingService eventTrackingService; @Setter private GradebookService gradebookService; + @Setter private GradebookExternalAssessmentService gradebookExternalAssessmentService; @Setter private UserDirectoryService userDirectoryService; + @Setter private SiteService siteService; + @Setter private SessionManager sessionManager; // whether to update all scores or single submission (based on event type) private enum SubmissionUpdateMode { @@ -101,10 +122,73 @@ public void update(Observable o, Object arg) { } } break; + case "realm.upd": // group membership(s) have been changed + // NOTE: don't care about realm.add since new groups won't be associated + // don't care about realm.del since associated groups are locked against deletion + if (parts.length >= 5) { + final String siteId = parts[2]; + final String groupId = parts[4]; + + try { + syncAssignmentGroups(siteId, groupId); + } catch (Exception e) { + log.warn( + "caught exception syncing groups for s {}, g {}: {} ; {}", + siteId, + groupId, + e.getMessage(), + e.getStackTrace()); + } + } + break; default: log.debug("This observer is not interested in event [{}]", event); break; } + } else if (event.getModify() && StringUtils.isNotBlank(event.getEvent())) { + // realm updates posted by CM have a null context + String[] parts = StringUtils.split(event.getResource(), '/'); + switch (event.getEvent()) { + case "realm.upd": // group membership(s) have been changed + // NOTE: don't care about realm.add since new groups won't be associated + // don't care about realm.del since associated groups are locked against deletion + if (parts.length >= 5) { + final String siteId = parts[2]; + final String groupId = parts[4]; + + // CmSync logs in and runs as admin, but the follow-up authzGroup provider sync + // triggered by CM runs as 'null' user. This fails various assignment/gradebook + // user permission checks, so we'll execute the sync method for these realms + // as the 'admin' user as well + Session session = sessionManager.getCurrentSession(); + final String sessionUserId = session.getUserId(); + final String sessionUserEid = session.getUserEid(); + if (sessionUserId == null) { + session.setUserId(UserDirectoryService.ADMIN_ID); + session.setUserEid(UserDirectoryService.ADMIN_EID); + } + + try { + syncAssignmentGroups(siteId, groupId); + } catch (Exception e) { + log.warn( + "caught exception syncing groups for s {}, g {}: {} ; {}", + siteId, + groupId, + e.getMessage(), + e.getStackTrace()); + } + + // restore the old session user if we switched it + if (sessionUserId == null) { + session.setUserId(sessionUserId); + session.setUserEid(sessionUserEid); + } + } + break; + default: + break; + } } } } @@ -286,4 +370,311 @@ private void updateSubmissionScore( log.warn("Can't retrieve user {}, {}", studentId, e.getMessage()); } } + + // syncAssignmentGroups() + // compare current group-released submitter list w/ site group's membership, and keep in sync + private void syncAssignmentGroups(final String siteId, final String groupId) { + // start clock + StopWatch stopWatch = new StopWatch(); + stopWatch.start(); + + // get the list of submissions scoped to the given group + Set submissions = new HashSet(); + assignmentRepository + .findAssignmentsBySite(siteId) + .stream() + .filter(a -> a.getIsGroup()) + .forEach( + a -> { + AssignmentSubmission s = null; + try { + s = assignmentRepository.findSubmissionForGroup(a.getId(), groupId); + } catch (Exception e) { + log.warn( + "exception caught during submission lookup for assignment {}, group {}: {}, {}", + a.getId(), + groupId, + e.getMessage(), + e.getStackTrace()); + } + + if (s != null) { + submissions.add(s); + } + }); + + final String realmId = "/site/" + siteId + "/group/" + groupId; + log.debug("TIMER ({}): submissions fetch: {}", realmId, stopWatch.getTime()); + + if (submissions == null || submissions.isEmpty()) { + // no group-submission submissions, nothing to do + return; + } + + // get the list of intended group members via site group lookup + Site site; + Set groupSubmitters = new HashSet<>(); + try { + site = siteService.getSite(siteId); + if (site == null) { + return; + } + Group group = site.getGroup(groupId); + if (group != null) { + group + .getMembers() + .stream() + .filter( + m -> + (m.getRole().isAllowed(SECURE_ADD_ASSIGNMENT_SUBMISSION) + || group.isAllowed(m.getUserId(), SECURE_ADD_ASSIGNMENT_SUBMISSION)) + && !m.getRole().isAllowed(SECURE_GRADE_ASSIGNMENT_SUBMISSION) + && !group.isAllowed(m.getUserId(), SECURE_GRADE_ASSIGNMENT_SUBMISSION)) + .forEach( + member -> { + groupSubmitters.add(member.getUserId()); + }); + } else { + log.warn("Group lookup failed for {} in site {}", groupId, siteId); + } + + } catch (IdUnusedException iue) { + log.warn( + "Site not found while attempting to sync assignment submitters for site: {}, group: {}", + groupId, + siteId); + return; + } catch (Exception e) { + log.warn( + "exception caught during site lookup for site-id {}: {}, {}", + siteId, + e.getMessage(), + e.getStackTrace()); + return; + } + + log.debug("TIMER ({}): group membership fetch: {}", realmId, stopWatch.getTime()); + + for (AssignmentSubmission s : submissions) { + // assignment info (for logging and grade sync later) + final String assignmentId = s.getAssignment() != null ? s.getAssignment().getId() : null; + + // get current list of submission's submitters + Set submissionSubmitters = s.getSubmitters(); + if (submissionSubmitters == null) + submissionSubmitters = new HashSet(); + + // set up add/drop lists + Set submittersToAdd = new HashSet(); + Set submittersToRemove = new HashSet(); + if (groupSubmitters.isEmpty()) { + submittersToRemove.addAll(submissionSubmitters); + submissionSubmitters.clear(); + } else { + submittersToAdd.addAll(groupSubmitters); + for (AssignmentSubmissionSubmitter ss : submissionSubmitters) { + if (!groupSubmitters.contains(ss.getSubmitter())) { + // remove + submittersToRemove.add(ss); + } else { + // leave alone (remove from to-add list) + submittersToAdd.remove(ss.getSubmitter()); + } + } + + if (submittersToAdd.isEmpty() && submittersToRemove.isEmpty()) { + // NOTE: assuming groups are kept in sync (and thus all submissions w/ same group have + // same submitter list), could potentially return here and re-use lists? + continue; + } + + // remove the deleted submitters + submissionSubmitters.removeAll(submittersToRemove); + + // now add any new submitters + for (String sta : submittersToAdd) { + AssignmentSubmissionSubmitter ass = new AssignmentSubmissionSubmitter(); + ass.setSubmitter(sta); + ass.setSubmission(s); + submissionSubmitters.add(ass); + } + } + + try { + // update the submitter list + assignmentRepository.updateSubmission(s); + log.info( + "COL-8459 assignment submitter list synced w/ site group; site: {}, group: {}, assignment {}, submission {}", + siteId, + groupId, + assignmentId, + s.getId()); + } catch (Exception e) { + log.warn( + "unexpected exception attempting to update submission {}: {} ; {}", + s.getId(), + e.getMessage(), + e.getStackTrace()); + } + + log.debug( + "TIMER ({}): finished group membership sync for submission {}: {}", + realmId, + s.getId(), + stopWatch.getTime()); + + // sync w/ gradebook for externally-maintained assignments, if needed + // basic gist: + // push/add a grade for any added submitters (overwrite OK) + // remove grades for removed users (after excluding same student in other groups) + if (s.getAssignment() != null) { + final Assignment a = s.getAssignment(); + final String integrateWithGradebook = + a.getProperties().get(NEW_ASSIGNMENT_ADD_TO_GRADEBOOK); + String aRef = a.getProperties().get(PROP_ASSIGNMENT_ASSOCIATE_GRADEBOOK_ASSIGNMENT); + if (aRef == null) + aRef = AssignmentReferenceReckoner.reckoner().assignment(a).reckon().getReference(); + + if (Assignment.GradeType.SCORE_GRADE_TYPE.equals(a.getTypeOfGrade()) + && aRef != null + && integrateWithGradebook != null + && !integrateWithGradebook.equals(GRADEBOOK_INTEGRATION_NO)) { + + try { + if (gradebookService.isGradebookDefined(siteId)) { + + boolean isExternalAssignmentDefined = + gradebookExternalAssessmentService.isExternalAssignmentDefined(siteId, aRef); + + // only sync gradebook for externally-maintained assignments + // don't want to overwrite or remove grades entered into gradebook + if (!isExternalAssignmentDefined) { + continue; + } + + // set up maps/lists + HashMap gradesToAdd = new HashMap(); + HashMap commentsToAdd = new HashMap(); + + if (!submittersToAdd.isEmpty() && s.getGradeReleased()) { + // lookup/format grade/comment + String gradeString = StringUtils.trimToNull(s.getGrade()); + String commentString = + FormattedText.convertFormattedTextToPlaintext(s.getFeedbackComment()); + + String grade = + gradeString != null ? getDisplayGrade(gradeString, a.getScaleFactor()) : null; + + for (String sta : submittersToAdd) { + gradesToAdd.put(sta, grade); + commentsToAdd.put(sta, commentString); + } + } + + Set gradesToRemove = new HashSet(); + if (!submittersToRemove.isEmpty()) { + // get the list of other submissions for the assignment that are also released + Set assignmentSubmissions = + a.getSubmissions() + .stream() + .filter( + aSub -> + (!s.getId().equals(aSub.getId()) + && aSub.getSubmitters() != null + && !aSub.getSubmitters().isEmpty() + && aSub.getGradeReleased() + && (StringUtils.isNotEmpty(aSub.getGrade()) + || StringUtils.isNotEmpty(aSub.getFeedbackComment())))) + .collect(Collectors.toSet()); + for (AssignmentSubmissionSubmitter str : submittersToRemove) { + // need to check for possibility of other groups containing same user + // multiple-membership is an error state and assignments tool will warn + // users of it, but don't want to delete released grades that may still be + // valid + AssignmentSubmission firstDupe = null; + for (AssignmentSubmission aSub : assignmentSubmissions) { + for (AssignmentSubmissionSubmitter subSub : aSub.getSubmitters()) { + if (str.getSubmitter().equals(subSub.getSubmitter())) { + firstDupe = aSub; + break; + } + } + if (firstDupe != null) break; + } + + if (firstDupe != null) { + // if we have at least one other group sub w/ submitter, update gb with + // that one + final String firstDupeGradeString = + StringUtils.trimToNull(firstDupe.getGrade()); + final String firstDupeGrade = + firstDupeGradeString != null + ? getDisplayGrade(firstDupeGradeString, a.getScaleFactor()) + : null; + gradesToAdd.put(str.getSubmitter(), firstDupeGrade); + + final String firstDupeComment = + FormattedText.convertFormattedTextToPlaintext( + firstDupe.getFeedbackComment()); + commentsToAdd.put(str.getSubmitter(), firstDupeComment); + + } else { + // if not, queue it for removal + // note: skipping release check, because currently, resubmitted submissions + // clear the released bit, but leave grades in gradebook; students + // removed from group(s) w/ submissions in this state still need their + // grade removed from gb + gradesToRemove.add(str.getSubmitter()); + } + } + } + + // now, process the gradebook adds/updates + for (String gta : gradesToAdd.keySet()) { + final String grade = gradesToAdd.get(gta); + final String commentString = commentsToAdd.get(gta); + // following code lifted from AssignmentToolUtils.integrateGradebook() + gradebookExternalAssessmentService.updateExternalAssessmentScore( + siteId, aRef, gta, (grade != null) ? grade : ""); + gradebookExternalAssessmentService.updateExternalAssessmentComment( + siteId, aRef, gta, (commentString != null) ? commentString : ""); + } // end foreach gradesToAdd + + gradebookExternalAssessmentService.removeExternalAssessmentScores( + siteId, aRef, gradesToRemove); + + if (!gradesToAdd.isEmpty() || !gradesToRemove.isEmpty()) { + log.info( + "COL-8459 assignment grades synced w/ gradebook; site {}, group {}, assignment {}, submission {}", + siteId, + groupId, + assignmentId, + s.getId()); + } + } // end if (gbItemDefined) + } catch (Exception e) { + log.warn( + "unexpected exception attempting to sync submission {} to gradebook {}: {} ; {}", + s.getId(), + siteId, + e.getMessage(), + e.getStackTrace()); + } + + log.debug( + "TIMER ({}): finished gradebook sync for submission {}: {}", + realmId, + s.getId(), + stopWatch.getTime()); + } // end if (various assignment checks) + } // end if s.getAssignment() != null, end of gb-sync code + } // end foreach submissions + + log.debug("TIMER ({}): assignment group sync complete: {}", realmId, stopWatch.getTime()); + stopWatch.stop(); + } + + private String getDisplayGrade(final String gradeString, final int factor) { + return assignmentService.getGradeDisplay(gradeString, SCORE_GRADE_TYPE, factor); + } } diff --git a/assignment/impl/src/webapp/WEB-INF/components.xml b/assignment/impl/src/webapp/WEB-INF/components.xml index 20fa0deaa..77c880d3f 100644 --- a/assignment/impl/src/webapp/WEB-INF/components.xml +++ b/assignment/impl/src/webapp/WEB-INF/components.xml @@ -289,8 +289,12 @@ init-method="init" destroy-method="destroy"> + + + + diff --git a/assignment/tool/src/java/org/sakaiproject/assignment/tool/RangeAndGroupsDelegate.java b/assignment/tool/src/java/org/sakaiproject/assignment/tool/RangeAndGroupsDelegate.java index ce58a6cb6..2373e6a34 100644 --- a/assignment/tool/src/java/org/sakaiproject/assignment/tool/RangeAndGroupsDelegate.java +++ b/assignment/tool/src/java/org/sakaiproject/assignment/tool/RangeAndGroupsDelegate.java @@ -231,13 +231,25 @@ void postSaveAssignmentGroupLocking( lockedGroupsReferences.add(group.getReference()); log.debug("Adding group to lock list: {}", group.getReference()); + // COL-8459 instead of lock.ALL, use lock.DELETE (prevent group deletion but not editing) + // also replace existing lock.ALL so that saving pre-existing assignments enables editing if (!aOldGroups.contains(group.getReference()) - || group.getLockForReference(assignmentReference) == AuthzGroup.RealmLockMode.NONE) { - log.debug("locking group: {}", group.getReference()); - group.setLockForReference(assignmentReference, AuthzGroup.RealmLockMode.ALL); - log.debug("locked group: {}", group.getReference()); - + || group.getLockForReference(assignmentReference) == AuthzGroup.RealmLockMode.NONE + || group.getLockForReference(assignmentReference) == AuthzGroup.RealmLockMode.ALL) { try { + if (group.getLockForReference(assignmentReference) == AuthzGroup.RealmLockMode.ALL) { + // have to clear/save the ALL before setting DELETE + // NOTE: if assignments ever has cause to set locks to MODIFY, or other reasons to set + // to ALL, this logic will need adjustment + log.debug("clearing existing ALL lock"); + group.setLockForReference(assignmentReference, AuthzGroup.RealmLockMode.NONE); + siteService.save(group.getContainingSite()); + } + + log.debug("locking group: {}", group.getReference()); + group.setLockForReference(assignmentReference, AuthzGroup.RealmLockMode.DELETE); + log.debug("locked group: {}", group.getReference()); + siteService.save(group.getContainingSite()); } catch (IdUnusedException e) { log.warn("Cannot find site with id {}", siteId); diff --git a/edu-services/gradebook-service/api/src/java/org/sakaiproject/service/gradebook/shared/GradebookExternalAssessmentService.java b/edu-services/gradebook-service/api/src/java/org/sakaiproject/service/gradebook/shared/GradebookExternalAssessmentService.java index 27ec9a76a..3456db31d 100644 --- a/edu-services/gradebook-service/api/src/java/org/sakaiproject/service/gradebook/shared/GradebookExternalAssessmentService.java +++ b/edu-services/gradebook-service/api/src/java/org/sakaiproject/service/gradebook/shared/GradebookExternalAssessmentService.java @@ -18,6 +18,7 @@ import java.util.List; import java.util.Map; import java.util.OptionalLong; +import java.util.Set; /** * This service is designed for use by external assessment engines. These use the Gradebook as a @@ -213,6 +214,14 @@ public void updateExternalAssessment( public void removeExternalAssessment(String gradebookUid, String externalId) throws GradebookNotFoundException, AssessmentNotFoundException; + public void removeExternalAssessmentScore( + String gradebookUid, String externalId, String studentId) + throws GradebookNotFoundException, AssessmentNotFoundException; + + public void removeExternalAssessmentScores( + String gradebookUid, String externalId, Set studentIds) + throws GradebookNotFoundException, AssessmentNotFoundException; + /** * Updates an external score for an external assignment in the gradebook. * diff --git a/edu-services/gradebook-service/impl/src/java/org/sakaiproject/component/gradebook/GradebookExternalAssessmentServiceImpl.java b/edu-services/gradebook-service/impl/src/java/org/sakaiproject/component/gradebook/GradebookExternalAssessmentServiceImpl.java index bb1dd1935..03f327e5f 100644 --- a/edu-services/gradebook-service/impl/src/java/org/sakaiproject/component/gradebook/GradebookExternalAssessmentServiceImpl.java +++ b/edu-services/gradebook-service/impl/src/java/org/sakaiproject/component/gradebook/GradebookExternalAssessmentServiceImpl.java @@ -351,6 +351,70 @@ public void removeExternalAssessment(final String gradebookUid, final String ext getUserUid()); } + @Override + public void removeExternalAssessmentScore( + final String gradebookUid, final String externalId, final String studentId) + throws GradebookNotFoundException, AssessmentNotFoundException { + Set studentIds = new HashSet(); + studentIds.add(studentId); + this.removeExternalAssessmentScores(gradebookUid, externalId, studentIds); + } + + @Override + public void removeExternalAssessmentScores( + final String gradebookUid, final String externalId, final Set studentIds) + throws GradebookNotFoundException, AssessmentNotFoundException { + // null/empty student list, nothing to do + if (studentIds == null || studentIds.isEmpty()) return; + + // Get the external assignment + final GradebookAssignment asn = getExternalAssignment(gradebookUid, externalId); + if (asn == null) { + throw new AssessmentNotFoundException( + "There is no external assessment id=" + externalId + " in gradebook uid=" + gradebookUid); + } + + final HibernateTemplate hibTempl = getHibernateTemplate(); + hibTempl.execute( + (HibernateCallback) + session -> { + int numDeleted = + session + .createQuery( + "delete GradingEvent where gradableObject=:go and studentId in (:stu)") + .setParameter("go", asn) + .setParameterList("stu", studentIds) + .executeUpdate(); + log.debug("Deleted {} externally defined events", numDeleted); + + numDeleted = + session + .createQuery( + "delete AssignmentGradeRecord where gradableObject=:go and studentId in (:stu)") + .setParameter("go", asn) + .setParameterList("stu", studentIds) + .executeUpdate(); + log.debug("Deleted {} externally defined scores", numDeleted); + + numDeleted = + session + .createQuery( + "delete Comment where gradableObject=:go and studentId in (:stu)") + .setParameter("go", asn) + .setParameterList("stu", studentIds) + .executeUpdate(); + log.debug("Deleted {} externally defined comments", numDeleted); + return null; + }); + + log.debug( + "External assessment scores removed from gradebookUid={}, externalId={} for {} by userUid={}", + gradebookUid, + externalId, + studentIds, + getUserUid()); + } + private GradebookAssignment getExternalAssignment( final String gradebookUid, final String externalId) throws GradebookNotFoundException { final Gradebook gradebook = getGradebook(gradebookUid);