SAK-51623 Search clean up the admin screen #13951
Conversation
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a 32px height to the search input, removes its label, adds nine admin UI message keys for index types, hides two refresh options from the admin option lists (commands still handled server-side), and replaces the admin UI with a Bootstrap panel layout including an index-type selector and explanatory text. Public APIs unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ern
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
search/search-tool/tool/src/webapp/WEB-INF/vm/index.vm (1)
48-56: Restoring an accessible name for the search field.The label for the search input was removed. Add a visually hidden label to preserve accessibility without changing the visual layout.
- <form action="?#" method="get" class="inlineForm" > + <form action="?#" method="get" class="inlineForm"> + <label for="search" class="visually-hidden">${rlb.jsp_search}</label> <input type="hidden" name="panel" value="Main" /> - <input type="text" id="search" name="search" maxlength="1024" value="${searchModel.search}"/> + <input type="text" id="search" name="search" maxlength="1024" value="${searchModel.search}"/> <input type="submit" name="sb" value="${rlb.jsp_search}" class="mt-1 mt-sm-0"/>
🧹 Nitpick comments (5)
library/src/skins/default/src/sass/modules/tool/search/_search.scss (1)
4-4: Avoid fixed height on inputs; prefer scalable sizing (and add the missing semicolon).A hard-coded 32px height can clip content at larger zoom/OS font settings and harms accessibility. Use min-height/line-height to scale with fonts (Bootstrap 5 conventions), and add the semicolon to avoid fragile parsing in future edits.
Apply:
- height: 32px + /* Let the control scale with font size; keep a sensible minimum */ + min-height: 2.25rem; + line-height: 2.25rem; + height: auto;search/search-tool/tool/src/java/org/sakaiproject/search/tool/SearchAdminBeanImpl.java (2)
504-505: OK to hide “Refresh Site Index” in the new UI, but consider removing it from legacy getAdminOptions() too.You removed the option from getDefaultOptions() (used by the new admin VM). getAdminOptions() still exposes REFRESHSITE; if that view is obsolete, remove it there as well to avoid conflicting UIs.
512-515: Inconsistent semantics of AdminOptionImpl.url between site vs. index options.Here url carries the command token ("rebuildinstance"), while default options use a prebuilt query string ("?command=rebuildsite"). This dual meaning complicates the template logic. Consider normalizing url to always be a command token and let the view compose URLs/params.
If you want to normalize, I can produce a coordinated patch updating:
- getDefaultOptions(): use REBUILDSITE instead of COMMAND_REBUILDSITE
- admin/index.vm: submit Site Functions via a GET form with name="command"
Would you like that change?search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm (2)
56-61: Filter by command token, not localized label.Comparing $option.name to a localized string is fragile. Use the command token instead (e.g., "refreshinstance"). This also reads better after normalizing AdminOptionImpl.url to command tokens.
- #if ($option.name != "${rlb.searchadmin_cmd_refreshind}") + #if ($option.url != "refreshinstance") <button class="btn btn-primary" type="submit" name="command" value="${option.url}">${option.name}</button> #end
27-31: Redundant filter after backend change; safe to drop.Since getDefaultOptions() no longer includes “Refresh Site Index,” this check is now redundant. You can remove it for clarity.
- #if ($option.name != "${rlb.searchadmin_cmd_refreshsiteind}") - <button class="btn btn-primary" onclick="window.location='${option.url}'" ${option.attributes}>${option.name}</button> - #end + <button class="btn btn-primary" onclick="window.location='${option.url}'" ${option.attributes}>${option.name}</button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
library/src/skins/default/src/sass/modules/tool/search/_search.scss(1 hunks)search/search-tool/tool/src/bundle/org/sakaiproject/search/tool/bundle/Messages.properties(1 hunks)search/search-tool/tool/src/java/org/sakaiproject/search/tool/SearchAdminBeanImpl.java(2 hunks)search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm(1 hunks)search/search-tool/tool/src/webapp/WEB-INF/vm/index.vm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{html,xhtml,vm}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{html,xhtml,vm}: Use Bootstrap 5.2 as the preferred UI framework
Ensure responsive design across different screen sizes
Leverage Bootstrap 5 components for consistent UI/UX
Prefer kebab-case for values of HTML class and id attributes
Files:
search/search-tool/tool/src/webapp/WEB-INF/vm/index.vmsearch/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
search/search-tool/tool/src/java/org/sakaiproject/search/tool/SearchAdminBeanImpl.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (2)
search/search-tool/tool/src/bundle/org/sakaiproject/search/tool/bundle/Messages.properties (1)
15-15: Great addition, but it isn’t used by the template yet.The new key is defined but admin/index.vm still hardcodes the explanation text. Wire this message into the template to keep localization intact.
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm (1)
46-64: Switch admin actions to POST with CSRF protectionThe rebuild/refresh buttons perform side-effecting operations and must not be invoked via GET. Submitting them via GET opens up CSRF attack vectors, even if access is permission-gated. Please update the Velocity template to use method="POST" and include the platform’s CSRF token helper.
• File: search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm
– Change<form method="GET">to<form method="POST">
– Immediately inside the form add the Sakai CSRF token snippet (for example, if available in your context):
<input type="hidden" name="csrfToken" value="$!{sakai.csrfToken}" />
(Adjust to the actual helper macro or taglib your version of Sakai provides, e.g.#csrfInput().)• Verify that no existing CSRF-injection macro is already available in this module’s Velocity context. A search for “csrf” across
search-tool/tool/srcreturned no matches, so it appears you’ll need to wire in the token manually.Please confirm that you’ve located—or added—the proper CSRF helper and updated the form accordingly before merging.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm (3)
26-33: Add an accessible name to the toolbar.AccessLint flagged a missing accessible name/label. Provide an aria-label so assistive tech can announce the toolbar purpose.
- <div class="btn-toolbar" role="toolbar"> + <div class="btn-toolbar" role="toolbar" aria-label="${rlb.searchadmin_index_site_functions}">
40-44: Localize the explanatory text and make the icon decorative.Avoid hardcoded English and ensure the info icon is ignored by screen readers. The PR summary mentions a new bundle key; use it.
- <div class="alert alert-info"> - <h4><i class="fa fa-info-circle"></i> Index Types</h4> - <p><strong>Default Index:</strong> Contains general site content including resources, announcements, forums, wikis, lessons, and other course materials.</p> - <p><strong>Questions Index:</strong> Contains only assessment content from Tests & Quizzes, including question text, answers, question pools, and published assessments. This is useful for faculty searching through their question banks.</p> - </div> + <div class="alert alert-info"> + <h4><i class="fa fa-info-circle" aria-hidden="true"></i> Index Types</h4> + <p>${rlb.searchadmin_index_functions_explanation}</p> + </div>
46-61: Modernize form controls for Bootstrap 5 and improve semantics.
- .form-group is legacy; use spacing utilities (.mb-3).
- Selects use .form-select in BS5, not .form-control.
- .input-group-btn is obsolete; wrap multiple actions with .btn-group or use direct .btn siblings.
- Prefer kebab-case for the id per guidelines (keep name="indexbuildername" for backend binding).
- <div class="form-group"> - <label for="indexbuildername">Select Index Type:</label> - <div class="input-group"> - <select name="indexbuildername" id="indexbuildername" class="form-control me-1 rounded"> + <div class="mb-3"> + <label for="index-builder-name">Select Index Type:</label> + <div class="input-group"> + <select name="indexbuildername" id="index-builder-name" class="form-select me-1"> #foreach( $iName in ${adminModel.indexBuilderNames} ) <option value="${iName}">#if($iName == "default")Default (General Content)#elseif($iName == "questions")Questions (Tests & Quizzes)#{else}${iName}#end</option> #end </select> - <div class="input-group-btn"> + <div class="btn-group" role="group" aria-label="${rlb.searchadmin_index_functions}"> #foreach( $option in ${adminModel.indexSpecificOptions} ) #if ($option.name != "${rlb.searchadmin_cmd_refreshind}") <button class="btn btn-primary" type="submit" name="command" value="${option.url}">${option.name}</button> #end #end </div> </div> </div>
🧹 Nitpick comments (3)
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm (3)
27-31: Prefer anchors for navigation over onclick buttons.For accessibility and progressive enhancement, use instead of JS onclick on a . Keeps behavior with keyboard and without JS.
- #if ($option.name != "${rlb.searchadmin_cmd_refreshsiteind}") - <button class="btn btn-primary" onclick="window.location='${option.url}'" ${option.attributes}>${option.name}</button> - #end + #if ($option.name != "${rlb.searchadmin_cmd_refreshsiteind}") + <a class="btn btn-primary" href="${option.url}" ${option.attributes}>${option.name}</a> + #end
51-53: Externalize option display names for i18n.The “Default (General Content)” and “Questions (Tests & Quizzes)” strings are hardcoded. Consider using bundle keys (e.g., rlb.searchadmin_index_type_default and rlb.searchadmin_index_type_questions) so labels are localized.
27-31: Avoid comparing localized labels in the admin VM — use a stable identifierShort: index.vm is filtering options by the localized label ($option.name), which is fragile. I verified the code: SearchAdminBeanImpl builds AdminOptionImpl (name,url,attributes) and the bundles contain the localized strings; AdminOptionImpl has no stable id/command field. The template checks are therefore brittle and (given the bean changes) redundant.
Files to check / change:
- search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm — lines ~27-31 and ~55-57 (the two #if ($option.name != "${rlb...}") checks).
- search/search-tool/tool/src/java/org/sakaiproject/search/tool/SearchAdminBeanImpl.java — getDefaultOptions(), getIndexSpecificOptions() and inner class AdminOptionImpl (no command/id property).
- search/search-tool/tool/src/bundle/.../Messages*.properties — contains localized labels (what the template currently compares to).
Recommended fixes (choose one):
- Short-term (small change)
- Remove the name-based filter (it's redundant today — SearchAdminBeanImpl no longer adds the refresh options).
- OR change the VM to compare a stable value already exposed: compare against ${option.url} instead of ${option.name}. Example replacement for the default-options block:
(and similarly for the index-specific loop, use the appropriate command string)#foreach( $option in ${adminModel.defaultOptions} ) #if ($option.url != "?command=refreshsite") <button class="btn btn-primary" onclick="window.location='${option.url}'" ${option.attributes}>${option.name}</button> #end #end
- Recommended long-term (robust)
- Add a stable identifier to AdminOption (e.g., command or id field) and populate it from the server-side constants (REBUILDSITE, REFRESHSITE, REBUILDINSTANCE, REFRESHINSTANCE).
- Update SearchAdminBeanImpl.* to construct AdminOptionImpl(..., command=) for every option.
- Update index.vm to test $option.command (not the localized $option.name).
Why this matters:
- Localized labels change per bundle/locale; testing against them can break hiding/showing logic across locales.
- Adding/using a stable command/id avoids future regressions and keeps presentation logic independent from localization.
Tag:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{html,xhtml,vm}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{html,xhtml,vm}: Use Bootstrap 5.2 as the preferred UI framework
Ensure responsive design across different screen sizes
Leverage Bootstrap 5 components for consistent UI/UX
Prefer kebab-case for values of HTML class and id attributes
Files:
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm
🧠 Learnings (2)
📚 Learning: 2025-08-11T22:32:08.596Z
Learnt from: CR
PR: sakaiproject/sakai#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T22:32:08.596Z
Learning: Applies to **/*.{html,xhtml,vm} : Leverage Bootstrap 5 components for consistent UI/UX
Applied to files:
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm
📚 Learning: 2025-08-11T22:32:08.596Z
Learnt from: CR
PR: sakaiproject/sakai#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T22:32:08.596Z
Learning: Applies to **/*.{html,xhtml,vm} : Use Bootstrap 5.2 as the preferred UI framework
Applied to files:
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (4)
search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm (4)
21-25: Nice migration to Bootstrap 5 cards.The card/card-header/card-body structure aligns with the Bootstrap 5.2 guidance and our UI guidelines. Good move away from legacy panels.
35-39: Good conditional rendering for disabled state.Card layout and the warning alert for disabled search read clearly and meet the responsive/Bootstrap guidance.
Also applies to: 64-70
29-29: Resolved:option.attributesposes no injection risk as it’s always an empty stringI’ve traced
option.attributesback toAdminOptionImplinSearchAdminBeanImpl.java, where it’s initialized only via the constructor’s third parameter. In bothgetDefaultOptions()(line 503) andgetIndexSpecificOptions()(line 512), that parameter is the literal"", so no untrusted data ever reaches the template.• File:
search/search-tool/tool/src/java/org/sakaiproject/search/tool/SearchAdminBeanImpl.java
–getDefaultOptions():
o.add(new AdminOptionImpl(COMMAND_REBUILDSITE, Messages.getString(...), ""));
–getIndexSpecificOptions():
o.add(new AdminOptionImpl(REBUILDINSTANCE, Messages.getString(...), ""));Since there’s no dynamic content flowing into
option.attributes, the interpolation in the Velocity template is safe. No further sanitization or allow-listing is necessary at this time.
45-63: Ensure Search Admin Actions Use POST with CSRF ProtectionI didn’t find any server‐side handling in the Search admin code that validates a CSRF token when these buttons invoke state‐changing commands via GET. In contrast, most other admin forms in this codebase use method="post" with an embedded
<input name="sakai_csrf_token" .../>and validate it in their corresponding Action/Bean classes.Please verify and update as follows:
- Change the
<form method="GET">insearch/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm(lines 45–63) to usemethod="POST".- Add the platform‐standard CSRF hidden input (
<input type="hidden" name="sakai_csrf_token" value="$sakai_csrf_token"/>) in that form, mirroring other admin VMs.- In the backend SearchAdminBean (or equivalent), ensure the handler only accepts POST and checks the CSRF token—similarly to
VelocityPortletPaneledAction.checkCSRFToken()or other existing patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
search/search-tool/tool/src/bundle/org/sakaiproject/search/tool/bundle/Messages.properties (4)
15-15: Standardize capitalization and remove “etc.” for clarityUse consistent naming (“Default Index”, “Questions Index”) to match the new titles (Lines 81–82) and avoid vague “etc.” in admin UI copy.
Apply this diff:
-searchadmin_index_functions_explanation=The default index contains general site content (resources, announcements, forums, etc.). The questions index contains only assessment questions and answers from Tests & Quizzes, including question pools and published assessments. +searchadmin_index_functions_explanation=The Default Index contains general site content (resources, announcements, forums). The Questions Index contains only assessment questions and answers from Tests & Quizzes, including question pools and published assessments.
75-82: New i18n keys look good; suggest a tiny microcopy tweakOverall LGTM. One optional tightening for Line 77 to read more naturally.
Optional diff:
-searchadmin_questions_index_description=Contains only assessment content from Tests & Quizzes, including question text, answers, question pools, and published assessments. This is useful for faculty searching through their question banks. +searchadmin_questions_index_description=Contains only assessment content from Tests & Quizzes, including question text, answers, question pools, and published assessments. This is useful for faculty searching their question banks.Suggestion (no diff needed): consider adding short translator comments above these new keys to clarify UI placement (section title, select label, option labels, descriptions) for localization teams.
16-16: Typo: “woksite” → “worksite”Not introduced in this PR, but easy win in user-facing text.
-searchbeanfact_siteerror=You must access the Search through a woksite +searchbeanfact_siteerror=You must access the Search through a worksite
45-45: Typo: “expected fo finish” → “expected to finish”Also preexisting but worth fixing.
-jsp_searchadmin_index_last_loaded = Last loaded at {0} in {1} <br/> Being indexed by {2} expected fo finish before {3} <br /> Index contains {4} documents and {5} pending +jsp_searchadmin_index_last_loaded = Last loaded at {0} in {1} <br/> Being indexed by {2} expected to finish before {3} <br /> Index contains {4} documents and {5} pending
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
search/search-tool/tool/src/bundle/org/sakaiproject/search/tool/bundle/Messages.properties(2 hunks)search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- search/search-tool/tool/src/webapp/WEB-INF/vm/admin/index.vm
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
search/search-tool/tool/src/bundle/org/sakaiproject/search/tool/bundle/Messages.properties
Show resolved
Hide resolved
|
the updated ui screenshot: |
Summary by CodeRabbit