SAK-52336 Samigo improve bulk delete efficiency#14364
Open
ottenhoff wants to merge 6 commits intosakaiproject:masterfrom
Open
SAK-52336 Samigo improve bulk delete efficiency#14364ottenhoff wants to merge 6 commits intosakaiproject:masterfrom
ottenhoff wants to merge 6 commits intosakaiproject:masterfrom
Conversation
Contributor
WalkthroughCollects assessment and published-assessment targets, then performs batched removals, group-unlocks, calendar/task cleanups, event postings, and notification updates; changes published-assessment authorization to use settings-only fetch with null checks; adds assessment-aware bulk document deletion scheduling to the question search index builder. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java`:
- Around line 159-167: In RemoveAssessmentListener's loop over
deleteableAssessments, get the siteId by calling
assessmentService.getAssessmentSiteId(assessmentId) (or fallback to
AgentFacade.getCurrentSiteId()) before calling
assessmentService.removeAssessment(assessmentId), then use that stored siteId
when posting the EventTrackingService.post events and adding to
removedAssessmentIds; do not call getAssessmentSiteId after removeAssessment to
avoid null/exception on hard delete.
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemovePublishedAssessmentListener.java`:
- Around line 85-86: The call to
assessmentService.getSettingsOfPublishedAssessment(assessmentId.toString()) can
return null and will NPE when calling assessment.getCreatedBy(); update
RemovePublishedAssessmentListener to (1) call getSettingsOfPublishedAssessment
using assessmentId directly (remove the redundant .toString()), (2) check the
returned PublishedAssessmentFacade (e.g., variable assessment) for null and bail
out/continue the loop similarly to RemoveAssessmentListener before accessing
assessment.getCreatedBy(), and (3) keep existing logic unchanged otherwise.
In
`@samigo/samigo-impl/src/java/org/sakaiproject/samigo/search/QuestionElasticSearchIndexBuilder.java`:
- Around line 363-374: In deleteAllDocumentForAssessment change the log.error
call to use SLF4J parameterized logging instead of string concatenation; replace
the current concatenated message with something like log.error("Failed to remove
all documents from question index for assessment: {}", assessmentId, e) so the
assessmentId is a placeholder and the IOException (e) is passed as the throwable
to preserve stacktrace.
🧹 Nitpick comments (4)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java (4)
170-170: RedundantPublishedAssessmentServiceinstantiation.
publishedAssessmentServiceis already created on line 94 and used on line 132. This creates a second unnecessary instance. Reuse the existing one.Proposed fix
- PublishedAssessmentService pubAssessmentService = new PublishedAssessmentService(); for (String assessmentId : deleteablePublishedAssessmentIds) { log.debug("assessmentId = {}", assessmentId); - pubAssessmentService.removeAssessment(assessmentId, "remove"); + publishedAssessmentService.removeAssessment(assessmentId, "remove");
106-107: Misleading map variable name:groupIdsByAssessmentIdactually maps groupId → Set<assessmentId>.Looking at
collectGroupUnlocks(line 262), the key inserted isgroupIdand the value set containsassessmentIds. The namegroupIdsByAssessmentIdimplies the opposite mapping. This propagates through lines 106, 157, 177, 195, 271.Consider renaming to
assessmentIdsByGroupIdfor clarity.Also applies to: 256-269
304-325: Use typed collections in new helper method.Lines 309–310 use raw
Listtypes in newly added code. This is inconsistent with the typed collections used earlier in this PR (lines 99–107) and can be improved for type safety.Proposed fix
private void updateInactivePublishedAssessments(AuthorBean author, Set<String> removedPublishedAssessmentIds) { if (removedPublishedAssessmentIds.isEmpty()) { return; } - List inactivePublishedAssessmentList = author.getInactivePublishedAssessments(); - List inactiveList = new ArrayList(); + `@SuppressWarnings`("unchecked") + List<PublishedAssessmentFacade> inactivePublishedAssessmentList = (List<PublishedAssessmentFacade>) author.getInactivePublishedAssessments(); + List<PublishedAssessmentFacade> inactiveList = new ArrayList<>(); boolean isAnyAssessmentRetractForEdit = false; - for (int i = 0; i < inactivePublishedAssessmentList.size(); i++) { - PublishedAssessmentFacade pa = (PublishedAssessmentFacade) inactivePublishedAssessmentList.get(i); + for (PublishedAssessmentFacade pa : inactivePublishedAssessmentList) { if (!removedPublishedAssessmentIds.contains(pa.getPublishedAssessmentId().toString())) { inactiveList.add(pa); if (Integer.valueOf(3).equals(pa.getStatus())) {
256-269: ConsidercomputeIfAbsentto simplify the map-population logic.Simplification
private void collectGroupUnlocks(String assessmentId, Set<String> selectedGroupIds, Map<String, Set<String>> groupIdsByAssessmentId) { if (selectedGroupIds == null || selectedGroupIds.isEmpty()) { return; } for (String groupId : selectedGroupIds) { - Set<String> assessmentIds = groupIdsByAssessmentId.get(groupId); - if (assessmentIds == null) { - assessmentIds = new HashSet<>(); - groupIdsByAssessmentId.put(groupId, assessmentIds); - } - assessmentIds.add(assessmentId); + groupIdsByAssessmentId.computeIfAbsent(groupId, k -> new HashSet<>()).add(assessmentId); } }
...p/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java
Show resolved
Hide resolved
Comment on lines
85
to
86
| // get settings without loading sections/items | ||
| PublishedAssessmentFacade assessment = assessmentService.getSettingsOfPublishedAssessment(assessmentId.toString()); |
Contributor
There was a problem hiding this comment.
Missing null check for getSettingsOfPublishedAssessment result.
RemoveAssessmentListener (lines 132–135) guards against a null return from getSettingsOfPublishedAssessment with a continue. Here, no such guard exists — if the published assessment was already removed or the ID is stale, this will NPE on line 89 (assessment.getCreatedBy()).
Also, assessmentId is already a String (line 80), so .toString() on line 86 is redundant.
Proposed fix
// get settings without loading sections/items
- PublishedAssessmentFacade assessment = assessmentService.getSettingsOfPublishedAssessment(assessmentId.toString());
+ PublishedAssessmentFacade assessment = assessmentService.getSettingsOfPublishedAssessment(assessmentId);
+ if (assessment == null) {
+ log.warn("Published assessment not found for id {}", assessmentId);
+ return;
+ }🤖 Prompt for AI Agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemovePublishedAssessmentListener.java`
around lines 85 - 86, The call to
assessmentService.getSettingsOfPublishedAssessment(assessmentId.toString()) can
return null and will NPE when calling assessment.getCreatedBy(); update
RemovePublishedAssessmentListener to (1) call getSettingsOfPublishedAssessment
using assessmentId directly (remove the redundant .toString()), (2) check the
returned PublishedAssessmentFacade (e.g., variable assessment) for null and bail
out/continue the loop similarly to RemoveAssessmentListener before accessing
assessment.getCreatedBy(), and (3) keep existing logic unchanged otherwise.
...o/samigo-impl/src/java/org/sakaiproject/samigo/search/QuestionElasticSearchIndexBuilder.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java`:
- Around line 171-194: The batch removal loop in RemoveAssessmentListener calls
publishedAssessmentService.removeAssessment and removeFromGradebook (and then
does cleanup like collectGroupUnlocks, calendarService.removeCalendarEvent,
taskService.removeTaskByReference,
samigoAvailableNotificationService.removeScheduledAssessmentNotification)
without per-iteration error handling, so one exception aborts the whole batch;
wrap the body of the loop that iterates deleteablePublishedAssessmentIds in a
try/catch around the sequence that calls
publishedAssessmentService.removeAssessment(assessmentId, "remove") and
removeFromGradebook(assessmentId), log the exception (including assessmentId)
and continue to the next assessment, and only add assessmentId to
removedPublishedAssessmentIds after successful completion of the cleanup steps;
apply the same per-iteration try/catch pattern to the earlier assessment removal
loop that calls publishedAssessmentService.removeAssessment and
removeFromGradebook so one failure doesn’t skip remaining assessments.
🧹 Nitpick comments (1)
samigo/samigo-impl/src/java/org/sakaiproject/samigo/search/QuestionElasticSearchIndexBuilder.java (1)
606-611: Chainedif(notelse if) for resource token rewriting is consistent with existing pattern but has a subtle ordering dependency.Lines 600–611 use sequential
ifblocks, so each condition operates on the (potentially already-rewritten)resourceName. In practice the tokens are mutually exclusive in a single event resource, so this works. Flagging only because anelse ifchain would make the mutual exclusivity explicit and avoid a future maintenance pitfall.
...p/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java
Show resolved
Hide resolved
ern
reviewed
Feb 6, 2026
| } | ||
| } | ||
|
|
||
| private void unlockGroupsForDeletion(Map<String, Set<String>> assessmentIdsByGroupId) { |
Contributor
There was a problem hiding this comment.
When a group assessment is being removed it is correct to remove any locks on the group
- the method name seems in correct because it's not unlocking the group it is simply removing a lock reference, is the group being deleted here, doesn't appear that way
- if all that is happening is removing a lock reference, there probably is no need to save the site which is a heavy operation, just save the authz group if it was updated
- also the authz group deals in references this appears to use an assessmentId? is this correct?
Contributor
Author
There was a problem hiding this comment.
-
Renamed methods to match behavior:
- collectGroupLockRemovals(...) at .../RemoveAssessmentListener.java:291
- clearGroupDeletionLocks(...) at .../RemoveAssessmentListener.java:301- Reduced heavy save cost:
- Only call siteService.save(site) if at least one lock reference was actually changed (siteUpdated flag) at .../RemoveAssessmentListener.java:317 and .../RemoveAssessmentListener.java:332.
- Kept lock key as assessment id intentionally:
- We clear with group.setLockForReference(assessmentId, RealmLockMode.NONE) at .../RemoveAssessmentListener.java:326.
- This matches how locks are created on publish with published assessment id at samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/PublishAssessmentListener.java:278.
I did not switch to direct authz-group save because that is a broader behavior change; current Samigo pattern already uses site save for these group lock updates, and this patch keeps that pattern while avoiding unnecessary saves.
- Reduced heavy save cost:
Contributor
There was a problem hiding this comment.
example from assignments, again site.save is a very heavy operation in comparison:
for (String groupReference : assignment.getGroups()) {
try {
AuthzGroup group = authzGroupService.getAuthzGroup(groupReference);
group.setLockForReference(reference, AuthzGroup.RealmLockMode.NONE);
authzGroupService.save(group);
} catch (GroupNotDefinedException | AuthzPermissionException e) {
log.warn("Exception while removing lock for assignment {}, {}", assignment.getId(), e.toString());
}
}
also I suggest:
- clearRealmLocks or removeRealmLocks
- getRealmLocksForRemoval
…t/ui/listener/author/RemoveAssessmentListener.java:
- Added per-item failure isolation for draft deletions:
- Core delete is wrapped in try/catch and continue on failure (:164-:170).
- Event posting is best-effort with separate try/catch (:172-:177).
- Added per-item failure isolation for published deletions:
- Core delete path (removeAssessment + removeFromGradebook) is wrapped in try/catch and continue on failure (:184-:191).
- removedPublishedAssessmentIds is updated after successful core delete (:187), not blocked by cleanup failures.
- Cleanup operations are individually best-effort and logged:
- group unlock collection (:193-:197)
- calendar event removal (:201-:205)
- event posts (:208-:213)
- task removal (:217-:221)
- scheduled notification removal (:223-:227)
- collectGroupLockRemovals(...) at .../RemoveAssessmentListener.java:291
- clearGroupDeletionLocks(...) at .../RemoveAssessmentListener.java:301
- Reduced heavy save cost:
- Only call siteService.save(site) if at least one lock reference was actually changed (siteUpdated flag) at .../RemoveAssessmentListener.java:317 and .../RemoveAssessmentListener.java:332.
- Kept lock key as assessment id intentionally:
- We clear with group.setLockForReference(assessmentId, RealmLockMode.NONE) at .../RemoveAssessmentListener.java:326.
- This matches how locks are created on publish with published assessment id at samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/PublishAssessmentListener.java:278.
I did not switch to direct authz-group save because that is a broader behavior change; current Samigo pattern already uses site save for these group lock updates, and this patch keeps that pattern while avoiding unnecessary saves.
ern
reviewed
Feb 6, 2026
...p/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java`:
- Around line 184-191: After successfully calling
publishedAssessmentService.removeAssessment(assessmentId, "remove") in
RemoveAssessmentListener, add removedPublishedAssessmentIds.add(assessmentId)
immediately (before calling removeFromGradebook) so the in-memory list reflects
the actual DB deletion even if removeFromGradebook or
SpringBeanLocator.getBean() throws; keep removeFromGradebook(assessmentId) and
subsequent cleanup logic the same but ensure any cleanup relying on
removedPublishedAssessmentIds still runs for that assessment.
...p/src/java/org/sakaiproject/tool/assessment/ui/listener/author/RemoveAssessmentListener.java
Show resolved
Hide resolved
…i/listener/author/RemoveAssessmentListener.java so that in the published-delete loop:
- removedPublishedAssessmentIds.add(assessmentId) now happens immediately after successful:
- publishedAssessmentService.removeAssessment(assessmentId, "remove")
- and before:
- removeFromGradebook(assessmentId)
This ensures UI/in-memory removal tracking reflects actual DB deletion even if gradebook cleanup throws.
ern
reviewed
Feb 6, 2026
Comment on lines
+83
to
+84
| private SiteService siteService = ComponentManager.get(SiteService.class); | ||
| private ToolManager toolManager = ComponentManager.get(ToolManager.class); |
Contributor
There was a problem hiding this comment.
Another improvement you could do is (although you don't have to) is have this class extend SpringBeanAutowiringSupport and then you can use @Autowired to wire up your services managed by spring, this removes the need for ComponentManager all together :).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor
New Features