From e57ed79b35b8161da5c1cb6c8b610a6d6a3db62b Mon Sep 17 00:00:00 2001
From: Marijn Doeve <marijn@doeve.me>
Date: Tue, 7 Jul 2026 08:58:55 +0200
Subject: [PATCH] fix: address CodeRabbit review findings on PR #183

- Add SeasonVoter::EDIT guard to QuizQuestionController::view()
- Add full-count check in reorder() to reject partial ordering payloads
- Retry once in _persistOrder(); lock drag and show dismissible alert on failure
- Remove tabindex="-1" from correct-answer toggle button (accessibility)
- Replace 'EMPTY'|trans placeholder with proper copy
- Add data-modal-title to question bank Edit button to prevent stale title
---
 .../bo/question_list_controller.js            | 31 +++++++++++++------
 .../Backoffice/QuizQuestionController.php     |  5 +++
 .../backoffice/partials/answer_row.html.twig  |  2 +-
 .../backoffice/quiz/tab_overview.html.twig    |  5 +--
 .../season/tab_question_bank.html.twig        |  1 +
 5 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/assets/controllers/bo/question_list_controller.js b/assets/controllers/bo/question_list_controller.js
index 3121894..2566a93 100644
--- a/assets/controllers/bo/question_list_controller.js
+++ b/assets/controllers/bo/question_list_controller.js
@@ -8,9 +8,11 @@ export default class extends Controller {
     canModify: Boolean,
     savedLabel: String,
     errorLabel: String,
+    errorHint: String,
   };
 
   connect() {
+    this._locked = false;
     if (this.canModifyValue) {
       this._setupDrag();
     }
@@ -66,7 +68,7 @@ export default class extends Controller {
 
     this.listTarget.addEventListener('drop', async (e) => {
       e.preventDefault();
-      if (!this._dragging || !this._placeholder) return;
+      if (!this._dragging || !this._placeholder || this._locked) return;
       this.listTarget.insertBefore(this._dragging, this._placeholder);
       this._removePlaceholder();
       await this._persistOrder();
@@ -107,15 +109,26 @@ export default class extends Controller {
       if (numberEl) numberEl.textContent = String(i + 1);
     });
 
-    try {
-      const res = await fetch(this.reorderUrlValue, {method: 'POST', body: params});
-      if (res.ok) {
-        this._setStatus('saved');
-      } else {
-        this._setStatus('error');
+    for (let attempt = 0; attempt < 2; attempt++) {
+      try {
+        const res = await fetch(this.reorderUrlValue, {method: 'POST', body: params});
+        if (res.ok) {
+          this._setStatus('saved');
+          return;
+        }
+      } catch {
+        // network error — retry on first attempt
       }
-    } catch {
-      this._setStatus('error');
     }
+
+    this._locked = true;
+    this._setStatus('error');
+
+    const alert = document.createElement('div');
+    alert.className = 'alert alert-danger alert-dismissible mt-3';
+    alert.setAttribute('role', 'alert');
+    const hint = this.errorHintValue || 'Refresh the page to try again.';
+    alert.innerHTML = `${this.errorLabelValue || 'Error saving order'} &mdash; ${hint} <button type="button" class="btn-close" data-bs-dismiss="alert" aria-label="Close"></button>`;
+    this.listTarget.after(alert);
   }
 }
diff --git a/src/Controller/Backoffice/QuizQuestionController.php b/src/Controller/Backoffice/QuizQuestionController.php
index 27d3669..269046f 100644
--- a/src/Controller/Backoffice/QuizQuestionController.php
+++ b/src/Controller/Backoffice/QuizQuestionController.php
@@ -83,6 +83,7 @@ class QuizQuestionController extends AbstractController
         return $response;
     }
 
+    #[IsGranted(SeasonVoter::EDIT, subject: 'season')]
     #[Route(
         '/backoffice/season/{seasonCode:season}/quiz/{quiz}/question/{question}/view',
         name: 'tvdt_backoffice_quiz_question_view',
@@ -127,6 +128,10 @@ class QuizQuestionController extends AbstractController
             }
         }
 
