SAK-52039 Polls migrate ui to ThymeLeaf and migrate persistence to spring repsoitory with JPA #14157
Conversation
WalkthroughThe Polls tool migration replaces the RSF-based web layer with Spring MVC + Thymeleaf, converts Hibernate XML mappings to JPA-annotated entities, replaces DAOs with Spring Data repositories and repository-backed implementations, and introduces new controllers, form models, service wiring, templates, and test updates. Changes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (3)
polls/tool/src/webapp/WEB-INF/templates/polls/vote.html (1)
11-18: Verify URL patterns match Spring MVC routing.Same as in permissions.html, the URLs use
/faces/*patterns which appear to be legacy RSF/JSF conventions. Confirm these paths match the actual Spring MVC controller mappings.polls/tool/src/webapp/WEB-INF/templates/polls/thanks.html (1)
11-25: Verify URL patterns match Spring MVC routing.Same as other templates, the URLs use
/faces/*patterns (lines 11, 14, 17, 25). Ensure consistency across all templates and controllers.polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html (1)
11-38: Verify URL patterns match Spring MVC routing.Same as other templates, ensure the
/faces/*URL patterns (lines 11, 14, 17, 27, 38) match actual Spring MVC controller mappings.
🧹 Nitpick comments (1)
polls/tool/src/webapp/WEB-INF/templates/polls/vote.html (1)
32-36: Consider using th:field for Spring form binding.Using
name="selectedOptionIds"with manual binding may not properly populate thevoteForm.selectedOptionIdslist. Consider using Thymeleaf'sth:field="*{selectedOptionIds}"for proper Spring form binding with checkboxes/radios.Example refactor for checkbox binding:
<input th:type="${multipleChoice} ? 'checkbox' : 'radio'" class="form-check-input flex-shrink-0" - name="selectedOptionIds" - th:id="${'option-' + option.optionId}" - th:value="${option.optionId}" /> + th:field="*{selectedOptionIds}" + th:value="${option.optionId}" + th:id="${'option-' + option.optionId}" />Note: For radio buttons in Spring forms, you may need to handle single-selection binding differently than checkboxes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
polls/tool/pom.xml(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/beans/ErrorHandler.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/constants/NavigationConstants.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/evolvers/SakaiFCKTextEvolver.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollActionResultInterceptor.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollBeanLocator.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionBatchViewParameters.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionViewParameters.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/PermissionAction.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/PollToolBean.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/AddPollProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/ConfirmProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PermissionsProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionBatchProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionDeleteProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollToolProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollVoteProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/ResultsProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/validators/OptionValidator.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/validators/VoteValidator.java(0 hunks)polls/tool/src/webapp/WEB-INF/applicationContext.xml(0 hunks)polls/tool/src/webapp/WEB-INF/requestContext.xml(0 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/edit.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/list.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-edit.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/permissions.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/results.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/thanks.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/vote.html(1 hunks)polls/tool/src/webapp/WEB-INF/web.xml(1 hunks)polls/tool/src/webapp/component-templates/Messages.html(0 hunks)polls/tool/src/webapp/js/polls.js(0 hunks)polls/tool/src/webapp/templates/pollOption.html(0 hunks)polls/tool/src/webapp/templates/pollOptionBatch.html(0 hunks)polls/tool/src/webapp/templates/pollOptionDelete.html(0 hunks)polls/tool/src/webapp/templates/voteAdd.html(0 hunks)polls/tool/src/webapp/templates/votePermissions.html(0 hunks)polls/tool/src/webapp/templates/votePolls.html(0 hunks)polls/tool/src/webapp/templates/voteQuestion.html(0 hunks)polls/tool/src/webapp/templates/voteResults.html(0 hunks)polls/tool/src/webapp/templates/voteThanks.html(0 hunks)
💤 Files with no reviewable changes (33)
- polls/tool/src/webapp/WEB-INF/requestContext.xml
- polls/tool/src/webapp/templates/pollOption.html
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionDeleteProducer.java
- polls/tool/src/webapp/templates/voteAdd.html
- polls/tool/src/webapp/component-templates/Messages.html
- polls/tool/src/java/org/sakaiproject/poll/tool/constants/NavigationConstants.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionBatchProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/validators/VoteValidator.java
- polls/tool/src/java/org/sakaiproject/poll/tool/beans/ErrorHandler.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/ResultsProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/ConfirmProducer.java
- polls/tool/src/webapp/templates/voteQuestion.html
- polls/tool/src/java/org/sakaiproject/poll/tool/evolvers/SakaiFCKTextEvolver.java
- polls/tool/src/java/org/sakaiproject/poll/tool/params/PollToolBean.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/AddPollProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/validators/OptionValidator.java
- polls/tool/src/webapp/js/polls.js
- polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollBeanLocator.java
- polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionBatchViewParameters.java
- polls/tool/src/webapp/templates/pollOptionBatch.html
- polls/tool/src/webapp/templates/voteResults.html
- polls/tool/src/webapp/templates/pollOptionDelete.html
- polls/tool/src/webapp/templates/votePolls.html
- polls/tool/src/webapp/templates/voteThanks.html
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollVoteProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollActionResultInterceptor.java
- polls/tool/src/java/org/sakaiproject/poll/tool/params/PermissionAction.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PermissionsProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollToolProducer.java
- polls/tool/src/webapp/WEB-INF/applicationContext.xml
- polls/tool/src/webapp/templates/votePermissions.html
- polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionViewParameters.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java
**/*.{html,xhtml}
📄 CodeRabbit inference engine (AGENTS.md)
Use kebab-case for values of HTML class and id attributes
Files:
polls/tool/src/webapp/WEB-INF/templates/polls/results.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/permissions.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/option-batch.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/option-edit.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/vote.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/thanks.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/option-delete.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/edit.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/list.html
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
🧬 Code graph analysis (13)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java (1)
Configuration(44-109)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (5)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (2)
Value(216-220)Controller(50-221)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (2)
Value(157-164)Controller(43-165)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
Controller(50-213)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.java (1)
Controller(23-31)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
Controller(44-113)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (2)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-24)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (2)
Value(203-216)Controller(46-217)
polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
Data(24-28)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
Data(24-28)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
Data(24-28)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java (1)
Configuration(32-92)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (5)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-24)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (1)
Controller(50-221)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (1)
Controller(43-165)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
Controller(44-113)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-24)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
Controller(50-213)
⏰ 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 (5)
polls/tool/src/webapp/WEB-INF/web.xml (1)
1-6: LGTM! Minimal web.xml aligns with programmatic Spring configuration.The empty web.xml declaration is appropriate for the migration to Spring MVC with programmatic configuration via
WebApplicationInitializer(as mentioned in the PR context withPollsWebApplicationInitializer). Version 3.1 (Servlet 3.1 / Java EE 7) is suitable for Spring Framework 6.x.polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
24-28: LGTM! Clean model for vote form binding.The use of Lombok's
@Dataannotation is appropriate for this Spring MVC form model. Field initialization ofselectedOptionIdsto an emptyArrayListensures safe handling when no options are selected.polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
22-26: LGTM! Appropriate model for batch upload.Clean Lombok model for binding multipart file uploads. The use of Spring's
MultipartFileis the correct approach for file upload handling in Spring MVC.polls/tool/src/webapp/WEB-INF/templates/polls/permissions.html (1)
11-14: No changes needed for URL patterns. The/faces/votePollsand/faces/voteAddlinks match the@RequestMapping("/faces")in the poll controllers.polls/tool/pom.xml (1)
79-91: Upgrade Thymeleaf version: In master/pom.xml (around line 69), bumpsakai.thymeleaf.versionfrom3.0.15.RELEASEto at least3.1.3.RELEASEfor Spring 6 compatibility.⛔ Skipped due to learnings
Learnt from: ern PR: sakaiproject/sakai#14152 File: reset-pass/account-validator-impl/pom.xml:37-42 Timestamp: 2025-10-07T16:35:33.965Z Learning: The Sakai project cannot upgrade to Spring Framework 6 yet and must remain on Spring 5.x for the time being.Learnt from: CR PR: sakaiproject/sakai#0 File: AGENTS.md:0-0 Timestamp: 2025-10-07T16:11:59.621Z Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new developmentLearnt from: CR PR: sakaiproject/sakai#0 File: AGENTS.md:0-0 Timestamp: 2025-10-07T16:11:33.008Z Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java
Show resolved
Hide resolved
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java
Show resolved
Hide resolved
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java
Show resolved
Hide resolved
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java
Show resolved
Hide resolved
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (2)
61-65: Add null checks after getPollById to prevent NPE.
pollListManager.getPollById(pollId)can return null if the poll doesn't exist. Currently, the code directly passes the result topollIsVotable(poll)without checking for null, which will cause a NullPointerException.Apply this diff:
Poll poll = pollListManager.getPollById(pollId); + if (poll == null) { + redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale)); + return "redirect:/faces/votePolls"; + } if (!pollVoteManager.pollIsVotable(poll)) {
86-94: Add null checks after getPollById to prevent NPE.
pollListManager.getPollById(voteForm.getPollId())can return null. Line 93 dereferencespoll.getPollId()in the catch block without checking for null, which will cause a NullPointerException.Apply this diff:
Poll poll = pollListManager.getPollById(voteForm.getPollId()); + if (poll == null) { + redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale)); + return "redirect:/faces/votePolls"; + } try {polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (2)
68-92: Guard againstnullpolls before dereferencing
pollListManager.getPollById(resolvedId)can legitimately returnnull(stale/deleted poll). We then callcanEditPoll(poll)and later dereferencepollfor options/votes, so the first stale hit will throw an NPE and yield a 500. Please bail out early (set the alert + redirect) before touching the poll when the lookup fails. This is the same issue flagged previously by coderabbitai[bot]; it still needs to be fixed.
109-152: Handle missing poll in the POST flow
preparePollEntityreturnsnullwhen the incomingpollIdno longer exists, yet we immediately write to that reference—NPE and 500. ThePollValidationExceptionrecovery path also blindly dereferencescontextPoll. We need the same stale-id guard as in the GET path (redirect or surface a binding error) before mutating/reading the poll. This mirrors coderabbitai[bot]’s earlier warning and is still outstanding.polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (2)
145-163: Restore the add-permission guard before batch saves.The POST handler still skips
isAllowedPollAdd(). Any caller can upload options if they can reach this endpoint, which breaks poll-admin authorization (see prior review). Mirror the check used in the single-option save before callingpollsUiService.saveOptionsBatch(...).batchForm.setFile(file); + if (!isAllowedPollAdd()) { + bindingResult.addError(new FieldError("batchForm", "file", messageSource.getMessage("new_poll_noperms", null, locale))); + model.addAttribute("poll", pollListManager.getPollById(batchForm.getPollId())); + model.addAttribute("canAdd", isAllowedPollAdd()); + model.addAttribute("isSiteOwner", isSiteOwner()); + return "polls/option-batch"; + } try { pollsUiService.saveOptionsBatch(batchForm.getPollId(), batchForm.getFile());
190-201: Enforce add permission before deleting options.
confirmDeletestill lacks theisAllowedPollAdd()gate, so unauthorized POSTs can delete options (previously flagged). Check permissions up front, add a flash alert, and short-circuit before callingpollsUiService.deleteOption(...).public String confirmDelete(@RequestParam("optionId") Long optionId, @RequestParam(value = "orphanHandling", required = false) String orphanHandling, RedirectAttributes redirectAttributes, Locale locale) { + if (!isAllowedPollAdd()) { + redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("new_poll_noperms", null, locale)); + return "redirect:/faces/votePolls"; + } if (StringUtils.isBlank(orphanHandling)) { orphanHandling = HANDLE_DELETE_OPTION_DO_NOTHING; }polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (2)
111-116: Fix max-option validation comparison.Line 114 should reject counts above
maxOptions, not below. As written, any poll with fewer than the max choices throwsinvalid_poll_limits, making most edits fail. Switch the comparison to> poll.getMaxOptions().- if (pollOptions.size() < poll.getMinOptions() || pollOptions.size() < poll.getMaxOptions()) { + if (pollOptions.size() < poll.getMinOptions() || pollOptions.size() > poll.getMaxOptions()) { throw new PollValidationException("invalid_poll_limits"); }
59-77: Handle missing polls before delete/reset loops.
PollListManager.getPollById(...)returnsnullfor stale IDs (see PollListManagerImpl#getPollById). Passing that straight intodeletePolloruserCanDeletePolltriggers a NullPointerException/IllegalArgumentException and aborts the bulk operation. Guard for null in both loops, log, and continue.for (Long pollId : pollIds) { Poll poll = pollListManager.getPollById(pollId); + if (poll == null) { + log.warn("Poll {} not found during bulk delete", pollId); + continue; + } try { pollListManager.deletePoll(poll); } catch (SecurityException e) { log.warn("User {} is not permitted to delete poll {}", externalLogic.getCurrentUserId(), pollId); } }for (Long pollId : pollIds) { Poll poll = pollListManager.getPollById(pollId); + if (poll == null) { + log.warn("Poll {} not found during bulk vote reset", pollId); + continue; + } if (pollListManager.userCanDeletePoll(poll)) { List<Vote> votes = pollVoteManager.getAllVotesForPoll(poll); pollVoteManager.deleteAll(votes); } }
🧹 Nitpick comments (3)
polls/api/src/java/org/sakaiproject/poll/repository/PollRepository.java (1)
16-16: Custom repository implementation found in PollRepositoryImpl
PollRepositoryImpl#findOpenPollsBySiteIds filters by siteId, voteOpen < now, voteClose > now, and orders by creationDate. If inclusive semantics (<=, >=) are needed, replace with Restrictions.le and Restrictions.ge.polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java (1)
60-62: Use explicit type parameter for ArgumentCaptor.The raw
ArgumentCaptor<List>should specify the generic type parameter for type safety and clarity.Apply this diff:
- ArgumentCaptor<List> siteCaptor = ArgumentCaptor.forClass(List.class); + ArgumentCaptor<List<String>> siteCaptor = ArgumentCaptor.forClass(List.class);Based on coding guidelines: "Java: Never use local variable type inference (var). Always declare explicit types."
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (1)
145-151: Avoid N repeated option lookups during batch saves.
getOptionsForPoll(pollId).size()inside the loop hits the repository every iteration, yielding O(n²) queries on batch uploads. Cache the starting size and increment locally before saving each option.- for (String optionText : optionTexts) { + int nextOrder = pollListManager.getOptionsForPoll(pollId).size(); + for (String optionText : optionTexts) { Option option = new Option(); option.setPollId(pollId); option.setText(PollUtils.cleanupHtmlPtags(optionText)); - option.setOptionOrder(pollListManager.getOptionsForPoll(pollId).size()); + option.setOptionOrder(nextOrder++); pollListManager.saveOption(option); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
polls/api/pom.xml(1 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Option.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Poll.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Vote.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/model/Option.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Poll.java(6 hunks)polls/api/src/java/org/sakaiproject/poll/model/Vote.java(2 hunks)polls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/PollRepository.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java(1 hunks)polls/impl/pom.xml(1 hunks)polls/impl/src/bundle/org/sakaiproject/poll/spring-hibernate.xml(0 hunks)polls/impl/src/java/org/sakaiproject/poll/dao/impl/PollDoaImpl.java(0 hunks)polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java(2 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java(18 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java(4 hunks)polls/impl/src/test/hibernate-test.xml(2 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java(2 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java(2 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.java(2 hunks)polls/impl/src/webapp/WEB-INF/components.xml(1 hunks)polls/impl/src/webapp/WEB-INF/hibernate-hbms.xml(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.java(2 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java(2 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/vote.html(1 hunks)
💤 Files with no reviewable changes (6)
- polls/impl/src/bundle/org/sakaiproject/poll/spring-hibernate.xml
- polls/api/src/java/org/sakaiproject/poll/hbm/Poll.hbm.xml
- polls/impl/src/java/org/sakaiproject/poll/dao/impl/PollDoaImpl.java
- polls/impl/src/webapp/WEB-INF/hibernate-hbms.xml
- polls/api/src/java/org/sakaiproject/poll/hbm/Option.hbm.xml
- polls/api/src/java/org/sakaiproject/poll/hbm/Vote.hbm.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- polls/tool/src/webapp/WEB-INF/templates/polls/vote.html
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.javapolls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.javapolls/api/src/java/org/sakaiproject/poll/repository/PollRepository.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.javapolls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.javapolls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.javapolls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.javapolls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.javapolls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.javapolls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.javapolls/api/src/java/org/sakaiproject/poll/model/Vote.javapolls/api/src/java/org/sakaiproject/poll/model/Poll.javapolls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.javapolls/api/src/java/org/sakaiproject/poll/model/Option.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.javapolls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
🧬 Code graph analysis (9)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
Repository(16-66)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
Repository(14-87)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-31)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (4)
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (1)
Slf4j(58-343)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-31)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
Controller(50-213)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
Controller(44-113)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-28)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
Repository(14-87)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(39-82)
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (2)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(39-82)polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(42-103)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(42-103)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-28)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
Repository(16-66)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-31)
⏰ 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 (20)
polls/impl/pom.xml (1)
65-68: LGTM: Spring Data JPA dependency added for repository-based persistence.The addition of
spring-data-jpacorrectly supports the migration from GenericDao to Spring Data JPA repositories, as evidenced by the new repository interfaces (PollRepository, OptionRepository, VoteRepository) introduced across the PR.polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
19-31: LGTM: Clean validation exception for i18n support.The exception design correctly separates the message key from its arguments, which is appropriate for internationalized error messages where the key is resolved by a message source using the provided args.
polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java (2)
102-102: LGTM: Logging updated to use correct identifier method.The change from
poll.getId()topoll.getPollId()aligns with the Poll entity's method naming. This is a logging-only change with no functional impact.
115-115: LGTM: Logging updated to use correct identifier method.The change from
option.getId()tooption.getOptionId()aligns with the Option entity's method naming. This is a logging-only change with no functional impact.polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.java (2)
123-125: LGTM: UUID initialization for new polls.The change from
poll.getId()topoll.getUuid()correctly initializes the UUID field for new poll entities. This aligns with the broader PR migration to UUID-based identifiers for external references (as seen in PollsEntityProvider and other entity providers).
176-176: LGTM: UUID excluded from entity updates.The change to exclude
"uuid"(instead of"id") from the copyBean operation correctly prevents overwriting the existing poll's UUID during updates. This is consistent with the UUID migration and ensures identifier stability.polls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.java (1)
1-11: LGTM: Well-defined Spring Data JPA repository.The repository interface correctly extends
SpringCrudRepositoryand defines a query method using Spring Data JPA's query derivation convention. The method namefindByPollIdOrderByOptionOrderwill automatically generate a query that finds options bypollIdand orders them byoptionOrder.polls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.java (2)
101-112: LGTM: Test data preload migrated to repository-based persistence.The changes correctly migrate from DAO-based persistence to Spring Data JPA repositories. The explicit UUID assignment using
UUID.randomUUID().toString()before saving is appropriate for test data setup.
118-129: LGTM: Option entities correctly persisted via repository.The option creation follows the same pattern as poll creation, using
optionRepository.save()and explicitly setting UUIDs. TheoptionOrderfield is also properly initialized.polls/api/src/java/org/sakaiproject/poll/repository/PollRepository.java (2)
12-12: LGTM: Standard Spring Data JPA query methods.These methods follow Spring Data JPA's query derivation conventions and will be automatically implemented:
findBySiteIdOrderByCreationDateDescqueries bysiteIdwith descending date orderingfindByUuidqueries by theuuidfieldAlso applies to: 18-18
14-14: Custom repository method implemented
PollRepositoryImpl providesfindBySiteIdsOrderByCreationDatevia a Criteria query withRestrictions.in("siteId", siteIds)and ordering bycreationDate. No further action required.polls/api/pom.xml (1)
37-42: Keep javax.persistence for compatibility with the current stack.
All Sakai modules consistently usejavax.persistence-api:2.2, and Spring 5.3/Hibernate 5.6 require thejavax.persistencenamespace. Migration to Jakarta EE namespaces should wait for the planned Spring 6/Hibernate 6 upgrade.Likely an incorrect or invalid review comment.
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
14-27: LGTM! Repository implementation follows established patterns.The implementation correctly uses Hibernate Criteria API with appropriate null handling and is consistent with sibling repository implementations (PollRepositoryImpl and VoteRepositoryImpl).
polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (2)
357-359: LGTM! UUID migration is correctly applied.The change from numeric IDs to UUIDs for poll initialization is consistent with the broader UUID migration across the codebase.
416-418: LGTM! Property exclusion list updated for UUID.The copyBean exclusion list correctly references "uuid" instead of "id", maintaining consistency with the UUID migration.
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java (2)
48-49: LGTM! Repository dependencies correctly injected.The replacement of DAO with repository-based dependencies aligns with the broader migration to Spring Data JPA repositories.
77-96: LGTM! getVotesForUser correctly refactored to use repositories.The refactored implementation:
- Properly handles null inputs and empty pollIds
- Uses explicit type declarations (Line 82:
List<Vote> votes;)- Simplifies logic by replacing Search/Restrictions with repository methods
- Correctly constructs the result map using computeIfAbsent
polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java (1)
8-21: LGTM! Repository interface methods are well-defined.The interface provides appropriate finder methods (by pollId, userId, combinations) and utility methods (existence check, aggregation) that align with the repository pattern and support the poll voting domain logic.
polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
16-65: LGTM! Repository implementation is consistent and correct.All query methods:
- Handle null inputs appropriately
- Use Hibernate Criteria API correctly
- Apply appropriate ordering (asc/desc based on query semantics)
- Return proper result types (List or Optional)
The date-based filtering in
findOpenPollsBySiteIds(lines 45-54) correctly checks that polls are currently open (voteOpen < now < voteClose).polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
14-86: LGTM! Repository implementation is consistent and correct.All query methods properly:
- Handle null inputs with appropriate returns
- Use Hibernate Criteria API correctly
- Apply Projections for aggregation queries (rowCount, countDistinct)
- Follow the same patterns as sibling repository implementations
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java (1)
60-62: Verification logic correctly validates site filtering.The ArgumentCaptor usage and assertion properly verify that only the explicitly requested site ("site3") is passed to the repository query, which is the specific bug this test targets.
For more comprehensive test coverage, consider also verifying the method's return value:
+ List<Poll> result = impl.findAllPollsForUserAndSitesAndPermission("userId", new String[]{"site3"}, PollListManager.PERMISSION_VOTE); ArgumentCaptor<List<String>> siteCaptor = ArgumentCaptor.forClass(List.class); Mockito.verify(pollRepository).findOpenPollsBySiteIds(siteCaptor.capture(), Mockito.any()); Assert.assertEquals(List.of("site3"), siteCaptor.getValue()); + // Optional: Assert.assertNotNull(result); or verify result contentspolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (3)
114-115: Null‑safe details and sanitize poll text too.Avoid possible NPE on details and normalize the question text consistently.
Apply this diff:
- poll.setDetails(PollUtils.cleanupHtmlPtags(externalLogic.processFormattedText(poll.getDetails(), new StringBuilder()))); + String details = StringUtils.defaultString(poll.getDetails()); + poll.setDetails(PollUtils.cleanupHtmlPtags(externalLogic.processFormattedText(details, new StringBuilder()))); + poll.setText(PollUtils.cleanupHtmlPtags(externalLogic.processFormattedText(poll.getText(), new StringBuilder())));
143-161: Use the same sanitization pipeline as saveOption.For consistency and safety, process batch option text via ExternalLogic before saving.
Apply this diff:
- option.setText(PollUtils.cleanupHtmlPtags(optionText)); + option.setText(PollUtils.cleanupHtmlPtags( + externalLogic.processFormattedText(optionText, new StringBuilder(), true, true) + ));
130-141: Ordering with deleted options (optional).If soft-deleted options remain in the list, using size() may create gaps. Consider basing optionOrder on visible options or the max existing order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
polls/api/src/bundle/org/sakaiproject/poll/bundle/Messages.properties(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java(1 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java(2 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java
- polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java
- polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
🧬 Code graph analysis (2)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (3)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java (1)
Slf4j(43-208)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
Slf4j(73-718)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-31)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (3)
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (1)
Slf4j(58-343)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java (1)
Slf4j(43-208)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
Slf4j(73-718)
⏰ 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 (9)
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java (3)
26-28: Repository imports support the DAO-to-JPA migration.The new repository imports correctly replace the DAO pattern and align with the PR's goal of migrating to Spring Data repositories.
45-47: Mock setup correctly initializes repository dependencies.The test properly creates mocks for all three repository interfaces that replace the previous DAO dependency.
51-53: Repository injection via setters is appropriate for testing.The test correctly wires the repository mocks using setter injection, replacing the previous DAO-based configuration.
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (2)
105-112: LGTM!The confirmation page handler is straightforward and correctly adds the necessary model attributes for the view.
114-120: LGTM!The permission helper methods are clear and correctly check admin status or location-based permissions.
polls/api/src/bundle/org/sakaiproject/poll/bundle/Messages.properties (1)
85-85: LGTM! Clear message for missing polls.The new
poll_missingkey is well-placed among vote-related messages and provides clear user feedback. Note: VoteController currently usesvote_noperm.voteCollectioninstead of this key when polls are null—this has been flagged in the VoteController review.polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (3)
59-72: Bulk delete null-guard looks good.Prevents NPEs on stale/missing poll IDs; exceptions handled cleanly.
74-86: Bulk vote reset null-guard looks good.Skips missing polls and respects permission via userCanDeletePoll.
119-125: Option-count validation fix is correct.Using > max instead of < max resolves the previous logic bug.
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java
Outdated
Show resolved
Hide resolved
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java
Outdated
Show resolved
Hide resolved
| Locale locale) { | ||
| Poll poll = pollListManager.getPollById(voteForm.getPollId()); | ||
| if (poll == null) { | ||
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale)); |
There was a problem hiding this comment.
Use the new poll_missing message key for better UX.
When the poll is null (doesn't exist), use poll_missing instead of vote_noperm.voteCollection to distinguish between "poll not found" and "permission denied" scenarios.
Apply this diff:
- redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale));
+ redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll_missing", null, locale));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale)); | |
| if (poll == null) { | |
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll_missing", null, locale)); | |
| return "redirect:/faces/votePolls"; | |
| } |
🤖 Prompt for AI Agents
In polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java around
line 92, the code currently sets the flash alert to "vote_noperm.voteCollection"
when the poll is null; update it to use the "poll_missing" message key instead.
Replace the call to messageSource.getMessage("vote_noperm.voteCollection", ...)
with messageSource.getMessage("poll_missing", null, locale) so the UI
differentiates missing polls from permission errors and ensure the rest of the
redirectAttributes usage remains unchanged.
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java
Outdated
Show resolved
Hide resolved
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
Show resolved
Hide resolved
| public Poll deleteOption(Long optionId, String orphanVoteHandling) { | ||
| Option option = pollListManager.getOptionById(optionId); | ||
| if (option == null) { | ||
| throw new IllegalArgumentException("Option not found"); | ||
| } | ||
|
|
||
| Poll poll = pollListManager.getPollById(option.getPollId()); | ||
| List<Vote> votes = pollVoteManager.getAllVotesForOption(option); | ||
|
|
||
| if (votes != null && !votes.isEmpty()) { | ||
| if (HANDLE_DELETE_OPTION_RETURN_VOTES.equals(orphanVoteHandling)) { | ||
| Set<String> userIds = new TreeSet<>(); | ||
| pollListManager.deleteOption(option); | ||
| for (Vote vote : votes) { | ||
| if (vote.getUserId() != null) { | ||
| String userEid = externalLogic.getUserEidFromId(vote.getUserId()); | ||
| if (StringUtils.isNotBlank(userEid)) { | ||
| userIds.add(userEid); | ||
| } | ||
| } | ||
| pollVoteManager.deleteVote(vote); | ||
| } | ||
| externalLogic.notifyDeletedOption(new ArrayList<>(userIds), externalLogic.getSiteTile(poll.getSiteId()), poll.getText()); | ||
| } else { | ||
| Option persistentOption = pollListManager.getOptionById(optionId); | ||
| pollListManager.deleteOption(persistentOption, true); | ||
| } | ||
| } else { | ||
| pollListManager.deleteOption(option); | ||
| } | ||
|
|
||
| return pollListManager.getPollById(option.getPollId()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm authorization for option delete flows.
Service deletes options without explicit permission checks. Ensure controllers gate this or add checks here (e.g., owner/admin or edit/delete perms).
🏁 Script executed:
#!/bin/bash
# Inspect controllers for permission checks around option CRUD
rg -nP --type=java -C3 'class\s+OptionController\b|deleteOption\(|saveOption\(|saveOptionsBatch\('
rg -nP --type=java -C3 'isAllowedInLocation|PERMISSION_EDIT|PERMISSION_DELETE|userCanDeletePoll|PERMISSION_ADD'Length of output: 85022
Ensure authorization in option deletion flow
OptionController.deleteOption (polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java:174–213) and PollsUiService.deleteOption (polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java:172–204) perform deletions with no permission gating. Enforce poll.deleteOwn/deleteAny (via externalLogic.isAllowedInLocation or pollListManager.userCanDeletePoll) before deleting options.
🤖 Prompt for AI Agents
In polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
around lines 172–204, the deleteOption flow performs deletions without checking
user permissions; before any deletion (both the orphan-vote handling branch and
the simple delete branch) call the appropriate authorization check
(externalLogic.isAllowedInLocation(siteId, "poll.deleteAny") or
pollListManager.userCanDeletePoll(poll) / poll.deleteOwn as applicable) and if
the user is not allowed abort by throwing an authorization exception; ensure
checks use the poll’s siteId/poll context and are performed before
pollListManager.deleteOption or pollVoteManager.deleteVote calls so that
unauthorized users cannot delete options or votes.
🧩 Analysis chain
Handle missing parent poll and batch-delete votes; verify method name.
- Add a null check for the parent poll to avoid downstream NPEs.
- Replace per-vote deletes with a single deleteAll call.
Apply this diff:
- Poll poll = pollListManager.getPollById(option.getPollId());
+ Poll poll = pollListManager.getPollById(option.getPollId());
+ if (poll == null) {
+ throw new PollValidationException("poll_not_found");
+ }
@@
- for (Vote vote : votes) {
- if (vote.getUserId() != null) {
- String userEid = externalLogic.getUserEidFromId(vote.getUserId());
- if (StringUtils.isNotBlank(userEid)) {
- userIds.add(userEid);
- }
- }
- pollVoteManager.deleteVote(vote);
- }
+ for (Vote vote : votes) {
+ if (vote.getUserId() != null) {
+ String userEid = externalLogic.getUserEidFromId(vote.getUserId());
+ if (StringUtils.isNotBlank(userEid)) {
+ userIds.add(userEid);
+ }
+ }
+ }
+ pollVoteManager.deleteAll(votes);Also, confirm the ExternalLogic API: getSiteTile vs getSiteTitle.
🏁 Script executed:
#!/bin/bash
# Check for ExternalLogic site title accessor naming
rg -nP -C2 'getSiteTile\s*\('
rg -nP -C2 'getSiteTitle\s*\('
# Locate ExternalLogic interface/class to confirm
fd -a ExternalLogic.javaLength of output: 55788
🏁 Script executed:
#!/bin/bash
# Search for PollValidationException definition
rg -n "class PollValidationException" -g "*.java" || echo "No PollValidationException found"
# Locate PollVoteManager file(s)
echo "Polling for PollVoteManager files:"
fd PollVoteManager.java
# For each found, check for deleteAll method
for file in $(fd PollVoteManager.java); do
echo -e "\n==> $file"
grep -n "deleteAll" "$file" || echo " No deleteAll method in $file"
doneLength of output: 375
Add poll null check and batch-delete votes
- After
getPollById, if the returnedpollis null, throwPollValidationException("poll_not_found"). - Collect user IDs as before, then replace the individual
deleteVote(vote)calls with a singlepollVoteManager.deleteAll(votes). - No change needed for
externalLogic.getSiteTile(...)—that matches the API.
🤖 Prompt for AI Agents
In polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
around lines 172 to 204, after calling getPollById you must check for a null
poll and throw new PollValidationException("poll_not_found") if null; when
handling orphan votes collect userEids into the userIds set as before, then call
pollVoteManager.deleteAll(votes) once instead of looping and calling
deleteVote(vote) for each item (ensure you still call
pollListManager.deleteOption(option) before deleting votes), and finally call
externalLogic.notifyDeletedOption(new ArrayList<>(userIds),
externalLogic.getSiteTile(poll.getSiteId()), poll.getText()) as before.
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
polls/impl/pom.xml (1)
121-152: Remove unused Jackson dependencies
No imports of com.fasterxml.jackson.* found in polls-impl; drop explicit declarations for jackson-annotations, jackson-core, jackson-databind, jackson-dataformat-xml, jackson-module-parameter-names, jackson-datatype-jdk8, jackson-datatype-jsr310, and woodstox-core.polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
206-207: Minor: Unnecessary Set conversion.Converting the List to HashSet before calling
deleteAllis unnecessary sincedeleteAllacceptsIterable<? extends T>. You can pass the List directly.- Set<Vote> voteSet = new HashSet<>(vote); - voteRepository.deleteAll(voteSet); + voteRepository.deleteAll(vote);- Set<Option> optionSet = new HashSet<>(options); - optionRepository.deleteAll(optionSet); + optionRepository.deleteAll(options);Also applies to: 216-217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
polls/impl/pom.xml(2 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java(18 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.javapolls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
🧬 Code graph analysis (1)
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java (2)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
Slf4j(73-728)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java (1)
Slf4j(43-208)
⏰ 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
🔇 Additional comments (17)
polls/impl/pom.xml (1)
65-68: LGTM!The addition of
spring-data-jpaaligns with the migration from generic-dao to repository-based data access.Based on learnings.
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java (2)
59-84: LGTM!The explicit repository creation and wiring clearly demonstrates the dependencies and is appropriate for unit testing. The manual setup provides better isolation and control compared to full Spring context autowiring.
111-111: LGTM!Logging expected exceptions at debug level is the correct approach. This reduces noise in test output while preserving diagnostic information when needed.
Also applies to: 156-156, 168-168, 177-177, 180-180, 203-203
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (14)
65-67: LGTM! Clean repository migration.The migration from DAO to Spring Data JPA repositories (PollRepository, OptionRepository, VoteRepository) aligns with modern Spring practices and the PR's objective to modernize the codebase.
Based on learnings about preferring Spring MVC/Boot with Hibernate for new tools.
Also applies to: 81-83
121-131: LGTM! Repository queries properly implemented.The migration from DAO-based queries to repository methods is correct, with appropriate filtering by permissions and proper defensive copying of results.
148-156: LGTM! Robust UUID generation.The UUID generation strategy with IdManager fallback to UUID.randomUUID() is a solid defensive approach.
239-239: LGTM! Proper repository usage with defensive copying.Also applies to: 243-243
252-268: LGTM! Well-structured repository retrieval with optional eager loading.The implementation properly:
- Uses repository findById
- Handles null case gracefully
- Conditionally loads options only when needed (performance optimization)
- Validates permissions
275-289: LGTM! Excellent input validation and error handling.The null checks and poll existence validation are well-implemented. Throwing
IllegalArgumentExceptionfor a non-existent poll when retrieving options is semantically correct - options should only be retrieved for polls that exist.
306-306: LGTM! Proper repository usage with UUID migration support.The
getOptionByIdmethod includes a defensive pattern to backfill UUIDs for legacy options that might not have them, which is good for data migration.Also applies to: 310-316
319-331: LGTM! Clean implementation of hard and soft delete.Both delete variants properly use the repository and maintain appropriate audit logging.
334-343: LGTM! Proper option persistence with UUID validation.The method ensures data integrity by validating UUID presence before save and uses parameterized logging correctly.
515-552: LGTM! Well-designed adapter pattern for Entity interface.The
PollEntityAdaptercleanly adaptsPollto theEntityinterface, properly exposing UUID as the public identifier while delegating all behavior to the wrapped Poll object. This is a textbook adapter pattern implementation.
500-506: LGTM! Proper entity retrieval with adapter pattern.The method correctly retrieves the poll and wraps it in the
PollEntityAdapterfor Entity interface compliance.
559-559: LGTM! Correct UUID usage in entity mapping.The entity map properly uses UUID as the public identifier, consistent with the migration strategy.
668-684: LGTM! Robust reference parsing with UUID migration support.The method properly:
- Parses entity references to extract UUID
- Queries by UUID using repository
- Backfills UUIDs for legacy data that might not have them
- Handles edge cases gracefully
697-700: LGTM! Efficient vote existence check.Using
existsByPollIdAndUserIdis a performance optimization over loading and checking vote collections. This is a good use of Spring Data JPA's derived query methods.
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
558-558: Replace deprecated EMPTY_LIST constant.
Collections.EMPTY_LISTis deprecated. UseCollections.emptyList()instead.Apply this diff:
- return Collections.EMPTY_LIST; + return Collections.emptyList();polls/impl/pom.xml (1)
53-131: Add missing dependencyManagement entries for new artifacts
The new dependencies (org.springframework.data:spring-data-jpa, all com.fasterxml.jackson.* modules, and com.fasterxml.woodstox:woodstox-core) aren’t declared under<dependencyManagement>in thepollsparent POM and will break the build—import the appropriate BOM or add explicit version entries.
🧹 Nitpick comments (4)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (4)
120-130: Redundant ArrayList wrapping.Lines 122 and 124 already retrieve lists from the repository, and line 130 wraps the result again in
new ArrayList<>(). If the repository returns mutable lists (which Spring Data JPA typically does), the final wrapping on line 130 is unnecessary.Apply this diff to remove redundant wrapping:
- return new ArrayList<>(polls); + return polls;
275-283: Remove redundant existence check.The
existsByIdcall on line 279 is unnecessary. If the poll doesn't exist,findByPollIdOrderByOptionOrderwill simply return an empty list. This extra database round-trip adds no value and degrades performance.Apply this diff:
- if (!pollRepository.existsById(pollId)) { - throw new IllegalArgumentException("Cannot get options for a poll (" + pollId + ") that does not exist"); - } return optionRepository.findByPollIdOrderByOptionOrder(pollId);
322-325: Use primitive boolean instead of Boolean.TRUE.Line 322 uses
Boolean.TRUEwheretruesuffices. The primitive form is clearer and avoids unnecessary object creation.Apply this diff:
- option.setDeleted(Boolean.TRUE); + option.setDeleted(true);
509-546: Consider using a Java record.If the project uses Java 14+,
PollEntityAdaptercan be simplified to a record, reducing boilerplate while maintaining immutability.Example:
private record PollEntityAdapter(Poll delegate) implements Entity { @Override public String getId() { return delegate.getUuid(); } @Override public String getReference() { return delegate.getReference(); } @Override public String getReference(String rootProperty) { return delegate.getReference(rootProperty); } @Override public String getUrl() { return delegate.getUrl(); } @Override public String getUrl(String rootProperty) { return delegate.getUrl(rootProperty); } @Override public ResourceProperties getProperties() { return delegate.getProperties(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
polls/impl/pom.xml(2 hunks)polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java(4 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java(18 hunks)polls/tool/pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java
- polls/tool/pom.xml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
⏰ 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: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (3)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (3)
167-167: LGTM!The save result is correctly assigned back to
t, ensuring the entity's generated ID is captured.
246-262: LGTM!The null-safe retrieval with
orElse(null)and subsequent null check is correct. The optional inclusion of options based on the parameter is a good design choice.
691-693: LGTM!The repository-based existence check is clean and idiomatic for Spring Data JPA.
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
Outdated
Show resolved
Hide resolved
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
Outdated
Show resolved
Hide resolved
| if (ref == null) { | ||
| return null; | ||
| } | ||
|
|
||
| String[] parts = split(ref, Entity.SEPARATOR); | ||
| String uuid = parts.length > 0 ? parts[parts.length - 1] : ref; | ||
| if (uuid == null || uuid.isEmpty()) { | ||
| uuid = ref; | ||
| } | ||
|
|
||
| Poll poll = pollRepository.findByUuid(uuid).orElse(null); | ||
| if (poll != null && poll.getUuid() == null) { | ||
| poll.setUuid(idManager.createUuid()); | ||
| savePoll(poll); | ||
| } | ||
| return poll; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Side effect in getter method.
getPoll modifies and saves the poll if its UUID is null (lines 673-675). This violates command-query separation. If this is migration helper code for legacy data, document it clearly or extract to a separate ensureUuid(Poll) method.
Additionally, verify that the reference parsing logic correctly handles all expected reference formats:
Based on learnings
🏁 Script executed:
#!/bin/bash
# Find all places where poll references are constructed to verify format
rg -n 'REFERENCE_ROOT|getReference\(\)' --type=java -A2 -B2 -g '**/poll/**'Length of output: 4625
Separate UUID initialization from getPoll to preserve command–query separation
Move the logic that checks for a null UUID and persists the Poll (currently lines 673–675) into a dedicated ensureUuid(Poll) method invoked in a command or migration context. Add tests or documentation to confirm the parsing logic correctly extracts the UUID from both absolute references (via getReference()) and path-based (REFERENCE_ROOT) formats.
ottenhoff
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
kernel/api/src/main/java/org/sakaiproject/springframework/data/SpringCrudRepositoryImpl.java(0 hunks)polls/api/pom.xml(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Option.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Poll.java(6 hunks)polls/api/src/java/org/sakaiproject/poll/model/Vote.java(2 hunks)
💤 Files with no reviewable changes (1)
- kernel/api/src/main/java/org/sakaiproject/springframework/data/SpringCrudRepositoryImpl.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/api/src/java/org/sakaiproject/poll/model/Poll.javapolls/api/src/java/org/sakaiproject/poll/model/Vote.javapolls/api/src/java/org/sakaiproject/poll/model/Option.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
🧬 Code graph analysis (3)
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (2)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(39-82)polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(42-103)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(39-82)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(42-103)
⏰ 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: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
78-94: Move business logic from constructor to service layer.The constructor accesses static covers (
SessionManager,UsageSessionService) and contains business logic for fetching user/session context. This violates separation of concerns, complicates testing, and couples the entity to infrastructure. The TODO comment on line 86 acknowledges this issue.Refactor by:
- Create a factory method in the service layer (e.g.,
PollVoteManager.createVote(Poll, Option, String)) that populatesuserId,ip, andvoteDate.- Keep only simple field assignments in this constructor, or accept these values as parameters.
Example service-layer factory:
// In PollVoteManager or similar service public Vote createVote(Poll poll, Option option, String submissionId) { Vote vote = new Vote(); vote.setPollId(poll.getPollId()); vote.setPollOption(option.getOptionId()); vote.setSubmissionId(submissionId); vote.setVoteDate(Instant.now()); vote.setUserId(SessionManager.getCurrentSessionUserId()); UsageSession session = UsageSessionService.getSession(); if (session != null) { vote.setIp(session.getIpAddress()); } return vote; }Do you want me to generate a complete refactoring solution or open an issue to track this?
🧹 Nitpick comments (4)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (2)
64-66: Consider migrating fromDatetojava.time.Instant.The
@Temporalannotation withjava.util.Dateis deprecated in Jakarta Persistence 3.2. Modern JPA directly supportsjava.timetypes likeInstantorLocalDateTimewithout requiring@Temporal.If you choose to migrate, apply this diff:
- @Temporal(TemporalType.TIMESTAMP) @Column(name = "VOTE_DATE", nullable = false) - private Date voteDate; + private Instant voteDate;And update the import:
-import java.util.Date; +import java.time.Instant;Then adjust the constructor initialization (line 84):
- voteDate = new Date(); + voteDate = Instant.now();Based on learnings
74-76: Update constructor comment.The comment references "hibernate", but this is now a JPA entity. Update for accuracy.
Apply this diff:
public Vote() { - // needed by hibernate + // JPA requires a no-arg constructor }polls/api/src/java/org/sakaiproject/poll/model/Poll.java (2)
114-115: Boolean wrapper withnullable = falseis redundant.The fields
limitVotingandisPublicare declared asBoolean(wrapper type) withnullable = false. This is inconsistent:nullable = falseimplies the column is NOT NULL in the database, but the Java wrapper type can still benull, potentially causing NPEs. For non-nullable boolean columns, use the primitivebooleantype instead.Apply this diff to use primitive types:
- @Column(name = "POLL_LIMIT_VOTE", nullable = false) - private Boolean limitVoting = Boolean.TRUE; + @Column(name = "POLL_LIMIT_VOTE", nullable = false) + private boolean limitVoting = true;- @Column(name = "POLL_IS_PUBLIC", nullable = false) - private Boolean isPublic = Boolean.FALSE; + @Column(name = "POLL_IS_PUBLIC", nullable = false) + private boolean isPublic = false;Also update the constructor initialization at lines 136-137:
- this.limitVoting = Boolean.TRUE; - this.isPublic = Boolean.FALSE; + this.limitVoting = true; + this.isPublic = false;Also applies to: 123-124
84-92: Consider lazy fetching for LOB fields.The
textanddescriptionfields are marked as@Lobwith explicit@Basic(fetch = FetchType.EAGER). Eager loading of potentially large text fields can impact performance when loading polls. Unless these fields are always required immediately, consider usingFetchType.LAZYto defer loading until accessed.If lazy loading is acceptable:
- @Basic(fetch = FetchType.EAGER) + @Basic(fetch = FetchType.LAZY) @Column(name = "POLL_TEXT", nullable = false) private String text;- @Basic(fetch = FetchType.EAGER) + @Basic(fetch = FetchType.LAZY) @Column(name = "POLL_DETAILS") private String description;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
polls/api/src/java/org/sakaiproject/poll/model/Option.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Poll.java(6 hunks)polls/api/src/java/org/sakaiproject/poll/model/Vote.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/api/src/java/org/sakaiproject/poll/model/Option.javapolls/api/src/java/org/sakaiproject/poll/model/Vote.javapolls/api/src/java/org/sakaiproject/poll/model/Poll.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
🧬 Code graph analysis (3)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(43-105)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(40-84)
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (2)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(40-84)polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(43-105)
⏰ 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
🔇 Additional comments (9)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
55-57: Verify that eager loading of@Lobtext is necessary.The
textfield is marked with@Loband@Basic(fetch = FetchType.EAGER). Eager fetching of large objects loads the full content in every query, which can impact performance when loading many options or when the text is large.Consider whether the text is always needed immediately. If not, remove the
@Basic(fetch = FetchType.EAGER)to allow the JPA provider to use its default (typically LAZY for LOBs):@Lob - @Basic(fetch = FetchType.EAGER) @Column(name = "OPTION_TEXT", nullable = false) private String text;If eager loading is required for your access patterns, document why in a comment to inform future maintainers.
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
26-34: Migrate fromjavax.persistencetojakarta.persistence.The PR migrates to modern JPA, but these imports use the legacy
javax.persistencepackage from Java EE. Jakarta Persistence (the successor under Jakarta EE) uses thejakarta.persistencepackage and is required for modern providers and runtimes.Apply this diff to update the imports:
-import javax.persistence.Column; -import javax.persistence.Entity; -import javax.persistence.GeneratedValue; -import javax.persistence.GenerationType; -import javax.persistence.Id; -import javax.persistence.SequenceGenerator; -import javax.persistence.Table; -import javax.persistence.Temporal; -import javax.persistence.TemporalType; +import jakarta.persistence.Column; +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.GenerationType; +import jakarta.persistence.Id; +import jakarta.persistence.SequenceGenerator; +import jakarta.persistence.Table; +import jakarta.persistence.Temporal; +import jakarta.persistence.TemporalType;Based on learnings
⛔ Skipped due to learnings
Learnt from: ottenhoff PR: sakaiproject/sakai#14152 File: reset-pass/account-validator-api/src/main/java/org/sakaiproject/accountvalidator/model/ValidationAccount.java:24-33 Timestamp: 2025-10-06T22:15:40.541Z Learning: The Sakai project (as of this PR) uses Hibernate 5.6.15.Final and Spring 5.3.39, which requires javax.persistence imports, not jakarta.persistence. The jakarta namespace migration will happen when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.polls/api/src/java/org/sakaiproject/poll/model/Poll.java (7)
59-64: LGTM!The JPA entity mapping and Spring Data interface implementation are correctly declared. The class properly maps to the
POLL_POLLtable and implementsPersistableEntity<Long>.
68-72: LGTM! Previous issue resolved.The primary key strategy now correctly uses
GenerationType.AUTOwith an explicit@SequenceGenerator, which aligns with Sakai's legacy schema (sequence/trigger pattern). This resolves the previous concern aboutIDENTITYmismatching the existing database structure and is consistent with theOptionandVoteentities.
131-144: LGTM!The constructor correctly initializes default values for new poll instances. The initialization logic is sound.
146-157: LGTM!The
getId()method correctly implements thePersistableEntity<Long>interface contract, and the custom UUID accessors properly handle the field excluded from Lombok generation.
216-233: LGTM! UUID-based references correctly implemented.The
toString(),getUrl(), andgetReference()methods correctly use UUID-based identity instead of numeric IDs, aligning with the broader migration to UUID-based external references.
266-344: LGTM!The XML serialization methods correctly handle UUID-based identity while preserving backward compatibility with numeric poll IDs. Error handling for date parsing is appropriate, and null value handling is safe.
33-45: No changes needed: javax.persistence imports align with project-wide usage.
|
The JPA mapping between Poll, Option, and Vote Entities looks inadequate. |
I dont know what that means |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
54-55: Clarify the dual FK field + read-only relationship pattern.The
pollIdfield (line 54-55) and thepollrelationship (lines 74-76) both map to the same database columnOPTION_POLL_ID. The relationship is marked withinsertable = false, updatable = false, making it read-only. This pattern requires:
- The
pollIdfield must be manually set when creating or updating anOption- The
pollrelationship is populated by JPA for navigation but doesn't affect persistence- After persisting a new
Option, thepollfield will remain null until the entity is refreshed or reloadedThis mapping pattern can confuse developers about which field controls persistence. Consider adding documentation (Javadoc) to clarify:
pollId: "Foreign key field for persistence. Set this when creating/updating an Option."poll: "Read-only relationship field populated by JPA. Use pollId for persistence operations."Also applies to: 74-76
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
58-63: Dual FK field + read-only relationship pattern requires careful usage.Similar to the
Optionentity, bothpollId/poll(lines 58-63) andpollOption/option(lines 68-73) use a dual mapping pattern where:
- FK fields (
pollId,pollOption) control persistence- Relationship fields (
poll,option) are read-only (insertable = false, updatable = false) and used for navigationThe constructor correctly handles this by extracting IDs from the passed objects and setting the FK fields (lines 83-84). However, the pattern has implications:
- Only the FK fields affect database writes
- After persisting a new
Vote, the relationship fields will be null until the entity is refreshed- JPA cascade operations cannot be used effectively with read-only relationships
Consider adding Javadoc to clarify this pattern for maintainers.
Also applies to: 68-73
polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html (2)
8-8: Use kebab-case for class values (coding guideline).Rename or add a kebab-case alias for portletBody to comply.
As per coding guidelines
- <div class="portletBody container-fluid"> + <div class="portlet-body portletBody container-fluid">Consider removing the camelCase class once CSS is updated across the tool.
13-15: Gate “Add” nav link behind permissions for consistency.Currently always visible; other views conditionally show it. Align with capability (e.g., isSiteOwner or canAdd).
- <li class="nav-item"> + <li class="nav-item" th:if="${isSiteOwner}"> <a class="nav-link" th:href="@{/faces/voteAdd(pollId=${poll.pollId})}" th:text="#{action_add_poll}">Add</a> </li>If non-owners are allowed to add, please disregard.
polls/tool/src/webapp/WEB-INF/templates/polls/edit.html (3)
134-137: Avoid mixing th:field with explicit id; bind label via th:for to ensure accessible pairing.th:field also manages id; explicit id can be overridden, breaking the label association.
- <div class="form-check form-switch mt-4" th:if="${showPublicAccess}"> - <input type="checkbox" id="is-public" th:field="*{public}" /> - <label class="form-check-label" for="is-public" th:text="#{new_poll_access_public}">Public poll</label> - </div> + <div class="form-check form-switch mt-4" th:if="${showPublicAccess}"> + <input type="checkbox" th:field="*{public}" /> + <label class="form-check-label" th:for="*{public}" th:text="#{new_poll_access_public}">Public poll</label> + </div>
142-145: Same a11y issue for radio group: use th:for with th:field; drop manual th:id.Let Thymeleaf generate matching ids for each option.
- <div class="form-check" th:each="choice : ${displayResultChoices}"> - <input type="radio" th:field="*{displayResult}" th:value="${choice.value}" th:id="${'display-' + choice.value}" /> - <label class="form-check-label" th:for="${'display-' + choice.value}" th:text="#{${choice.labelKey}}">Choice</label> - </div> + <div class="form-check" th:each="choice : ${displayResultChoices}"> + <input type="radio" th:field="*{displayResult}" th:value="${choice.value}" /> + <label class="form-check-label" th:for="*{displayResult}" th:text="#{${choice.labelKey}}">Choice</label> + </div>
8-8: Use kebab-case for class values (coding guideline).CamelCase class portletBody violates the guideline.
As per coding guidelines
- <div class="portletBody container-fluid"> + <div class="portlet-body portletBody container-fluid">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
polls/api/src/java/org/sakaiproject/poll/model/Option.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Vote.java(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/edit.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/list.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- polls/tool/src/webapp/WEB-INF/templates/polls/list.html
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{html,xhtml}
📄 CodeRabbit inference engine (AGENTS.md)
Use kebab-case for values of HTML class and id attributes
Files:
polls/tool/src/webapp/WEB-INF/templates/polls/edit.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/api/src/java/org/sakaiproject/poll/model/Vote.javapolls/api/src/java/org/sakaiproject/poll/model/Option.java
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
📚 Learning: 2025-10-06T22:15:40.541Z
Learnt from: ottenhoff
PR: sakaiproject/sakai#14152
File: reset-pass/account-validator-api/src/main/java/org/sakaiproject/accountvalidator/model/ValidationAccount.java:24-33
Timestamp: 2025-10-06T22:15:40.541Z
Learning: The Sakai project (as of this PR) uses Hibernate 5.6.15.Final and Spring 5.3.39, which requires javax.persistence imports, not jakarta.persistence. The jakarta namespace migration will happen when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.
Applied to files:
polls/api/src/java/org/sakaiproject/poll/model/Option.java
🧬 Code graph analysis (2)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-90)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(40-102)
⏰ 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
🔇 Additional comments (2)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
24-36: Thejavax.persistenceimports are correct for the current Sakai version.The past review comment suggesting migration to
jakarta.persistenceis incorrect. According to learnings, Sakai currently uses Hibernate 5.6.15.Final and Spring 5.3.39, which requirejavax.persistenceimports. The jakarta namespace migration is planned for a future upgrade to JDK 21, Spring 6, and Hibernate 6+.Based on learnings
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
82-91: Setting relationship fields in the constructor maintains in-memory consistency.The constructor sets both FK fields (lines 83-84) and relationship fields (lines 85-86):
this.pollId = poll.getPollId(); this.pollOption = option.getOptionId(); this.poll = poll; this.option = option;While only the FK fields affect persistence (relationship fields are read-only), setting the relationship fields maintains in-memory object graph consistency. This is a valid pattern that allows immediate navigation after construction, though the relationship fields won't survive a persist/reload cycle without explicit refresh.
| public Option(Long optionId) { | ||
| this.optionId = optionId; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
The constructor doesn't initialize the required foreign key field.
The constructor Option(Long optionId) only sets the primary key optionId but doesn't initialize the required foreign key field pollId. When creating a new Option, pollId must be set separately, or the entity will fail validation at persistence time due to the nullable = false constraint.
Consider either:
- Adding a constructor that accepts both
optionIdandpollId - Adding documentation to clarify that
pollIdmust be set via the setter before persisting
Example constructor:
public Option(Long optionId, Long pollId) {
this.optionId = optionId;
this.pollId = pollId;
}🤖 Prompt for AI Agents
In polls/api/src/java/org/sakaiproject/poll/model/Option.java around lines 82 to
84, the one-arg constructor sets only optionId but leaves the non-nullable
foreign key pollId unset; add a second constructor that accepts both optionId
and pollId and assigns both fields (or alternatively document that pollId must
be set via its setter before persisting) so the entity meets the nullable=false
constraint at persistence time.
| </thead> | ||
| <tbody> | ||
| <tr th:each="option : ${options}"> | ||
| <td th:utext="${option.text}">Option text</td> |
There was a problem hiding this comment.
XSS risk: avoid unescaped HTML in option text.
th:utext renders raw HTML; option.text is user-provided. Use th:text unless you sanitize server-side.
- <td th:utext="${option.text}">Option text</td>
+ <td th:text="${option.text}">Option text</td>If rich text is required, confirm robust sanitization and document it here.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <td th:utext="${option.text}">Option text</td> | |
| <td th:text="${option.text}">Option text</td> |
🤖 Prompt for AI Agents
In polls/tool/src/webapp/WEB-INF/templates/polls/edit.html around line 94, the
template uses th:utext to render user-provided option.text which allows
unescaped HTML and creates an XSS risk; change it to th:text to escape HTML by
default so user input is rendered as plain text, or if rich HTML is truly
required, implement and call a robust server-side sanitizer for option.text
before rendering and add documentation/comments noting the sanitizer used and
why it’s safe.
polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
90-92: Constructor leaves required foreign key unset.The past review comment correctly identified that the
Option(Long optionId)constructor does not initialize the non-nullablepollIdfield. While it's common to have partially initialized entities, callers must remember to setpollIdbefore persisting, or a constraint violation will occur.Consider adding a constructor that accepts both
optionIdandpollId, or add documentation clarifying thatpollIdmust be set via the setter before persistence.
🧹 Nitpick comments (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
61-64: Reconsider EAGER fetch for @lob field.The
textfield is annotated with@Lob(indicating potentially large content) but usesFetchType.EAGER. This forces the database to always load the full text content, even when only the Option metadata (ID, order, etc.) is needed, which can degrade performance for queries that load many options.Unless immediate access to text is always required, consider using
FetchType.LAZYto defer loading until the field is accessed.Apply this diff to use lazy loading:
-@Basic(fetch = FetchType.EAGER) +@Basic(fetch = FetchType.LAZY)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
polls/api/src/java/org/sakaiproject/poll/model/Option.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Vote.java(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/edit.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- polls/tool/src/webapp/WEB-INF/templates/polls/edit.html
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/api/src/java/org/sakaiproject/poll/model/Option.javapolls/api/src/java/org/sakaiproject/poll/model/Vote.java
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
📚 Learning: 2025-10-06T22:15:40.541Z
Learnt from: ottenhoff
PR: sakaiproject/sakai#14152
File: reset-pass/account-validator-api/src/main/java/org/sakaiproject/accountvalidator/model/ValidationAccount.java:24-33
Timestamp: 2025-10-06T22:15:40.541Z
Learning: The Sakai project (as of this PR) uses Hibernate 5.6.15.Final and Spring 5.3.39, which requires javax.persistence imports, not jakarta.persistence. The jakarta namespace migration will happen when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.
Applied to files:
polls/api/src/java/org/sakaiproject/poll/model/Option.java
🧬 Code graph analysis (2)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(40-118)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-98)
⏰ 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 (8)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (3)
24-36: Skip commenting on javax.persistence imports.The past review comment suggesting migration to
jakarta.persistenceis incorrect. The Sakai project currently uses Hibernate 5.6.15.Final and Spring 5.3.39, which requirejavax.persistenceimports, notjakarta.persistence. Thejakartanamespace migration will occur when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.Based on learnings
78-84: LGTM!The read-only
@ManyToOnerelationship pattern is well-designed. Usinginsertable = false, updatable = falsewith the documentedpollIdforeign key column correctly separates navigation from persistence, preventing accidental updates through the relationship while allowing JPA to populate it for reads.
86-97: LGTM!The constructors and
getId()override are correctly implemented for JPA requirements and thePersistableEntity<Long>interface.polls/api/src/java/org/sakaiproject/poll/model/Vote.java (5)
26-34: Skip commenting on javax.persistence imports.The use of
javax.persistenceimports is correct for Sakai's current stack (Hibernate 5.6.15.Final and Spring 5.3.39). The migration tojakarta.persistencewill occur during the future upgrade to JDK 21, Spring 6, and Hibernate 6+.Based on learnings
46-74: LGTM!The JPA mappings for the primary key, user identification fields, and vote date are correctly configured. The use of
InstantforvoteDateis a good choice over legacy date types.
58-71: LGTM!The read-only
@ManyToOnerelationship toPollis correctly designed withinsertable = false, updatable = false. The pattern of using thepollIdforeign key for writes and thepollrelationship for navigation is well-documented and appropriate.
98-107: LGTM!The constructor correctly initializes both the foreign key fields (
pollId,pollOption) and the relationship objects (poll,option). This pattern allows the entity to be immediately usable without requiring a reload, while theinsertable = false, updatable = falseconstraints ensure JPA doesn't use the relationship fields for persistence operations.
109-116: LGTM!The
getId()override andtoString()implementations are correct and follow expected patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
80-81: Resolved: pollOption is non-null nowThis addresses the earlier review about null VOTE_OPTION mapping. Looks good.
🧹 Nitpick comments (5)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (5)
69-72: Make relations LAZY and non-optional; remove setters on read-only associationsAvoid default EAGER loads and reflect non-null FKs; prevent accidental state drift by not exposing setters on read-only relations.
- @ManyToOne - @JoinColumn(name = "VOTE_POLL_ID", nullable = false, insertable = false, updatable = false) - private Poll poll; + @ManyToOne(fetch = FetchType.LAZY, optional = false) + @JoinColumn(name = "VOTE_POLL_ID", nullable = false, insertable = false, updatable = false) + @Setter(AccessLevel.NONE) + private Poll poll; - @ManyToOne - @JoinColumn(name = "VOTE_OPTION", insertable = false, updatable = false) - private Option option; + @ManyToOne(fetch = FetchType.LAZY, optional = false) + @JoinColumn(name = "VOTE_OPTION", nullable = false, insertable = false, updatable = false) + @Setter(AccessLevel.NONE) + private Option option;Also applies to: 87-90
26-38: Add required imports for the above changesimport javax.persistence.Column; import javax.persistence.Entity; +import javax.persistence.FetchType; import javax.persistence.GeneratedValue; import javax.persistence.GenerationType; import javax.persistence.Id; +import javax.persistence.Index; import javax.persistence.JoinColumn; import javax.persistence.ManyToOne; import javax.persistence.SequenceGenerator; import javax.persistence.Table; +import lombok.AccessLevel; import lombok.Getter; import lombok.Setter;
48-48: Use SEQUENCE with @SequenceGenerator (clearer, matches intent)- @GeneratedValue(strategy = GenerationType.AUTO, generator = "poll_vote_id_sequence") + @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "poll_vote_id_sequence")
43-44: Add DB indexes on common lookup columnsHelps queries by poll, option, and user.
-@Table(name = "POLL_VOTE") +@Table(name = "POLL_VOTE", indexes = { + @Index(name = "IDX_POLL_VOTE_POLL", columnList = "VOTE_POLL_ID"), + @Index(name = "IDX_POLL_VOTE_OPTION", columnList = "VOTE_OPTION"), + @Index(name = "IDX_POLL_VOTE_USER", columnList = "USER_ID") +})
114-116: Nit: annotate toString- public String toString() { + @Override + public String toString() { return this.pollId + ":" + this.userId + ":" + this.ip + ":" + this.pollOption; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/api/src/java/org/sakaiproject/poll/model/Vote.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
🧬 Code graph analysis (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-98)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (2)
24-38: Consider jakarta.persistence if on Jakarta EE 9+ or Spring Boot 3+.The code uses
javax.persistence.*, which is the final JPA 2.2 API release. If this project is targeting Jakarta EE 9+ or Spring Boot 3+, migrating tojakarta.persistence.*is recommended. This requires updating imports, persistence.xml namespaces, and ensuring compatible JPA provider versions.Based on learnings
83-89: Add nullable=false to @joincolumn for consistency.The
optionrelationship's@JoinColumn(line 88) is missingnullable = false, while thepollrelationship (line 70) includes it. Although the FK column constraint on line 80 enforces non-null at the database level, includingnullable = falseon the@JoinColumnensures consistency in the entity model and may affect JPA validation behavior.Apply this diff for consistency:
@ManyToOne - @JoinColumn(name = "VOTE_OPTION", insertable = false, updatable = false) + @JoinColumn(name = "VOTE_OPTION", nullable = false, insertable = false, updatable = false) private Option option;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/api/src/java/org/sakaiproject/poll/model/Vote.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.621Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
🧬 Code graph analysis (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-98)
⏰ 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
🔇 Additional comments (5)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (5)
40-44: LGTM!Entity declaration is clean and follows JPA conventions with proper Lombok usage and PersistableEntity implementation.
46-74: LGTM!The ID generation strategy, column mappings, and read-only poll relationship are correctly implemented. The use of
InstantforvoteDatealigns with JPA 2.2 best practices.
91-92: LGTM!The
submissionIdfield is correctly mapped with appropriate constraints.
94-107: LGTM!The constructors are correctly implemented. Setting the read-only relationship fields (
poll,option) in the constructor is unconventional but harmless—JPA won't persist them due toinsertable=false, updatable=false, yet they're useful for navigation before the entity is persisted or reloaded.
109-116: LGTM!The
getId()override correctly implements thePersistableEntityinterface, and thetoString()method provides a reasonable debug representation.
|
I left a lot of comments in the other PR, should look at those. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
polls/impl/src/webapp/WEB-INF/components.xml (1)
35-48: Minor wording clarification in comment.Line 35 describes this as a "logic bean" but this is actually a service/manager bean. Consider updating the comment to reflect that it's a service bean that uses the repositories.
- <!-- create a logic bean, give it the repositories from above and some sakai services --> + <!-- PollListManager service bean with repositories and Sakai services -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java(1 hunks)polls/impl/src/webapp/WEB-INF/components.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java
- polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java
- polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.630Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
⏰ 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 (2)
polls/impl/src/webapp/WEB-INF/components.xml (2)
50-58: Verified: usageSessionService dependency is correctly configured and necessary.All three verification points confirm this change is sound:
- ✓ Used in implementation: PollVoteManagerImpl actively uses usageSessionService at lines 85–86 to retrieve session data
- ✓ Bean properly initialized: org.sakaiproject.event.api.UsageSessionService is defined in kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xml (lines 81–82)
- ✓ Necessary for functionality: The dependency is actively called in the implementation; similar patterns exist across syllabus, sitestats, samigo, presence, and other components
6-33: JPA entity mappings are adequate and properly configured.Verification confirms the entity classes (Poll, Option, Vote) use an intentional and well-documented design pattern:
- Option and Vote define read-only
@ManyToOnerelationships (insertable=false, updatable=false) with proper@JoinColumnmappings, while storing foreign keys as Long fields. Javadoc comments explicitly explain this pattern.- Poll uses
@Transientcollections rather than@OneToMany, allowing repository-based data loading instead of automatic ORM cascading.- All entities have proper JPA annotations (
@Entity,@Id,@SequenceGenerator,@Columnconstraints).- Repository implementations correctly extend
SpringCrudRepositoryImpland use HibernateCriteriaQueryfor custom queries.- The
components.xmlconfiguration properly registers all entity classes and injects theGlobalSessionFactoryinto each repository.The pattern is consistent, complete, and follows the project's repository-based access convention. No changes required.
ottenhoff
force-pushed
the
polls-thymeleaf
branch
from
dd90ca4 to
a4dfe0f
Compare
|
Regarding one repo vs multiple repos: The split repositories are intentional and line up with how the entities are modeled.
Given the current structure, keeping the three repositories is the cleanest and safest approach. If the goal is to reduce surface area, a better path would be wrapping them behind higher-level service methods rather than collapsing the repositories themselves. |
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 (5)
polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (5)
314-316: Fix SLF4J formattingFormat string lacks {}. Second arg is ignored.
As per coding guidelines
- log.debug("requestStorage" , requestStorage); + log.debug("requestStorage: {}", requestStorage);
435-438: String.format specifies %d but passes a StringThis will throw IllegalFormatConversionException at runtime.
- return String.format("Poll id %d removed", id); + return String.format("Poll id %s removed", id);
636-639: Incorrect property name casing in excludes"UUId" won’t match "uuid", risking accidental overwrite.
- developerHelperService.copyBean(option, current, 0, new String[] { - "id", "pollId", "UUId" }, true); + developerHelperService.copyBean(option, current, 0, new String[] { + "id", "pollId", "uuid" }, true);
562-565: Likely wrong accessor: Option.getId() vs getOptionId()Elsewhere Option uses getOptionId(). Returning getId() may NPE/compile-fail depending on the model.
- return option.getId() + ""; + return option.getOptionId() + "";
462-466: Catching UnsupportedOperationException for parseLong is incorrectUse NumberFormatException to handle bad numeric input.
- } catch (UnsupportedOperationException e) { + } catch (NumberFormatException e) { throw new IllegalArgumentException( "Invalid: pollId must be a long number: " + e.getMessage(), e); }
♻️ Duplicate comments (5)
polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html (1)
31-34: Use th:field for proper Spring form binding (repeat of prior feedback).The file input still uses
name="file"instead ofth:field="*{file}"for Spring form binding. This prevents proper error population and form state restoration.Apply this diff to align with Thymeleaf/Spring best practices:
<input class="form-control" th:classappend="${#fields.hasErrors('file')}? ' is-invalid'" - type="file" id="batch-file" name="file" accept=".csv,.xls,.xlsx,.txt" required /> + type="file" id="batch-file" th:field="*{file}" accept=".csv,.xls,.xlsx,.txt" required /> <div class="invalid-feedback" th:if="${#fields.hasErrors('file')}" th:errors="*{file}">Provide a valid file</div>polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (3)
68-71: Use poll_missing for not-found case (better UX)Switch message key to distinguish missing poll from permission denial.
- redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale)); + redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll_missing", null, locale));
96-100: Catch SecurityException in POST handler to avoid 500sWrap getPollById(...) and redirect on permission issues, mirroring the GET handler.
- Poll poll = pollListManager.getPollById(voteForm.getPollId()); + Poll poll; + try { + poll = pollListManager.getPollById(voteForm.getPollId()); + } catch (SecurityException e) { + log.debug("User lacks permission to view poll {}", voteForm.getPollId(), e); + redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale)); + return "redirect:/faces/votePolls"; + }
101-107: Null‑safe handling of selectedOptionIdsGuard against NPE when no options are selected.
- VoteCollection voteCollection = pollsUiService.submitVote(voteForm.getPollId(), new ArrayList<>(voteForm.getSelectedOptionIds())); + List<Long> optionIds = voteForm.getSelectedOptionIds() != null + ? new ArrayList<>(voteForm.getSelectedOptionIds()) + : new ArrayList<>(); + VoteCollection voteCollection = pollsUiService.submitVote(voteForm.getPollId(), optionIds);polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (1)
210-245: Enhance vote submission validation.The current validation (lines 247-262) checks counts but doesn't verify that submitted option IDs belong to the poll or are unique (excluding the 0L sentinel for minOptions=0). This allows cross-poll or duplicate selections.
Add membership and uniqueness checks before
validateVoteSelection:if (votesToProcess.isEmpty() && poll.getMinOptions() == 0) { votesToProcess.add(0L); } + + // Validate option membership and uniqueness (ignore sentinel 0L for zero-min polls) + if (!(votesToProcess.size() == 1 && votesToProcess.get(0) == 0L)) { + Set<Long> uniqueIds = new TreeSet<>(votesToProcess); + if (uniqueIds.size() != votesToProcess.size()) { + throw new PollValidationException("invalid_option_selection"); + } + List<Option> allowedOptions = pollListManager.getVisibleOptionsForPoll(pollId); + Set<Long> allowedIds = allowedOptions.stream() + .map(Option::getOptionId) + .collect(Collectors.toSet()); + if (!allowedIds.containsAll(votesToProcess)) { + throw new PollValidationException("invalid_option_selection"); + } + } validateVoteSelection(poll, votesToProcess);
🧹 Nitpick comments (14)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
24-28: Add bean validation to fail fast on bad inputGuard against null pollId and empty selections at binding time.
import lombok.Data; +import javax.validation.constraints.NotEmpty; +import javax.validation.constraints.NotNull; @Data public class VoteForm { - private Long pollId; - private List<Long> selectedOptionIds = new ArrayList<>(); + @NotNull + private Long pollId; + @NotEmpty + private List<Long> selectedOptionIds = new ArrayList<>(); }polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
19-31: Add serialVersionUID and overload with causeImproves serialization and exception chaining.
public class PollValidationException extends RuntimeException { + private static final long serialVersionUID = 1L; private final Object[] args; public PollValidationException(String messageKey, Object... args) { super(messageKey); this.args = args; } + public PollValidationException(String messageKey, Throwable cause, Object... args) { + super(messageKey, cause); + this.args = args; + } + public Object[] getArgs() { return args; } }polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (2)
25-50: Avoid stringly-typed paths; prefer JPA static metamodelUsing root.get("field") is brittle. Generate and use Poll_ to get compile-time checks (e.g., root.get(Poll_.siteId)).
20-93: Add DB indexes for query fieldsMethods filter/order on siteId, creationDate, uuid, voteOpen, voteClose. Ensure indexes/constraints exist (e.g., index(siteId, creationDate), unique(uuid)) to prevent scans.
If helpful, I can draft a migration script for these indexes.
polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (4)
123-125: Use parameterized logging, no concatenationAs per coding guidelines, use SLF4J placeholders.
As per coding guidelines
- if (log.isDebugEnabled()) { - log.debug("poll for site " + siteId); - } + if (log.isDebugEnabled()) { + log.debug("poll for site {}", siteId); + }
274-275: Prefer contextual message with placeholderLog the id with a message.
As per coding guidelines
- log.debug(id); + log.debug("poll id: {}", id);
718-719: Avoid string concatenation in logsAs per coding guidelines
- log.debug("got vote: " + vote.toString()); + log.debug("got vote: {}", vote);
788-789: Parameterize loggingAs per coding guidelines
- log.debug("current user is: " + currentUser); + log.debug("current user is: {}", currentUser);polls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.java (1)
118-129: Consider extracting option creation to reduce duplication.The two option creation blocks (option1 and option2) are nearly identical except for the text and order values. While acceptable for test data setup, extracting a helper method could improve maintainability.
Consider a helper like:
private Option createOption(Long pollId, String text, int order) { Option option = new Option(); option.setText(text); option.setPollId(pollId); option.setUuid(UUID.randomUUID().toString()); option.setOptionOrder(order); return option; }Then use it as:
- Option option1 = new Option(); - option1.setText("Option 1"); - option1.setPollId(poll1.getPollId()); - option1.setUuid(UUID.randomUUID().toString()); - option1.setOptionOrder(0); - optionRepository.save(option1); - - Option option2 = new Option(); - option2.setText("Option 2"); - option2.setPollId(poll1.getPollId()); - option2.setUuid(UUID.randomUUID().toString()); - option2.setOptionOrder(1); - optionRepository.save(option2); + Option option1 = createOption(poll1.getPollId(), "Option 1", 0); + optionRepository.save(option1); + + Option option2 = createOption(poll1.getPollId(), "Option 2", 1); + optionRepository.save(option2);polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (3)
27-33: Prefer JPA relationships over scalar FK fields in queriesUsing root.get("pollId") and root.get("pollOption") implies scalar FK fields on Vote. Model Vote with @manytoone Poll and Option, and query via joins (e.g., root.join("poll").get("id")), which enables cascades, referential integrity, and fetch strategies. This aligns with the PR goal of “replace HBM with JPA”. I can draft entity and repo diffs if helpful.
Also applies to: 44-49, 81-86
104-111: Optimize existence check to avoid COUNT scansReplace COUNT(*) with a constant-select and maxResults(1) to short‑circuit on the first match; typically faster with indexes.
- query.select(cb.count(root)) - .where(cb.and(pollPredicate, userPredicate)); - - Long count = sessionFactory.getCurrentSession() - .createQuery(query) - .uniqueResult(); - - return count != null && count > 0; + CriteriaQuery<Long> query = cb.createQuery(Long.class); + Root<Vote> root = query.from(Vote.class); + query.select(cb.literal(1L)) + .where(cb.and(pollPredicate, userPredicate)); + List<Long> hits = sessionFactory.getCurrentSession() + .createQuery(query) + .setMaxResults(1) + .getResultList(); + return !hits.isEmpty();
123-131: Use long return for countsDistinct submission count can exceed Integer.MAX_VALUE in large sites. Prefer long to avoid overflow.
- public int countDistinctSubmissionIds(Long pollId) { + public long countDistinctSubmissionIds(Long pollId) { ... - return count == null ? 0 : count.intValue(); + return count == null ? 0L : count;polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java (1)
153-170: Prefer assertThrows over try/catch in testsReplace manual try/catch + fail() with assertThrows for clarity and precise assertions.
- try { - pollListManager.savePoll(null); - Assert.fail(); - } - catch (IllegalArgumentException e) { - log.debug("Expected illegal argument when saving null poll", e); - } + Assert.assertThrows(IllegalArgumentException.class, () -> pollListManager.savePoll(null));Repeat for the other expected-exception branches.
Also applies to: 172-182, 199-209
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
193-201: Locale normalization ruleMapping en_ZA→UK is unusual. If intentional, add a brief comment documenting why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (92)
kernel/api/src/main/java/org/sakaiproject/springframework/data/SpringCrudRepositoryImpl.java(0 hunks)polls/api/pom.xml(1 hunks)polls/api/src/bundle/org/sakaiproject/poll/bundle/Messages.properties(1 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Option.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Poll.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Vote.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/logic/PollListManager.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/logic/PollVoteManager.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Option.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Poll.java(7 hunks)polls/api/src/java/org/sakaiproject/poll/model/Vote.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/PollRepository.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java(1 hunks)polls/impl/pom.xml(2 hunks)polls/impl/src/bundle/org/sakaiproject/poll/spring-hibernate.xml(0 hunks)polls/impl/src/java/org/sakaiproject/poll/dao/impl/PollDoaImpl.java(0 hunks)polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java(4 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java(19 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java(6 hunks)polls/impl/src/test/hibernate-test.xml(2 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java(2 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java(9 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.java(2 hunks)polls/impl/src/webapp/WEB-INF/components.xml(1 hunks)polls/impl/src/webapp/WEB-INF/hibernate-hbms.xml(0 hunks)polls/tool/pom.xml(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/beans/ErrorHandler.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/constants/NavigationConstants.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.java(2 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollVoteEntityProvider.java(3 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java(5 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/evolvers/SakaiFCKTextEvolver.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollActionResultInterceptor.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollBeanLocator.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionBatchViewParameters.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionViewParameters.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/PermissionAction.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/PollToolBean.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/AddPollProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/ConfirmProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PermissionsProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionBatchProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionDeleteProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollToolProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollVoteProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/ResultsProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/validators/OptionValidator.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/validators/VoteValidator.java(0 hunks)polls/tool/src/webapp/WEB-INF/applicationContext.xml(0 hunks)polls/tool/src/webapp/WEB-INF/requestContext.xml(0 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/edit.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/list.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-edit.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/permissions.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/results.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/thanks.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/vote.html(1 hunks)polls/tool/src/webapp/WEB-INF/web.xml(1 hunks)polls/tool/src/webapp/component-templates/Messages.html(0 hunks)polls/tool/src/webapp/js/polls.js(0 hunks)polls/tool/src/webapp/templates/pollOption.html(0 hunks)polls/tool/src/webapp/templates/pollOptionBatch.html(0 hunks)polls/tool/src/webapp/templates/pollOptionDelete.html(0 hunks)polls/tool/src/webapp/templates/voteAdd.html(0 hunks)polls/tool/src/webapp/templates/votePermissions.html(0 hunks)polls/tool/src/webapp/templates/votePolls.html(0 hunks)polls/tool/src/webapp/templates/voteQuestion.html(0 hunks)polls/tool/src/webapp/templates/voteResults.html(0 hunks)polls/tool/src/webapp/templates/voteThanks.html(0 hunks)
💤 Files with no reviewable changes (40)
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionDeleteProducer.java
- polls/tool/src/webapp/templates/voteQuestion.html
- kernel/api/src/main/java/org/sakaiproject/springframework/data/SpringCrudRepositoryImpl.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PermissionsProducer.java
- polls/tool/src/webapp/templates/votePermissions.html
- polls/tool/src/webapp/WEB-INF/requestContext.xml
- polls/api/src/java/org/sakaiproject/poll/hbm/Vote.hbm.xml
- polls/tool/src/webapp/templates/pollOptionDelete.html
- polls/tool/src/webapp/templates/voteResults.html
- polls/impl/src/webapp/WEB-INF/hibernate-hbms.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionBatchViewParameters.java
- polls/tool/src/webapp/templates/pollOption.html
- polls/tool/src/java/org/sakaiproject/poll/tool/beans/ErrorHandler.java
- polls/tool/src/webapp/templates/votePolls.html
- polls/impl/src/java/org/sakaiproject/poll/dao/impl/PollDoaImpl.java
- polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollBeanLocator.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionBatchProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionProducer.java
- polls/tool/src/webapp/templates/pollOptionBatch.html
- polls/tool/src/webapp/component-templates/Messages.html
- polls/tool/src/java/org/sakaiproject/poll/tool/validators/OptionValidator.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/ConfirmProducer.java
- polls/tool/src/webapp/templates/voteAdd.html
- polls/tool/src/java/org/sakaiproject/poll/tool/validators/VoteValidator.java
- polls/tool/src/java/org/sakaiproject/poll/tool/constants/NavigationConstants.java
- polls/api/src/java/org/sakaiproject/poll/hbm/Option.hbm.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/params/PollToolBean.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollVoteProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionViewParameters.java
- polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollActionResultInterceptor.java
- polls/api/src/java/org/sakaiproject/poll/hbm/Poll.hbm.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/ResultsProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/AddPollProducer.java
- polls/tool/src/webapp/js/polls.js
- polls/tool/src/java/org/sakaiproject/poll/tool/params/PermissionAction.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollToolProducer.java
- polls/tool/src/webapp/templates/voteThanks.html
- polls/impl/src/bundle/org/sakaiproject/poll/spring-hibernate.xml
- polls/tool/src/webapp/WEB-INF/applicationContext.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/evolvers/SakaiFCKTextEvolver.java
🚧 Files skipped from review as they are similar to previous changes (24)
- polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java
- polls/api/src/java/org/sakaiproject/poll/logic/PollVoteManager.java
- polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java
- polls/tool/src/webapp/WEB-INF/templates/polls/permissions.html
- polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java
- polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java
- polls/tool/src/webapp/WEB-INF/templates/polls/results.html
- polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java
- polls/tool/src/webapp/WEB-INF/templates/polls/vote.html
- polls/tool/src/webapp/WEB-INF/templates/polls/thanks.html
- polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java
- polls/impl/src/test/hibernate-test.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java
- polls/api/src/bundle/org/sakaiproject/poll/bundle/Messages.properties
- polls/tool/src/webapp/WEB-INF/templates/polls/list.html
- polls/api/src/java/org/sakaiproject/poll/model/Option.java
- polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java
- polls/tool/pom.xml
- polls/tool/src/webapp/WEB-INF/templates/polls/edit.html
- polls/tool/src/webapp/WEB-INF/web.xml
- polls/tool/src/webapp/WEB-INF/templates/polls/option-edit.html
- polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java
- polls/api/src/java/org/sakaiproject/poll/repository/PollRepository.java
- polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollVoteEntityProvider.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/api/src/java/org/sakaiproject/poll/logic/PollListManager.javapolls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.javapolls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.javapolls/api/src/java/org/sakaiproject/poll/model/Poll.javapolls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.javapolls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.javapolls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.javapolls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.javapolls/api/src/java/org/sakaiproject/poll/model/Vote.javapolls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.javapolls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.java
**/*.{html,xhtml}
📄 CodeRabbit inference engine (AGENTS.md)
Use kebab-case for values of HTML class and id attributes
Files:
polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.htmlpolls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.630Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
🧬 Code graph analysis (11)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-31)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java (1)
Configuration(44-109)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (2)
Value(232-236)Controller(50-237)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (2)
Value(157-164)Controller(43-165)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
Controller(44-127)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-34)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
Repository(17-94)
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (2)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(40-132)polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-98)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (5)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (2)
Value(203-216)Controller(46-217)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
Slf4j(69-686)polls/api/src/java/org/sakaiproject/poll/model/Poll.java (1)
Slf4j(59-345)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java (1)
Slf4j(46-241)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-31)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-34)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
Repository(15-132)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-98)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
polls/api/src/java/org/sakaiproject/poll/util/PollUtil.java (1)
PollUtil(30-82)
🔇 Additional comments (42)
polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html (1)
27-29: CSRF protection now in place.The form correctly includes a CSRF token (line 28) with a null-check guard. This addresses the prior critical security concern. The defensive
th:if="${_csrf != null}"ensures graceful handling if CSRF is not available.polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (3)
357-361: Initialize uuid on create (LGTM)Setting uuid when missing is correct and idempotent.
417-419: Preserve uuid on update (LGTM)Including "uuid" in the excludes list ensures it’s not overwritten.
765-766: Vote.voteDate correctly uses Instant type—no issues foundVerification confirms Vote.voteDate is declared as
private Instant voteDate;with JPA@Columnmapping. The Vote class uses Lombok's@Setterannotation, which generates asetVoteDate(Instant)method. The codevote.setVoteDate(Instant.now());at lines 765-766 is type-safe and compiles correctly.polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.java (2)
123-125: UUID initialization looks correct.The UUID assignment when
poll.getUuid() == nullaligns with the migration from ID-based to UUID-based identity. This ensures new polls have UUIDs while preserving existing ones.
176-176: No additional concerns identified. Review comment verification complete.The Poll entity properly implements UUID-based identity:
- ✅ UUID methods exposed:
getUuid()andsetUuid(String uuid)are explicitly defined (lines 138–141)- ✅ ID field consistent:
getId()returns the database-generatedpollId; nosetId()exists (appropriate for @GeneratedValue fields)- ✅ Exclusion list correct: The copyBean call excludes both "uuid" and "pollId", preventing identifier duplication during entity copy operations
- ✅ No confusion: Clear separation between
getId()for database operations andgetUuid()for API/reference operationsThe UUID field is properly stored as a persistent column (
POLL_UUID, nullable=false) and accessed through dedicated getter/setter methods.polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.java (1)
26-37: LGTM! Clean Spring MVC controller.The controller follows Spring MVC best practices with constructor-based dependency injection via Lombok's
@RequiredArgsConstructorand straightforward request mapping. The single responsibility of rendering the permissions view is clear.polls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.java (1)
101-112: LGTM! Repository migration and UUID handling are correct.The method signature change from
GenericDaoto specific repository parameters aligns with the repository pattern migration. UUID assignment before persistence ensures test data consistency with production entity handling.polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.java (2)
40-58: LGTM! Standard Spring MVC bootstrap configuration.The initializer correctly sets up the Spring application context with configuration classes, registers Sakai-specific listeners and filters, and configures the DispatcherServlet. The dual mapping of "/" and "/faces/*" ensures both root and prefixed paths are handled.
52-52: File upload is a legitimate requirement for the Polls tool.The Polls tool uses multipart file uploads for batch importing poll options. The
OptionController.uploadBatch()method acceptsMultipartFilerequests via the form atpolls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html, processed throughPollsUiService.saveOptionsBatch(). TheRequestFilter.CONFIG_UPLOAD_ENABLED="true"configuration is necessary and appropriate.polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ToolEntryController.java (1)
23-30: LGTM! Clean entry point redirect.The controller provides a simple redirect from the tool root to the main polls listing view, replacing the RSF-based entry point. The dual path mapping ("" and "/") ensures both variants are handled consistently.
polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java (2)
20-20: Implementation is correct; review comment is based on a false premise.The custom implementation already exists in
VoteRepositoryImpl.java(lines 115–131) and correctly performsCOUNT(DISTINCT submissionId). Line 123 usescb.countDistinct(root.get("submissionId")), which properly applies the distinct count to thesubmissionIdfield, exactly as required. The method is properly implemented using Hibernate's CriteriaBuilder API with null handling and correct type conversion.Likely an incorrect or invalid review comment.
16-16: Remove the review comment—implementation is already correct.The
findByUserIdAndPollIdsmethod already has a proper custom implementation inVoteRepositoryImpl(lines 73–90) that correctly accesses the singularpollIdfield usingroot.get("pollId").in(pollIds). The method name's plural parameter form (pollIds) is acceptable for a collection parameter and does not require changes. The implementation is sound and follows standard Spring Data patterns.polls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.java (1)
10-10: Option entity has properly mapped fields for the derived query—no issues found.The Option entity at
polls/api/src/java/org/sakaiproject/poll/model/Option.javacontains both required fields with correct JPA mappings:
pollIdfield (line 59):@Column(name = "OPTION_POLL_ID", nullable = false) private Long pollId;optionOrderfield (line 76):@Column(name = "OPTION_ORDER", nullable = false) private Integer optionOrder;The implementation in
OptionRepositoryImpl.javaconfirms these fields are correctly used in the query logic. Spring Data's query derivation will work as expected.polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
18-33: LGTM on null‑safe guards and Criteria usageEarly returns on null inputs and typed Criteria queries look clean and consistent.
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java (1)
35-50: Bean wiring looks correctSetter injection is consistent with the existing providers. No issues spotted.
Also applies to: 52-63, 65-74, 76-91
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java (1)
230-237: Good: switched voteDate to InstantAligns with modern time API and avoids Date mutability pitfalls.
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (2)
84-92: Time/locale handling looks solidUTC sort keys and localized display via UserTimeService are appropriate.
Also applies to: 106-118
146-172: Confirm permissions are enforced in bulk actionsEnsure PollsUiService.deletePolls/resetPollVotes perform per‑poll permission checks server‑side; controllers should not rely solely on UI constraints. If not, add checks here.
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java (3)
68-93: LGTM! Vote creation logic is sound.The validation flow correctly delegates parameter checks to the Vote constructor (which enforces poll/option relationship integrity per Vote.java lines 110-112), and the IP fallback to
DEFAULT_IP_ADDRESSaligns with existing patterns in the entity providers.
110-130: LGTM! Repository-backed vote retrieval.The conditional query logic and map-building pattern are correct, and explicit types are properly declared throughout.
178-232: LGTM! Boolean accessor updates align with entity conventions.The changes from
getLimitVoting()/getIsPublic()toisLimitVoting()/isPublic()match the updated Poll entity (Poll.java lines 115, 124) boolean accessor patterns.polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (3)
62-99: LGTM! Comprehensive null and permission guards.The GET handler correctly validates poll existence (lines 70-73) and edit permissions (lines 75-78), addressing the concerns from previous reviews. Explicit types are properly declared throughout.
101-166: LGTM! Robust save flow with proper error handling.The POST handler includes appropriate permission checks (lines 109-112), null guards (lines 146-149), and comprehensive validation error mapping (lines 133-144). All concerns from previous reviews have been addressed.
168-188: LGTM! Poll entity preparation with proper type declarations.Explicit types are correctly used throughout (lines 169, 183-185), and the null-handling pattern (lines 170-172) is appropriate.
polls/impl/pom.xml (1)
53-130: LGTM! Dependency migration aligns with repository pattern.The addition of Spring Data JPA (lines 53-54) and Jackson ecosystem (lines 101-130) supports the PR's migration from legacy DAO to repository-based persistence and modern JSON/XML handling.
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (2)
68-72: LGTM! JPA identity strategy correctly configured.The use of
GenerationType.AUTOwith an explicit@SequenceGenerator(lines 69-70) properly addresses the legacy Sakai schema requirements, avoiding theIDENTITYmismatch issue noted in previous reviews.
126-157: LGTM! UUID-based external identity with proper accessors.The uuid field mapping (lines 128-129) and dedicated accessors (lines 151-157) correctly separate internal database identity (pollId) from external references (uuid), aligning with the broader entity migration pattern.
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (4)
131-172: LGTM! Poll save flow with UUID generation.The UUID generation logic (lines 145-149) appropriately falls back to
UUID.randomUUID()whenidManager.createUuid()returns blank, and the repository save pattern (line 162) correctly replaces the legacy DAO approach.
240-258: LGTM! Safe poll retrieval with option hydration.The null check after
findById(lines 243-245) prevents NPE, and the conditional option loading (lines 247-250) provides flexibility for callers who don't need the full object graph.
294-304: LGTM! Side-effect extracted from getter.The new
ensureOptionUuidAndSavemethod (lines 294-304) correctly addresses the previous review concern by separating mutation from thegetOptionByIdread operation, improving command-query separation.Based on learnings
637-660: LGTM! Robust UUID-based poll lookup.The reference parsing logic (lines 642-652) handles multiple formats safely with
StringUtilsguards, and the null check (line 654) prevents issues with missing polls. The UUID initialization on line 656 appears to be a migration helper for legacy data.polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (4)
59-86: LGTM! Bulk operations with proper null guards.Both
deletePolls(lines 59-72) andresetPollVotes(lines 74-86) correctly check for null polls and log warnings instead of crashing, addressing previous review concerns. Explicit types are properly declared (lines 61, 76).
88-125: LGTM! Poll save with comprehensive validation.The validation flow properly checks text (lines 89-91), dates (lines 93-99), min/max constraints (lines 101-103), and includes the null guard for existing polls (lines 105-109) as noted in past reviews. Explicit types are correctly used throughout.
127-145: LGTM! Option save with order management.The option persistence logic correctly handles both new (lines 133-134) and existing (lines 136-142) options, with proper explicit type declarations (line 136:
Option existing).
176-208: LGTM! Option deletion with orphan vote handling.The deleteOption flow properly handles the two orphan strategies (lines 186-202), uses batch delete for votes (line 196 per past review suggestion to replace loop), and includes null checks. Explicit types are correctly declared (lines 177, 182, 183, 187).
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (3)
46-50: LGTM! Identity strategy properly configured.The use of
GenerationType.AUTOwith explicit@SequenceGenerator(lines 47-48) correctly aligns with Sakai's legacy schema, matching the pattern in Poll.java and Option.java.
76-92: LGTM! Foreign key mappings with read-only relationships.The
pollOptionfield (line 80) correctly includesnullable=falseas addressed in past reviews, and the read-only@ManyToOnerelationship (lines 87-89) properly separates write concerns from navigation.
98-121: LGTM! Comprehensive constructor validation.The validation logic (lines 99-112) thoroughly checks for null parameters and verifies poll/option membership, preventing data integrity issues. The fallback logic for
optionPollId(lines 107-109) handles both detached and managed Option instances.polls/impl/src/webapp/WEB-INF/components.xml (3)
6-18: LGTM! JPA entity registration for Hibernate.The
AdditionalHibernateMappingsbean (lines 6-15) correctly registers the Poll, Option, and Vote entities as annotated classes, replacing the legacy HBM XML mappings.
20-48: LGTM! Repository wiring and manager integration.The repository bean definitions (lines 20-33) properly reference
GlobalSessionFactory, and the updatedPollListManagerwiring (lines 36-48) correctly replaces DAO dependencies with the three repository instances.
50-58: LGTM! PollVoteManager wiring includes session service.The addition of
usageSessionService(line 57) provides the IP resolution capability used inPollVoteManagerImpl.createVote(lines 85-90), completing the dependency chain for vote creation.
| <dependency> | ||
| <groupId>javax.persistence</groupId> | ||
| <artifactId>javax.persistence-api</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.data</groupId> | ||
| <artifactId>spring-data-jpa</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
Reconsider Spring Data dependency in API module
Placing spring-data-jpa in the API leaks JPA implementation concerns downstream. If repository interfaces only need Spring Data abstractions, depend on spring-data-commons (or mark as optional/provided).
- <dependency>
- <groupId>org.springframework.data</groupId>
- <artifactId>spring-data-jpa</artifactId>
- </dependency>
+ <!-- If interfaces only need Spring Data base types, prefer commons -->
+ <dependency>
+ <groupId>org.springframework.data</groupId>
+ <artifactId>spring-data-commons</artifactId>
+ <optional>true</optional>
+ </dependency>Also verify javax.persistence-api vs jakarta.persistence alignment with the parent BOM.
🏁 Script executed:
#!/bin/bash
# Inspect imports to confirm javax vs jakarta usage across modules
rg -nP '\b(jakarta|javax)\.persistence\b' -g '!**/target/**' -C2Length of output: 238432
🏁 Script executed:
#!/bin/bash
# Get full context of polls/api/pom.xml and check parent BOM
echo "=== POLLS/API POM.XML ===" && \
cat polls/api/pom.xml && \
echo -e "\n=== PARENT POM BOM SECTION ===" && \
grep -A 30 "<dependencyManagement>" master/pom.xml | grep -A 20 "spring-data" && \
echo -e "\n=== CHECK FOR REPOSITORY INTERFACES IN POLLS/API ===" && \
find polls/api -name "*.java" -type f -exec grep -l "Repository\|@Repository" {} \;Length of output: 2270
🏁 Script executed:
#!/bin/bash
# More targeted search for repository usage in polls/api
echo "=== JAVA FILES IN POLLS/API ===" && \
find polls/api/src -name "*.java" -type f 2>/dev/null | head -20 && \
echo -e "\n=== SEARCH FOR REPOSITORY PATTERN IN POLLS/API ===" && \
rg -n "extends.*Repository|CrudRepository|PagingAndSortingRepository" polls/api/src --type java 2>/dev/null || echo "(no matches)" && \
echo -e "\n=== CHECK POLLS PARENT POM ===" && \
cat polls/pom.xml | grep -A 5 "spring-data-jpa" && \
echo -e "\n=== VERIFY IMPORTS IN POLLS/API ===" && \
rg -n "spring.data|org.springframework" polls/api/src --type java -C 1 2>/dev/null || echo "(no matches)"Length of output: 1854
🏁 Script executed:
#!/bin/bash
# More targeted search for repository usage in polls/api
echo "=== JAVA FILES IN POLLS/API ===" && \
find polls/api/src -name "*.java" -type f 2>/dev/null | head -20 && \
echo -e "\n=== SEARCH FOR REPOSITORY PATTERN IN POLLS/API ===" && \
rg -n "extends.*Repository|CrudRepository|PagingAndSortingRepository" polls/api/src --type java 2>/dev/null || echo "(no matches)" && \
echo -e "\n=== CHECK POLLS PARENT POM ===" && \
cat polls/pom.xml 2>/dev/null | grep -A 10 "spring-data-jpa" && \
echo -e "\n=== VERIFY IMPORTS IN POLLS/API ===" && \
rg -n "spring.data|org.springframework" polls/api/src --type java -C 1 2>/dev/null | head -30 || echo "(no matches)"Length of output: 1854
🏁 Script executed:
#!/bin/bash
# Check what's actually in polls/impl to see if spring-data-jpa is used there
echo "=== REPOSITORY INTERFACES IN POLLS/IMPL ===" && \
rg -n "extends.*Repository|@Repository" polls/impl/src --type java -C 2 2>/dev/null | head -40 && \
echo -e "\n=== SPRING DATA IMPORTS IN POLLS/IMPL ===" && \
rg -n "import.*springframework.data" polls/impl/src --type java 2>/dev/null | head -20Length of output: 2659
Remove spring-data-jpa from polls/api; it is not directly used
Repository interfaces in polls/api extend Sakai's custom SpringCrudRepository abstraction, not Spring Data directly. The actual Spring Data JPA implementations reside in polls/impl. Since polls/api only exports repository interfaces and JPA-annotated entities, javax.persistence-api is sufficient; spring-data-jpa introduces unnecessary transitive dependencies downstream.
- <dependency>
- <groupId>org.springframework.data</groupId>
- <artifactId>spring-data-jpa</artifactId>
- </dependency>Verify that parent BOM (polls/pom.xml) defines version constraints for both dependencies and ensure alignment with project standards (currently javax.persistence-api uses javax, not jakarta).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <dependency> | |
| <groupId>javax.persistence</groupId> | |
| <artifactId>javax.persistence-api</artifactId> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.springframework.data</groupId> | |
| <artifactId>spring-data-jpa</artifactId> | |
| </dependency> | |
| <dependency> | |
| <groupId>javax.persistence</groupId> | |
| <artifactId>javax.persistence-api</artifactId> | |
| </dependency> |
🤖 Prompt for AI Agents
In polls/api/pom.xml around lines 37 to 44, remove the unnecessary
spring-data-jpa dependency because polls/api only exposes repository interfaces
that extend Sakai’s SpringCrudRepository and JPA entities (the actual Spring
Data JPA implementation lives in polls/impl); keep only the
javax.persistence-api dependency and ensure its artifactId and groupId remain
javax-based (not jakarta), then verify the parent BOM (polls/pom.xml) provides
the version for javax.persistence-api (and any other transitive constraints) and
align the dependency management with project standards so no explicit
spring-data-jpa entry remains in polls/api.
| /** | ||
| * Populate and persist a UUID for the supplied option when one is missing. | ||
| * | ||
| * @param option the option to update; may be {@code null} | ||
| * @return the same option instance after ensuring a UUID | ||
| */ | ||
| public Option ensureOptionUuidAndSave(Option option); |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify null handling and return behavior.
The method documentation states the option parameter "may be {@code null}", but doesn't specify what the method returns in that case. If null is passed, does it return null, throw an exception, or create a new Option? This ambiguity could lead to NPEs in calling code.
Additionally, the method name ensureOptionUuidAndSave mixes two responsibilities: ensuring UUID population and persisting. Consider whether this violates single responsibility principle or if callers might need these operations separately.
🏁 Script executed:
#!/bin/bash
# Check how this method is used in the codebase to understand expected null behavior
rg -n -C3 'ensureOptionUuidAndSave'Length of output: 3248
I need to see the complete implementation of ensureOptionUuidAndSave and check the PollUtil.optionToXml behavior to verify potential NPE issues at line 395-396.
[/run_scripts]
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get full implementation of ensureOptionUuidAndSave method
rg -n -A10 'public Option ensureOptionUuidAndSave' polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.javaLength of output: 465
🏁 Script executed:
#!/bin/bash
# Check PollUtil.optionToXml null handling
rg -n -C5 'optionToXml' polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.javaLength of output: 509
I need to check if PollUtil.optionToXml handles null and whether getVisibleOptionsForPoll can return null elements.
🏁 Script executed:
#!/bin/bash
# Find and check PollUtil.optionToXml implementation
rg -n -A15 'optionToXml' polls/ -g '**/*.java' --type java | head -40Length of output: 3240
Update Javadoc to document null return behavior and add defensive null check at call site (line 395-396).
The implementation at lines 294-295 returns null when the option parameter is null, but the Javadoc is incomplete: it documents the parameter as "may be {@code null}" but doesn't specify what the method returns in that case.
More critically, at line 395-396, the code reassigns the result without a null check, then immediately passes it to PollUtil.optionToXml() which calls option.getUuid() at line 55 of PollUtil.java without any null guard—causing an NPE if null is returned.
Fixes needed:
- Update Javadoc to state: "@return the same option instance after ensuring a UUID, or null if the input option is null"
- Add null check after line 395:
if (option != null) { option = ensureOptionUuidAndSave(option); }or check result before line 396
The SRP concern (mixing UUID generation and persistence) is valid but separate from this null safety issue.
🤖 Prompt for AI Agents
In polls/api/src/java/org/sakaiproject/poll/logic/PollListManager.java around
lines 104–110, the Javadoc for ensureOptionUuidAndSave does not document the
null return when the input is null; update the Javadoc to read: "@return the
same option instance after ensuring a UUID, or null if the input option is
null". Then, at the call site around lines 395–396, add a defensive null check
so you do not reassign and immediately use a possibly null result—either wrap
the call in if (option != null) { option = ensureOptionUuidAndSave(option); } or
check the method's return before passing it to PollUtil.optionToXml to avoid
NPE.
| Predicate sitePredicate = root.get("siteId").in(siteIds); | ||
| Predicate openPredicate = cb.lessThanOrEqualTo(root.get("voteOpen"), now); | ||
| Predicate closePredicate = cb.greaterThanOrEqualTo(root.get("voteClose"), now); | ||
|
|
||
| query.select(root) | ||
| .where(cb.and(sitePredicate, openPredicate, closePredicate)) | ||
| .orderBy(cb.asc(root.get("creationDate"))); |
There was a problem hiding this comment.
Open/close date predicates exclude polls with null bounds
Current AND of voteOpen <= now and voteClose >= now filters out polls with null voteOpen/voteClose (often meaning “no limit”). Use null-friendly predicates.
- Predicate sitePredicate = root.get("siteId").in(siteIds);
- Predicate openPredicate = cb.lessThanOrEqualTo(root.get("voteOpen"), now);
- Predicate closePredicate = cb.greaterThanOrEqualTo(root.get("voteClose"), now);
+ Predicate sitePredicate = root.get("siteId").in(siteIds);
+ Predicate openPredicate = cb.or(
+ cb.isNull(root.get("voteOpen")),
+ cb.lessThanOrEqualTo(root.get("voteOpen"), now)
+ );
+ Predicate closePredicate = cb.or(
+ cb.isNull(root.get("voteClose")),
+ cb.greaterThanOrEqualTo(root.get("voteClose"), now)
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Predicate sitePredicate = root.get("siteId").in(siteIds); | |
| Predicate openPredicate = cb.lessThanOrEqualTo(root.get("voteOpen"), now); | |
| Predicate closePredicate = cb.greaterThanOrEqualTo(root.get("voteClose"), now); | |
| query.select(root) | |
| .where(cb.and(sitePredicate, openPredicate, closePredicate)) | |
| .orderBy(cb.asc(root.get("creationDate"))); | |
| Predicate sitePredicate = root.get("siteId").in(siteIds); | |
| Predicate openPredicate = cb.or( | |
| cb.isNull(root.get("voteOpen")), | |
| cb.lessThanOrEqualTo(root.get("voteOpen"), now) | |
| ); | |
| Predicate closePredicate = cb.or( | |
| cb.isNull(root.get("voteClose")), | |
| cb.greaterThanOrEqualTo(root.get("voteClose"), now) | |
| ); | |
| query.select(root) | |
| .where(cb.and(sitePredicate, openPredicate, closePredicate)) | |
| .orderBy(cb.asc(root.get("creationDate"))); |
🤖 Prompt for AI Agents
In
polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java
around lines 65 to 71, the current predicates
cb.lessThanOrEqualTo(root.get("voteOpen"), now) and
cb.greaterThanOrEqualTo(root.get("voteClose"), now) exclude rows where voteOpen
or voteClose is null (which represent "no limit"); change each predicate to be
null-friendly by combining an isNull check with the comparison using cb.or —
i.e. replace openPredicate with cb.or(cb.isNull(root.get("voteOpen")),
cb.lessThanOrEqualTo(root.get("voteOpen"), now)) and replace closePredicate with
cb.or(cb.isNull(root.get("voteClose")),
cb.greaterThanOrEqualTo(root.get("voteClose"), now)) so polls with null bounds
are included.
| public void saveOptionsBatch(Long pollId, MultipartFile file) { | ||
| if (file == null || file.isEmpty()) { | ||
| throw new PollValidationException("error_batch_options"); | ||
| } | ||
|
|
||
| List<String> optionTexts = extractOptions(file); | ||
| if (optionTexts.isEmpty()) { | ||
| throw new PollValidationException("error_batch_options"); | ||
| } | ||
|
|
||
| int nextOrder = pollListManager.getOptionsForPoll(pollId).size(); | ||
| for (String optionText : optionTexts) { | ||
| Option option = new Option(); | ||
| option.setPollId(pollId); | ||
| option.setText(PollUtils.cleanupHtmlPtags(optionText)); | ||
| option.setOptionOrder(nextOrder++); | ||
| pollListManager.saveOption(option); | ||
| } | ||
| } | ||
|
|
||
| private List<String> extractOptions(MultipartFile file) { | ||
| try (InputStream inputStream = file.getInputStream()) { | ||
| return OptionsFileConverterUtil.convertInputStreamToOptionList(inputStream); | ||
| } catch (IOException e) { | ||
| log.warn("Unable to parse batch options file {}", file.getOriginalFilename(), e); | ||
| return List.of(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify batch option text sanitization.
Line 161 applies PollUtils.cleanupHtmlPtags but doesn't call externalLogic.processFormattedText like saveOption does (line 128). Confirm whether batch-imported options should skip formatted text processing or if this is an inconsistency.
🏁 Script executed:
#!/bin/bash
# Check if batch options intentionally skip processFormattedText
rg -nP 'processFormattedText.*Option' --type=java -C3Length of output: 0
I need to broaden my search strategy since the initial pattern didn't match. Let me search for the relevant methods and understand the sanitization logic.
:::
🏁 Script executed:
#!/bin/bash
# Search for processFormattedText calls more broadly
rg -n 'processFormattedText' --type=java -C2Length of output: 168617
🏁 Script executed:
#!/bin/bash
# Find the saveOption method definition to see current sanitization
rg -n 'public.*saveOption' --type=java -C5Length of output: 6070
🏁 Script executed:
#!/bin/bash
# Search for externalLogic to understand what it does
rg -n 'externalLogic\.' --type=java -C1 | head -50Length of output: 5284
I can now see the issue clearly. Let me search for more details on the sanitization utilities to understand if this is an intentional design decision or a bug.
:::
🏁 Script executed:
#!/bin/bash
# Search for PollUtils.cleanupHtmlPtags to understand its function
rg -n 'cleanupHtmlPtags' --type=java -B2 -A5Length of output: 3557
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for saveOptionsBatch
rg -n 'saveOptionsBatch' --type=java -C3Length of output: 1727
Add externalLogic.processFormattedText call to batch option sanitization (line 161).
The batch import method skips the XSS protection that single options receive. Single options sanitize with externalLogic.processFormattedText(option.getText(), new StringBuilder(), true, true) plus cleanupHtmlPtags, but batch options only call cleanupHtmlPtags. Since cleanupHtmlPtags only removes P tags and doesn't perform evil tag checking or whitespace normalization, batch-imported options have weaker security sanitization than individually-saved options. Apply the same processFormattedText call used in saveOption (line 128) to ensure consistent protection:
option.setText(PollUtils.cleanupHtmlPtags(externalLogic.processFormattedText(optionText, new StringBuilder(), true, true)));🤖 Prompt for AI Agents
In polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java
around lines 147 to 174, the batch import path only calls
PollUtils.cleanupHtmlPtags on optionText and therefore skips the XSS/formatting
sanitization applied to single-option saves; update the batch flow to call
externalLogic.processFormattedText(optionText, new StringBuilder(), true, true)
before cleanupHtmlPtags so that each Option text is sanitized the same way as in
saveOption (i.e., wrap the call to processFormattedText around the optionText
and then pass its result into PollUtils.cleanupHtmlPtags before setting
option.setText).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (1)
795-798: Fix dead code: missing return statement.Line 797 creates a
Voteobject but doesn't return it. This appears to be an incomplete early-return guard for null IDs. Either add the missingreturnstatement or remove this dead code, especially since line 817 already handles this case.Apply this diff:
// is this a new object? if (ref.getId() == null) { - new Vote(); + return new Vote(); }
♻️ Duplicate comments (5)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (5)
106-113: Critical: Add null check for existing poll to prevent NPE.If
getPollByIdreturns null (stale/invalid ID), line 109 will throw NPE when accessingexisting.getCreationDate().Apply this diff:
if (poll.getPollId() != null) { Poll existing = pollListManager.getPollById(poll.getPollId(), false); + if (existing == null) { + throw new PollValidationException("poll_not_found"); + } if (poll.getCreationDate() == null) { poll.setCreationDate(existing.getCreationDate()); } } else { poll.setCreationDate(new Date()); }
162-162: Critical: Apply XSS sanitization to batch-imported options.Line 162 only calls
cleanupHtmlPtags, skipping theprocessFormattedTextXSS/formatting protection thatsaveOptionapplies at line 129. This creates a security inconsistency where batch-imported options have weaker sanitization.Apply this diff:
-option.setText(PollUtils.cleanupHtmlPtags(optionText)); +option.setText(PollUtils.cleanupHtmlPtags(externalLogic.processFormattedText(optionText, new StringBuilder(), true, true)));
183-183: Critical: Add null check for parent poll.If
getPollByIdreturns null, subsequent operations (line 199 accessingpoll.getSiteId()andpoll.getText()) will throw NPE.Apply this diff:
Poll poll = pollListManager.getPollById(option.getPollId()); +if (poll == null) { + throw new PollValidationException("poll_not_found"); +} List<Vote> votes = pollVoteManager.getAllVotesForOption(option);
190-197: Use batch deletion for votes.Individual
deleteVotecalls are inefficient. Collect user IDs first, then delete all votes in one operation.Apply this diff:
pollListManager.deleteOption(option); for (Vote vote : votes) { if (vote.getUserId() != null) { String userEid = externalLogic.getUserEidFromId(vote.getUserId()); if (StringUtils.isNotBlank(userEid)) { userIds.add(userEid); } } - pollVoteManager.deleteVote(vote); } + pollVoteManager.deleteAll(votes); externalLogic.notifyDeletedOption(new ArrayList<>(userIds), externalLogic.getSiteTile(poll.getSiteId()), poll.getText());
177-209: Critical: Add permission checks before option deletion.The method performs deletions without verifying the user has permission. Add authorization checks (e.g.,
externalLogic.isAllowedInLocationforpoll.deleteAnyorpollListManager.userCanDeletePoll) before any delete operations.Apply this diff at the beginning of the method:
public Poll deleteOption(Long optionId, String orphanVoteHandling) { Option option = pollListManager.getOptionById(optionId); if (option == null) { throw new IllegalArgumentException("Option not found"); } Poll poll = pollListManager.getPollById(option.getPollId()); + if (poll == null) { + throw new PollValidationException("poll_not_found"); + } + if (!pollListManager.userCanDeletePoll(poll)) { + throw new SecurityException("User not permitted to delete option"); + } List<Vote> votes = pollVoteManager.getAllVotesForOption(option);
🧹 Nitpick comments (5)
polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (1)
575-611: Consider adding Instant support to copyParamsToObject.The helper method handles
Dateconversion (lines 586-597) but lacks support forInstantfields. While current vote creation explicitly setsvoteDatewithInstant.now()(line 765), adding Instant handling would make the method more robust for future Instant fields and align with the temporal type migration.Apply this diff to add Instant support:
if (types[0].equals(Date.class)) { Date dateValue = new Date( Long.valueOf(value.toString())); try { m.invoke(object, new Object[] { dateValue }); } catch (IllegalAccessException e) { log.debug(e.getMessage(), e); } catch (IllegalArgumentException e) { log.debug(e.getMessage(), e); } catch (InvocationTargetException e) { log.debug(e.getMessage(), e); } + } else if (types[0].equals(Instant.class)) { + Instant instantValue = Instant.ofEpochMilli( + Long.valueOf(value.toString())); + try { + m.invoke(object, new Object[] { instantValue }); + } catch (IllegalAccessException e) { + log.debug(e.getMessage(), e); + } catch (IllegalArgumentException e) { + log.debug(e.getMessage(), e); + } catch (InvocationTargetException e) { + log.debug(e.getMessage(), e); + } } else { // use generic converter from BeanUtilspolls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (4)
39-41: Rename parameter for clarity: pollOption → optionId.The method filters by an Option PK; name it optionId to avoid confusion with an Option entity instance.
If the interface method is public API, confirm ripple effects before renaming.
62-66: Treat blank userId as empty input.Guard against "" or whitespace to avoid full table scans when callers pass blanks.
- if (userId == null) { + if (userId == null || userId.trim().isEmpty()) { return Collections.emptyList(); }
135-140: Safe narrowing and JPA-standard retrieval for distinct count.Use getSingleResult() and Math.toIntExact to avoid silent overflow.
- Long count = sessionFactory.getCurrentSession() - .createQuery(query) - .uniqueResult(); - - return count == null ? 0 : count.intValue(); + Long count = sessionFactory.getCurrentSession() + .createQuery(query) + .getSingleResult(); + return Math.toIntExact(count);
112-120: Use getSingleResult() for JPA portability; COUNT queries always return a value.JPA COUNT queries consistently return a numeric value (0 when no rows match), never null. Switching from Hibernate's uniqueResult() to the JPA standard getSingleResult() eliminates the redundant null check and aligns with PollRepositoryImpl.
- Long count = sessionFactory.getCurrentSession() - .createQuery(query) - .uniqueResult(); - - return count != null && count > 0; + Long count = sessionFactory.getCurrentSession() + .createQuery(query) + .getSingleResult(); + return count > 0;Note: The same pattern appears at line 137; consider applying the same fix there for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java(1 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java(9 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java(14 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html
- polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.630Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
🧬 Code graph analysis (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-34)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
Repository(17-94)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)
⏰ 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 (18)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
17-30: LGTM! Clean form model implementation.The VoteForm follows standard Spring MVC form model patterns. The initialization of
selectedOptionIdsto an emptyArrayListis good defensive programming that prevents potential NPE issues downstream.polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
109-111: Defensive null check is good practice.While
VoteForminitializesselectedOptionIdsto an emptyArrayList(VoteForm.java:29), this null-safe check protects against manual instantiation or future changes where the initialization might be removed. This is good defensive programming.polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (7)
60-73: LGTM - proper null handling in bulk delete.The null check and warning log for missing polls prevents NPEs, and SecurityException handling ensures graceful degradation when permissions are insufficient.
75-87: LGTM - proper permission and null checks.The implementation correctly validates poll existence, checks deletion permissions, and uses batch deletion for efficiency.
128-146: LGTM - proper sanitization and validation.The method correctly applies
processFormattedTextbeforecleanupHtmlPtags, validates non-empty text, handles both new and existing options, and includes null checks.
168-175: LGTM - proper resource handling.Try-with-resources ensures the input stream is closed, and exceptions are logged with context before returning an empty list.
211-261: LGTM - comprehensive vote validation and submission.The method properly validates votability, enforces voting limits, checks option membership and uniqueness (excluding the 0L sentinel), and persists votes correctly.
263-278: LGTM - clear validation logic.The method correctly enforces min/max option constraints with descriptive error messages.
280-295: LGTM - proper date/time conversions.Both utility methods correctly handle time zone conversions with appropriate null checks.
polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (6)
124-124: LGTM: Logging improved with parameterized format.The logging statements now properly use SLF4J parameterized logging with placeholders, which improves performance and follows best practices.
Also applies to: 274-274, 314-314, 718-718, 788-788
358-360: LGTM: UUID initialization for polls.Proper null-check before generating a UUID for the poll's public identity, aligning with the JPA migration approach described in the PR.
417-419: LGTM: UUID properly excluded from updates.The
copyBeancalls correctly excludeuuidfrom being overwritten during entity updates, preserving the immutable public identifiers.Also applies to: 636-638
460-466: LGTM: More specific exception handling.Catching
NumberFormatExceptionforLong.parseLong()is more precise than the previousUnsupportedOperationException, improving error handling clarity.
564-564: LGTM: Return value updated to use getOptionId().The change from
getId()togetOptionId()aligns with the entity model refactoring for the JPA migration.
26-28: Mixed Date/Instant temporal types represent an acceptable gradual migration pattern.Verification confirms:
- Vote uses Instant for voteDate and Poll uses Date for creationDate — this follows standard JPA 2.2+ support where both types are valid and properly mapped
- voteDate is intentionally server-side only (set via
Instant.now()at lines 765, 978) and never passed via REST parameters- The
copyParamsToObjectmethod lacks Instant conversion logic but doesn't cause issues since voteDate is not exposed as a client-settable parameterThe mixed temporal types reflect an intentional partial migration strategy. This pattern is consistent with industry best practices for gradual Java 8+ adoption in legacy codebases. No blocking issues identified, though consistency could be improved if future temporal fields are added.
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (3)
5-9: Javax/Jakarta alignment confirmed – no issues detected.Verification across the entire codebase confirms consistent and correct usage of
javax.persistenceimports throughout. All 500+ Java files usejavax.persistence.*, and the pom.xml declaresjavax.persistence-api v2.2. This alignment is correct for the project's current Hibernate 5.6.15.Final and Spring 5.3.39 stack. No mixing detected; no action required.
29-33: The review comment is based on incorrect assumptions about entity attribute naming and should be dismissed.Poll's primary key attribute is
pollId(notid), and Option's primary key attribute isoptionId(notid). The code correctly accesses these PK attributes by their actual names in JPA criteria queries:
pollJoin.get("pollId")– correct; Poll's @id field is namedpollIdoptionJoin.get("optionId")– correct; Option's @id field is namedoptionIdChanging these to
get("id")as suggested would causeIllegalArgumentExceptionat runtime because those attributes do not exist. The current code is correct and requires no changes.Likely an incorrect or invalid review comment.
21-37: Unbounded vote queries lack pagination and database indexes—address scalability concerns.The repository contains 7 methods returning unbounded lists across VoteRepositoryImpl, PollRepositoryImpl, and OptionRepositoryImpl. No pagination support or database indexes exist. The Vote entity lacks @Index annotations, and
getAllVotesForPoll()is called from 9+ locations (includingdeleteAll(votes)in PollsUiService and vote aggregation in ResultsController), potentially loading thousands of records into memory on busy sites.Consider:
- Adding paged variants (
findByPollIdPaged, etc.) or result limits where feasible- Defining database indexes: Vote(poll_id), Vote(poll_id, user_id), Vote(user_id), Vote(submission_id); Option(poll_id, option_order)
- Refactoring batch delete operations to use limit-based pagination
| Poll poll; | ||
| try { | ||
| poll = pollListManager.getPollById(pollId); | ||
| } catch (SecurityException e) { | ||
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale)); | ||
| return "redirect:/faces/votePolls"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add logging for consistency and debuggability.
The SecurityException handler in showVote (line 64) doesn't log the exception, while the identical handler in submitVote (line 100) logs it properly. For consistency, debugging, and audit purposes, both security-related exceptions should be logged.
Apply this diff:
Poll poll;
try {
poll = pollListManager.getPollById(pollId);
} catch (SecurityException e) {
+ log.debug("User lacks permission to view poll {}", pollId, e);
redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale));
return "redirect:/faces/votePolls";
}As per coding guidelines
🤖 Prompt for AI Agents
In polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java around
lines 61 to 67, the SecurityException caught in showVote is not being logged
while the identical handler in submitVote logs the exception; add the same
logging call used in submitVote (e.g., logger.warn("No permission to view poll "
+ pollId, e) or the class's chosen logging level and message) immediately inside
the catch before adding the flash attribute and redirect so the exception
details are recorded for consistency and debugging.
| public Object[] getArgs() { | ||
| return args; | ||
| } |
There was a problem hiding this comment.
Return a defensive copy of the args array.
Returning the internal args array directly allows callers to modify the exception's state after construction, violating immutability.
Apply this diff:
public Object[] getArgs() {
- return args;
+ return args == null ? null : args.clone();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Object[] getArgs() { | |
| return args; | |
| } | |
| public Object[] getArgs() { | |
| return args == null ? null : args.clone(); | |
| } |
🤖 Prompt for AI Agents
In
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java
around lines 34 to 36, the getArgs() method currently returns the internal args
array directly which allows external mutation; change getArgs() to return a
defensive copy (e.g., return args == null ? null : args.clone()) so callers
cannot modify the exception's internal state; also ensure any constructor that
accepts an args array copies its input into the internal field to avoid
retaining external references.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (2)
165-165: Security: Apply processFormattedText to batch options.Line 165 only calls
cleanupHtmlPtags(optionText), skipping the XSS sanitization thatsaveOptionapplies (line 132). Batch-imported options are therefore less protected against malicious HTML than individually saved options. Apply the same sanitization chain used insaveOption:- option.setText(PollUtils.cleanupHtmlPtags(optionText)); + option.setText(PollUtils.cleanupHtmlPtags(externalLogic.processFormattedText(optionText, new StringBuilder(), true, true)));
180-215: Security: Add authorization check before option deletion.The method deletes options and votes without verifying the current user has permission. Before any deletion (line 195, 203, 208, 211), check authorization via
pollListManager.userCanDeletePoll(poll)orexternalLogic.isAllowedInLocation(poll.getSiteId(), PollListManager.PERMISSION_ADD). Unauthorized users should not be able to delete options.Refactor: Use batch delete for votes.
Lines 196-204 loop through votes calling
deleteVoteindividually. Replace with a singlepollVoteManager.deleteAll(votes)call after collecting user IDs:for (Vote vote : votes) { if (vote.getUserId() != null) { String userEid = externalLogic.getUserEidFromId(vote.getUserId()); if (StringUtils.isNotBlank(userEid)) { userIds.add(userEid); } } - pollVoteManager.deleteVote(vote); } + pollVoteManager.deleteAll(votes);
🧹 Nitpick comments (4)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (4)
40-59: Consider querying foreign key fields directly if exposed on Vote.Similar to
findByPollId, this method joins to both Poll and Option entities to access their IDs. If Vote exposespollIdandoptionIdas fields, direct queries would be more efficient:- Join<Vote, Poll> pollJoin = root.join("poll"); - Join<Vote, Option> optionJoin = root.join("option"); - - Predicate pollPredicate = cb.equal(pollJoin.get("pollId"), pollId); - Predicate optionPredicate = cb.equal(optionJoin.get("optionId"), optionId); + Predicate pollPredicate = cb.equal(root.get("pollId"), pollId); + Predicate optionPredicate = cb.equal(root.get("optionId"), optionId);This eliminates two INNER JOINs from the SQL.
79-97: Consider direct field access for the IN clause.The join to Poll could be eliminated if Vote has a
pollIdfield:- Join<Vote, Poll> pollJoin = root.join("poll"); - Predicate userPredicate = cb.equal(root.get("userId"), userId); - Predicate pollsPredicate = pollJoin.get("pollId").in(pollIds); + Predicate pollsPredicate = root.get("pollId").in(pollIds);This would make the query more efficient by avoiding the join.
100-120: Consider direct field access to avoid join.The join to Poll can be eliminated if Vote has a
pollIdfield:- Join<Vote, Poll> pollJoin = root.join("poll"); - - Predicate pollPredicate = cb.equal(pollJoin.get("pollId"), pollId); + Predicate pollPredicate = cb.equal(root.get("pollId"), pollId); Predicate userPredicate = cb.equal(root.get("userId"), userId);The count-based existence check is fine for typical datasets.
123-140: Consider direct field access to avoid join.The join to Poll can be eliminated if Vote has a
pollIdfield:- Join<Vote, Poll> pollJoin = root.join("poll"); - query.select(cb.countDistinct(root.get("submissionId"))) - .where(cb.equal(pollJoin.get("pollId"), pollId)); + .where(cb.equal(root.get("pollId"), pollId));Note:
Math.toIntExactat line 139 will throwArithmeticExceptionif the count exceedsInteger.MAX_VALUE. This is acceptable for typical poll scenarios, but be aware of the limitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java(15 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.javapolls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.javapolls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Learnt from: CR
PR: sakaiproject/sakai#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:59.630Z
Learning: Prefer Spring MVC/Boot with Hibernate and Thymeleaf for new tools; avoid RSF for new development
🧬 Code graph analysis (1)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
Repository(17-94)polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-34)
⏰ 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
🔇 Additional comments (13)
polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java (5)
124-124: LGTM! Parameterized logging follows coding guidelines.The changes correctly use SLF4J parameterized logging with
{}placeholders instead of string concatenation.Also applies to: 274-274, 314-314, 718-718, 788-788
358-360: LGTM! UUID initialization is correct.Defensive check ensures new polls always have a UUID, aligning with the PR's migration to UUID-based identities.
417-419: LGTM! UUID properly excluded from updates.Adding
"uuid"to the exclusion list prevents accidental overwrites during poll updates, which is the correct behavior for immutable identifiers.
462-466: LGTM! Explicit exception handling improves clarity.Catching
NumberFormatExceptionspecifically provides better error context than a generic catch block.
765-765: Vote model correctly uses Instant for voteDate—no changes needed.Verification confirms the Vote entity's
voteDatefield (polls/api/src/java/org/sakaiproject/poll/model/Vote.java, line 73) is properly declared asInstantwith appropriate JPA annotations. All call sites consistently usevote.setVoteDate(Instant.now()), which matches the field type. The migration fromDatetoInstantis complete and compatible.polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (6)
60-73: LGTM! Null checks and error handling are correct.The method properly guards against missing polls (lines 63-66) and handles authorization failures (lines 69-70) without propagating errors that would abort the entire batch operation.
Based on learnings
75-87: LGTM! Null and permission checks are correct.The method guards against missing polls (lines 78-81) and verifies delete permissions before resetting votes (line 82).
Based on learnings
89-129: LGTM! Validation and null checks are comprehensive.The method validates all required fields and constraints, includes the null check for existing polls (lines 108-110), and properly sanitizes HTML content (line 118).
Based on learnings
131-149: LGTM! Option sanitization and validation are correct.The method properly sanitizes HTML (line 132) using both
processFormattedTextandcleanupHtmlPtags, validates content, and maintains option ordering.
217-267: LGTM! Vote validation is comprehensive.The method validates votability (line 220), prevents duplicate voting (line 224), ensures option uniqueness (lines 238-242), verifies option membership (lines 243-249), and enforces min/max selection constraints. The validation logic correctly handles the sentinel value (0L) for polls with minOptions=0.
Based on learnings
269-301: LGTM! Helper methods handle date conversions correctly.The validation and date conversion helpers properly handle timezone-aware conversions between
DateandLocalDateTime, and validate vote selection constraints with appropriate error messages.polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java (1)
1-21: LGTM! Clean repository interface design.The interface is well-structured with clear method signatures covering the necessary query operations for Vote entities. The separation of concerns between find, exists, and count operations follows good repository design patterns.
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
62-76: LGTM! Clean and efficient implementation.The null and empty-string checks are appropriate, and the direct field query is efficient without unnecessary joins.
| public List<Vote> findByPollId(Long pollId) { | ||
| if (pollId == null) { | ||
| return Collections.emptyList(); | ||
| } | ||
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | ||
| CriteriaQuery<Vote> query = cb.createQuery(Vote.class); | ||
| Root<Vote> root = query.from(Vote.class); | ||
| Join<Vote, Poll> pollJoin = root.join("poll"); | ||
|
|
||
| query.select(root) | ||
| .where(cb.equal(pollJoin.get("pollId"), pollId)); | ||
|
|
||
| return sessionFactory.getCurrentSession() | ||
| .createQuery(query) | ||
| .getResultList(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider querying pollId directly if available as a field.
The implementation joins to the Poll entity to access its pollId. If the Vote entity has a pollId field (common with @JoinColumn on the @ManyToOne relationship), you could query it directly without the join:
- Join<Vote, Poll> pollJoin = root.join("poll");
-
query.select(root)
- .where(cb.equal(pollJoin.get("pollId"), pollId));
+ .where(cb.equal(root.get("pollId"), pollId));This would eliminate an unnecessary INNER JOIN in the generated SQL and improve query performance. Note that OptionRepositoryImpl (lines 13-33 in snippet) follows this pattern by querying pollId directly.
Verify whether Vote entity has a pollId field:
🏁 Script executed:
#!/bin/bash
# Check if Vote entity has a direct pollId field
ast-grep --pattern 'class Vote {
$$$
pollId
$$$
}'
# Also check for the @JoinColumn annotation details
rg -nP --type=java -A5 -B5 '@ManyToOne.*Poll|@JoinColumn.*poll' polls/api/src/java/org/sakaiproject/poll/model/Vote.javaLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find Vote entity file
echo "=== Finding Vote entity ==="
fd -e java "Vote\.java" polls/
echo -e "\n=== Finding Poll entity ==="
fd -e java "Poll\.java" polls/
echo -e "\n=== Finding OptionRepositoryImpl ==="
fd -e java "OptionRepositoryImpl\.java" polls/Length of output: 292
🏁 Script executed:
#!/bin/bash
# Read Vote entity if found
if [ -f "polls/api/src/java/org/sakaiproject/poll/model/Vote.java" ]; then
echo "=== Vote Entity Contents ==="
cat polls/api/src/java/org/sakaiproject/poll/model/Vote.java
fiLength of output: 4757
🏁 Script executed:
#!/bin/bash
# Read Poll entity if found
if [ -f "polls/api/src/java/org/sakaiproject/poll/model/Poll.java" ]; then
echo "=== Poll Entity Contents ==="
cat polls/api/src/java/org/sakaiproject/poll/model/Poll.java
fiLength of output: 10577
🏁 Script executed:
#!/bin/bash
# Read OptionRepositoryImpl to verify the pattern mentioned in review
fd -e java "OptionRepositoryImpl\.java" polls/ -x cat {}Length of output: 1221
🏁 Script executed:
#!/bin/bash
# Search for pollId field across the model directory
echo "=== Searching for pollId in model classes ==="
rg -n "pollId" polls/api/src/java/org/sakaiproject/poll/model/Length of output: 1470
Remove unnecessary join and query pollId directly for better performance.
The Vote entity has a direct pollId field (marked as @Column(name = "VOTE_POLL_ID")), and the Poll relationship is read-only (with insertable = false, updatable = false). The current join creates an unnecessary INNER JOIN in the generated SQL. Replace it:
- Join<Vote, Poll> pollJoin = root.join("poll");
-
query.select(root)
- .where(cb.equal(pollJoin.get("pollId"), pollId));
+ .where(cb.equal(root.get("pollId"), pollId));This aligns with the pattern used in OptionRepositoryImpl (lines 21–22), where pollId is queried directly without a join.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<Vote> findByPollId(Long pollId) { | |
| if (pollId == null) { | |
| return Collections.emptyList(); | |
| } | |
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | |
| CriteriaQuery<Vote> query = cb.createQuery(Vote.class); | |
| Root<Vote> root = query.from(Vote.class); | |
| Join<Vote, Poll> pollJoin = root.join("poll"); | |
| query.select(root) | |
| .where(cb.equal(pollJoin.get("pollId"), pollId)); | |
| return sessionFactory.getCurrentSession() | |
| .createQuery(query) | |
| .getResultList(); | |
| } | |
| public List<Vote> findByPollId(Long pollId) { | |
| if (pollId == null) { | |
| return Collections.emptyList(); | |
| } | |
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | |
| CriteriaQuery<Vote> query = cb.createQuery(Vote.class); | |
| Root<Vote> root = query.from(Vote.class); | |
| query.select(root) | |
| .where(cb.equal(root.get("pollId"), pollId)); | |
| return sessionFactory.getCurrentSession() | |
| .createQuery(query) | |
| .getResultList(); | |
| } |
🤖 Prompt for AI Agents
In
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java
around lines 22 to 37, remove the unnecessary join to Poll and query the
Vote.pollId column directly to avoid generating an INNER JOIN; keep the null
check, build the CriteriaQuery<Vote> and use cb.equal(root.get("pollId"),
pollId) in the where clause, then execute the query and return the result list
as before.
ern
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (7)
polls/api/src/java/org/sakaiproject/poll/logic/PollListManager.java (1)
104-110: Past review comment remains valid.The null handling and Javadoc concerns for this method were already flagged in the previous review. The method documentation should specify return behavior when null is passed, and defensive null checks should be added at call sites to prevent NPEs.
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
90-92: Constructor concern already flagged in past review.The past review correctly identified that the one-arg constructor leaves the non-nullable
pollIdfield uninitialized. While this is a valid design (pollId can be set via setter before persisting), adding a two-arg constructor or documenting the requirement would improve clarity.polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
22-37: Remove unnecessary join to Poll entity for better performance.The Vote entity has a direct
pollIdfield, yet this method joins to the Poll entity just to accesspollId. This creates an unnecessary INNER JOIN in the generated SQL. QuerypollIddirectly from the Vote entity instead:- Join<Vote, Poll> pollJoin = root.join("poll"); - query.select(root) - .where(cb.equal(pollJoin.get("pollId"), pollId)); + .where(cb.equal(root.get("pollId"), pollId));This aligns with the pattern used in
OptionRepositoryImpl(line 21 in relevant snippets) and improves query performance.polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
65-71: Handle null open/close dates to include polls without time restrictions.The current predicates exclude polls where
voteOpenorvoteCloseis null. Null dates typically represent "no limit," meaning the poll is always open or never closes. Make the predicates null-friendly:Predicate sitePredicate = root.get("siteId").in(siteIds); - Predicate openPredicate = cb.lessThanOrEqualTo(root.get("voteOpen"), now); - Predicate closePredicate = cb.greaterThanOrEqualTo(root.get("voteClose"), now); + Predicate openPredicate = cb.or( + cb.isNull(root.get("voteOpen")), + cb.lessThanOrEqualTo(root.get("voteOpen"), now) + ); + Predicate closePredicate = cb.or( + cb.isNull(root.get("voteClose")), + cb.greaterThanOrEqualTo(root.get("voteClose"), now) + );This ensures polls without date bounds are included in the results.
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
34-36: Return defensive copy to prevent external mutation.The
getArgs()method returns the internalargsarray directly, allowing callers to modify the exception's state after construction.Apply this diff:
public Object[] getArgs() { - return args; + return args == null ? null : args.clone(); }As noted in past review comments.
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (1)
165-165: Apply XSS sanitization to batch-imported options.Batch options (line 165) skip
externalLogic.processFormattedText(...), which performs XSS protection, while single options (line 132) apply bothprocessFormattedTextandcleanupHtmlPtags. This inconsistency leaves batch-imported options vulnerable to XSS injection.Apply this diff:
- option.setText(PollUtils.cleanupHtmlPtags(optionText)); + option.setText(PollUtils.cleanupHtmlPtags(externalLogic.processFormattedText(optionText, new StringBuilder(), true, true)));Based on past review comment flagging this same issue.
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
655-657: Side effect in getPoll query method.The method mutates and saves the poll when its UUID is null (lines 655-657), violating command-query separation. This is identical to the issue flagged in previous reviews for
getOptionByIdand the originalgetPollmethod.If this is temporary migration logic for legacy data, document it clearly with a comment explaining the one-time nature. Otherwise, extract this UUID initialization into a separate
ensurePollUuid(Poll)method invoked explicitly by command methods or migration paths.Apply this diff to separate the side effect:
Poll poll = pollRepository.findByUuid(uuid).orElse(null); - if (poll != null && poll.getUuid() == null) { - poll.setUuid(idManager.createUuid()); - savePoll(poll); - } return poll;Add a dedicated method for UUID initialization:
public Poll ensurePollUuid(Poll poll) { if (poll != null && StringUtils.isBlank(poll.getUuid())) { String generatedUuid = idManager.createUuid(); if (StringUtils.isBlank(generatedUuid)) { generatedUuid = UUID.randomUUID().toString(); } poll.setUuid(generatedUuid); savePoll(poll); } return poll; }Call this method explicitly where UUID initialization is needed (e.g., in migration or archive flows).
🧹 Nitpick comments (5)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.java (1)
42-48: Refactor duplicated permission helpers to reduce maintenance burden.The
isAllowedPollAdd()andisSiteOwner()methods are duplicated across at least four controllers: PermissionsController, OptionController, PollController, and VoteController (see relevant snippets). Consider extracting these to a shared base controller class or a dedicated permission helper service to eliminate duplication and ensure consistency.Example refactor using a base class:
public abstract class BasePermissionController { protected final ExternalLogic externalLogic; protected BasePermissionController(ExternalLogic externalLogic) { this.externalLogic = externalLogic; } protected boolean isAllowedPollAdd() { return externalLogic.isUserAdmin() || externalLogic.isAllowedInLocation( PollListManager.PERMISSION_ADD, externalLogic.getCurrentLocationReference()); } protected boolean isSiteOwner() { return externalLogic.isUserAdmin() || externalLogic.isAllowedInLocation( "site.upd", externalLogic.getCurrentLocationReference()); } }Then each controller extends this base class.
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java (1)
35-92: Consider constructor-based dependency injection for entity providers.The current implementation uses setter-based injection, which is verbose. If the entity provider classes support constructor injection, consider refactoring to a more concise pattern:
Example for
PollsEntityProvider:@Bean public PollsEntityProvider pollsEntityProvider( PollListManager pollListManager, PollVoteManager pollVoteManager, UsageSessionService usageSessionService, UserDirectoryService userDirectoryService, EntityProviderManager entityProviderManager, DeveloperHelperService developerHelperService) { return new PollsEntityProvider( pollListManager, pollVoteManager, usageSessionService, userDirectoryService, entityProviderManager, developerHelperService ); }This requires adding constructors to the provider classes but reduces boilerplate and makes dependencies more explicit.
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (1)
116-118: Use explicit locale in String.format for consistency.Line 116 calls
String.formatwithout specifying a locale, which defaults to the JVM's locale rather than the user'slocaleparameter. This can cause inconsistent number formatting (e.g., comma vs. period as decimal separator) compared to other localized elements on the page.Apply this diff:
- String pollSizeDetails = String.format("%d / %d (%.02f %%)", distinctVoters, potentialVoters, voterPercent.setScale(2, RoundingMode.HALF_UP)); + String pollSizeDetails = String.format(locale, "%d / %d (%.02f %%)", distinctVoters, potentialVoters, voterPercent.setScale(2, RoundingMode.HALF_UP));polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java (1)
196-204: Batch-delete votes for better performance.Lines 196-204 call
deleteVote(vote)in a loop, which issues individual database deletes. Replace with a singlepollVoteManager.deleteAll(votes)call after collecting user IDs.Apply this diff:
Set<String> userIds = new TreeSet<>(); pollListManager.deleteOption(option); for (Vote vote : votes) { if (vote.getUserId() != null) { String userEid = externalLogic.getUserEidFromId(vote.getUserId()); if (StringUtils.isNotBlank(userEid)) { userIds.add(userEid); } } - pollVoteManager.deleteVote(vote); } + pollVoteManager.deleteAll(votes); externalLogic.notifyDeletedOption(new ArrayList<>(userIds), externalLogic.getSiteTile(poll.getSiteId()), poll.getText());As noted in past review comments.
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
62-92: Consider adding permission checks to GET handlers.The
editOptionGET handler allows any authenticated user to view the option edit form. While the POST handler at line 101 enforces permissions, showing the edit interface without checkingisAllowedPollAdd()could disclose poll structure to unauthorized users.Consider adding a permission check:
public String editOption(@RequestParam(value = "optionId", required = false) Long optionId, @RequestParam(value = "pollId", required = false) Long pollId, Model model, Locale locale) { + if (!isAllowedPollAdd()) { + return "redirect:/votePolls"; + } OptionForm form = new OptionForm();Similar consideration applies to
deleteOptionGET handler at lines 173-195.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (92)
kernel/api/src/main/java/org/sakaiproject/springframework/data/SpringCrudRepositoryImpl.java(0 hunks)polls/api/pom.xml(1 hunks)polls/api/src/bundle/org/sakaiproject/poll/bundle/Messages.properties(2 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Option.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Poll.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/hbm/Vote.hbm.xml(0 hunks)polls/api/src/java/org/sakaiproject/poll/logic/PollListManager.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/logic/PollVoteManager.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Option.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/model/Poll.java(7 hunks)polls/api/src/java/org/sakaiproject/poll/model/Vote.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/PollRepository.java(1 hunks)polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java(1 hunks)polls/impl/pom.xml(2 hunks)polls/impl/src/bundle/org/sakaiproject/poll/spring-hibernate.xml(0 hunks)polls/impl/src/java/org/sakaiproject/poll/dao/impl/PollDoaImpl.java(0 hunks)polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java(4 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java(1 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java(19 hunks)polls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.java(6 hunks)polls/impl/src/test/hibernate-test.xml(2 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java(2 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java(9 hunks)polls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.java(2 hunks)polls/impl/src/webapp/WEB-INF/components.xml(1 hunks)polls/impl/src/webapp/WEB-INF/hibernate-hbms.xml(0 hunks)polls/tool/pom.xml(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/beans/ErrorHandler.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/constants/NavigationConstants.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.java(2 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollVoteEntityProvider.java(3 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java(15 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/evolvers/SakaiFCKTextEvolver.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollActionResultInterceptor.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollBeanLocator.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionBatchViewParameters.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionViewParameters.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/PermissionAction.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/params/PollToolBean.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/AddPollProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/ConfirmProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PermissionsProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionBatchProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionDeleteProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollToolProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollVoteProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/producers/ResultsProducer.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/validators/OptionValidator.java(0 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/validators/VoteValidator.java(0 hunks)polls/tool/src/webapp/WEB-INF/applicationContext.xml(0 hunks)polls/tool/src/webapp/WEB-INF/requestContext.xml(0 hunks)polls/tool/src/webapp/WEB-INF/templates/fragments/menus.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/edit.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/list.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/option-edit.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/permissions.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/results.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/thanks.html(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/vote.html(1 hunks)polls/tool/src/webapp/WEB-INF/web.xml(1 hunks)polls/tool/src/webapp/component-templates/Messages.html(0 hunks)polls/tool/src/webapp/js/polls.js(0 hunks)polls/tool/src/webapp/templates/pollOption.html(0 hunks)polls/tool/src/webapp/templates/pollOptionBatch.html(0 hunks)polls/tool/src/webapp/templates/pollOptionDelete.html(0 hunks)polls/tool/src/webapp/templates/voteAdd.html(0 hunks)polls/tool/src/webapp/templates/votePermissions.html(0 hunks)polls/tool/src/webapp/templates/votePolls.html(0 hunks)polls/tool/src/webapp/templates/voteQuestion.html(0 hunks)polls/tool/src/webapp/templates/voteResults.html(0 hunks)polls/tool/src/webapp/templates/voteThanks.html(0 hunks)
💤 Files with no reviewable changes (40)
- polls/tool/src/java/org/sakaiproject/poll/tool/constants/NavigationConstants.java
- polls/api/src/java/org/sakaiproject/poll/hbm/Vote.hbm.xml
- polls/impl/src/webapp/WEB-INF/hibernate-hbms.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionViewParameters.java
- polls/tool/src/webapp/templates/pollOptionBatch.html
- polls/tool/src/java/org/sakaiproject/poll/tool/params/PermissionAction.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionDeleteProducer.java
- polls/impl/src/bundle/org/sakaiproject/poll/spring-hibernate.xml
- polls/tool/src/webapp/templates/voteThanks.html
- polls/tool/src/webapp/templates/voteResults.html
- polls/tool/src/webapp/templates/votePermissions.html
- polls/tool/src/webapp/templates/pollOption.html
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/ResultsProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollToolProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/ConfirmProducer.java
- polls/tool/src/webapp/WEB-INF/requestContext.xml
- polls/api/src/java/org/sakaiproject/poll/hbm/Poll.hbm.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollVoteProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/params/OptionBatchViewParameters.java
- polls/tool/src/webapp/templates/voteQuestion.html
- polls/tool/src/webapp/templates/votePolls.html
- polls/tool/src/webapp/js/polls.js
- polls/tool/src/java/org/sakaiproject/poll/tool/params/PollToolBean.java
- kernel/api/src/main/java/org/sakaiproject/springframework/data/SpringCrudRepositoryImpl.java
- polls/tool/src/java/org/sakaiproject/poll/tool/beans/ErrorHandler.java
- polls/tool/src/webapp/component-templates/Messages.html
- polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollActionResultInterceptor.java
- polls/tool/src/java/org/sakaiproject/poll/tool/validators/VoteValidator.java
- polls/tool/src/webapp/templates/voteAdd.html
- polls/tool/src/webapp/WEB-INF/applicationContext.xml
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PermissionsProducer.java
- polls/impl/src/java/org/sakaiproject/poll/dao/impl/PollDoaImpl.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/evolvers/SakaiFCKTextEvolver.java
- polls/tool/src/webapp/templates/pollOptionDelete.html
- polls/tool/src/java/org/sakaiproject/poll/tool/locators/PollBeanLocator.java
- polls/tool/src/java/org/sakaiproject/poll/tool/validators/OptionValidator.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/AddPollProducer.java
- polls/tool/src/java/org/sakaiproject/poll/tool/producers/PollOptionBatchProducer.java
- polls/api/src/java/org/sakaiproject/poll/hbm/Option.hbm.xml
🚧 Files skipped from review as they are similar to previous changes (18)
- polls/impl/src/test/hibernate-test.xml
- polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java
- polls/api/pom.xml
- polls/api/src/java/org/sakaiproject/poll/repository/PollRepository.java
- polls/tool/src/webapp/WEB-INF/templates/polls/option-edit.html
- polls/api/src/java/org/sakaiproject/poll/repository/OptionRepository.java
- polls/tool/src/webapp/WEB-INF/templates/polls/option-batch.html
- polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollVoteEntityProvider.java
- polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollEntityProvider.java
- polls/tool/src/webapp/WEB-INF/templates/polls/edit.html
- polls/tool/src/webapp/WEB-INF/templates/polls/results.html
- polls/api/src/bundle/org/sakaiproject/poll/bundle/Messages.properties
- polls/impl/src/test/org/sakaiproject/poll/logic/test/TestDataPreload.java
- polls/tool/src/webapp/WEB-INF/templates/polls/thanks.html
- polls/api/src/java/org/sakaiproject/poll/repository/VoteRepository.java
- polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java
- polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollsEntityProvider.java
- polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.
Files:
polls/api/src/java/org/sakaiproject/poll/logic/PollListManager.javapolls/api/src/java/org/sakaiproject/poll/logic/PollVoteManager.javapolls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.javapolls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.javapolls/api/src/java/org/sakaiproject/poll/model/Poll.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollsUiService.javapolls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.javapolls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.javapolls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.javapolls/api/src/java/org/sakaiproject/poll/model/Vote.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.javapolls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.javapolls/api/src/java/org/sakaiproject/poll/model/Option.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.javapolls/impl/src/java/org/sakaiproject/poll/service/impl/PollVoteManagerImpl.javapolls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerImplTest.java
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
Applied to files:
polls/tool/pom.xml
📚 Learning: 2025-10-07T16:35:33.991Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/pom.xml:37-42
Timestamp: 2025-10-07T16:35:33.991Z
Learning: The Sakai project cannot upgrade to Spring Framework 6 yet and must remain on Spring 5.x for the time being.
Applied to files:
polls/tool/pom.xml
📚 Learning: 2025-10-06T22:15:40.541Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-api/src/main/java/org/sakaiproject/accountvalidator/model/ValidationAccount.java:24-33
Timestamp: 2025-10-06T22:15:40.541Z
Learning: The Sakai project (as of this PR) uses Hibernate 5.6.15.Final and Spring 5.3.39, which requires javax.persistence imports, not jakarta.persistence. The jakarta namespace migration will happen when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.
Applied to files:
polls/tool/pom.xmlpolls/api/src/java/org/sakaiproject/poll/model/Poll.javapolls/api/src/java/org/sakaiproject/poll/model/Option.javapolls/impl/pom.xml
📚 Learning: 2025-10-10T15:13:08.361Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/src/test/java/org/sakaiproject/accountvalidator/test/AccountValidationTestConfiguration.java:69-72
Timestamp: 2025-10-10T15:13:08.361Z
Learning: In the Sakai project, test-specific mock configurations should remain in the test class itself as part of the test logic. Only mock configurations that are common across multiple test classes should be centralized in test configuration classes.
Applied to files:
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java
📚 Learning: 2025-10-10T15:51:42.892Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14110
File: scormplayer/scorm-api/src/java/org/adl/sequencer/SeqActivityTree.java:72-74
Timestamp: 2025-10-10T15:51:42.892Z
Learning: In the SeqActivityTree class (scormplayer/scorm-api/src/java/org/adl/sequencer/SeqActivityTree.java), the `id` field is managed via a database sequence rather than being initialized in Java code. The equals/hashCode methods correctly rely on this database-generated ID for entity identity.
Applied to files:
polls/api/src/java/org/sakaiproject/poll/model/Poll.java
📚 Learning: 2025-09-28T04:09:37.259Z
Learnt from: csev
Repo: sakaiproject/sakai PR: 14094
File: lti/lti-common/src/java/org/sakaiproject/lti/util/SakaiLTIUtil.java:1439-1447
Timestamp: 2025-09-28T04:09:37.259Z
Learning: csev prefers to keep POJO adapter methods thin/minimal during the transition phase in SAK-51970. The long-term plan is to eventually flip roles where POJOs become primary implementations and Map versions become thin adapters, then get deprecated and removed. This transition will happen over multiple commits over time.
Applied to files:
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
📚 Learning: 2025-09-15T03:45:22.342Z
Learnt from: kunaljaykam
Repo: sakaiproject/sakai PR: 13987
File: calendar/calendar-impl/impl/src/java/org/sakaiproject/calendar/impl/CalendarContentProducer.java:43-43
Timestamp: 2025-09-15T03:45:22.342Z
Learning: SearchUtils in Sakai is located in the org.sakaiproject.search.api package, not org.sakaiproject.search.util package. Multiple content producers across the codebase import it from org.sakaiproject.search.api.SearchUtils.
Applied to files:
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
🧬 Code graph analysis (17)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
Data(24-30)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
Data(24-30)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
Data(24-37)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java (1)
Configuration(32-92)
polls/api/src/java/org/sakaiproject/poll/model/Poll.java (2)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(40-132)polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-98)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsToolConfig.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebMvcConfig.java (1)
Configuration(44-109)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-34)polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (1)
Repository(17-94)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollEditorController.java (2)
Value(232-236)Controller(50-237)
polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (3)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
Data(24-30)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
Data(22-26)polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionForm.java (1)
Data(21-27)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (2)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-37)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
Controller(50-224)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/PollRepositoryImpl.java (2)
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
Repository(18-141)polls/impl/src/java/org/sakaiproject/poll/repository/impl/OptionRepositoryImpl.java (1)
Repository(14-34)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
Getter(42-98)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-37)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
polls/api/src/java/org/sakaiproject/poll/util/PollUtil.java (1)
PollUtil(30-82)
polls/api/src/java/org/sakaiproject/poll/model/Option.java (1)
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
Getter(40-132)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PermissionsController.java (2)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
Controller(50-224)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/PollController.java (1)
Controller(46-217)
⏰ 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
🔇 Additional comments (23)
polls/tool/src/java/org/sakaiproject/poll/tool/model/VoteForm.java (1)
1-30: LGTM! Clean form object following best practices.The VoteForm class is well-structured as a simple data holder for Spring MVC form binding. The initialization of
selectedOptionIdsto an empty ArrayList (line 29) is good defensive programming that prevents NPE and clearly communicates the field should never be null. The use of Lombok's@Dataannotation appropriately reduces boilerplate for this form object.polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.java (2)
73-74: LGTM: Explicit null check improves clarity.The replacement of
CollectionUtils.isNotEmptywith an explicit null and empty check is a good change. It removes the external dependency and makes the intent clearer.
83-84: LGTM: Consistent null-safety pattern.The explicit null check follows the same pattern as line 74 and maintains consistency throughout the method.
polls/tool/pom.xml (1)
75-83: LGTM! Thymeleaf dependencies correctly match Spring 5.x.The
thymeleaf-spring5dependency is appropriate for the Sakai project's Spring 5.x stack, and the comment clearly documents the purpose of the RSF replacement.Based on learnings
polls/impl/src/test/org/sakaiproject/poll/logic/test/PollListManagerTest.java (2)
60-86: LGTM! Repository-based wiring correctly replaces DAO.The test setup properly instantiates
PollRepositoryImpl,OptionRepositoryImpl, andVoteRepositoryImpl, and injects them via the new repository setters. The preload method call is also updated to accept repository implementations, aligning with the migration to repository-based persistence.
229-229: LGTM! Instant.now() aligns with entity model.The change from
new Date()toInstant.now()is consistent with the Vote entity's updated field type (voteDate is nowInstant), as seen in the relevant code snippets.polls/tool/src/java/org/sakaiproject/poll/tool/model/PollForm.java (1)
19-36: LGTM! Clean Spring MVC form model with appropriate date handling.The use of
LocalDateTimewith@DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME)is the correct approach for Spring MVC form binding. Lombok's@Datareduces boilerplate while maintaining type safety, and the default values are sensible.polls/api/src/java/org/sakaiproject/poll/model/Option.java (3)
24-36: The javax.persistence imports are correct for this project.The past review comment suggesting migration to
jakarta.persistenceis incorrect. The Sakai project uses Hibernate 5.6.15.Final and Spring 5.3.39, which requirejavax.persistenceimports. The jakarta namespace migration will occur when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.Based on learnings
48-52: LGTM! Primary key mapping uses portable sequence strategy.The use of
@SequenceGeneratorwithGenerationType.AUTOandallocationSize = 1is the correct portable approach for Sakai's schema, which uses sequences rather than identity columns.
78-84: LGTM! Read-only association correctly documented.The
@ManyToOnerelationship withinsertable = false, updatable = falseis properly configured for a read-only navigation property, and the Javadoc clearly explains that updates must go through thepollIdforeign key field.polls/api/src/java/org/sakaiproject/poll/model/Poll.java (4)
33-46: The javax.persistence imports are correct for this project.The Sakai project uses Hibernate 5.6.15.Final and Spring 5.3.39, which require
javax.persistenceimports, notjakarta.persistence. The jakarta namespace migration will happen when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.Based on learnings
68-72: LGTM! Sequence-based ID generation is correctly configured.The use of
@SequenceGeneratorwithGenerationType.AUTOis the correct portable approach for Sakai's existing schema, which uses database sequences rather than identity columns. This avoids the IDENTITY mismatch issue that would occur with other strategies.
126-157: LGTM! UUID accessors correctly exclude Lombok generation.The explicit
@Getter/@Setter(AccessLevel.NONE)on the uuid field with manualgetUuid()/setUuid()methods provides proper control over UUID access while keeping Lombok's convenience for other fields.
280-283: LGTM! XML serialization correctly uses UUID for external identity.The
toXmlmethod appropriately usesgetUuid()as the external identifier (ID attribute) while preservinggetPollId()as the internal POLL_ID attribute. This design supports UUID-based public references while maintaining backward compatibility.polls/tool/src/webapp/WEB-INF/templates/polls/permissions.html (1)
1-15: LGTM! Clean Thymeleaf template with proper i18n and fragment reuse.The template follows standard Thymeleaf conventions with proper namespace declarations, i18n message keys (
#{permissions_title}), and fragment inclusion for the navigation menu. Thesakai-permissionsweb component binding is appropriate for the Sakai architecture.polls/tool/src/webapp/WEB-INF/web.xml (1)
1-6: LGTM! Minimal web.xml aligns with Java-based configuration.The migration to a minimal Java EE 3.1 descriptor is appropriate when using
WebApplicationInitializerfor Java-based Spring configuration. The extensive XML-based wiring has been correctly replaced with Java config classes (PollsWebMvcConfig, PollsWebApplicationInitializer) as part of the RSF-to-Spring MVC migration.polls/tool/src/webapp/WEB-INF/templates/polls/list.html (1)
1-125: Well-structured Thymeleaf template with good accessibility practices.The template implementation looks solid with proper use of Thymeleaf syntax, accessibility features (visually-hidden captions and labels, ARIA roles), and modular JavaScript for interactive checkbox functionality. The bulk action flow and conditional rendering based on permissions are well-implemented.
polls/tool/src/java/org/sakaiproject/poll/tool/model/OptionBatchForm.java (1)
22-26: Clean form model implementation.The
OptionBatchFormclass is well-designed with appropriate fields for batch option upload functionality. The use of Lombok's@Dataannotation keeps the code concise.polls/tool/src/java/org/sakaiproject/poll/tool/config/PollsWebApplicationInitializer.java (1)
37-58: Proper Spring web application initialization.The bootstrap sequence correctly initializes the web application context, registers necessary listeners, configures the request filter with upload support, and sets up the dispatcher servlet. The implementation follows Spring MVC best practices for programmatic configuration.
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
56-119: Robust error handling and vote submission flow.The implementation properly handles edge cases with
SecurityExceptioncatching, null poll checks, and null-safe collection handling for selected options. The use of localized messages for different error scenarios (vote_noperm.voteCollection,poll_missing) provides clear user feedback. Past review concerns have been appropriately addressed.polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java (1)
40-59: The suggested field name in the review comment is incorrect.Vote entity has a direct field
pollOption(line 79), notoptionId. The review comment suggests usingroot.get("optionId"), which will fail because that field does not exist on the Vote entity. The correct field name ispollOption.The corrected suggestion should be:
- Join<Vote, Poll> pollJoin = root.join("poll"); - Join<Vote, Option> optionJoin = root.join("option"); - - Predicate pollPredicate = cb.equal(pollJoin.get("pollId"), pollId); - Predicate optionPredicate = cb.equal(optionJoin.get("optionId"), optionId); + Predicate pollPredicate = cb.equal(root.get("pollId"), pollId); + Predicate optionPredicate = cb.equal(root.get("pollOption"), optionId);Likely an incorrect or invalid review comment.
polls/api/src/java/org/sakaiproject/poll/model/Vote.java (1)
40-126: LGTM! Well-structured JPA entity with proper validation.The Vote entity migration to JPA is well-executed:
- Proper use of
@ManyToOnewith read-only relationships (insertable/updatable = false) while maintaining writable foreign key fields- Constructor validation prevents mismatched poll-option associations (addresses previous review comment)
nullable=falseonVOTE_OPTIONenforces referential integrity (addresses previous review comment)- Migration from
DatetoInstantforvoteDatealigns with modern Java time APIs- Clean separation between JPA-managed relationships and direct foreign key access
polls/impl/src/webapp/WEB-INF/components.xml (1)
6-58: LGTM! Clean migration to repository-based persistence.The Spring configuration properly supports the repository-based architecture:
AdditionalHibernateMappingsbean registers all JPA entities (Poll, Option, Vote) with Hibernate- Repository beans correctly wire the Hibernate
sessionFactory- Manager beans (
PollListManager,PollVoteManager) are updated to use repositories instead of the legacy DAO- All required dependencies are present and properly injected
The configuration aligns with the code changes in the service implementations.
Based on learnings
| /** | ||
| * Create a new vote for the supplied poll option with the current user's context. | ||
| * | ||
| * @param poll the poll being voted on | ||
| * @param option the poll option selected | ||
| * @param submissionId identifier that groups the vote submission | ||
| * @return a newly instantiated {@link Vote} | ||
| */ | ||
| public Vote createVote(Poll poll, Option option, String submissionId); |
There was a problem hiding this comment.
Adding an abstract method breaks existing implementations.
PollVoteManager lives in the public API module, so downstream deployments often supply their own implementations. Introducing a new abstract method forces every custom implementation to recompile and ship an override or they will fail at runtime the moment createVote is invoked. Please provide a binary-compatible default (e.g., a default method that delegates to a helper or throws a clear exception) or move the new behavior behind a separate extension point so released implementations keep working out of the box.
- public Vote createVote(Poll poll, Option option, String submissionId);
+ default Vote createVote(Poll poll, Option option, String submissionId) {
+ throw new UnsupportedOperationException("createVote must be implemented by PollVoteManager providers");
+ }🤖 Prompt for AI Agents
In polls/api/src/java/org/sakaiproject/poll/logic/PollVoteManager.java around
lines 49 to 57, adding the new abstract createVote(Poll, Option, String) breaks
binary compatibility for downstream implementations; make it a default method
instead (or add a new extension interface) so existing implementors do not need
to recompile. Change the method to a default implementation that either 1)
delegates to an existing legacy method if one exists, or 2) throws a clear,
documented UnsupportedOperationException with an explanatory message pointing
integrators to the new API/extension point; alternatively, move the new
signature into a separate interface (e.g., PollVoteManagerV2) so the original
interface remains unchanged.
| if( hasNullOptionOrder ) | ||
| { | ||
| log.info( "Poll ID {} has options with null order, processing...", poll.getId() ); | ||
| log.info( "Poll ID {} has options with null order, processing...", poll.getPollId() ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the new entity getter methods exist.
The change from getId() to getPollId() and getOptionId() suggests the entity model has been refactored. Please verify that these methods exist on the Poll and Option entities and are used consistently throughout the codebase.
Run the following script to verify the entity methods:
Also applies to: 113-113
🏁 Script executed:
#!/bin/bash
# Verify Poll.getPollId() and Option.getOptionId() methods exist
echo "=== Checking for Poll.getPollId() method definition ==="
ast-grep --pattern 'class Poll {
$$$
getPollId() {
$$$
}
$$$
}'
echo -e "\n=== Checking for Option.getOptionId() method definition ==="
ast-grep --pattern 'class Option {
$$$
getOptionId() {
$$$
}
$$$
}'
echo -e "\n=== Checking for any remaining getId() calls on Poll objects ==="
rg -nP --type=java -C2 'poll\.getId\(\)' polls/
echo -e "\n=== Checking for any remaining getId() calls on Option objects ==="
rg -nP --type=java -C2 'option\.getId\(\)' polls/Length of output: 901
---
🏁 Script executed:
```shell
#!/bin/bash
# Find Poll and Option entity classes
echo "=== Finding Poll entity class files ==="
fd -e java | xargs rg -l "class Poll"
echo -e "\n=== Finding Option entity class files ==="
fd -e java | xargs rg -l "class Option"
echo -e "\n=== Looking at Poll class definition ==="
fd -e java | xargs rg -nA 20 "^\s*public\s+class\s+Poll"
echo -e "\n=== Looking at Option class definition ==="
fd -e java | xargs rg -nA 20 "^\s*public\s+class\s+Option"
echo -e "\n=== Checking for Lombok @Data or @Getter annotations ==="
fd -e java | xargs rg -nB 2 "public class (Poll|Option)" | grep -E "@Data|@Getter"
Length of output: 80477
🏁 Script executed:
#!/bin/bash
# Get full Poll class header with annotations
echo "=== Poll class with annotations ==="
head -n 75 polls/api/src/java/org/sakaiproject/poll/model/Poll.java
echo -e "\n=== Option class with annotations ==="
head -n 85 polls/api/src/java/org/sakaiproject/poll/model/Option.java
echo -e "\n=== Check if Poll and Option have getter methods ==="
rg -nA 100 "public class Poll implements" polls/api/src/java/org/sakaiproject/poll/model/Poll.java | grep -E "getPollId|getId"
echo -e "\n=== Check Option for getter methods ==="
rg -nA 100 "public class Option implements" polls/api/src/java/org/sakaiproject/poll/model/Option.java | grep -E "getOptionId|getId"
echo -e "\n=== Context around PollOptionEntityProvider.java line 94 ==="
sed -n '85,100p' polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollOptionEntityProvider.javaLength of output: 6527
🏁 Script executed:
#!/bin/bash
# Get full Poll class header with annotations
echo "=== Poll class with annotations ==="
head -n 75 polls/api/src/java/org/sakaiproject/poll/model/Poll.java
echo -e "\n=== Option class with annotations ==="
head -n 85 polls/api/src/java/org/sakaiproject/poll/model/Option.java
echo -e "\n=== Context around PollOptionEntityProvider.java line 94 ==="
sed -n '85,100p' polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/PollOptionEntityProvider.javaLength of output: 6373
🏁 Script executed:
#!/bin/bash
# Search for all getId() calls in entity providers that might need updating
echo "=== All getId() calls in poll entity providers ==="
rg -n "\.getId\(\)" polls/tool/src/java/org/sakaiproject/poll/tool/entityproviders/
echo -e "\n=== All getPollId() and getOptionId() calls in impl files ==="
rg -n "getPollId\(\)|getOptionId\(\)" polls/impl/src/java/org/sakaiproject/poll/
echo -e "\n=== Check for getId() in PollOrderOptionBackFillJob ==="
rg -n "\.getId\(\)" polls/impl/src/java/org/sakaiproject/poll/logic/impl/PollOrderOptionBackFillJob.javaLength of output: 10494
Update PollOptionEntityProvider.java to use getOptionId() for consistency.
The entity methods getPollId() and getOptionId() do exist (generated by Lombok @Getter annotations on the Poll and Option entities). However, PollOptionEntityProvider.java line 94 still uses the deprecated option.getId() method instead of the new option.getOptionId(). This creates an inconsistency with the broader refactoring visible in PollListManagerImpl, PollVoteManagerImpl, and PollOrderOptionBackFillJob.
Change line 94 from:
return option.getId()+"";to:
return option.getOptionId()+"";🤖 Prompt for AI Agents
In
polls/impl/src/java/org/sakaiproject/poll/entity/PollOptionEntityProvider.java
around line 94, the code returns option.getId()+"" which uses the deprecated
getter; change it to return option.getOptionId()+"" so it uses the new
Lombok-generated accessor for consistency with the surrounding refactor (no
other behavior changes needed).
| public List<Vote> findByUserIdAndPollIds(String userId, List<Long> pollIds) { | ||
| if (userId == null || pollIds == null || pollIds.isEmpty()) { | ||
| return Collections.emptyList(); | ||
| } | ||
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | ||
| CriteriaQuery<Vote> query = cb.createQuery(Vote.class); | ||
| Root<Vote> root = query.from(Vote.class); | ||
| Join<Vote, Poll> pollJoin = root.join("poll"); | ||
|
|
||
| Predicate userPredicate = cb.equal(root.get("userId"), userId); | ||
| Predicate pollsPredicate = pollJoin.get("pollId").in(pollIds); | ||
|
|
||
| query.select(root) | ||
| .where(cb.and(userPredicate, pollsPredicate)); | ||
|
|
||
| return sessionFactory.getCurrentSession() | ||
| .createQuery(query) | ||
| .getResultList(); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary join in findByUserIdAndPollIds.
This method joins to Poll just to check if pollId is in the provided list. Query the Vote's pollId field directly:
- Join<Vote, Poll> pollJoin = root.join("poll");
-
Predicate userPredicate = cb.equal(root.get("userId"), userId);
- Predicate pollsPredicate = pollJoin.get("pollId").in(pollIds);
+ Predicate pollsPredicate = root.get("pollId").in(pollIds);🤖 Prompt for AI Agents
In
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java
around lines 79 to 97, the query unnecessarily joins the Poll entity just to
filter by pollId; remove the Join<Vote, Poll> pollJoin and instead reference the
Vote entity's pollId attribute directly (e.g. pollsPredicate =
root.get("pollId").in(pollIds)), keep the userPredicate as-is, combine them with
cb.and, and execute the query as before.
| public boolean existsByPollIdAndUserId(Long pollId, String userId) { | ||
| if (pollId == null || userId == null || userId.trim().isEmpty()) { | ||
| return false; | ||
| } | ||
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | ||
| CriteriaQuery<Long> query = cb.createQuery(Long.class); | ||
| Root<Vote> root = query.from(Vote.class); | ||
| Join<Vote, Poll> pollJoin = root.join("poll"); | ||
|
|
||
| Predicate pollPredicate = cb.equal(pollJoin.get("pollId"), pollId); | ||
| Predicate userPredicate = cb.equal(root.get("userId"), userId); | ||
|
|
||
| query.select(cb.count(root)) | ||
| .where(cb.and(pollPredicate, userPredicate)); | ||
|
|
||
| Long count = sessionFactory.getCurrentSession() | ||
| .createQuery(query) | ||
| .getSingleResult(); | ||
|
|
||
| return count > 0; | ||
| } |
There was a problem hiding this comment.
Remove unnecessary join in existsByPollIdAndUserId.
The join to Poll is not needed when checking existence by pollId:
- Join<Vote, Poll> pollJoin = root.join("poll");
-
- Predicate pollPredicate = cb.equal(pollJoin.get("pollId"), pollId);
+ Predicate pollPredicate = cb.equal(root.get("pollId"), pollId);
Predicate userPredicate = cb.equal(root.get("userId"), userId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public boolean existsByPollIdAndUserId(Long pollId, String userId) { | |
| if (pollId == null || userId == null || userId.trim().isEmpty()) { | |
| return false; | |
| } | |
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | |
| CriteriaQuery<Long> query = cb.createQuery(Long.class); | |
| Root<Vote> root = query.from(Vote.class); | |
| Join<Vote, Poll> pollJoin = root.join("poll"); | |
| Predicate pollPredicate = cb.equal(pollJoin.get("pollId"), pollId); | |
| Predicate userPredicate = cb.equal(root.get("userId"), userId); | |
| query.select(cb.count(root)) | |
| .where(cb.and(pollPredicate, userPredicate)); | |
| Long count = sessionFactory.getCurrentSession() | |
| .createQuery(query) | |
| .getSingleResult(); | |
| return count > 0; | |
| } | |
| public boolean existsByPollIdAndUserId(Long pollId, String userId) { | |
| if (pollId == null || userId == null || userId.trim().isEmpty()) { | |
| return false; | |
| } | |
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | |
| CriteriaQuery<Long> query = cb.createQuery(Long.class); | |
| Root<Vote> root = query.from(Vote.class); | |
| Predicate pollPredicate = cb.equal(root.get("pollId"), pollId); | |
| Predicate userPredicate = cb.equal(root.get("userId"), userId); | |
| query.select(cb.count(root)) | |
| .where(cb.and(pollPredicate, userPredicate)); | |
| Long count = sessionFactory.getCurrentSession() | |
| .createQuery(query) | |
| .getSingleResult(); | |
| return count > 0; | |
| } |
🤖 Prompt for AI Agents
In
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java
around lines 100 to 120, remove the unnecessary Join<Vote, Poll> pollJoin and
use a direct path from the Vote root to the poll identifier when building the
poll predicate (e.g. navigate root.get("poll").get("pollId") instead of
joining); update the pollPredicate to compare that path to pollId, delete the
join variable and any related join usage, and keep the rest of the
criteria/query logic the same.
| public int countDistinctSubmissionIds(Long pollId) { | ||
| if (pollId == null) { | ||
| return 0; | ||
| } | ||
| CriteriaBuilder cb = sessionFactory.getCriteriaBuilder(); | ||
| CriteriaQuery<Long> query = cb.createQuery(Long.class); | ||
| Root<Vote> root = query.from(Vote.class); | ||
| Join<Vote, Poll> pollJoin = root.join("poll"); | ||
|
|
||
| query.select(cb.countDistinct(root.get("submissionId"))) | ||
| .where(cb.equal(pollJoin.get("pollId"), pollId)); | ||
|
|
||
| Long count = sessionFactory.getCurrentSession() | ||
| .createQuery(query) | ||
| .getSingleResult(); | ||
|
|
||
| return Math.toIntExact(count); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary join in countDistinctSubmissionIds.
Query Vote's pollId directly instead of joining to Poll:
- Join<Vote, Poll> pollJoin = root.join("poll");
-
query.select(cb.countDistinct(root.get("submissionId")))
- .where(cb.equal(pollJoin.get("pollId"), pollId));
+ .where(cb.equal(root.get("pollId"), pollId));🤖 Prompt for AI Agents
In
polls/impl/src/java/org/sakaiproject/poll/repository/impl/VoteRepositoryImpl.java
around lines 123 to 140, the method countDistinctSubmissionIds uses an
unnecessary join to Poll; change the CriteriaQuery to filter directly on
Vote.pollId instead of joining to Poll by removing the Join and replacing
cb.equal(pollJoin.get("pollId"), pollId) with cb.equal(root.get("pollId"),
pollId); ensure the query still selects
cb.countDistinct(root.get("submissionId")) and return Math.toIntExact of the
resulting Long as before.
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
638-661: Separate UUID initialization from retrieval to preserve command–query separation.The
getPoll(String ref)method retrieves a Poll but also modifies and persists it when the UUID is null (lines 656–659). This side effect violates the command-query separation principle. If this is migration helper code for legacy data, document it clearly or extract the UUID assignment logic to a separate method (e.g.,ensureUuidAndSave(Poll)) that callers invoke explicitly in command or migration contexts.Based on learnings
Apply this diff to remove the side effect from the query method:
public Poll getPoll(String ref) { if (ref == null) { return null; } String normalized = StringUtils.stripEnd(ref, Entity.SEPARATOR); if (StringUtils.isEmpty(normalized)) { normalized = ref; } String uuid = StringUtils.substringAfterLast(normalized, Entity.SEPARATOR); if (StringUtils.isEmpty(uuid)) { uuid = normalized; } if (StringUtils.isEmpty(uuid)) { uuid = ref; } Poll poll = pollRepository.findByUuid(uuid).orElse(null); - if (poll != null && poll.getUuid() == null) { - poll.setUuid(idManager.createUuid()); - savePoll(poll); - } return poll; }Then add a separate method for UUID initialization:
public Poll ensurePollUuidAndSave(Poll poll) { if (poll != null && poll.getUuid() == null) { poll.setUuid(idManager.createUuid()); savePoll(poll); } return poll; }Update callers that require UUID persistence to invoke
ensurePollUuidAndSaveexplicitly.
🧹 Nitpick comments (7)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (2)
91-105: Optimize vote counting to avoid O(N×M) complexity.The current implementation filters the entire votes list for each option inside the loop, resulting in O(options × votes) complexity. For polls with many options and votes, this becomes inefficient.
Consider pre-grouping votes by option before the loop:
+ Map<Long, Long> voteCountsByOption = votes.stream() + .collect(Collectors.groupingBy(Vote::getPollOption, Collectors.counting())); + for (int i = 0; i < options.size(); i++) { Option option = options.get(i); - long voteCount = votes.stream() - .filter(v -> option.getOptionId().equals(v.getPollOption())) - .count(); + long voteCount = voteCountsByOption.getOrDefault(option.getOptionId(), 0L); double percentage = calculatePercentage(poll, voteCount, totalVotes, distinctVoters);
127-128: Consider extracting permission checks to helper methods.These inline permission checks duplicate the pattern used in OptionController (lines 221-227). Extracting them to private helper methods would improve consistency and readability.
Add helper methods similar to OptionController:
private boolean isAllowedPollAdd() { return externalLogic.isUserAdmin() || externalLogic.isAllowedInLocation(PollListManager.PERMISSION_ADD, externalLogic.getCurrentLocationReference()); } private boolean isSiteOwner() { return externalLogic.isUserAdmin() || externalLogic.isAllowedInLocation("site.upd", externalLogic.getCurrentLocationReference()); }Then use them in the model population:
- model.addAttribute("canAdd", externalLogic.isUserAdmin() || externalLogic.isAllowedInLocation(PollListManager.PERMISSION_ADD, externalLogic.getCurrentLocationReference())); - model.addAttribute("isSiteOwner", externalLogic.isUserAdmin() || externalLogic.isAllowedInLocation("site.upd", externalLogic.getCurrentLocationReference())); + model.addAttribute("canAdd", isAllowedPollAdd()); + model.addAttribute("isSiteOwner", isSiteOwner());polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (5)
104-111: Add defensive null checks when retrieving polls.Lines 106 and 127 retrieve polls without null checks. While these occur during error handling, a null poll would cause an NPE when adding to the model. Consider adding null checks and returning an error redirect if the poll no longer exists.
Add null checks in both locations:
if (!isAllowedPollAdd()) { bindingResult.addError(new FieldError("optionForm", "text", messageSource.getMessage("new_poll_noperms", null, locale))); - model.addAttribute("poll", pollListManager.getPollById(optionForm.getPollId())); + Poll poll = pollListManager.getPollById(optionForm.getPollId()); + if (poll == null) { + return "redirect:/votePolls"; + } + model.addAttribute("poll", poll); model.addAttribute("isNew", optionForm.getOptionId() == null);And similarly at line 127:
} catch (PollValidationException ex) { bindingResult.addError(new FieldError("optionForm", "text", messageSource.getMessage(ex.getMessage(), ex.getArgs(), locale))); - model.addAttribute("poll", pollListManager.getPollById(optionForm.getPollId())); + Poll poll = pollListManager.getPollById(optionForm.getPollId()); + if (poll == null) { + return "redirect:/votePolls"; + } + model.addAttribute("poll", poll); model.addAttribute("isNew", optionForm.getOptionId() == null);Also applies to: 127-127
138-141: Add null check for poll and permission guard.Line 138 retrieves a poll without checking for null, and there's no permission check at the method entry. While the POST handler has both guards, the GET endpoint should also validate early to avoid exposing the form to unauthorized users or for non-existent polls.
Add guards at the beginning:
@GetMapping("/pollOptionBatch") public String batchOption(@RequestParam("pollId") Long pollId, Model model) { + if (!isAllowedPollAdd()) { + return "redirect:/votePolls"; + } Poll poll = pollListManager.getPollById(pollId); + if (poll == null) { + return "redirect:/votePolls"; + } OptionBatchForm form = new OptionBatchForm();
156-162: Good permission fix; add null checks for polls.The permission guard at lines 156-162 properly addresses the previous review concern. However, lines 158 and 169 retrieve polls without null checks before adding them to the model.
Add defensive null checks:
if (!isAllowedPollAdd()) { bindingResult.addError(new FieldError("batchForm", "file", messageSource.getMessage("new_poll_noperms", null, locale))); - model.addAttribute("poll", pollListManager.getPollById(batchForm.getPollId())); + Poll poll = pollListManager.getPollById(batchForm.getPollId()); + if (poll == null) { + return "redirect:/votePolls"; + } + model.addAttribute("poll", poll); model.addAttribute("canAdd", isAllowedPollAdd());And at line 169:
} catch (PollValidationException ex) { bindingResult.addError(new FieldError("batchForm", "file", messageSource.getMessage(ex.getMessage(), ex.getArgs(), locale))); - model.addAttribute("poll", pollListManager.getPollById(batchForm.getPollId())); + Poll poll = pollListManager.getPollById(batchForm.getPollId()); + if (poll == null) { + return "redirect:/votePolls"; + } + model.addAttribute("poll", poll); model.addAttribute("canAdd", isAllowedPollAdd());Also applies to: 158-158, 169-169
186-191: Add null check for poll.Line 186 retrieves the poll without checking for null before using it at line 187 and adding it to the model at line 191.
Add a null check:
Option option = pollListManager.getOptionById(optionId); if (option == null) { return "redirect:/votePolls"; } Poll poll = pollListManager.getPollById(option.getPollId()); + if (poll == null) { + return "redirect:/votePolls"; + } List<Option> pollOptions = pollListManager.getOptionsForPoll(poll);
208-218: Good permission fix; consider null check on delete result.The permission guard at lines 208-211 properly addresses the previous review concern. However, line 216's
deleteOptionresult is used directly at line 218 without null checking. While unlikely, if the poll was concurrently deleted, this could cause an NPE.Add a defensive null check:
} Poll poll = pollsUiService.deleteOption(optionId, orphanHandling); + if (poll == null) { + redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll_missing", null, locale)); + return "redirect:/votePolls"; + } redirectAttributes.addFlashAttribute("success", messageSource.getMessage("poll_option_deleted_success", null, locale)); return "redirect:/voteAdd?pollId=" + poll.getPollId();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java(20 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java(1 hunks)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java(1 hunks)polls/tool/src/webapp/WEB-INF/templates/polls/vote.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- polls/tool/src/webapp/WEB-INF/templates/polls/vote.html
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.
Files:
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.javapolls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java
🧠 Learnings (2)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-api/src/main/java/org/sakaiproject/accountvalidator/model/ValidationAccount.java:24-33
Timestamp: 2025-10-06T22:15:40.541Z
Learning: The Sakai project (as of this PR) uses Hibernate 5.6.15.Final and Spring 5.3.39, which requires javax.persistence imports, not jakarta.persistence. The jakarta namespace migration will happen when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: For new tools, prefer Spring MVC/Boot with Hibernate and ThymeLeaf; avoid RSF for new development and consider modernization when changing legacy tools
📚 Learning: 2025-09-15T03:45:22.342Z
Learnt from: kunaljaykam
Repo: sakaiproject/sakai PR: 13987
File: calendar/calendar-impl/impl/src/java/org/sakaiproject/calendar/impl/CalendarContentProducer.java:43-43
Timestamp: 2025-09-15T03:45:22.342Z
Learning: SearchUtils in Sakai is located in the org.sakaiproject.search.api package, not org.sakaiproject.search.util package. Multiple content producers across the codebase import it from org.sakaiproject.search.api.SearchUtils.
Applied to files:
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java
🧬 Code graph analysis (3)
polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java (1)
polls/api/src/java/org/sakaiproject/poll/util/PollUtil.java (1)
PollUtil(30-82)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
polls/tool/src/java/org/sakaiproject/poll/tool/service/PollValidationException.java (1)
PollValidationException(19-37)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java (2)
polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java (1)
Controller(50-230)polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java (1)
Controller(44-137)
⏰ 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
| if (optionId != null) { | ||
| Option option = pollListManager.getOptionById(optionId); | ||
| if (option == null) { | ||
| return "redirect:/votePolls"; | ||
| } | ||
| form.setOptionId(option.getOptionId()); | ||
| form.setPollId(option.getPollId()); | ||
| form.setText(option.getText()); | ||
| poll = pollListManager.getPollById(option.getPollId()); | ||
| } else { | ||
| if (pollId == null) { | ||
| return "redirect:/votePolls"; | ||
| } | ||
| form.setPollId(pollId); | ||
| poll = pollListManager.getPollById(pollId); | ||
| } |
There was a problem hiding this comment.
Guard against null poll before use.
At lines 80 and 86, getPollById can return null (as shown in PollListManagerImpl), but line 89 adds the poll to the model without checking. While the view might handle null gracefully, it's safer to check and redirect early.
Apply this diff to add null checks:
if (optionId != null) {
Option option = pollListManager.getOptionById(optionId);
if (option == null) {
return "redirect:/votePolls";
}
form.setOptionId(option.getOptionId());
form.setPollId(option.getPollId());
form.setText(option.getText());
poll = pollListManager.getPollById(option.getPollId());
+ if (poll == null) {
+ return "redirect:/votePolls";
+ }
} else {
if (pollId == null) {
return "redirect:/votePolls";
}
form.setPollId(pollId);
poll = pollListManager.getPollById(pollId);
+ if (poll == null) {
+ return "redirect:/votePolls";
+ }
}🤖 Prompt for AI Agents
In polls/tool/src/java/org/sakaiproject/poll/tool/mvc/OptionController.java
around lines 72 to 87, getPollById can return null but the code uses the poll
later; after each call to pollListManager.getPollById (both when optionId !=
null and in the else branch) check if the returned poll is null and if so return
"redirect:/votePolls" immediately to avoid NPEs and avoid adding a null poll to
the model.
| Poll poll = pollListManager.getPollById(pollId); | ||
| if (!pollListManager.isAllowedViewResults(poll, externalLogic.getCurrentUserId())) { | ||
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll.noviewresult", null, locale)); | ||
| return "redirect:/votePolls"; | ||
| } |
There was a problem hiding this comment.
Guard against null poll before use.
getPollById can return null (as shown in PollListManagerImpl line ~298), but line 69 calls isAllowedViewResults(poll, ...) without checking. This will throw a NullPointerException if the poll doesn't exist.
Apply this diff to add a null check and redirect with an appropriate error message:
public String showResults(@RequestParam("pollId") Long pollId,
Model model,
Locale locale,
RedirectAttributes redirectAttributes) {
Poll poll = pollListManager.getPollById(pollId);
+ if (poll == null) {
+ redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll_missing", null, locale));
+ return "redirect:/votePolls";
+ }
if (!pollListManager.isAllowedViewResults(poll, externalLogic.getCurrentUserId())) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Poll poll = pollListManager.getPollById(pollId); | |
| if (!pollListManager.isAllowedViewResults(poll, externalLogic.getCurrentUserId())) { | |
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll.noviewresult", null, locale)); | |
| return "redirect:/votePolls"; | |
| } | |
| Poll poll = pollListManager.getPollById(pollId); | |
| if (poll == null) { | |
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll_missing", null, locale)); | |
| return "redirect:/votePolls"; | |
| } | |
| if (!pollListManager.isAllowedViewResults(poll, externalLogic.getCurrentUserId())) { | |
| redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("poll.noviewresult", null, locale)); | |
| return "redirect:/votePolls"; | |
| } |
🤖 Prompt for AI Agents
In polls/tool/src/java/org/sakaiproject/poll/tool/mvc/ResultsController.java
around lines 68 to 72, the code calls isAllowedViewResults(poll, ...) without
checking whether poll is null; add a guard: if poll == null then add a flash
attribute (e.g. redirectAttributes.addFlashAttribute("alert",
messageSource.getMessage("poll.notfound", null, locale))) and return
"redirect:/votePolls" so the method never calls isAllowedViewResults with a null
poll; otherwise proceed as existing.
2 participants
Summary by CodeRabbit
New Features
Improvements