From 3f6f80f7af283f186930a47f640b2393d143a85e Mon Sep 17 00:00:00 2001 From: ahmido Date: Sat, 14 Mar 2026 20:09:54 +0000 Subject: [PATCH] feat: refine onboarding draft flow and RBAC diff UX (#171) ## Summary - add the RBAC role definition diff UX upgrade as the first concrete consumer of the shared diff presentation foundation - refine managed tenant onboarding draft routing, CTA labeling, and cancellation redirect behavior - tighten related Filament and diff rendering regression coverage ## Testing - updated focused Pest coverage for onboarding draft routing and lifecycle behavior - updated focused Pest coverage for shared diff partials and RBAC finding rendering ## Notes - Livewire v4.0+ compliance is preserved within the existing Filament v5 surfaces - provider registration remains unchanged in bootstrap/providers.php - no new Filament assets were added; existing deployment practice still relies on php artisan filament:assets when assets change Co-authored-by: Ahmed Darrazi Reviewed-on: https://git.cloudarix.de/ahmido/TenantAtlas/pulls/171 --- .github/agents/copilot-instructions.md | 4 +- .../ManagedTenantOnboardingWizard.php | 27 +- .../TenantResource/Pages/ListTenants.php | 69 ++++- .../Diff/RbacRoleDefinitionDiffBuilder.php | 205 ++++++++++++ .../ui/shared-diff-presentation-foundation.md | 22 +- .../rbac-role-definition-diff.blade.php | 123 ++------ .../filament/partials/context-bar.blade.php | 2 + .../partials/diff/row-changed.blade.php | 2 +- .../partials/diff/row-unchanged.blade.php | 2 +- .../plan.md | 5 +- .../spec.md | 6 +- .../tasks.md | 3 + .../checklists/requirements.md | 34 ++ .../rbac-finding-diff-view.openapi.yaml | 135 ++++++++ .../data-model.md | 137 ++++++++ .../plan.md | 223 +++++++++++++ .../quickstart.md | 49 +++ .../research.md | 57 ++++ .../spec.md | 126 ++++++++ .../tasks.md | 190 ++++++++++++ .../Filament/CreateCtaPlacementTest.php | 101 ++++++ .../Filament/FindingViewRbacEvidenceTest.php | 292 ++++++++++++++---- .../Monitoring/HeaderContextBarTest.php | 27 ++ .../OnboardingDraftLifecycleTest.php | 9 +- .../Onboarding/OnboardingDraftRoutingTest.php | 69 +++++ .../Support/Diff/SharedDiffRowPartialTest.php | 36 ++- .../Diff/SharedDiffSummaryPartialTest.php | 17 + .../Diff/SharedInlineListDiffPartialTest.php | 20 +- .../RbacRoleDefinitionDiffBuilderTest.php | 212 +++++++++++++ .../Support/Diff/ValueStringifierTest.php | 4 +- 30 files changed, 2011 insertions(+), 197 deletions(-) create mode 100644 app/Support/Diff/RbacRoleDefinitionDiffBuilder.php create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/checklists/requirements.md create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/contracts/rbac-finding-diff-view.openapi.yaml create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/data-model.md create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/plan.md create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/quickstart.md create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/research.md create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/spec.md create mode 100644 specs/142-rbac-role-definition-diff-ux-upgrade/tasks.md create mode 100644 tests/Unit/Support/Diff/RbacRoleDefinitionDiffBuilderTest.php diff --git a/.github/agents/copilot-instructions.md b/.github/agents/copilot-instructions.md index 37ab553..74c529c 100644 --- a/.github/agents/copilot-instructions.md +++ b/.github/agents/copilot-instructions.md @@ -73,6 +73,8 @@ ## Active Technologies - PHP 8.4.15 + Laravel 12, Filament v5, Livewire v4, PostgreSQL, Laravel Sail, Pest v4, existing `OperationRunService`, `ProviderOperationStartGate`, onboarding services, workspace audit logging (140-onboarding-lifecycle-operation-checkpoints-concurrency-mvp) - PostgreSQL tables including `managed_tenant_onboarding_sessions`, `operation_runs`, `tenants`, and provider-connection-backed tenant records (140-onboarding-lifecycle-operation-checkpoints-concurrency-mvp) - PostgreSQL via Laravel Sail for existing source records and JSON payloads; no new persistence introduced (141-shared-diff-presentation-foundation) +- PHP 8.4.15 / Laravel 12 + Filament v5, Livewire v4.0+, Tailwind CSS v4, shared `App\Support\Diff` foundation from Spec 141 (142-rbac-role-definition-diff-ux-upgrade) +- PostgreSQL via Laravel Sail for existing `findings.evidence_jsonb`; no schema or persistence changes (142-rbac-role-definition-diff-ux-upgrade) - PHP 8.4.15 (feat/005-bulk-operations) @@ -92,8 +94,8 @@ ## Code Style PHP 8.4.15: Follow standard conventions ## Recent Changes +- 142-rbac-role-definition-diff-ux-upgrade: Added PHP 8.4.15 / Laravel 12 + Filament v5, Livewire v4.0+, Tailwind CSS v4, shared `App\Support\Diff` foundation from Spec 141 - 141-shared-diff-presentation-foundation: Added PHP 8.4.15 / Laravel 12 + Filament v5, Livewire v4.0+, Tailwind CSS v4 - 140-onboarding-lifecycle-operation-checkpoints-concurrency-mvp: Added PHP 8.4.15 + Laravel 12, Filament v5, Livewire v4, PostgreSQL, Laravel Sail, Pest v4, existing `OperationRunService`, `ProviderOperationStartGate`, onboarding services, workspace audit logging -- 139-verify-access-permissions-assist: Added PHP 8.4 (Laravel 12) + Filament v5 (Livewire v4), Laravel Blade, existing onboarding/verification support classes diff --git a/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php b/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php index 60214f6..f787576 100644 --- a/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php +++ b/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php @@ -147,7 +147,7 @@ protected function getHeaderActions(): array { $actions = []; - if ($this->onboardingSession instanceof TenantOnboardingSession) { + if ($this->shouldShowDraftLandingAction()) { $actions[] = Action::make('back_to_onboarding_landing') ->label('All onboarding drafts') ->color('gray') @@ -921,6 +921,8 @@ private function cancelOnboardingDraft(): void ->title('Onboarding draft cancelled') ->success() ->send(); + + $this->redirect(route('admin.onboarding.draft', ['onboardingDraft' => $this->onboardingSession])); } private function showsNonResumableSummary(): bool @@ -929,6 +931,29 @@ private function showsNonResumableSummary(): bool && ! $this->onboardingSession->isResumable(); } + private function shouldShowDraftLandingAction(): bool + { + if (! $this->onboardingSession instanceof TenantOnboardingSession) { + return false; + } + + if (! $this->onboardingSession->isResumable()) { + return false; + } + + if (! isset($this->workspace)) { + return false; + } + + $user = $this->currentUser(); + + if (! $user instanceof User) { + return false; + } + + return $this->availableDraftsFor($user)->count() > 1; + } + private function draftTitle(TenantOnboardingSession $draft): string { $state = is_array($draft->state) ? $draft->state : []; diff --git a/app/Filament/Resources/TenantResource/Pages/ListTenants.php b/app/Filament/Resources/TenantResource/Pages/ListTenants.php index 6f69899..ff0f6c4 100644 --- a/app/Filament/Resources/TenantResource/Pages/ListTenants.php +++ b/app/Filament/Resources/TenantResource/Pages/ListTenants.php @@ -1,8 +1,14 @@ label('Add tenant') - ->icon('heroicon-m-plus') - ->url(route('admin.onboarding')) + $this->makeOnboardingEntryAction() ->visible(fn (): bool => $this->getTableRecords()->count() > 0), ]; } @@ -24,10 +27,60 @@ protected function getHeaderActions(): array protected function getTableEmptyStateActions(): array { return [ - Actions\Action::make('add_tenant') - ->label('Add tenant') - ->icon('heroicon-m-plus') - ->url(route('admin.onboarding')), + $this->makeOnboardingEntryAction(), ]; } + + private function makeOnboardingEntryAction(): Actions\Action + { + return Actions\Action::make('add_tenant') + ->label($this->onboardingEntryLabel()) + ->icon($this->onboardingEntryIcon()) + ->url(route('admin.onboarding')); + } + + private function onboardingEntryLabel(): string + { + $draftCount = $this->accessibleResumableDraftCount(); + + return match (true) { + $draftCount === 1 => 'Continue onboarding', + $draftCount > 1 => 'Choose onboarding draft', + default => 'Add tenant', + }; + } + + private function onboardingEntryIcon(): string + { + $draftCount = $this->accessibleResumableDraftCount(); + + return match (true) { + $draftCount === 1 => 'heroicon-m-arrow-path', + $draftCount > 1 => 'heroicon-m-queue-list', + default => 'heroicon-m-plus', + }; + } + + private function accessibleResumableDraftCount(): int + { + $user = auth()->user(); + + if (! $user instanceof User) { + return 0; + } + + $workspaceId = app(WorkspaceContext::class)->currentWorkspaceId(request()); + + if (! is_int($workspaceId)) { + return 0; + } + + $workspace = Workspace::query()->whereKey($workspaceId)->first(); + + if (! $workspace instanceof Workspace) { + return 0; + } + + return app(OnboardingDraftResolver::class)->resumableDraftsFor($user, $workspace)->count(); + } } diff --git a/app/Support/Diff/RbacRoleDefinitionDiffBuilder.php b/app/Support/Diff/RbacRoleDefinitionDiffBuilder.php new file mode 100644 index 0000000..6a6ab39 --- /dev/null +++ b/app/Support/Diff/RbacRoleDefinitionDiffBuilder.php @@ -0,0 +1,205 @@ + Display name' => 100, + 'Role definition > Description' => 200, + 'Role definition > Role source' => 300, + 'Role definition > Permission blocks' => 400, + 'Role definition > Scope tag IDs' => 500, + ]; + + private const PERMISSION_FIELD_ORDER = [ + 'Allowed actions' => 100, + 'Denied actions' => 200, + 'Conditions' => 300, + ]; + + public function __construct( + private readonly DiffPresenter $presenter, + ) {} + + /** + * @param array $payload + */ + public function build(array $payload): DiffPresentation + { + $baseline = $this->normalizeSide( + is_array($payload['baseline'] ?? null) ? $payload['baseline'] : [], + ); + $current = $this->normalizeSide( + is_array($payload['current'] ?? null) ? $payload['current'] : [], + ); + $changedKeys = $this->changedKeys($payload); + + $presentation = $this->presenter->present( + baseline: $baseline, + current: $current, + changedKeys: $changedKeys, + labels: $this->labelsFor($baseline, $current, $changedKeys), + ); + + if ($presentation->rows === []) { + return $presentation; + } + + $rows = array_map( + fn (DiffRow $row): DiffRow => $this->normalizeRow($row), + $presentation->rows, + ); + + usort($rows, fn (DiffRow $left, DiffRow $right): int => $this->sortKey($left->key) <=> $this->sortKey($right->key)); + + return new DiffPresentation( + summary: DiffSummary::fromRows($rows, $presentation->summary->message), + rows: $rows, + ); + } + + /** + * @param array $payload + * @return list + */ + private function changedKeys(array $payload): array + { + $changedKeys = is_array($payload['changed_keys'] ?? null) ? $payload['changed_keys'] : []; + + return array_values(array_filter( + $changedKeys, + fn (mixed $key): bool => is_string($key) && trim($key) !== '', + )); + } + + /** + * @param array $side + * @return array + */ + private function normalizeSide(array $side): array + { + $normalized = []; + $rows = is_array($side['normalized'] ?? null) ? $side['normalized'] : []; + + foreach ($rows as $key => $value) { + if (! is_string($key) || trim($key) === '') { + continue; + } + + $normalized[trim($key)] = $value; + } + + if (! array_key_exists('Role definition > Role source', $normalized)) { + $roleSource = $this->roleSourceLabel($side['is_built_in'] ?? null); + + if ($roleSource !== null) { + $normalized['Role definition > Role source'] = $roleSource; + } + } + + if ( + ! array_key_exists('Role definition > Permission blocks', $normalized) + && is_numeric($side['role_permission_count'] ?? null) + ) { + $normalized['Role definition > Permission blocks'] = (int) $side['role_permission_count']; + } + + return $normalized; + } + + private function roleSourceLabel(mixed $isBuiltIn): ?string + { + return match ($isBuiltIn) { + true => 'Built-in', + false => 'Custom', + default => null, + }; + } + + /** + * @param array $baseline + * @param array $current + * @param list $changedKeys + * @return array + */ + private function labelsFor(array $baseline, array $current, array $changedKeys): array + { + $labels = []; + + foreach ([array_keys($baseline), array_keys($current), $changedKeys] as $sourceKeys) { + foreach ($sourceKeys as $key) { + if (! is_string($key) || trim($key) === '') { + continue; + } + + $labels[$key] = $key; + } + } + + return $labels; + } + + private function normalizeRow(DiffRow $row): DiffRow + { + $shouldRenderAsList = $row->isListLike && $this->isDesignatedListField($row->key); + + if ($row->isListLike === $shouldRenderAsList) { + return $row; + } + + return new DiffRow( + key: $row->key, + label: $row->label, + status: $row->status, + oldValue: $row->oldValue, + newValue: $row->newValue, + isListLike: $shouldRenderAsList, + addedItems: $shouldRenderAsList ? $row->addedItems : [], + removedItems: $shouldRenderAsList ? $row->removedItems : [], + unchangedItems: $shouldRenderAsList ? $row->unchangedItems : [], + meta: $row->meta, + ); + } + + private function isDesignatedListField(string $key): bool + { + if ($key === 'Role definition > Scope tag IDs') { + return true; + } + + return preg_match('/^Permission block \d+ > (Allowed actions|Denied actions|Conditions)$/', $key) === 1; + } + + /** + * @return array{int, int, int, string} + */ + private function sortKey(string $key): array + { + if (array_key_exists($key, self::ROLE_FIELD_ORDER)) { + return [0, self::ROLE_FIELD_ORDER[$key], 0, mb_strtolower($key)]; + } + + if (preg_match('/^Role definition > (?P