OF-2549: Fix JavaScript error and improve UX in MUC Create Permission page#3239
OF-2549: Fix JavaScript error and improve UX in MUC Create Permission page#3239MilanTyagi2004 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the admin console MUC room creation permissions page to eliminate a JavaScript focus error and make the “Allowed Users” section respond immediately to policy selection without requiring a reload.
Changes:
- Adds
toggleAllowedUsers()to show/hide the Allowed Users UI and conditionally focus the JID input. - Replaces server-side conditional rendering of the Allowed Users section with always-rendered markup that is toggled via CSS
display. - Removes broken inline click handlers that referenced the wrong form scope.
Comments suppressed due to low confidence (1)
xmppserver/src/main/webapp/muc-create-permission.jsp:252
- The visibility of
allowedUsersContaineris rendered frommucService.isRoomCreationRestricted(), but the new UI toggles it client-side based on the radio selection. If an admin selects “Only specific…” (shows the section) and then submits the Add/checkbox form, the server redirect can re-render withisRoomCreationRestricted()==false, hiding the section again even though the user just interacted with it. Consider persisting the selected policy across these submits (e.g., include/openPerms as a hidden field in the add form and/or derive initial display from a request param), or prevent using the Allowed Users form until the policy is saved.
<div id="allowedUsersContainer" style="display: <%= (mucService.isRoomCreationRestricted() ? "block" : "none") %>">
<!-- BEGIN 'Allowed Users' -->
<form action="muc-create-permission.jsp?add" method="post">
<input type="hidden" name="csrf" value="${csrf}">
<input type="hidden" name="mucname" value="<%= StringUtils.escapeForXML(mucname) %>" />
<div class="jive-contentBoxHeader">
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@guusdk changes are done, now this PR is okay to merge. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
xmppserver/src/main/webapp/muc-create-permission.jsp:252
- With the Allowed Users form always present in the DOM, it’s now possible to select “Only specific…” (without submitting the Save form) and submit the Add form. The server-side
addhandler does not enablesetRoomCreationRestricted(true), and after the redirect the page will revert to the current service setting (potentially hiding the Allowed Users section), which can make the Add appear to “disappear” and can leave configuration in an inconsistent/unclear state. Consider either preventing Add/Delete unless the service is restricted, or persisting/applying the policy choice when performing Add/Delete (eg submit the Save form automatically, or include/handle a policy param in the Add/Delete actions).
<div id="allowedUsersContainer" style="display: <%= (mucService.isRoomCreationRestricted() ? "block" : "none") %>">
<!-- BEGIN 'Allowed Users' -->
<form action="muc-create-permission.jsp?add" method="post">
<input type="hidden" name="csrf" value="${csrf}">
<input type="hidden" name="mucname" value="<%= StringUtils.escapeForXML(mucname) %>" />
<div class="jive-contentBoxHeader">
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @MilanTyagi2004 - I wonder if something is broken. When I open the page, and select "Only specific users/groups can create a chat room", and fill out a username, like below: If I then press the 'add' button, then I get a 'success' message, but the option "Anyone can create a chat room" is selected. 
You would expect the option "Only specific users/groups can create a chat room" to be selected! Can you please fix this? |
guusdk
left a comment
guusdk
left a comment
There was a problem hiding this comment.
(as mentioned in my previous commit: the setting no longer seems to be saved correctly).
|
sure i will do this and squash commits into one commit. |
i fix this, now it is working fine. |
|
hi @guusdk, |
|
I squashes all the commits into one with all description. |
|
@Fishbowler if you are not busy could you please review this. |
|
When I pick All Registered Users... 
...it's cleared on save 
|
Fishbowler
left a comment
Fishbowler
left a comment
There was a problem hiding this comment.
Note the issue above. I suspect this might be pre-existing, but I haven't checked.
Added one nitpick below.
| <meta name="helpPage" content="set_group_chat_room_creation_permissions.html"/> | ||
| <script> | ||
| function toggleAllowedUsers() { | ||
| const isRestricted = document.getElementById('rb02').checked; |
There was a problem hiding this comment.
If we're gonna use the ID in JS, would renaming the ID to something more friendly make sense?
There was a problem hiding this comment.
Thanks for review,
i will look into this.
|
request changes are fixed, |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis JSP page refactors MUC room-creation permission handling to persist the allow-all-registered-users setting server-side and replace server-side JSP conditionals with client-side visibility toggling. The form now uses a hidden field to synchronize checkbox state, permission radios trigger visibility changes via JavaScript, and userJID values are XML-escaped. Copyright year updated to 2026. ChangesMUC Room Creation Permission Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
- Fix issue where "Only specific users/groups" selection was not preserved after adding users by ensuring roomCreationRestricted is set correctly - Replace onclick with onchange on radio buttons to improve accessibility and ensure proper event handling - Remove auto-submit from "allow all registered users" checkbox to prevent unintended state resets - Improve UI responsiveness by correctly toggling Allowed Users section without page reload - Clean up JavaScript handling for safer focus behavior
The 'All registered users' checkbox was in the 'Add' form but not in the 'Save Settings' form. When the user checked the checkbox and clicked 'Save Settings', the value was never submitted, causing it to revert to unchecked on page reload. - Add hidden input for allowAllRegisteredUsers in the Save Settings form - Sync visible checkbox state to the hidden input via JavaScript - Persist allowAllRegisteredUsers in the save handler's restricted branch - Rename radio button id to restrictedPermissionsRadio for clarity
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
xmppserver/src/main/webapp/muc-create-permission.jsp (2)
240-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the label target for the restricted radio option.
Line 245 still points to
rb02, but the input id is nowrestrictedPermissionsRadio. Clicking the label will no longer select the radio or trigger the visibility toggle.Suggested fix
- <label for="rb02"><fmt:message key="muc.create.permission.specific_created" /></label> + <label for="restrictedPermissionsRadio"><fmt:message key="muc.create.permission.specific_created" /></label>🤖 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 `@xmppserver/src/main/webapp/muc-create-permission.jsp` around lines 240 - 245, The label's for attribute is still pointing to "rb02" instead of the radio input's id "restrictedPermissionsRadio", so update the label element associated with the restricted option in muc-create-permission.jsp to use for="restrictedPermissionsRadio" (so clicking the label selects the input and triggers toggleAllowedUsers()); ensure the label that currently references rb02 is the one next to the input with id "restrictedPermissionsRadio" and change it accordingly.
76-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwap the audit log messages in the save branches.
These messages are inverted relative to the state being applied: the
openPermsbranch disables restriction, while theelsebranch enables it. Leaving this as-is will make the admin audit trail misleading.Suggested fix
if (openPerms) { // Remove all users who have the ability to create rooms for (JID user : mucService.getUsersAllowedToCreate()) { mucService.removeUserAllowedToCreate(user); } mucService.setRoomCreationRestricted(false); // Log the event - webManager.logEvent("set MUC room creation to restricted for service "+mucname, null); + webManager.logEvent("set MUC room creation to not restricted for service "+mucname, null); response.sendRedirect("muc-create-permission.jsp?success=true&mucname="+URLEncoder.encode(mucname, StandardCharsets.UTF_8)); return; } else { mucService.setRoomCreationRestricted(true); mucService.setAllRegisteredUsersAllowedToCreate(allowAllRegisteredUsers); // Log the event - webManager.logEvent("set MUC room creation to not restricted for service "+mucname, null); + webManager.logEvent("set MUC room creation to restricted for service "+mucname, null); response.sendRedirect("muc-create-permission.jsp?success=true&mucname="+URLEncoder.encode(mucname, StandardCharsets.UTF_8)); return; }🤖 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 `@xmppserver/src/main/webapp/muc-create-permission.jsp` around lines 76 - 92, The audit log messages are inverted: in the openPerms branch you call mucService.setRoomCreationRestricted(false) but log "set MUC room creation to restricted...", and in the else branch you set setRoomCreationRestricted(true) but log "to not restricted...". Swap or correct the log message strings used in the webManager.logEvent calls so the message matches the action performed (i.e., when calling mucService.setRoomCreationRestricted(false) log that room creation was set to not restricted/open, and when calling setRoomCreationRestricted(true) log that room creation was set to restricted/closed); update the two webManager.logEvent(...) calls near the openPerms conditional to reflect 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 `@xmppserver/src/main/webapp/muc-create-permission.jsp`:
- Around line 240-245: The label's for attribute is still pointing to "rb02"
instead of the radio input's id "restrictedPermissionsRadio", so update the
label element associated with the restricted option in muc-create-permission.jsp
to use for="restrictedPermissionsRadio" (so clicking the label selects the input
and triggers toggleAllowedUsers()); ensure the label that currently references
rb02 is the one next to the input with id "restrictedPermissionsRadio" and
change it accordingly.
- Around line 76-92: The audit log messages are inverted: in the openPerms
branch you call mucService.setRoomCreationRestricted(false) but log "set MUC
room creation to restricted...", and in the else branch you set
setRoomCreationRestricted(true) but log "to not restricted...". Swap or correct
the log message strings used in the webManager.logEvent calls so the message
matches the action performed (i.e., when calling
mucService.setRoomCreationRestricted(false) log that room creation was set to
not restricted/open, and when calling setRoomCreationRestricted(true) log that
room creation was set to restricted/closed); update the two
webManager.logEvent(...) calls near the openPerms conditional to reflect the
correct state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed1f8652-7695-4990-9c85-9c83eb33197c
📒 Files selected for processing (3)
build/ci/updater/pom.xmlxmppserver/pom.xmlxmppserver/src/main/webapp/muc-create-permission.jsp
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)
xmppserver/src/main/webapp/muc-create-permission.jsp (1)
240-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBroken label association after radio button ID rename.
Line 240 renames the second radio from
rb02torestrictedPermissionsRadio, but the corresponding<label>on line 245 still referencesfor="rb02". No element withid="rb02"exists in the DOM, so clicking the label text no longer selects the radio button.🐛 Proposed fix
- <label for="rb02"><fmt:message key="muc.create.permission.specific_created" /></label> + <label for="restrictedPermissionsRadio"><fmt:message key="muc.create.permission.specific_created" /></label>🤖 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 `@xmppserver/src/main/webapp/muc-create-permission.jsp` around lines 240 - 245, The label's for attribute no longer matches the radio button id after renaming the radio to restrictedPermissionsRadio, breaking click-to-select; update the <label> that currently uses for="rb02" to use for="restrictedPermissionsRadio" so it targets the input with id="restrictedPermissionsRadio" (leave name="openPerms" and onchange="toggleAllowedUsers()" unchanged).
🤖 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 `@xmppserver/src/main/webapp/muc-create-permission.jsp`:
- Around line 240-245: The label's for attribute no longer matches the radio
button id after renaming the radio to restrictedPermissionsRadio, breaking
click-to-select; update the <label> that currently uses for="rb02" to use
for="restrictedPermissionsRadio" so it targets the input with
id="restrictedPermissionsRadio" (leave name="openPerms" and
onchange="toggleAllowedUsers()" unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa99d316-6dd1-4f8f-9970-90ec0f695493
📒 Files selected for processing (1)
xmppserver/src/main/webapp/muc-create-permission.jsp
Fishbowler
left a comment
Fishbowler
left a comment
There was a problem hiding this comment.
LGTM! I've pushed one commit to do a label rename to match an id rename.
This PR resolves a JavaScript error (Uncaught TypeError: Cannot read properties of undefined (reading 'focus')) on the MUC room creation permissions page and enhances the user interface to be more responsive.
Root Cause
The error was caused by a scope conflict where a radio button tried to focus an input field (userJID) located in a separate form. Additionally, the target field was often missing from the DOM because it was only rendered by the server when the room creation restriction was already enabled.
Changes
Dynamic Visibility: Replaced server-side JSP conditional rendering with client-side CSS toggling (display: block/none) for the "Allowed Users" section .
Robust Event Handling: Introduced a toggleAllowedUsers() JavaScript function to manage visibility and safely apply focus only when the element is available.
Improved UX: The "Allowed Users" section now appears or disappears immediately upon selecting a policy, without requiring a page reload.
Code Cleanup: Removed redundant and broken inline onclick handlers from JID and Group selection fields that were using incorrect this.form references.
Verification