From a8bdfc5a7744ffe1b481a6053a949eddeb526b95 Mon Sep 17 00:00:00 2001 From: ahmido Date: Fri, 2 Jan 2026 14:33:29 +0000 Subject: [PATCH] feat: always capture policy when adding to backup (#22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: Beim Hinzufügen zu einem Backup Set kann ein lokaler “Reuse” dazu führen, dass ein Backup nicht den aktuellen Intune-Stand reflektiert, wenn last_synced_at nicht frisch ist. Lösung: BackupService führt beim Add immer orchestrated capture aus (Graph Fetch), damit “Backup = current state” gilt. Trotzdem kein unnötiges Version-Wachstum: PolicyCaptureOrchestrator re-used bestehende PolicyVersions via Snapshot-Hash, wenn sich nichts geändert hat. Tests: Added BackupServiceVersionReuseTest.php Specs: Updated spec.md + plan.md + tasks checked off. Co-authored-by: Ahmed Darrazi Reviewed-on: https://git.cloudarix.de/ahmido/TenantAtlas/pulls/22 --- app/Services/Intune/BackupService.php | 60 ++++++-- .../checklists/requirements.md | 9 ++ specs/016-backup-version-reuse/plan.md | 18 +++ specs/016-backup-version-reuse/spec.md | 29 ++++ specs/016-backup-version-reuse/tasks.md | 18 +++ .../Feature/BackupServiceVersionReuseTest.php | 144 ++++++++++++++++++ 6 files changed, 262 insertions(+), 16 deletions(-) create mode 100644 specs/016-backup-version-reuse/checklists/requirements.md create mode 100644 specs/016-backup-version-reuse/plan.md create mode 100644 specs/016-backup-version-reuse/spec.md create mode 100644 specs/016-backup-version-reuse/tasks.md create mode 100644 tests/Feature/BackupServiceVersionReuseTest.php diff --git a/app/Services/Intune/BackupService.php b/app/Services/Intune/BackupService.php index 22abe4c..255e2e1 100644 --- a/app/Services/Intune/BackupService.php +++ b/app/Services/Intune/BackupService.php @@ -5,6 +5,7 @@ use App\Models\BackupItem; use App\Models\BackupSet; use App\Models\Policy; +use App\Models\PolicyVersion; use App\Models\Tenant; use App\Services\AssignmentBackupService; use Carbon\CarbonImmutable; @@ -289,13 +290,46 @@ private function snapshotPolicy( $captured = $captureResult['captured']; $payload = $captured['payload']; $metadata = $captured['metadata'] ?? []; - $metadataWarnings = $captured['warnings'] ?? []; - // Validate snapshot - $validation = $this->snapshotValidator->validate(is_array($payload) ? $payload : []); + return [ + $this->createBackupItemFromVersion( + tenant: $tenant, + backupSet: $backupSet, + policy: $policy, + version: $version, + payload: is_array($payload) ? $payload : [], + assignments: $captured['assignments'] ?? null, + scopeTags: $captured['scope_tags'] ?? null, + metadata: is_array($metadata) ? $metadata : [], + warnings: $captured['warnings'] ?? [], + ), + null, + ]; + } + + /** + * @param array $payload + * @param array $metadata + * @param array $warnings + * @param array{ids:array,names:array}|null $scopeTags + */ + private function createBackupItemFromVersion( + Tenant $tenant, + BackupSet $backupSet, + Policy $policy, + PolicyVersion $version, + array $payload, + ?array $assignments, + ?array $scopeTags, + array $metadata, + array $warnings = [], + ): BackupItem { + $metadataWarnings = $warnings; + + $validation = $this->snapshotValidator->validate($payload); $metadataWarnings = array_merge($metadataWarnings, $validation['warnings']); - $odataWarning = BackupItem::odataTypeWarning(is_array($payload) ? $payload : [], $policy->policy_type, $policy->platform); + $odataWarning = BackupItem::odataTypeWarning($payload, $policy->policy_type, $policy->platform); if ($odataWarning) { $metadataWarnings[] = $odataWarning; @@ -305,29 +339,23 @@ private function snapshotPolicy( $metadata['warnings'] = array_values(array_unique($metadataWarnings)); } - $capturedScopeTags = $captured['scope_tags'] ?? null; - if (is_array($capturedScopeTags)) { - $metadata['scope_tag_ids'] = $capturedScopeTags['ids'] ?? null; - $metadata['scope_tag_names'] = $capturedScopeTags['names'] ?? null; + if (is_array($scopeTags)) { + $metadata['scope_tag_ids'] = $scopeTags['ids'] ?? null; + $metadata['scope_tag_names'] = $scopeTags['names'] ?? null; } - // Create BackupItem as a copy/reference of the PolicyVersion - $backupItem = BackupItem::create([ + return BackupItem::create([ 'tenant_id' => $tenant->id, 'backup_set_id' => $backupSet->id, 'policy_id' => $policy->id, - 'policy_version_id' => $version->id, // Link to version + 'policy_version_id' => $version->id, 'policy_identifier' => $policy->external_id, 'policy_type' => $policy->policy_type, 'platform' => $policy->platform, 'payload' => $payload, 'metadata' => $metadata, - // Copy assignments from version (already captured) - // Note: scope_tags are only stored in PolicyVersion - 'assignments' => $captured['assignments'] ?? null, + 'assignments' => $assignments, ]); - - return [$backupItem, null]; } /** diff --git a/specs/016-backup-version-reuse/checklists/requirements.md b/specs/016-backup-version-reuse/checklists/requirements.md new file mode 100644 index 0000000..9452bf7 --- /dev/null +++ b/specs/016-backup-version-reuse/checklists/requirements.md @@ -0,0 +1,9 @@ +# Specification Quality Checklist: Backup Version Reuse + +**Created**: 2026-01-02 +**Feature**: [spec.md](../spec.md) + +- [x] User story and acceptance scenarios defined +- [x] Requirements are testable and unambiguous +- [x] Scope bounded +- [x] No implementation details required by spec diff --git a/specs/016-backup-version-reuse/plan.md b/specs/016-backup-version-reuse/plan.md new file mode 100644 index 0000000..e123e5b --- /dev/null +++ b/specs/016-backup-version-reuse/plan.md @@ -0,0 +1,18 @@ +# Plan: Backup Version Reuse (016) + +**Branch**: `016-backup-version-reuse` +**Date**: 2026-01-02 +**Input**: [spec.md](./spec.md) + +## Goal +Reduce unnecessary `PolicyVersion` creation when policies are added to backup sets by reusing an existing suitable latest version where safe. + +## Approach +1. Always capture from Intune when a policy is added to a backup set (admin expectation: "backup = current state"). +2. Rely on `PolicyCaptureOrchestrator` snapshot-hash reuse to avoid redundant `PolicyVersion` creation when nothing changed. +3. Still respect capture options (assignments / scope tags) via orchestrator backfill behavior. +4. Add tests for both reuse and capture paths. + +## Out of scope +- UI toggles/config flags unless required. +- Cross-policy dedup or historical compaction. diff --git a/specs/016-backup-version-reuse/spec.md b/specs/016-backup-version-reuse/spec.md new file mode 100644 index 0000000..35756a4 --- /dev/null +++ b/specs/016-backup-version-reuse/spec.md @@ -0,0 +1,29 @@ +# Feature Specification: Backup Version Reuse (016) + +**Feature Branch**: `016-backup-version-reuse` +**Created**: 2026-01-02 +**Status**: Draft + +## User Scenarios & Testing + +### User Story 1 — Avoid unnecessary version growth (Priority: P1) +As an admin, I want adding policies to a backup set to reuse an existing recent policy version when safe, so backups don’t create redundant versions and operations stay fast. + +**Acceptance Scenarios** +1. Given a policy already has an identical captured snapshot, when I add it to a backup set, then the backup item links to the existing version (no new version is created). +2. Given a policy has no suitable version, when I add it to a backup set, then a new version is captured and linked. + +## Requirements + +### Functional Requirements +- **FR-001**: Adding policies to a backup set SHOULD avoid creating redundant `PolicyVersion` records by reusing an existing version when the captured snapshot is unchanged. +- **FR-002**: If reuse is not safe/possible, the system MUST capture a new `PolicyVersion` as it does today. +- **FR-003**: Reuse MUST respect capture options: + - If assignments are requested, the reused version must include assignments. + - If scope tags are requested, the reused version must include scope tags. +- **FR-005**: Adding a policy to a backup set MUST capture from Intune to ensure the backup reflects the current state. +- **FR-004**: Behavior changes MUST be covered by automated tests. + +## Success Criteria +- **SC-001**: Backups avoid creating redundant policy versions in the common case. +- **SC-002**: Backup correctness is preserved (no missing required data for restore/preview). diff --git a/specs/016-backup-version-reuse/tasks.md b/specs/016-backup-version-reuse/tasks.md new file mode 100644 index 0000000..8e85570 --- /dev/null +++ b/specs/016-backup-version-reuse/tasks.md @@ -0,0 +1,18 @@ +# Tasks: Backup Version Reuse (016) + +**Branch**: `016-backup-version-reuse` | **Date**: 2026-01-02 +**Input**: [spec.md](./spec.md), [plan.md](./plan.md) + +## Phase 1: Setup +- [X] T001 Create spec/plan/tasks and checklist. + +## Phase 2: Tests (TDD) +- [X] T002 Add tests for reusing an existing suitable PolicyVersion. +- [X] T003 Add tests for capturing a new PolicyVersion when reuse is not possible. + +## Phase 3: Core +- [X] T004 Implement reuse decision + reuse path in BackupService. + +## Phase 4: Verification +- [X] T005 Run targeted tests. +- [X] T006 Run Pint (`./vendor/bin/pint --dirty`). diff --git a/tests/Feature/BackupServiceVersionReuseTest.php b/tests/Feature/BackupServiceVersionReuseTest.php new file mode 100644 index 0000000..32f2d12 --- /dev/null +++ b/tests/Feature/BackupServiceVersionReuseTest.php @@ -0,0 +1,144 @@ +create(); + $tenant->makeCurrent(); + + $user = User::factory()->create(); + $this->actingAs($user); + + $backupSet = BackupSet::factory()->create([ + 'tenant_id' => $tenant->id, + 'status' => 'completed', + ]); + + $policy = Policy::factory()->create([ + 'tenant_id' => $tenant->id, + 'last_synced_at' => now(), + 'ignored_at' => null, + ]); + + $existingVersion = PolicyVersion::factory()->create([ + 'tenant_id' => $tenant->id, + 'policy_id' => $policy->id, + 'captured_at' => now(), + 'snapshot' => ['id' => $policy->external_id, 'name' => $policy->display_name], + 'assignments' => null, + 'scope_tags' => null, + ]); + + $this->mock(PolicyCaptureOrchestrator::class, function (MockInterface $mock) use ($existingVersion) { + $mock->shouldReceive('capture') + ->once() + ->andReturn([ + 'version' => $existingVersion, + 'captured' => [ + 'payload' => $existingVersion->snapshot, + 'assignments' => $existingVersion->assignments, + 'scope_tags' => $existingVersion->scope_tags, + 'metadata' => [], + ], + ]); + }); + + $service = app(BackupService::class); + + $service->addPoliciesToSet( + tenant: $tenant, + backupSet: $backupSet, + policyIds: [$policy->id], + actorEmail: $user->email, + actorName: $user->name, + includeAssignments: false, + includeScopeTags: false, + includeFoundations: false, + ); + + expect(PolicyVersion::query()->where('policy_id', $policy->id)->count())->toBe(1); + + $item = $backupSet->items()->first(); + expect($item)->not->toBeNull(); + expect($item->policy_version_id)->toBe($existingVersion->id); +}); + +it('captures a new policy version for backup when no suitable existing version is available', function () { + $tenant = Tenant::factory()->create(); + $tenant->makeCurrent(); + + $user = User::factory()->create(); + $this->actingAs($user); + + $backupSet = BackupSet::factory()->create([ + 'tenant_id' => $tenant->id, + 'status' => 'completed', + ]); + + $policy = Policy::factory()->create([ + 'tenant_id' => $tenant->id, + 'last_synced_at' => now(), + 'ignored_at' => null, + ]); + + $staleVersion = PolicyVersion::factory()->create([ + 'tenant_id' => $tenant->id, + 'policy_id' => $policy->id, + 'version_number' => 1, + 'captured_at' => now()->subDays(2), + 'snapshot' => ['id' => $policy->external_id, 'name' => $policy->display_name], + ]); + + $policy->update(['last_synced_at' => now()]); + + $this->mock(PolicyCaptureOrchestrator::class, function (MockInterface $mock) use ($policy, $tenant) { + $mock->shouldReceive('capture') + ->once() + ->andReturnUsing(function () use ($policy, $tenant) { + $newVersion = PolicyVersion::factory()->create([ + 'tenant_id' => $tenant->id, + 'policy_id' => $policy->id, + 'version_number' => 2, + 'captured_at' => now(), + 'snapshot' => ['id' => $policy->external_id, 'name' => $policy->display_name, 'changed' => true], + ]); + + return [ + 'version' => $newVersion, + 'captured' => [ + 'payload' => $newVersion->snapshot, + 'assignments' => null, + 'scope_tags' => null, + 'metadata' => [], + ], + ]; + }); + }); + + $service = app(BackupService::class); + + $service->addPoliciesToSet( + tenant: $tenant, + backupSet: $backupSet, + policyIds: [$policy->id], + actorEmail: $user->email, + actorName: $user->name, + includeAssignments: false, + includeScopeTags: false, + includeFoundations: false, + ); + + $item = $backupSet->items()->first(); + expect($item)->not->toBeNull(); + expect($item->policy_version_id)->not->toBe($staleVersion->id); +});