coderabbit suggestions

Author: Sakai Developer

polls/api/src/java/org/sakaiproject/poll/logic/PollListManager.java

@@ -100,6 +100,14 @@ public interface PollListManager extends EntityProducer {
      */
     public Option getOptionById(Long optionId);
 
+    /**
+     * 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);
+
     /**
      * Get all options for a specific poll
      * @param pollId the id for a poll

polls/api/src/java/org/sakaiproject/poll/model/Vote.java

@@ -96,6 +96,20 @@ public Vote() {
     }
 
     public Vote(Poll poll, Option option, String subId, Instant voteDate, String userId, String ip) {
+        if (poll == null) {
+            throw new IllegalArgumentException("Poll must not be null when creating a vote");
+        }
+        if (option == null) {
+            throw new IllegalArgumentException("Option must not be null when creating a vote");
+        }
+        Long pollIdValue = poll.getPollId();
+        Long optionPollId = option.getPollId();
+        if (optionPollId == null && option.getPoll() != null) {
+            optionPollId = option.getPoll().getPollId();
+        }
+        if (pollIdValue != null && optionPollId != null && !pollIdValue.equals(optionPollId)) {
+            throw new IllegalArgumentException(String.format("Option %s does not belong to poll %s", String.valueOf(option.getOptionId()), String.valueOf(pollIdValue)));
+        }
         this.pollId = poll.getPollId();
         this.pollOption = option.getOptionId();
         this.poll = poll;

polls/impl/src/java/org/sakaiproject/poll/service/impl/PollListManagerImpl.java

@@ -141,11 +141,8 @@ public boolean savePoll(Poll t) throws SecurityException, IllegalArgumentExcepti
 
         if (t.getPollId() == null) {
             newPoll = true;
-            String generatedUuid = null;
-            if (idManager != null) {
-                generatedUuid = idManager.createUuid();
-            }
-            if (generatedUuid == null || generatedUuid.trim().isEmpty()) {
+            String generatedUuid = idManager.createUuid();
+            if (StringUtils.isBlank(generatedUuid)) {
                 generatedUuid = UUID.randomUUID().toString();
             }
             t.setUuid(generatedUuid);
@@ -290,10 +287,17 @@ public Poll getPollWithVotes(Long pollId) {
     }
 
     public Option getOptionById(Long optionId) {
-        Option option = optionRepository.findById(optionId).orElse(null);
-        if (option != null && option.getUuid() == null) {
+        return optionRepository.findById(optionId).orElse(null);
+    }
+
+    public Option ensureOptionUuidAndSave(Option option) {
+        if (option == null) {
+            return null;
+        }
+        if (StringUtils.isBlank(option.getUuid())) {
             option.setUuid(UUID.randomUUID().toString());
-            saveOption(option);
+            option = optionRepository.save(option);
+            log.debug("Option id {} assigned UUID {}", option.getOptionId(), option.getUuid());
         }
         return option;
     }
@@ -314,12 +318,17 @@ public void deleteOption(Option option, boolean soft) {
     }
 
     public boolean saveOption(Option t) {
-        if (t.getUuid() == null || t.getUuid().trim().isEmpty()) {
-            t.setUuid( UUID.randomUUID().toString() );
+        if (t == null) {
+            throw new IllegalArgumentException("Option cannot be null when saving");
         }
 
-        optionRepository.save(t);
-        log.debug("Option {} successfully saved", t.toString());
+        Option persisted;
+        if (StringUtils.isBlank(t.getUuid())) {
+            persisted = ensureOptionUuidAndSave(t);
+        } else {
+            persisted = optionRepository.save(t);
+        }
+        log.debug("Option {} successfully saved", persisted);
         return true;
     }
 
@@ -383,6 +392,7 @@ public String archive(String siteId, Document doc, Stack<Element> stack, String
                 List<Option> options = getVisibleOptionsForPoll(poll.getPollId());
 
                 for (Option option : options) {
+                    option = ensureOptionUuidAndSave(option);
                     Element el2 = PollUtil.optionToXml(option, doc, stack);
                     el.appendChild(el2);
                 }

polls/tool/src/java/org/sakaiproject/poll/tool/mvc/VoteController.java

@@ -58,7 +58,13 @@ public String showVote(@RequestParam("pollId") Long pollId,
                            Model model,
                            Locale locale,
                            RedirectAttributes redirectAttributes) {
-        Poll poll = pollListManager.getPollById(pollId);
+        Poll poll;
+        try {
+            poll = pollListManager.getPollById(pollId);
+        } catch (SecurityException e) {
+            redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale));
+            return "redirect:/faces/votePolls";
+        }
         if (poll == null) {
             redirectAttributes.addFlashAttribute("alert", messageSource.getMessage("vote_noperm.voteCollection", null, locale));
             return "redirect:/faces/votePolls";

polls/tool/src/webapp/WEB-INF/templates/polls/edit.html

@@ -24,6 +24,7 @@
     <h1 class="h3 mb-3" th:text="${isNew} ? #{new_poll_title} : #{new_poll_title_edit}">Poll</h1>
 
     <form th:action="@{/faces/voteAdd}" th:object="${pollForm}" method="post" class="needs-validation" novalidate>
+      <input type="hidden" th:if="${_csrf != null}" th:name="${_csrf.parameterName}" th:value="${_csrf.token}" />
       <input type="hidden" th:field="*{pollId}" />
 
       <div class="mb-3">

polls/tool/src/webapp/WEB-INF/templates/polls/option-delete.html

@@ -25,6 +25,7 @@ <h1 class="h4 mb-3" th:text="#{delete_confirm}">Confirm delete</h1>
     </div>
 
     <form th:action="@{/faces/pollOptionDelete}" method="post">
+      <input type="hidden" th:if="${_csrf != null}" th:name="${_csrf.parameterName}" th:value="${_csrf.token}" />
       <input type="hidden" name="optionId" th:value="${option.optionId}" />
 
       <div class="mb-3" th:if="${hasVotes}">