From 7805bd057eadfb50875962602d797ff927029c46 Mon Sep 17 00:00:00 2001 From: Koda Date: Tue, 19 May 2026 22:20:15 +0000 Subject: [PATCH] Refactor acceptance criteria into model --- app/Actions/Tickets/CreateProjectTicket.php | 3 +- .../Support/TicketAcceptanceCriteriaData.php | 97 +++++++++++++++---- app/Actions/Tickets/UpdateProjectTicket.php | 27 +++--- app/Http/Resources/TicketResource.php | 6 +- app/Mcp/Support/KanbanGitMcp.php | 6 +- app/Models/AcceptanceCriterion.php | 45 +++++++++ app/Models/Ticket.php | 9 +- ...eptance_criteria_data_to_tickets_table.php | 28 ------ ...03500_create_acceptance_criteria_table.php | 39 ++++++++ .../Projects/ProjectTicketControllerTest.php | 29 +++++- 10 files changed, 217 insertions(+), 72 deletions(-) create mode 100644 app/Models/AcceptanceCriterion.php delete mode 100644 database/migrations/2026_04_24_203500_add_acceptance_criteria_data_to_tickets_table.php create mode 100644 database/migrations/2026_04_24_203500_create_acceptance_criteria_table.php diff --git a/app/Actions/Tickets/CreateProjectTicket.php b/app/Actions/Tickets/CreateProjectTicket.php index 2f8c551..2598012 100644 --- a/app/Actions/Tickets/CreateProjectTicket.php +++ b/app/Actions/Tickets/CreateProjectTicket.php @@ -55,7 +55,6 @@ class CreateProjectTicket $ticket = Ticket::query()->create([ 'acceptance_criteria' => TicketAcceptanceCriteriaData::toLegacyText($acceptanceCriteria), - 'acceptance_criteria_data' => TicketAcceptanceCriteriaData::forStorage($acceptanceCriteria), 'assignee_user_id' => Arr::get($attributes, 'assignee_user_id'), 'author_user_id' => $actor?->id, 'description' => Arr::get($attributes, 'description'), @@ -69,6 +68,8 @@ class CreateProjectTicket 'story_points' => Arr::get($attributes, 'story_points'), ]); + TicketAcceptanceCriteriaData::syncTicket($ticket, $acceptanceCriteria); + $occurredAt = CarbonImmutable::now(); if ($actor !== null) { diff --git a/app/Actions/Tickets/Support/TicketAcceptanceCriteriaData.php b/app/Actions/Tickets/Support/TicketAcceptanceCriteriaData.php index 9a066f3..3378505 100644 --- a/app/Actions/Tickets/Support/TicketAcceptanceCriteriaData.php +++ b/app/Actions/Tickets/Support/TicketAcceptanceCriteriaData.php @@ -2,6 +2,9 @@ namespace App\Actions\Tickets\Support; +use App\Models\AcceptanceCriterion; +use App\Models\Ticket; +use Illuminate\Database\Eloquent\Collection as EloquentCollection; use Illuminate\Support\Arr; class TicketAcceptanceCriteriaData @@ -9,6 +12,7 @@ class TicketAcceptanceCriteriaData public const MAX_INDENT_LEVEL = 8; /** + * @param iterable $criteria * @return array{ * summary: string|null, * items: array * } */ - public static function fromStored(mixed $storedData, ?string $legacyText): array + public static function fromModels(iterable $criteria, ?string $legacyText): array { - $normalizedStoredData = is_array($storedData) - ? self::normalize($storedData) - : self::empty(); + $summary = self::normalize($legacyText)['summary']; + $models = ($criteria instanceof EloquentCollection ? $criteria : collect($criteria)) + ->sort(function (AcceptanceCriterion $left, AcceptanceCriterion $right): int { + return [ + $left->parent_acceptance_criterion_id ?? 0, + $left->position, + $left->id, + ] <=> [ + $right->parent_acceptance_criterion_id ?? 0, + $right->position, + $right->id, + ]; + }) + ->values(); - if (self::hasContent($normalizedStoredData)) { - return $normalizedStoredData; + if ($models->isEmpty()) { + return self::normalize($legacyText); } - return self::normalize($legacyText); + /** @var array> $grouped */ + $grouped = $models->groupBy('parent_acceptance_criterion_id')->all(); + $items = []; + + self::flattenItems($items, $grouped, null, 0); + + return [ + 'summary' => $summary, + 'items' => $items, + ]; } /** @@ -71,6 +95,7 @@ class TicketAcceptanceCriteriaData } /** + * @param Ticket $ticket * @param array{ * summary: string|null, * items: array * } $acceptanceCriteria - * @return array{ - * summary: string|null, - * items: array - * }|null */ - public static function forStorage(array $acceptanceCriteria): ?array + public static function syncTicket(Ticket $ticket, array $acceptanceCriteria): void { $normalized = self::normalize($acceptanceCriteria); - return self::hasContent($normalized) - ? $normalized - : null; + $ticket->acceptanceCriteria()->delete(); + + if ($normalized['items'] === []) { + return; + } + + $parentStack = []; + $positionsByParent = []; + + foreach ($normalized['items'] 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'); } /** @@ -154,6 +196,23 @@ class TicketAcceptanceCriteriaData ]; } + /** + * @param array $items + * @param array> $grouped + */ + private static function flattenItems(array &$items, array $grouped, ?int $parentId, int $indent): void + { + foreach ($grouped[$parentId] ?? [] as $criterion) { + $items[] = [ + 'checked' => $criterion->is_checked, + 'indent' => $indent, + 'text' => $criterion->text, + ]; + + self::flattenItems($items, $grouped, $criterion->id, $indent + 1); + } + } + /** * @param array $value * @return array{ diff --git a/app/Actions/Tickets/UpdateProjectTicket.php b/app/Actions/Tickets/UpdateProjectTicket.php index 9b21690..cf83e3c 100644 --- a/app/Actions/Tickets/UpdateProjectTicket.php +++ b/app/Actions/Tickets/UpdateProjectTicket.php @@ -52,9 +52,12 @@ class UpdateProjectTicket array $validated, ): bool { $fieldValueAttributes = Arr::get($validated, 'field_values', []); + $nextAcceptanceCriteria = TicketAcceptanceCriteriaData::normalize( + Arr::get($validated, 'acceptance_criteria'), + ); $ticketAttributes = $this->buildTicketAttributes($ticket, $validated); - return DB::transaction(function () use ($actor, $fieldValueAttributes, $ticket, $ticketAttributes): bool { + return DB::transaction(function () use ($actor, $fieldValueAttributes, $nextAcceptanceCriteria, $ticket, $ticketAttributes): bool { if (! $ticketAttributes['is_blocked']) { $ticketAttributes['blocked_comment'] = null; } @@ -62,6 +65,7 @@ class UpdateProjectTicket $ticket->loadMissing([ 'assignee', 'author', + 'acceptanceCriteria', 'fieldValues.ticketFieldDefinition', 'fieldValues.userValue', 'parentTicket', @@ -85,14 +89,10 @@ class UpdateProjectTicket /** @var Collection $existingFieldValues */ $existingFieldValues = $ticket->fieldValues ->keyBy('ticket_field_definition_id'); - $currentAcceptanceCriteria = TicketAcceptanceCriteriaData::fromStored( - $ticket->acceptance_criteria_data, + $currentAcceptanceCriteria = TicketAcceptanceCriteriaData::fromModels( + $ticket->acceptanceCriteria, $ticket->acceptance_criteria, ); - $nextAcceptanceCriteria = TicketAcceptanceCriteriaData::fromStored( - $ticketAttributes['acceptance_criteria_data'], - $ticketAttributes['acceptance_criteria'], - ); $changes = []; $summaryLabels = []; @@ -198,6 +198,10 @@ class UpdateProjectTicket $ticket->save(); } + if ($currentAcceptanceCriteria !== $nextAcceptanceCriteria) { + TicketAcceptanceCriteriaData::syncTicket($ticket, $nextAcceptanceCriteria); + } + $fieldValueChangeSet = $this->syncProjectTicketFieldValues->handle( $ticket, $fieldDefinitions, @@ -252,14 +256,6 @@ class UpdateProjectTicket * } $validated * @return array{ * acceptance_criteria: string|null, - * acceptance_criteria_data: array{ - * summary: string|null, - * items: array - * }|null, * assignee_user_id: int|null, * author_user_id: int|null, * blocked_comment: string|null, @@ -280,7 +276,6 @@ class UpdateProjectTicket return [ 'acceptance_criteria' => TicketAcceptanceCriteriaData::toLegacyText($acceptanceCriteria), - 'acceptance_criteria_data' => TicketAcceptanceCriteriaData::forStorage($acceptanceCriteria), 'assignee_user_id' => Arr::exists($validated, 'assignee_user_id') ? Arr::get($validated, 'assignee_user_id') : $ticket->assignee_user_id, diff --git a/app/Http/Resources/TicketResource.php b/app/Http/Resources/TicketResource.php index e4dee66..1b38660 100644 --- a/app/Http/Resources/TicketResource.php +++ b/app/Http/Resources/TicketResource.php @@ -17,8 +17,10 @@ class TicketResource extends JsonResource { return [ 'id' => $this->id, - 'acceptance_criteria' => TicketAcceptanceCriteriaData::fromStored( - $this->acceptance_criteria_data, + 'acceptance_criteria' => TicketAcceptanceCriteriaData::fromModels( + $this->relationLoaded('acceptanceCriteria') + ? $this->acceptanceCriteria + : $this->acceptanceCriteria()->get(), $this->acceptance_criteria, ), 'acceptance_criteria_text' => $this->acceptance_criteria, diff --git a/app/Mcp/Support/KanbanGitMcp.php b/app/Mcp/Support/KanbanGitMcp.php index 6f0acec..85444a4 100644 --- a/app/Mcp/Support/KanbanGitMcp.php +++ b/app/Mcp/Support/KanbanGitMcp.php @@ -227,8 +227,10 @@ class KanbanGitMcp public function updatePayload(Ticket $ticket): array { return [ - 'acceptance_criteria' => TicketAcceptanceCriteriaData::fromStored( - $ticket->acceptance_criteria_data, + 'acceptance_criteria' => TicketAcceptanceCriteriaData::fromModels( + $ticket->relationLoaded('acceptanceCriteria') + ? $ticket->acceptanceCriteria + : $ticket->acceptanceCriteria()->get(), $ticket->acceptance_criteria, ), 'assignee_user_id' => $ticket->assignee_user_id, diff --git a/app/Models/AcceptanceCriterion.php b/app/Models/AcceptanceCriterion.php new file mode 100644 index 0000000..e1b112d --- /dev/null +++ b/app/Models/AcceptanceCriterion.php @@ -0,0 +1,45 @@ + + */ + protected function casts(): array + { + return [ + 'is_checked' => 'boolean', + ]; + } + + public function children(): HasMany + { + return $this->hasMany(self::class, 'parent_acceptance_criterion_id') + ->orderBy('position') + ->orderBy('id'); + } + + public function parent(): BelongsTo + { + return $this->belongsTo(self::class, 'parent_acceptance_criterion_id'); + } + + public function ticket(): BelongsTo + { + return $this->belongsTo(Ticket::class); + } +} diff --git a/app/Models/Ticket.php b/app/Models/Ticket.php index 5936e56..d82a367 100644 --- a/app/Models/Ticket.php +++ b/app/Models/Ticket.php @@ -11,7 +11,6 @@ use Illuminate\Database\Eloquent\Relations\HasMany; #[Fillable([ 'acceptance_criteria', - 'acceptance_criteria_data', 'assignee_user_id', 'author_user_id', 'blocked_comment', @@ -38,11 +37,17 @@ class Ticket extends Model protected function casts(): array { return [ - 'acceptance_criteria_data' => 'array', 'is_blocked' => 'boolean', ]; } + public function acceptanceCriteria(): HasMany + { + return $this->hasMany(AcceptanceCriterion::class) + ->orderBy('position') + ->orderBy('id'); + } + public function assignee(): BelongsTo { return $this->belongsTo(User::class, 'assignee_user_id'); diff --git a/database/migrations/2026_04_24_203500_add_acceptance_criteria_data_to_tickets_table.php b/database/migrations/2026_04_24_203500_add_acceptance_criteria_data_to_tickets_table.php deleted file mode 100644 index d74bd70..0000000 --- a/database/migrations/2026_04_24_203500_add_acceptance_criteria_data_to_tickets_table.php +++ /dev/null @@ -1,28 +0,0 @@ -json('acceptance_criteria_data')->nullable(); - }); - } - - /** - * Reverse the migrations. - */ - public function down(): void - { - Schema::table('tickets', function (Blueprint $table): void { - $table->dropColumn('acceptance_criteria_data'); - }); - } -}; diff --git a/database/migrations/2026_04_24_203500_create_acceptance_criteria_table.php b/database/migrations/2026_04_24_203500_create_acceptance_criteria_table.php new file mode 100644 index 0000000..d073fdc --- /dev/null +++ b/database/migrations/2026_04_24_203500_create_acceptance_criteria_table.php @@ -0,0 +1,39 @@ +id(); + $table->foreignId('ticket_id') + ->constrained() + ->cascadeOnDelete(); + $table->foreignId('parent_acceptance_criterion_id') + ->nullable() + ->constrained('acceptance_criteria') + ->cascadeOnDelete(); + $table->unsignedInteger('position')->default(0); + $table->text('text'); + $table->boolean('is_checked')->default(false); + $table->timestamps(); + + $table->index(['ticket_id', 'parent_acceptance_criterion_id', 'position']); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::dropIfExists('acceptance_criteria'); + } +}; diff --git a/tests/Feature/Projects/ProjectTicketControllerTest.php b/tests/Feature/Projects/ProjectTicketControllerTest.php index 696367a..7dc0ba7 100644 --- a/tests/Feature/Projects/ProjectTicketControllerTest.php +++ b/tests/Feature/Projects/ProjectTicketControllerTest.php @@ -527,9 +527,34 @@ class ProjectTicketControllerTest extends TestCase 'ticket_priority_id' => $highPriority->id, ]); - $ticket->refresh(); + $ticket->refresh()->load('acceptanceCriteria'); - $this->assertEquals($updatedAcceptanceCriteria, $ticket->acceptance_criteria_data); + $this->assertEquals( + $updatedAcceptanceCriteria, + \App\Actions\Tickets\Support\TicketAcceptanceCriteriaData::fromModels( + $ticket->acceptanceCriteria, + $ticket->acceptance_criteria, + ), + ); + $this->assertDatabaseHas('acceptance_criteria', [ + 'ticket_id' => $ticket->id, + 'parent_acceptance_criterion_id' => null, + 'position' => 0, + 'text' => 'Shows success banner', + 'is_checked' => false, + ]); + $rootCriterionId = \App\Models\AcceptanceCriterion::query() + ->where('ticket_id', $ticket->id) + ->where('text', 'Shows success banner') + ->value('id'); + $this->assertNotNull($rootCriterionId); + $this->assertDatabaseHas('acceptance_criteria', [ + 'ticket_id' => $ticket->id, + 'parent_acceptance_criterion_id' => $rootCriterionId, + 'position' => 0, + 'text' => 'Includes nested confirmations', + 'is_checked' => true, + ]); $this->assertDatabaseHas('ticket_field_values', [ 'ticket_id' => $ticket->id, 'ticket_field_definition_id' => $reviewerField->id,