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
This commit is contained in:
2026-07-07 08:58:55 +02:00
parent e2558c70f5
commit e57ed79b35
5 changed files with 32 additions and 12 deletions
@@ -8,9 +8,11 @@ export default class extends Controller {
canModify: Boolean, canModify: Boolean,
savedLabel: String, savedLabel: String,
errorLabel: String, errorLabel: String,
errorHint: String,
}; };
connect() { connect() {
this._locked = false;
if (this.canModifyValue) { if (this.canModifyValue) {
this._setupDrag(); this._setupDrag();
} }
@@ -66,7 +68,7 @@ export default class extends Controller {
this.listTarget.addEventListener('drop', async (e) => { this.listTarget.addEventListener('drop', async (e) => {
e.preventDefault(); e.preventDefault();
if (!this._dragging || !this._placeholder) return; if (!this._dragging || !this._placeholder || this._locked) return;
this.listTarget.insertBefore(this._dragging, this._placeholder); this.listTarget.insertBefore(this._dragging, this._placeholder);
this._removePlaceholder(); this._removePlaceholder();
await this._persistOrder(); await this._persistOrder();
@@ -107,15 +109,26 @@ export default class extends Controller {
if (numberEl) numberEl.textContent = String(i + 1); if (numberEl) numberEl.textContent = String(i + 1);
}); });
for (let attempt = 0; attempt < 2; attempt++) {
try { try {
const res = await fetch(this.reorderUrlValue, {method: 'POST', body: params}); const res = await fetch(this.reorderUrlValue, {method: 'POST', body: params});
if (res.ok) { if (res.ok) {
this._setStatus('saved'); this._setStatus('saved');
} else { return;
this._setStatus('error');
} }
} catch { } catch {
this._setStatus('error'); // network error — retry on first attempt
} }
} }
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);
}
} }
@@ -83,6 +83,7 @@ class QuizQuestionController extends AbstractController
return $response; return $response;
} }
#[IsGranted(SeasonVoter::EDIT, subject: 'season')]
#[Route( #[Route(
'/backoffice/season/{seasonCode:season}/quiz/{quiz}/question/{question}/view', '/backoffice/season/{seasonCode:season}/quiz/{quiz}/question/{question}/view',
name: 'tvdt_backoffice_quiz_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; $position = 1;
foreach ($ordering as $questionId) { foreach ($ordering as $questionId) {
$questionsById[$questionId]->ordering = $position++; $questionsById[$questionId]->ordering = $position++;
@@ -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> <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="flex-grow-1">{{ form_widget(answerForm.text) }}</div>
<div class="d-none">{{ form_widget(answerForm.isRightAnswer) }}</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' }}" class="btn btn-sm {{ answerForm.isRightAnswer.vars.checked ? 'btn-success' : 'btn-danger' }}"
title="{{ 'Toggle correct answer'|trans }}" 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'"> 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'">
@@ -91,7 +91,8 @@
data-bo--question-list-csrf-value="{{ csrf_token('question_reorder') }}" 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-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-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"> <h4 class="mb-3 d-flex align-items-center gap-2">
{{ 'Questions'|trans }} {{ 'Questions'|trans }}
@@ -135,7 +136,7 @@
</div> </div>
</div> </div>
{% else %} {% else %}
{{ 'EMPTY'|trans }} {{ 'No questions have been added to this quiz yet.'|trans }}
{% endfor %} {% endfor %}
</div> </div>
@@ -129,6 +129,7 @@
<button type="button" class="btn btn-outline-secondary" <button type="button" class="btn btn-outline-secondary"
data-action="click->bo--modal#open" data-action="click->bo--modal#open"
data-src="{{ path('tvdt_backoffice_question_bank_edit', {seasonCode: season.seasonCode, bankQuestion: bankQuestion.id}) }}" 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> title="{{ 'Edit'|trans }}"><i class="bi bi-pencil"></i></button>
<button type="button" class="btn btn-outline-danger" data-bs-toggle="modal" <button type="button" class="btn btn-outline-danger" data-bs-toggle="modal"
data-bs-target="#deleteBankQuestion-{{ bankQuestion.id }}" data-bs-target="#deleteBankQuestion-{{ bankQuestion.id }}"