SAK-48981 msgcntr handle forum and topic permission levels when using Lesson's prerequisites - #14604
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughForum access restrictions were refactored to use injected Sakai services, new membership-aware data-access queries were added, and DiscussionForumManager gained APIs and implementations to apply group-based contributor restrictions. LessonBuilder ForumEntity now uses Optional-based site/tool lookups and delegates group updates to the new APIs. ChangesForum group access restriction refactoring
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
Author
|
This was a nasty issue to solve but I am pretty happy with how this fix evolved, here are the highlights
|
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java`:
- Around line 672-690: The current setGroups method maps incoming group IDs to
site group titles and then blindly passes the resulting collection to
discussionForumManager, but if the caller supplied a non-empty groups collection
and none of the IDs resolve, the code will pass an empty collection which
downstream treats as "clear restrictions"; instead, after resolving IDs (using
siteService and toolManager as you already do in setGroups), detect the three
cases: 1) groups == null -> do nothing and return, 2) groups.isEmpty() -> caller
explicitly wants to clear restrictions so proceed to call
discussionForumManager.setTopicGroupRestrictions / setForumGroupRestrictions
with an empty collection, and 3) groups non-empty but groupNames.isEmpty() ->
treat as unresolved IDs and abort (return) without calling
discussionForumManager (optionally log a warning); make this change in setGroups
(handling TYPE_FORUM_TOPIC / TYPE_FORUM_FORUM) before invoking
discussionForumManager.setTopicGroupRestrictions or setForumGroupRestrictions.
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java`:
- Around line 2429-2446: Validate groupNames against the site membership before
creating DBMembershipItems: in setTopicGroupRestrictions (use topic to get
site/area via topic.getBaseForum().getArea()) and in setForumGroupRestrictions
(use forum.getArea()) resolve each entry in groupNames to an existing site group
id/title and filter out or reject any names that do not map to a current group;
if any unknown names are found, fail fast (return or throw) or omit them per
policy, and only pass the validated collection to
applyGroupRestrictions(topic.getMembershipItemSet(), validatedGroupNames) or
applyGroupRestrictions(forum.getMembershipItemSet(), validatedGroupNames) so no
unmatched titles become persisted DBMembershipItems.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b1b4baa-d411-43f2-a12e-934a1abd1599
📒 Files selected for processing (7)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/MessageForumsForumManager.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/ui/DiscussionForumManager.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/MessageForumsForumManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.javamsgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/OpenForum.hbm.xmlmsgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/Topic.hbm.xml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java (1)
671-677:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
nullgroup updates as a no-op.Line 672 collapses
nullinto an empty set, and the new manager treats an empty collection as “clear all restrictions”. That means a caller that omits groups can accidentally reopen a previously restricted forum/topic to the whole site.Suggested fix
public void setGroups(Collection<String> groups) { - Set<String> groupIds = (groups == null) ? Collections.emptySet() : new HashSet<>(groups); + if (groups == null) { + return; + } + Set<String> groupIds = new HashSet<>(groups); if (type == TYPE_FORUM_TOPIC) { discussionForumManager.setTopicGroupRestrictions(id, groupIds); } else if (type == TYPE_FORUM_FORUM) { discussionForumManager.setForumGroupRestrictions(id, groupIds); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java` around lines 671 - 677, The current setGroups method collapses a null groups argument into an empty set which the discussionForumManager interprets as "clear restrictions"; change setGroups to treat null as a no-op by returning early when groups == null, and only compute groupIds and call discussionForumManager.setTopicGroupRestrictions(id, groupIds) or setForumGroupRestrictions(id, groupIds) when groups is non-null; keep references to the existing symbols (setGroups, groups, groupIds, type, TYPE_FORUM_TOPIC, TYPE_FORUM_FORUM, id, discussionForumManager) so the behavior change is limited to adding the null-check and early return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java`:
- Around line 671-677: The current setGroups method collapses a null groups
argument into an empty set which the discussionForumManager interprets as "clear
restrictions"; change setGroups to treat null as a no-op by returning early when
groups == null, and only compute groupIds and call
discussionForumManager.setTopicGroupRestrictions(id, groupIds) or
setForumGroupRestrictions(id, groupIds) when groups is non-null; keep references
to the existing symbols (setGroups, groups, groupIds, type, TYPE_FORUM_TOPIC,
TYPE_FORUM_FORUM, id, discussionForumManager) so the behavior change is limited
to adding the null-check and early return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38320702-0b6b-4371-80cf-6850056468eb
📒 Files selected for processing (3)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/ui/DiscussionForumManager.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- msgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/ui/DiscussionForumManager.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java`:
- Around line 1700-1705: The comparison misses changes when
oldItem.getPermissionLevel() is null for a custom level because
getPermissionLevelByName(...) returns the shared PERMISSION_LEVEL_NAME_CUSTOM
entry; update the logic around PermissionLevel oldLevel (and the derived
oldModerate) to detect the case where oldItem.getPermissionLevel() == null &&
PERMISSION_LEVEL_NAME_CUSTOM.equals(oldItem.getPermissionLevelName()) and treat
that as "changed" (e.g., force oldModerate to a value that will not equal
permBean.getModeratePostings() so the condition in the if
(permBean.getModeratePostings() != Boolean.TRUE.equals(oldModerate)) branch
evaluates true); adjust the same pattern found at the other locations (around
lines 7288-7308) using the same checks and symbols
(oldItem.getPermissionLevel(), oldItem.getPermissionLevelName(),
permissionLevelManager.getPermissionLevelByName(...),
permBean.getModeratePostings()) to ensure custom-permission nulls trigger a
refresh.
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java`:
- Around line 75-89: setPermissionsForLevel may leave displayLevel null when
permissionLevelManager.getPermissionLevelByName(selectedLevel) returns null and
also calls displayLevel.getTypeUuid() which can NPE; fix by making the
non-"Custom" branch assign a safe fallback when lookup returns null (e.g.,
create or load a default level) and make the custom-type check null-safe by
testing displayLevel != null before calling getTypeUuid(), or by comparing
typeUuid from displayLevel via Objects.equals(displayLevel == null ? null :
displayLevel.getTypeUuid(), typeManager.getCustomLevelType()); update
references: setPermissionsForLevel, displayLevel,
permissionLevelManager.getPermissionLevelByName,
MessageForumsTypeManager.getCustomLevelType, displayLevel.getTypeUuid, and
permissionLevelManager.createPermissionLevel so displayLevel is never left null
and the custom-type check cannot throw an NPE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55a70d80-4784-42fc-85c5-de011ef45d06
📒 Files selected for processing (11)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertiesmsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/Area.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/DiscussionForumServiceImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/PrivateMessageManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/UIPermissionsManagerImpl.javamsgcntr/messageforums-component-impl/src/webapp/WEB-INF/components.xmlmsgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/AreaImpl.java
💤 Files with no reviewable changes (1)
- msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/PrivateMessageManagerImpl.java
✅ Files skipped from review due to trivial changes (1)
- msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/DiscussionForumServiceImpl.java
ern
marked this pull request as ready for review
May 21, 2026 20:51
ern
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java (1)
1652-1659:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForum moderation changes still skip synoptic refreshes.
In the
DiscussionForumbranch,isModerated/isModeratedOldare never populated, and the later permission-diff block is guarded bytarget instanceof Topic. That makes the inner forum branch unreachable, so forum-level moderation ormoderatePostingspermission changes can still save without refreshing synoptic counts.Suggested fix
if (target instanceof DiscussionForum){ DiscussionForum forum = ((DiscussionForum) target); + isModerated = forum.getModerated(); DiscussionForum oldForum = forumManager.getForumById(forum.getId()); + isModeratedOld = oldForum.getModerated(); isDraftOld = oldForum.getDraft(); availabilityChanged = availabilityChanged(forum, oldForum); } @@ - if(!update && isModerated && permissions != null && target instanceof Topic){ + if(!update && isModerated && permissions != null){ //only need to look up permission changes for moderate postings if it is moderated if (target instanceof DiscussionForum){ oldMembershipItemSet = uiPermissionsManager.getForumItemsSet((DiscussionForum) target); }else if (target instanceof DiscussionTopic){Also applies to: 1677-1684
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java` around lines 1652 - 1659, The DiscussionForum branch never sets moderation flags and the permission-diff logic is only executed for Topic, so forum-level moderation or moderatePostings permission changes skip synoptic refresh; in the DiscussionForum branch (where you call DiscussionForum forum = (DiscussionForum) target and fetch oldForum via forumManager.getForumById(forum.getId())), set isModerated = forum.getModerated() and isModeratedOld = oldForum.getModerated() (in addition to the existing isDraftOld and availabilityChanged assignment) and ensure the later permission-diff / synoptic-refresh code that currently runs only under target instanceof Topic is also executed for DiscussionForum (or factor the permission-diff logic into a shared method and call it for both DiscussionForum and Topic) so changes to moderatePostings or forum-level moderation trigger the synoptic refresh.
♻️ Duplicate comments (1)
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java (1)
81-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't preserve a stale
displayLevelwhen the level lookup fails.If
getPermissionLevelByName(selectedLevel)returnsnull,selectedLevelhas already changed butdisplayLevelstays on the previous object. That can write permission changes to the wrong level, and if the previous value wasnullthe setters below still throw. Fail fast or clear/reject the selection here instead of keeping stale state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java` around lines 81 - 84, The code in PermissionBean uses permissionLevelManager.getPermissionLevelByName(selectedLevel) and if it returns null leaves this.displayLevel unchanged, which can lead to stale state; update the logic so that when level == null you either clear this.displayLevel (set to null) or reject the selection (throw/return after logging) instead of preserving the old value—locate the block referencing selectedLevel and this.displayLevel and replace the current if (level != null) { this.displayLevel = level; } with explicit handling for the null case (e.g., this.displayLevel = null; processLogger.warn(...) or throw an IllegalArgumentException) so subsequent setters operate on the correct state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java`:
- Around line 1652-1659: The DiscussionForum branch never sets moderation flags
and the permission-diff logic is only executed for Topic, so forum-level
moderation or moderatePostings permission changes skip synoptic refresh; in the
DiscussionForum branch (where you call DiscussionForum forum = (DiscussionForum)
target and fetch oldForum via forumManager.getForumById(forum.getId())), set
isModerated = forum.getModerated() and isModeratedOld = oldForum.getModerated()
(in addition to the existing isDraftOld and availabilityChanged assignment) and
ensure the later permission-diff / synoptic-refresh code that currently runs
only under target instanceof Topic is also executed for DiscussionForum (or
factor the permission-diff logic into a shared method and call it for both
DiscussionForum and Topic) so changes to moderatePostings or forum-level
moderation trigger the synoptic refresh.
---
Duplicate comments:
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java`:
- Around line 81-84: The code in PermissionBean uses
permissionLevelManager.getPermissionLevelByName(selectedLevel) and if it returns
null leaves this.displayLevel unchanged, which can lead to stale state; update
the logic so that when level == null you either clear this.displayLevel (set to
null) or reject the selection (throw/return after logging) instead of preserving
the old value—locate the block referencing selectedLevel and this.displayLevel
and replace the current if (level != null) { this.displayLevel = level; } with
explicit handling for the null case (e.g., this.displayLevel = null;
processLogger.warn(...) or throw an IllegalArgumentException) so subsequent
setters operate on the correct state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cb48be6-9e0c-464c-bc41-707f6be9cd41
📒 Files selected for processing (2)
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java (1)
671-677:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
nullas “no change” instead of “open to site”.Line 672 turns
nullinto an empty set, and the newDiscussionForumManagercontract treats empty as “clear restrictions”. If Lessons omits groups in an update path, this silently broadens access.Suggested fix
public void setGroups(Collection<String> groups) { - Set<String> groupIds = (groups == null) ? Collections.emptySet() : new HashSet<>(groups); + if (groups == null) { + return; + } + Set<String> groupIds = new HashSet<>(groups); if (type == TYPE_FORUM_TOPIC) { discussionForumManager.setTopicGroupRestrictions(id, groupIds); } else if (type == TYPE_FORUM_FORUM) { discussionForumManager.setForumGroupRestrictions(id, groupIds); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java` around lines 671 - 677, The setGroups method currently converts a null groups parameter into Collections.emptySet(), which the DiscussionForumManager now interprets as "clear restrictions"; instead preserve null as "no change": in setGroups, compute groupIds only when groups != null (e.g., new HashSet<>(groups)) and pass null through to discussionForumManager.setTopicGroupRestrictions(id, groupIds) and setForumGroupRestrictions(id, groupIds) when groups was null so the manager receives null for no-op and a non-null Set when an explicit change is intended; key symbols: setGroups, discussionForumManager.setTopicGroupRestrictions, discussionForumManager.setForumGroupRestrictions, TYPE_FORUM_TOPIC, TYPE_FORUM_FORUM.
🧹 Nitpick comments (2)
msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/DiscussionForumServiceImpl.java (1)
662-664: ⚡ Quick winReuse the area fetched before the loop to avoid redundant calls.
The
toAreais already fetched at line 561 and saved at line 579 before this loop begins. Re-fetching the area on every iteration (line 663) is redundant and causes N additional calls where N is the number of forums being imported.♻️ Proposed fix to reuse the existing area reference
- // Save the forum in the target discussion area after it has been initialized for this import mode. - Area area = areaManager.getDiscussionArea(toContext, createDefaultForum); - newForum.setArea(area); + // Save the forum in the target discussion area after it has been initialized for this import mode. + newForum.setArea(toArea);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/DiscussionForumServiceImpl.java` around lines 662 - 664, The code currently calls areaManager.getDiscussionArea(toContext, createDefaultForum) inside the loop and assigns it to a local Area variable before calling newForum.setArea(area); instead reuse the already-fetched toArea reference (the Area saved earlier) instead of re-fetching it each iteration — update the loop to call newForum.setArea(toArea) (or use the existing toArea variable) so you avoid redundant areaManager.getDiscussionArea calls.msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.java (1)
174-186: ⚖️ Poor tradeoffCasting to implementation class breaks encapsulation.
The cast
(DBMembershipItemImpl) itemto accesssetForum/setTopiccouples this code to the implementation. Consider addingsetForum/setTopicto theDBMembershipIteminterface to avoid this cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.java` around lines 174 - 186, The code in createDefaultMembershipItems casts DBMembershipItem to DBMembershipItemImpl to call setForum/setTopic, breaking encapsulation; modify the DBMembershipItem interface to declare setForum(DiscussionForum) and setTopic(DiscussionTopic), implement these methods in DBMembershipItemImpl, and then remove the casts in createDefaultMembershipItems so you call item.setForum(forum) / item.setTopic(topic) directly (ensure permissionLevelManager.createDBMembershipItem still returns DBMembershipItem).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java`:
- Around line 1204-1205: Reset the session attachment state before populating
when opening a different forum/topic settings screen: clear the session fields
attachments and prepareRemoveAttach at the start of the settings-population code
path so previously selected attachments are not carried over and appended to the
wrong object; locate the methods that assemble attachments for the settings
pages (where attachments and prepareRemoveAttach are referenced/modified) and
add explicit resets before appending new forum/topic attachments — apply the
same reset in the equivalent code paths noted in the review (also applies to the
other occurrence around the second referenced block).
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java`:
- Around line 106-110: The setter methods (e.g., setChangeSettings) call
this.displayLevel.setXxx(...) without guarding for displayLevel being null;
update the setters (setChangeSettings, setDeleteAny, etc.) to check for
displayLevel != null before calling its setters, or ensure displayLevel is
always initialized in the constructor when item.getPermissionLevel() and
permissionLevelManager.getPermissionLevelByName(...) both return null
(initialize displayLevel to a default PermissionLevelDisplay object); reference
the displayLevel field, the setter methods like setChangeSettings, and the
constructor initialization code that uses item.getPermissionLevel() and
permissionLevelManager.getPermissionLevelByName(...) when making the change.
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.java`:
- Around line 197-199: The isPrivateAreaEnabled() method risks NPE because
getPrivateArea().getEnabled() returns a nullable Boolean; change
isPrivateAreaEnabled() to defensively handle null (e.g., assign Boolean b =
getPrivateArea().getEnabled() and return Boolean.TRUE.equals(b) or return b !=
null && b.booleanValue()) so it never auto-unboxes a null; update any related
tests if needed.
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java`:
- Around line 2540-2578: applyGroupRestrictions currently demotes any
contributor-level DBMembershipItem (including TYPE_USER) when toggling group
restrictions; change the logic so only group and role membership items are
modified: in the "restricting" branch inside applyGroupRestrictions, only
promote/demote items whose getType() equals MembershipItem.TYPE_GROUP (do not
run the contributor->none demotion for other types like TYPE_USER), and in the
"clearing" branch only promote/demote items whose getType() equals
MembershipItem.TYPE_ROLE or TYPE_GROUP as appropriate; leave TYPE_USER entries
untouched (ensure the existing loops that call
permissionLevelManager.createDBMembershipItem and membershipItemSet.add(...)
remain the same for new group entries).
- Around line 2475-2505: Both setTopicGroupRestrictions and
setForumGroupRestrictions may call applyGroupRestrictions on an empty
membershipItemSet, which only adds group rows and leaves default role membership
rows missing so the entity remains open; before calling applyGroupRestrictions
ensure the entity has bootstrapped role membership items
(owner/contributor/reader/etc.) so role entries exist and can be switched to
"None". Locate setTopicGroupRestrictions and setForumGroupRestrictions and add
logic to initialize/insert default role membership rows into the Topic/Forum
membershipItemSet when it is empty (use the same membership model the codebase
uses for stored role rows), then proceed to resolveSiteGroupNames and call
applyGroupRestrictions; keep the bootstrap step idempotent and tied to the same
membership item structure applyGroupRestrictions expects.
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/UIPermissionsManagerImpl.java`:
- Around line 114-117: resolvePermissionLevel may return null when both
item.getPermissionLevel() and
permissionLevelManager.getPermissionLevelByName(...) are null, leading to NPEs
where callers do resolvePermissionLevel(item).getChangeSettings() etc.; change
resolvePermissionLevel(DBMembershipItem) to return Optional<PermissionLevel>
(wrap existing lookup with Optional.ofNullable(...)) and then update all
predicate lambdas that currently call resolvePermissionLevel(item).getXxx() to
use optional mapping and a safe default, e.g.
resolvePermissionLevel(item).map(pl -> pl.getChangeSettings()).orElse(false)
(apply for getChangeSettings/getCreateForums/getModerateMessages/etc.), or
otherwise handle the empty Optional appropriately so no direct get* is invoked
on null.
In
`@msgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/AreaImpl.java`:
- Around line 105-112: The remove methods (removePrivateForum,
removeDiscussionForum, removeOpenForum, removeMembershipItem) can NPE when their
backing sets (privateForumsSet, discussionForumsSet, openForumsSet,
membershipItemsSet) are null; update each method to guard against a null backing
set before calling remove()—for example, check if the set is non-null (and still
perform any needed inverse updates like forum.setArea(null) or
item.setArea(null)) and only call remove() when the set exists, mirroring the
initialization/null-safety pattern used in the add* methods.
- Around line 65-87: The getters getOpenForums, getPrivateForums and
getDiscussionForums currently call new
ArrayList<>(openForumsSet/privateForumsSet/discussionForumsSet) which will NPE
if the backing set is null; change each getter to defensively handle null by
returning an empty list (e.g., Collections.emptyList() or new ArrayList<>())
when the corresponding set is null, and update the setters
setOpenForums/setPrivateForums/setDiscussionForums to defensively handle a null
input list by assigning an empty HashSet instead of new HashSet<>(null) so the
backing fields are never left null.
---
Duplicate comments:
In
`@lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java`:
- Around line 671-677: The setGroups method currently converts a null groups
parameter into Collections.emptySet(), which the DiscussionForumManager now
interprets as "clear restrictions"; instead preserve null as "no change": in
setGroups, compute groupIds only when groups != null (e.g., new
HashSet<>(groups)) and pass null through to
discussionForumManager.setTopicGroupRestrictions(id, groupIds) and
setForumGroupRestrictions(id, groupIds) when groups was null so the manager
receives null for no-op and a non-null Set when an explicit change is intended;
key symbols: setGroups, discussionForumManager.setTopicGroupRestrictions,
discussionForumManager.setForumGroupRestrictions, TYPE_FORUM_TOPIC,
TYPE_FORUM_FORUM.
---
Nitpick comments:
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.java`:
- Around line 174-186: The code in createDefaultMembershipItems casts
DBMembershipItem to DBMembershipItemImpl to call setForum/setTopic, breaking
encapsulation; modify the DBMembershipItem interface to declare
setForum(DiscussionForum) and setTopic(DiscussionTopic), implement these methods
in DBMembershipItemImpl, and then remove the casts in
createDefaultMembershipItems so you call item.setForum(forum) /
item.setTopic(topic) directly (ensure
permissionLevelManager.createDBMembershipItem still returns DBMembershipItem).
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/DiscussionForumServiceImpl.java`:
- Around line 662-664: The code currently calls
areaManager.getDiscussionArea(toContext, createDefaultForum) inside the loop and
assigns it to a local Area variable before calling newForum.setArea(area);
instead reuse the already-fetched toArea reference (the Area saved earlier)
instead of re-fetching it each iteration — update the loop to call
newForum.setArea(toArea) (or use the existing toArea variable) so you avoid
redundant areaManager.getDiscussionArea calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9433fd46-35b3-4c00-8f31-106cb78bfaa9
📒 Files selected for processing (17)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/Area.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/MessageForumsForumManager.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/ui/DiscussionForumManager.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/DiscussionForumServiceImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/MessageForumsForumManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/PrivateMessageManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/UIPermissionsManagerImpl.javamsgcntr/messageforums-component-impl/src/webapp/WEB-INF/components.xmlmsgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/AreaImpl.javamsgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/OpenForum.hbm.xmlmsgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/Topic.hbm.xml
💤 Files with no reviewable changes (1)
- msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/PrivateMessageManagerImpl.java
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapi/src/main/java/org/sakaiproject/webapi/controllers/SiteEntityController.java (1)
517-526:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard forum ID parsing to avoid 500 on malformed input.
Line 525 parses
forumIdwithLong.valueOf(...)directly. A non-numeric ID will throwNumberFormatExceptionand skip the intendedOptional.empty()path, resulting in a server error instead of a clean bad request response.💡 Proposed fix
private Optional<OpenForum> findForumInArea(Area forumArea, String forumId) { if (forumArea == null) { return Optional.empty(); } Set<OpenForum> forums = forumArea.getOpenForumsSet(); + Long forumIdLong; + try { + forumIdLong = Long.valueOf(forumId); + } catch (NumberFormatException e) { + return Optional.empty(); + } return forums.stream() - .filter(forum -> Long.valueOf(forumId).equals(forum.getId())) + .filter(forum -> forumIdLong.equals(forum.getId())) .findAny(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapi/src/main/java/org/sakaiproject/webapi/controllers/SiteEntityController.java` around lines 517 - 526, The findForumInArea method currently calls Long.valueOf(forumId) inside the stream filter which will throw NumberFormatException for non-numeric forumId and cause a 500; change the logic to validate/parse forumId before streaming (e.g. try to parseLong forumId in a try/catch or use a numeric check) and if parsing fails return Optional.empty(); update references to Long.valueOf(...) in findForumInArea to use the pre-parsed long value (or skip filtering) so malformed IDs yield Optional.empty() instead of an exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@webapi/src/main/java/org/sakaiproject/webapi/controllers/SiteEntityController.java`:
- Around line 517-526: The findForumInArea method currently calls
Long.valueOf(forumId) inside the stream filter which will throw
NumberFormatException for non-numeric forumId and cause a 500; change the logic
to validate/parse forumId before streaming (e.g. try to parseLong forumId in a
try/catch or use a numeric check) and if parsing fails return Optional.empty();
update references to Long.valueOf(...) in findForumInArea to use the pre-parsed
long value (or skip filtering) so malformed IDs yield Optional.empty() instead
of an exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc6a3bb0-1eac-4bc3-840e-6eff3bdd047d
📒 Files selected for processing (1)
webapi/src/main/java/org/sakaiproject/webapi/controllers/SiteEntityController.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java (2)
2485-2491:⚠️ Potential issue | 🟠 MajorHandle the clear-restrictions path when memberships are still null.
resolveSiteGroupNames(...)returns an empty list fornull/empty input, so these methods still callapplyGroupRestrictions(...)when clearing restrictions. Because the bootstrap now runs only for non-emptygroupNames, an older forum/topic withmembershipItemSet == nullwill still hit aNullPointerExceptionon the first iteration insideapplyGroupRestrictions(...).Also applies to: 2507-2514
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java` around lines 2485 - 2491, When clearing group restrictions the code may pass a null membership set into applyGroupRestrictions and cause an NPE; modify the block around topic.getMembershipItemSet() so you fetch it into a local variable (e.g., topicItems), and if topicItems is null then only createDefaultMembershipItemsForTopic when groupNames is non-empty, otherwise skip calling applyGroupRestrictions; finally call applyGroupRestrictions(topicItems, groupNames) with the non-null topicItems (or return/continue when it remains null). Apply the same change to the similar block for the forum-level code that mirrors lines 2507-2514.
2554-2567:⚠️ Potential issue | 🔴 CriticalDemote role-level Contributor rows when enabling group restrictions.
The restricting branch now only demotes
TYPE_GROUPContributor rows. The defaultTYPE_ROLEContributor memberships stay active, so the forum/topic remains open to the whole site even after selecting prerequisite groups. This branch needs to demote Contributor rows for bothTYPE_ROLEandTYPE_GROUP, while still leavingTYPE_USERentries untouched.Suggested fix
- } else if (Objects.equals(item.getType(), MembershipItem.TYPE_GROUP) + } else if ((Objects.equals(item.getType(), MembershipItem.TYPE_ROLE) + || Objects.equals(item.getType(), MembershipItem.TYPE_GROUP)) && PermissionLevelManager.PERMISSION_LEVEL_NAME_CONTRIBUTOR.equals(item.getPermissionLevelName())) { item.setPermissionLevel(null); item.setPermissionLevelName(PermissionLevelManager.PERMISSION_LEVEL_NAME_NONE); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java` around lines 2554 - 2567, In DiscussionForumManagerImpl inside the loop over membershipItemSet (DBMembershipItem entries), update the restricting branch logic so Contributor rows are demoted for both MembershipItem.TYPE_ROLE and MembershipItem.TYPE_GROUP (but not TYPE_USER): keep the first branch that promotes groupNames (remove from toAdd only when item.getType() == TYPE_GROUP and groupNames.contains(item.getName())), and change the subsequent demotion condition to check if item.getType() is TYPE_GROUP OR TYPE_ROLE and the current permission equals PermissionLevelManager.PERMISSION_LEVEL_NAME_CONTRIBUTOR, then setPermissionLevel(null) and setPermissionLevelName(PermissionLevelManager.PERMISSION_LEVEL_NAME_NONE).msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java (1)
55-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback still leaves
displayLevelnullable.If
item.getPermissionLevelName()isnullor points at a non-"Custom"level that no longer resolves, this new constructor path still returns withdisplayLevel == nullbecausesetPermissionsForLevel(...)is a no-op in that case. The setters below still dereferencedisplayLevelunconditionally, so the first permission toggle will NPE. Please guarantee a non-null fallback here, or fail fast before the bean is used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java` around lines 55 - 57, The constructor leaves displayLevel null when item.getPermissionLevelName() is null or resolves to an unknown non-"Custom" level because setPermissionsForLevel(...) is a no-op for unresolved names; ensure displayLevel is set to a safe non-null default (e.g., the "Default" or "Inherited" PermissionLevel instance your app expects) or throw an IllegalStateException immediately from the PermissionBean constructor if no valid level can be resolved so subsequent setters that dereference displayLevel cannot NPE; update the constructor logic around the call to setPermissionsForLevel(selectedLevel) to assign a concrete fallback to displayLevel (or fail fast) before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/UIPermissionsManagerImpl.java`:
- Around line 114-117: resolvePermissionLevel currently assumes the
DBMembershipItem parameter is non-null and will NPE when callers (e.g.,
getAreaItemsByCurrentUser or forumManager.getDBMember) pass null; update
resolvePermissionLevel(DBMembershipItem item) to first check for a null item and
return Optional.empty() if so, then proceed to resolve using
item.getPermissionLevel() or
permissionLevelManager.getPermissionLevelByName(item.getPermissionLevelName());
also defensively handle a null permissionLevelName by returning Optional.empty()
instead of calling the manager with null.
---
Duplicate comments:
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java`:
- Around line 55-57: The constructor leaves displayLevel null when
item.getPermissionLevelName() is null or resolves to an unknown non-"Custom"
level because setPermissionsForLevel(...) is a no-op for unresolved names;
ensure displayLevel is set to a safe non-null default (e.g., the "Default" or
"Inherited" PermissionLevel instance your app expects) or throw an
IllegalStateException immediately from the PermissionBean constructor if no
valid level can be resolved so subsequent setters that dereference displayLevel
cannot NPE; update the constructor logic around the call to
setPermissionsForLevel(selectedLevel) to assign a concrete fallback to
displayLevel (or fail fast) before returning.
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java`:
- Around line 2485-2491: When clearing group restrictions the code may pass a
null membership set into applyGroupRestrictions and cause an NPE; modify the
block around topic.getMembershipItemSet() so you fetch it into a local variable
(e.g., topicItems), and if topicItems is null then only
createDefaultMembershipItemsForTopic when groupNames is non-empty, otherwise
skip calling applyGroupRestrictions; finally call
applyGroupRestrictions(topicItems, groupNames) with the non-null topicItems (or
return/continue when it remains null). Apply the same change to the similar
block for the forum-level code that mirrors lines 2507-2514.
- Around line 2554-2567: In DiscussionForumManagerImpl inside the loop over
membershipItemSet (DBMembershipItem entries), update the restricting branch
logic so Contributor rows are demoted for both MembershipItem.TYPE_ROLE and
MembershipItem.TYPE_GROUP (but not TYPE_USER): keep the first branch that
promotes groupNames (remove from toAdd only when item.getType() == TYPE_GROUP
and groupNames.contains(item.getName())), and change the subsequent demotion
condition to check if item.getType() is TYPE_GROUP OR TYPE_ROLE and the current
permission equals PermissionLevelManager.PERMISSION_LEVEL_NAME_CONTRIBUTOR, then
setPermissionLevel(null) and
setPermissionLevelName(PermissionLevelManager.PERMISSION_LEVEL_NAME_NONE).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e096c093-6e78-4dba-aef1-b28acc3ddb4e
📒 Files selected for processing (6)
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.javamsgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.javamsgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/UIPermissionsManagerImpl.javamsgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/AreaImpl.java
ern
added a commit
that referenced
this pull request
Jun 25, 2026
… Lesson's prerequisites (#14604) (cherry picked from commit f74a497) Conflicts: lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/ForumEntity.java msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/PermissionBean.java msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/AreaManagerImpl.java msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/PrivateMessageManagerImpl.java msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/UIPermissionsManagerImpl.java msgcntr/messageforums-component-impl/src/webapp/WEB-INF/components.xml webapi/src/main/java/org/sakaiproject/webapi/controllers/SiteEntityController.java
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.
https://sakaiproject.atlassian.net/browse/SAK-48981
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation