Merge pull request 'Update checklist sync without hard resets' (#73) from koda/recreate-67-checklist-sync-upsert into koda/recreate-67-checklist-acceptance-criteria

Reviewed-on: OpenSource/KanbanGit#73

app/Actions/Tickets/Support/AcceptanceCriteriaChecklistMapper.php

@@ -16,6 +16,162 @@ class AcceptanceCriteriaChecklistMapper
      * @return array<int, array{checked: bool, indent: int, text: string}>
      */
     public static function fromModels(iterable $criteria): array
+    {
+        return self::flattenModels($criteria, false);
+    }
+
+    /**
+     * @param  iterable<AcceptanceCriterion>  $criteria
+     * @return array<int, array{id: int, checked: bool, indent: int, text: string}>
+     */
+    public static function fromModelsWithIds(iterable $criteria): array
+    {
+        return self::flattenModels($criteria, true);
+    }
+
+    /**
+     * @return array<int, array{id?: int, checked: bool, indent: int, text: string}>
+     */
+    public static function normalize(mixed $value): array
+    {
+        if (is_array($value)) {
+            return self::normalizeFromArray($value);
+        }
+
+        return [];
+    }
+
+    /**
+     * @param  Ticket  $ticket
+     * @param  array<int, array{id?: int, checked: bool, indent: int, text: string}>  $items
+     */
+    public static function syncTicket(Ticket $ticket, array $items): void
+    {
+        $normalized = self::normalize($items);
+        $existingCriteria = $ticket->acceptanceCriteria()->get()->keyBy('id');
+
+        if ($normalized === []) {
+            $ticket->acceptanceCriteria()->delete();
+            $ticket->unsetRelation('acceptanceCriteria');
+
+            return;
+        }
+
+        $keptCriterionIds = [];
+        $parentStack = [];
+        $positionsByParent = [];
+        $fallbackCriteria = $existingCriteria->values();
+        $fallbackIndex = 0;
+
+        foreach ($normalized as $item) {
+            $indent = min($item['indent'], count($parentStack));
+            $parentId = $indent === 0 ? null : $parentStack[$indent - 1];
+            $positionKey = $parentId ?? 'root';
+            $position = $positionsByParent[$positionKey] ?? 0;
+            $criterionId = $item['id'] ?? null;
+            $criterion = $criterionId === null
+                ? null
+                : $existingCriteria->get($criterionId);
+
+            while (
+                $criterion === null
+                && $fallbackIndex < $fallbackCriteria->count()
+            ) {
+                /** @var AcceptanceCriterion $candidate */
+                $candidate = $fallbackCriteria[$fallbackIndex];
+                $fallbackIndex++;
+
+                if (in_array($candidate->id, $keptCriterionIds, true)) {
+                    continue;
+                }
+
+                $criterion = $candidate;
+            }
+
+            if ($criterion === null) {
+                $criterion = $ticket->acceptanceCriteria()->create([
+                    'parent_acceptance_criterion_id' => $parentId,
+                    'position' => $position,
+                    'text' => $item['text'],
+                    'is_checked' => $item['checked'],
+                ]);
+            } else {
+                $criterion->fill([
+                    'parent_acceptance_criterion_id' => $parentId,
+                    'position' => $position,
+                    'text' => $item['text'],
+                    'is_checked' => $item['checked'],
+                ]);
+
+                if ($criterion->isDirty()) {
+                    $criterion->save();
+                }
+            }
+
+            $keptCriterionIds[] = $criterion->id;
+            $positionsByParent[$positionKey] = $position + 1;
+            $parentStack[$indent] = $criterion->id;
+            $parentStack = array_slice($parentStack, 0, $indent + 1);
+        }
+
+        $idsToDelete = $existingCriteria->keys()->diff($keptCriterionIds);
+
+        if ($idsToDelete->isNotEmpty()) {
+            $ticket->acceptanceCriteria()
+                ->whereIn('id', $idsToDelete->all())
+                ->delete();
+        }
+
+        $ticket->unsetRelation('acceptanceCriteria');
+    }
+
+    /**
+     * @param  array<int|string, mixed>  $value
+     * @return array<int, array{id?: int, checked: bool, indent: int, text: string}>
+     */
+    private static function normalizeFromArray(array $value): array
+    {
+        $items = [];
+
+        foreach ($value as $rawItem) {
+            if (! is_array($rawItem)) {
+                continue;
+            }
+
+            $text = trim((string) Arr::get($rawItem, 'text', ''));
+
+            if ($text === '') {
+                continue;
+            }
+
+            $normalizedItem = [
+                'checked' => filter_var(Arr::get($rawItem, 'checked', false), FILTER_VALIDATE_BOOLEAN),
+                'indent' => self::clampIndent((int) Arr::get($rawItem, 'indent', 0)),
+                'text' => $text,
+            ];
+
+            $id = filter_var(Arr::get($rawItem, 'id'), FILTER_VALIDATE_INT);
+
+            if (is_int($id) && $id > 0) {
+                $normalizedItem['id'] = $id;
+            }
+
+            $items[] = $normalizedItem;
+        }
+
+        return $items;
+    }
+
+    private static function clampIndent(int $indent): int
+    {
+        return max(0, min(self::MAX_INDENT_LEVEL, $indent));
+    }
+
+    /**
+     * @param  iterable<AcceptanceCriterion>  $criteria
+     * @return array<int, array{id?: int, checked: bool, indent: int, text: string}>
+     */
+    private static function flattenModels(iterable $criteria, bool $includeIds): array
     {
         $models = ($criteria instanceof EloquentCollection ? $criteria : collect($criteria))
             ->sort(function (AcceptanceCriterion $left, AcceptanceCriterion $right): int {
@@ -39,108 +195,31 @@ class AcceptanceCriteriaChecklistMapper
         $grouped = $models->groupBy('parent_acceptance_criterion_id')->all();
         $items = [];
 
-        self::flattenItems($items, $grouped, null, 0);
+        self::flattenItems($items, $grouped, null, 0, $includeIds);
 
         return $items;
     }
 
     /**
-     * @return array<int, array{checked: bool, indent: int, text: string}>
-     */
-    public static function normalize(mixed $value): array
-    {
-        if (is_array($value)) {
-            return self::normalizeFromArray($value);
-        }
-
-        return [];
-    }
-
-    /**
-     * @param  Ticket  $ticket
-     * @param  array<int, array{checked: bool, indent: int, text: string}>  $items
-     */
-    public static function syncTicket(Ticket $ticket, array $items): void
-    {
-        $normalized = self::normalize($items);
-
-        $ticket->acceptanceCriteria()->delete();
-
-        if ($normalized === []) {
-            return;
-        }
-
-        $parentStack = [];
-        $positionsByParent = [];
-
-        foreach ($normalized as $item) {
-            $indent = min($item['indent'], count($parentStack));
-            $parentId = $indent === 0 ? null : $parentStack[$indent - 1];
-            $positionKey = $parentId ?? 'root';
-            $position = $positionsByParent[$positionKey] ?? 0;
-
-            $criterion = $ticket->acceptanceCriteria()->create([
-                'parent_acceptance_criterion_id' => $parentId,
-                'position' => $position,
-                'text' => $item['text'],
-                'is_checked' => $item['checked'],
-            ]);
-
-            $positionsByParent[$positionKey] = $position + 1;
-            $parentStack[$indent] = $criterion->id;
-            $parentStack = array_slice($parentStack, 0, $indent + 1);
-        }
-
-        $ticket->unsetRelation('acceptanceCriteria');
-    }
-    /**
-     * @param  array<int, array{checked: bool, indent: int, text: string}>  $items
+     * @param  array<int, array{id?: int, checked: bool, indent: int, text: string}>  $items
      * @param  array<int|string, iterable<AcceptanceCriterion>>  $grouped
      */
-    private static function flattenItems(array &$items, array $grouped, ?int $parentId, int $indent): void
+    private static function flattenItems(array &$items, array $grouped, ?int $parentId, int $indent, bool $includeIds): void
     {
         foreach ($grouped[$parentId] ?? [] as $criterion) {
-            $items[] = [
+            $item = [
                 'checked' => $criterion->is_checked,
                 'indent' => $indent,
                 'text' => $criterion->text,
             ];
 
-            self::flattenItems($items, $grouped, $criterion->id, $indent + 1);
-        }
+            if ($includeIds) {
+                $item['id'] = $criterion->id;
             }
 
-    /**
-     * @param  array<int|string, mixed>  $value
-     * @return array<int, array{checked: bool, indent: int, text: string}>
-     */
-    private static function normalizeFromArray(array $value): array
-    {
-        $items = [];
+            $items[] = $item;
 
-        foreach ($value as $rawItem) {
-            if (! is_array($rawItem)) {
-                continue;
+            self::flattenItems($items, $grouped, $criterion->id, $indent + 1, $includeIds);
         }
-
-            $text = trim((string) Arr::get($rawItem, 'text', ''));
-
-            if ($text === '') {
-                continue;
-            }
-
-            $items[] = [
-                'checked' => filter_var(Arr::get($rawItem, 'checked', false), FILTER_VALIDATE_BOOLEAN),
-                'indent' => self::clampIndent((int) Arr::get($rawItem, 'indent', 0)),
-                'text' => $text,
-            ];
-        }
-
-        return $items;
-    }
-
-    private static function clampIndent(int $indent): int
-    {
-        return max(0, min(self::MAX_INDENT_LEVEL, $indent));
     }
 }

app/Http/Requests/StoreProjectTicketRequest.php

@@ -49,6 +49,7 @@ class StoreProjectTicketRequest extends FormRequest
         return [
             'acceptance_criteria' => ['nullable', 'array'],
             'acceptance_criteria.*' => ['array'],
+            'acceptance_criteria.*.id' => ['nullable', 'integer'],
             'acceptance_criteria.*.text' => ['required', 'string'],
             'acceptance_criteria.*.checked' => ['required', 'boolean'],
             'acceptance_criteria.*.indent' => ['required', 'integer', 'min:0', 'max:'.AcceptanceCriteriaChecklistMapper::MAX_INDENT_LEVEL],

app/Http/Requests/UpdateProjectTicketRequest.php

@@ -55,6 +55,7 @@ class UpdateProjectTicketRequest extends FormRequest
         return [
             'acceptance_criteria' => ['nullable', 'array'],
             'acceptance_criteria.*' => ['array'],
+            'acceptance_criteria.*.id' => ['nullable', 'integer'],
             'acceptance_criteria.*.text' => ['required', 'string'],
             'acceptance_criteria.*.checked' => ['required', 'boolean'],
             'acceptance_criteria.*.indent' => ['required', 'integer', 'min:0', 'max:'.AcceptanceCriteriaChecklistMapper::MAX_INDENT_LEVEL],

app/Http/Resources/TicketResource.php

@@ -17,7 +17,7 @@ class TicketResource extends JsonResource
     {
         return [
             'id' => $this->id,
-            'acceptance_criteria' => AcceptanceCriteriaChecklistMapper::fromModels(
+            'acceptance_criteria' => AcceptanceCriteriaChecklistMapper::fromModelsWithIds(
                 $this->relationLoaded('acceptanceCriteria')
                     ? $this->acceptanceCriteria
                     : $this->acceptanceCriteria()->get(),

resources/js/lib/acceptance-criteria.ts

@@ -17,6 +17,9 @@ export const normalizeAcceptanceCriteria = (
     const items = Array.isArray(value)
         ? value
               .map((item) => ({
+                  ...(typeof item.id === 'number' && item.id > 0
+                      ? { id: item.id }
+                      : {}),
                   checked: item.checked === true,
                   indent: clampIndent(item.indent),
                   text: item.text.trim(),

resources/js/pages/projects/Ticket.vue

@@ -5,6 +5,8 @@ import { computed, ref, watch } from 'vue';
 import {
     acceptanceCriteriaToEditorState,
     buildAcceptanceCriteria,
+    formatChecklistLines,
+    normalizeAcceptanceCriteria,
     parseChecklistLines,
 } from '@/lib/acceptance-criteria';
 import ProjectTicketBranchController from '@/actions/App/Http/Controllers/ProjectTicketBranchController';
@@ -112,6 +114,11 @@ const acceptanceCriteriaEditorState = acceptanceCriteriaToEditorState(
     props.ticket.acceptance_criteria,
 );
 const acceptanceCriteriaChecklist = ref(acceptanceCriteriaEditorState.checklist);
+const savedAcceptanceCriteria = ref(
+    normalizeAcceptanceCriteria(props.ticket.acceptance_criteria),
+);
+const isPersistingAcceptanceCriteriaToggle = ref(false);
+const acceptanceCriteriaToggleError = ref('');
 
 const form = useForm({
     acceptance_criteria: buildAcceptanceCriteria(acceptanceCriteriaChecklist.value),
@@ -141,7 +148,13 @@ const form = useForm({
 });
 
 const acceptanceCriteriaPreviewItems = computed(() =>
-    parseChecklistLines(acceptanceCriteriaChecklist.value),
+    savedAcceptanceCriteria.value,
+);
+
+const hasUnsavedAcceptanceCriteriaDraft = computed(
+    () =>
+        acceptanceCriteriaChecklist.value !==
+        formatChecklistLines(savedAcceptanceCriteria.value),
 );
 
 const acceptanceCriteriaError = computed(() => {
@@ -337,12 +350,117 @@ const submitBranchCreation = (): void => {
     );
 };
 
+const buildAcceptanceCriteriaForSubmit = () => {
+    const draftItems = buildAcceptanceCriteria(acceptanceCriteriaChecklist.value);
+
+    return draftItems.map((item, index) => ({
+        ...item,
+        ...(savedAcceptanceCriteria.value[index]?.id !== undefined
+            ? { id: savedAcceptanceCriteria.value[index].id }
+            : {}),
+    }));
+};
+
+const buildSavedTicketPayload = (acceptanceCriteria = savedAcceptanceCriteria.value) => ({
+    acceptance_criteria: acceptanceCriteria,
+    assignee_user_id: props.ticket.assignee_user_id,
+    author_user_id: props.ticket.author_user_id,
+    blocked_comment: props.ticket.blocked_comment,
+    description: props.ticket.description,
+    field_values: (props.ticket.field_values ?? []).map((fieldValue) => ({
+        ticket_field_definition_id: fieldValue.ticket_field_definition_id,
+        value_number: fieldValue.value_number,
+        value_user_id: fieldValue.value_user_id,
+    })),
+    is_blocked: props.ticket.is_blocked,
+    parent_ticket_id: props.ticket.parent_ticket_id,
+    story_points: props.ticket.story_points,
+    ticket_priority_id: props.ticket.ticket_priority_id,
+    ticket_type_id: props.ticket.ticket_type_id,
+    title: props.ticket.title,
+});
+
+const persistAcceptanceCriteriaToggle = async (
+    index: number,
+    checked: boolean,
+): Promise<void> => {
+    if (
+        !props.canEditTicket ||
+        isPersistingAcceptanceCriteriaToggle.value ||
+        hasUnsavedAcceptanceCriteriaDraft.value
+    ) {
+        return;
+    }
+
+    const nextAcceptanceCriteria = savedAcceptanceCriteria.value.map((item, itemIndex) =>
+        itemIndex === index ? { ...item, checked } : item,
+    );
+    const previousChecklist = formatChecklistLines(savedAcceptanceCriteria.value);
+    const nextChecklist = formatChecklistLines(nextAcceptanceCriteria);
+    const csrfToken = document
+        .querySelector('meta[name="csrf-token"]')
+        ?.getAttribute('content');
+
+    isPersistingAcceptanceCriteriaToggle.value = true;
+    acceptanceCriteriaToggleError.value = '';
+
+    try {
+        const response = await fetch(
+            ProjectTicketController.update.url({
+                project: props.project.id,
+                ticket: props.ticket.id,
+            }),
+            {
+                method: 'PATCH',
+                credentials: 'same-origin',
+                headers: {
+                    Accept: 'text/html, application/xhtml+xml',
+                    'Content-Type': 'application/json',
+                    'X-CSRF-TOKEN': csrfToken ?? '',
+                    'X-Requested-With': 'XMLHttpRequest',
+                },
+                body: JSON.stringify(
+                    buildSavedTicketPayload(nextAcceptanceCriteria),
+                ),
+            },
+        );
+
+        if (!response.ok) {
+            throw new Error('Checklist toggle update failed.');
+        }
+
+        savedAcceptanceCriteria.value = nextAcceptanceCriteria;
+
+        if (acceptanceCriteriaChecklist.value === previousChecklist) {
+            acceptanceCriteriaChecklist.value = nextChecklist;
+        }
+    } catch (_error) {
+        acceptanceCriteriaToggleError.value =
+            'Checklist status could not be saved right now.';
+    } finally {
+        isPersistingAcceptanceCriteriaToggle.value = false;
+    }
+};
+
+const onAcceptanceCriteriaCheckboxChange = (
+    index: number,
+    event: Event,
+): void => {
+    const target = event.target;
+
+    if (!(target instanceof HTMLInputElement)) {
+        return;
+    }
+
+    void persistAcceptanceCriteriaToggle(index, target.checked);
+};
+
 const submit = (): void => {
     if (!props.canEditTicket) {
         return;
     }
 
-    form.acceptance_criteria = buildAcceptanceCriteria(acceptanceCriteriaChecklist.value);
+    form.acceptance_criteria = buildAcceptanceCriteriaForSubmit();
 
     form.submit(
         ProjectTicketController.update({
@@ -632,10 +750,20 @@ const formatDateTime = (value: string): string => {
                                     />
                                     <p class="text-muted-foreground text-xs">
                                         Use <code>[ ]</code> / <code>[x]</code> for checklist items. Indent with two spaces for nested steps.
+                                        Text changes are saved with the main ticket update.
+                                    </p>
+                                    <p
+                                        v-if="hasUnsavedAcceptanceCriteriaDraft"
+                                        class="text-muted-foreground text-xs"
+                                    >
+                                        Save checklist text changes before toggling completion directly.
                                     </p>
                                     <InputError
                                         :message="acceptanceCriteriaError"
                                     />
+                                    <InputError
+                                        :message="acceptanceCriteriaToggleError"
+                                    />
                                 </div>
 
                                 <div
@@ -643,19 +771,36 @@ const formatDateTime = (value: string): string => {
                                     class="border-border rounded-xl border p-4 text-sm"
                                 >
                                     <p class="mb-3 font-medium">
-                                        Checklist preview
+                                        Saved checklist
                                     </p>
                                     <ul class="space-y-2">
                                         <li
                                             v-for="(item, index) in acceptanceCriteriaPreviewItems"
-                                            :key="index"
+                                            :key="item.id ?? index"
                                             :style="{
                                                 marginLeft: `${item.indent * 1.25}rem`,
                                             }"
                                             class="flex items-start gap-2"
                                         >
-                                            <span class="mt-0.5">{{ item.checked ? '☑' : '☐' }}</span>
-                                            <span>{{ item.text }}</span>
+                                            <input
+                                                :id="`acceptance-criteria-${item.id ?? index}`"
+                                                :checked="item.checked"
+                                                type="checkbox"
+                                                :disabled="
+                                                    !canEditTicket ||
+                                                    form.processing ||
+                                                    isPersistingAcceptanceCriteriaToggle ||
+                                                    hasUnsavedAcceptanceCriteriaDraft
+                                                "
+                                                class="border-input text-primary focus:ring-ring mt-0.5 size-4 rounded"
+                                                @change="onAcceptanceCriteriaCheckboxChange(index, $event)"
+                                            />
+                                            <label
+                                                :for="`acceptance-criteria-${item.id ?? index}`"
+                                                class="flex-1"
+                                            >
+                                                {{ item.text }}
+                                            </label>
                                         </li>
                                     </ul>
                                 </div>

resources/js/types/models/acceptance-criteria-model.ts

@@ -1,4 +1,5 @@
 export type AcceptanceCriteriaItemModel = {
+    id?: number;
     checked: boolean;
     indent: number;
     text: string;

tests/Feature/Projects/ProjectTicketControllerTest.php

@@ -579,6 +579,88 @@ class ProjectTicketControllerTest extends TestCase
         ]);
     }
 
+    public function test_ticket_update_reuses_acceptance_criteria_ids_when_provided(): void
+    {
+        $editor = User::factory()->create();
+        $project = Project::factory()->create([
+            'key' => 'CORE',
+        ]);
+
+        app(InitializeProjectTicketMetadata::class)->handle($project);
+
+        $priority = $project->ticketPriorities()->firstOrFail();
+        $ticketType = $this->nonParentType($project);
+        $ticket = Ticket::factory()->create([
+            'project_id' => $project->id,
+            'sequence_number' => 1,
+            'ticket_key' => 'CORE-1',
+            'ticket_priority_id' => $priority->id,
+            'ticket_type_id' => $ticketType->id,
+            'title' => 'Acceptance criteria update test',
+        ]);
+        $rootCriterion = AcceptanceCriterion::query()->create([
+            'ticket_id' => $ticket->id,
+            'parent_acceptance_criterion_id' => null,
+            'position' => 0,
+            'text' => 'Original root item',
+            'is_checked' => false,
+        ]);
+        $childCriterion = AcceptanceCriterion::query()->create([
+            'ticket_id' => $ticket->id,
+            'parent_acceptance_criterion_id' => $rootCriterion->id,
+            'position' => 0,
+            'text' => 'Original child item',
+            'is_checked' => false,
+        ]);
+
+        $this->syncProjectPermissions($project, $editor, ['write', 'view']);
+
+        $this->actingAs($editor)
+            ->patch(route('projects.tickets.update', [$project, $ticket]), [
+                'acceptance_criteria' => [
+                    [
+                        'id' => $rootCriterion->id,
+                        'text' => 'Updated root item',
+                        'checked' => true,
+                        'indent' => 0,
+                    ],
+                    [
+                        'id' => $childCriterion->id,
+                        'text' => 'Updated child item',
+                        'checked' => false,
+                        'indent' => 1,
+                    ],
+                ],
+                'blocked_comment' => $ticket->blocked_comment,
+                'description' => $ticket->description,
+                'field_values' => [],
+                'is_blocked' => $ticket->is_blocked,
+                'parent_ticket_id' => $ticket->parent_ticket_id,
+                'story_points' => $ticket->story_points,
+                'ticket_priority_id' => $priority->id,
+                'ticket_type_id' => $ticketType->id,
+                'title' => $ticket->title,
+            ])
+            ->assertRedirect(route('projects.tickets.show', [$project, $ticket]));
+
+        $this->assertDatabaseHas('acceptance_criteria', [
+            'id' => $rootCriterion->id,
+            'ticket_id' => $ticket->id,
+            'parent_acceptance_criterion_id' => null,
+            'position' => 0,
+            'text' => 'Updated root item',
+            'is_checked' => true,
+        ]);
+        $this->assertDatabaseHas('acceptance_criteria', [
+            'id' => $childCriterion->id,
+            'ticket_id' => $ticket->id,
+            'parent_acceptance_criterion_id' => $rootCriterion->id,
+            'position' => 0,
+            'text' => 'Updated child item',
+            'is_checked' => false,
+        ]);
+    }
+
     /**
      * Ensure non-parent tickets cannot be assigned as a parent target.
      */