feat: always capture policy when adding to backup #22
@ -5,6 +5,7 @@
|
|||||||
use App\Models\BackupItem;
|
use App\Models\BackupItem;
|
||||||
use App\Models\BackupSet;
|
use App\Models\BackupSet;
|
||||||
use App\Models\Policy;
|
use App\Models\Policy;
|
||||||
|
use App\Models\PolicyVersion;
|
||||||
use App\Models\Tenant;
|
use App\Models\Tenant;
|
||||||
use App\Services\AssignmentBackupService;
|
use App\Services\AssignmentBackupService;
|
||||||
use Carbon\CarbonImmutable;
|
use Carbon\CarbonImmutable;
|
||||||
@ -289,13 +290,46 @@ private function snapshotPolicy(
|
|||||||
$captured = $captureResult['captured'];
|
$captured = $captureResult['captured'];
|
||||||
$payload = $captured['payload'];
|
$payload = $captured['payload'];
|
||||||
$metadata = $captured['metadata'] ?? [];
|
$metadata = $captured['metadata'] ?? [];
|
||||||
$metadataWarnings = $captured['warnings'] ?? [];
|
|
||||||
|
|
||||||
// Validate snapshot
|
return [
|
||||||
$validation = $this->snapshotValidator->validate(is_array($payload) ? $payload : []);
|
$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<string, mixed> $payload
|
||||||
|
* @param array<string, mixed> $metadata
|
||||||
|
* @param array<int, string> $warnings
|
||||||
|
* @param array{ids:array<int, string>,names:array<int, string>}|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']);
|
$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) {
|
if ($odataWarning) {
|
||||||
$metadataWarnings[] = $odataWarning;
|
$metadataWarnings[] = $odataWarning;
|
||||||
@ -305,29 +339,23 @@ private function snapshotPolicy(
|
|||||||
$metadata['warnings'] = array_values(array_unique($metadataWarnings));
|
$metadata['warnings'] = array_values(array_unique($metadataWarnings));
|
||||||
}
|
}
|
||||||
|
|
||||||
$capturedScopeTags = $captured['scope_tags'] ?? null;
|
if (is_array($scopeTags)) {
|
||||||
if (is_array($capturedScopeTags)) {
|
$metadata['scope_tag_ids'] = $scopeTags['ids'] ?? null;
|
||||||
$metadata['scope_tag_ids'] = $capturedScopeTags['ids'] ?? null;
|
$metadata['scope_tag_names'] = $scopeTags['names'] ?? null;
|
||||||
$metadata['scope_tag_names'] = $capturedScopeTags['names'] ?? null;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Create BackupItem as a copy/reference of the PolicyVersion
|
return BackupItem::create([
|
||||||
$backupItem = BackupItem::create([
|
|
||||||
'tenant_id' => $tenant->id,
|
'tenant_id' => $tenant->id,
|
||||||
'backup_set_id' => $backupSet->id,
|
'backup_set_id' => $backupSet->id,
|
||||||
'policy_id' => $policy->id,
|
'policy_id' => $policy->id,
|
||||||
'policy_version_id' => $version->id, // Link to version
|
'policy_version_id' => $version->id,
|
||||||
'policy_identifier' => $policy->external_id,
|
'policy_identifier' => $policy->external_id,
|
||||||
'policy_type' => $policy->policy_type,
|
'policy_type' => $policy->policy_type,
|
||||||
'platform' => $policy->platform,
|
'platform' => $policy->platform,
|
||||||
'payload' => $payload,
|
'payload' => $payload,
|
||||||
'metadata' => $metadata,
|
'metadata' => $metadata,
|
||||||
// Copy assignments from version (already captured)
|
'assignments' => $assignments,
|
||||||
// Note: scope_tags are only stored in PolicyVersion
|
|
||||||
'assignments' => $captured['assignments'] ?? null,
|
|
||||||
]);
|
]);
|
||||||
|
|
||||||
return [$backupItem, null];
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@ -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
|
||||||
18
specs/016-backup-version-reuse/plan.md
Normal file
18
specs/016-backup-version-reuse/plan.md
Normal file
@ -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.
|
||||||
29
specs/016-backup-version-reuse/spec.md
Normal file
29
specs/016-backup-version-reuse/spec.md
Normal file
@ -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).
|
||||||
18
specs/016-backup-version-reuse/tasks.md
Normal file
18
specs/016-backup-version-reuse/tasks.md
Normal file
@ -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`).
|
||||||
144
tests/Feature/BackupServiceVersionReuseTest.php
Normal file
144
tests/Feature/BackupServiceVersionReuseTest.php
Normal file
@ -0,0 +1,144 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
use App\Models\BackupSet;
|
||||||
|
use App\Models\Policy;
|
||||||
|
use App\Models\PolicyVersion;
|
||||||
|
use App\Models\Tenant;
|
||||||
|
use App\Models\User;
|
||||||
|
use App\Services\Intune\BackupService;
|
||||||
|
use App\Services\Intune\PolicyCaptureOrchestrator;
|
||||||
|
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||||
|
use Mockery\MockInterface;
|
||||||
|
|
||||||
|
uses(RefreshDatabase::class);
|
||||||
|
|
||||||
|
it('reuses latest policy version for backup when it is up-to-date and satisfies capture options', 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,
|
||||||
|
]);
|
||||||
|
|
||||||
|
$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);
|
||||||
|
});
|
||||||
Loading…
Reference in New Issue
Block a user