+        if (\count(array_unique($ordering)) !== \count($questionsById)) {
+            throw new BadRequestHttpException('Ordering must include every question exactly once');
+        }
+
         $position = 1;
         foreach ($ordering as $questionId) {
             $questionsById[$questionId]->ordering = $position++;
diff --git a/templates/backoffice/partials/answer_row.html.twig b/templates/backoffice/partials/answer_row.html.twig
index 73d24d6..e99b140 100644
--- a/templates/backoffice/partials/answer_row.html.twig
+++ b/templates/backoffice/partials/answer_row.html.twig
@@ -4,7 +4,7 @@
         <span class="text-muted" data-drag-handle style="cursor: grab" title="{{ 'Drag to reorder'|trans }}"><i class="bi bi-grip-vertical"></i></span>
         <div class="flex-grow-1">{{ form_widget(answerForm.text) }}</div>
         <div class="d-none">{{ form_widget(answerForm.isRightAnswer) }}</div>
-        <button type="button" tabindex="-1"
+        <button type="button"
                 class="btn btn-sm {{ answerForm.isRightAnswer.vars.checked ? 'btn-success' : 'btn-danger' }}"
                 title="{{ 'Toggle correct answer'|trans }}"
                 onclick="var cb=this.closest('[data-collection-item]').querySelector('input[type=checkbox]');cb.checked=!cb.checked;this.classList.toggle('btn-success',cb.checked);this.classList.toggle('btn-danger',!cb.checked);this.querySelector('i').className=cb.checked?'bi bi-check-lg':'bi bi-x-lg'">
diff --git a/templates/backoffice/quiz/tab_overview.html.twig b/templates/backoffice/quiz/tab_overview.html.twig
index 885809f..d9aacdf 100644
--- a/templates/backoffice/quiz/tab_overview.html.twig
+++ b/templates/backoffice/quiz/tab_overview.html.twig
@@ -91,7 +91,8 @@
          data-bo--question-list-csrf-value="{{ csrf_token('question_reorder') }}"
          data-bo--question-list-can-modify-value="{{ is_granted('QUIZ_MODIFY_CONTENT', quiz) ? 'true' : 'false' }}"
          data-bo--question-list-saved-label-value="{{ 'Order saved'|trans }}"
-         data-bo--question-list-error-label-value="{{ 'Error saving order'|trans }}">
+         data-bo--question-list-error-label-value="{{ 'Error saving order'|trans }}"
+         data-bo--question-list-error-hint-value="{{ 'Refresh the page to try again.'|trans }}">
 
         <h4 class="mb-3 d-flex align-items-center gap-2">
             {{ 'Questions'|trans }}
@@ -135,7 +136,7 @@
                     </div>
                 </div>
             {% else %}
-                {{ 'EMPTY'|trans }}
+                {{ 'No questions have been added to this quiz yet.'|trans }}
             {% endfor %}
         </div>
 
diff --git a/templates/backoffice/season/tab_question_bank.html.twig b/templates/backoffice/season/tab_question_bank.html.twig
index a715bc6..9b5334c 100644
--- a/templates/backoffice/season/tab_question_bank.html.twig
+++ b/templates/backoffice/season/tab_question_bank.html.twig
@@ -129,6 +129,7 @@
                             <button type="button" class="btn btn-outline-secondary"
                                     data-action="click->bo--modal#open"
                                     data-src="{{ path('tvdt_backoffice_question_bank_edit', {seasonCode: season.seasonCode, bankQuestion: bankQuestion.id}) }}"
+                                    data-modal-title="{{ 'Edit question'|trans }}"
                                     title="{{ 'Edit'|trans }}"><i class="bi bi-pencil"></i></button>
                             <button type="button" class="btn btn-outline-danger" data-bs-toggle="modal"
                                     data-bs-target="#deleteBankQuestion-{{ bankQuestion.id }}"