Fix: Datacenter template assignment defaults - #1277
Merged
Merged
Conversation
Refactors landing-page template assignment behavior so Resource and IGSN datacenter slots are fully independent, including GFZ reassignment to custom templates. Removing a datacenter from a custom template now restores the matching built-in copy template instead of leaving gaps, and copy templates can reclaim assignments. Adds a dedicated datacenter-assignment column resolver, a datacenter observer to enforce persistent default assignments on save/create, and a migration plus seeder updates to backfill missing defaults safely. Frontend messaging/selection behavior and error handling were updated to match, with docs, changelog, and broad Pest/Vitest coverage adjusted for the new rules.
10 tasks
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The seeder repair test is a false positive because the observer repairs its fixtures before the seeder runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors datacenter landing-page template assignments so Resource and IGSN templates remain independent with persistent built-in fallbacks.
Changes:
- Enables flexible GFZ and multi-datacenter assignments.
- Adds observer, migration, and seeder fallback handling.
- Updates UI, documentation, changelog, and coverage.
File summaries
| File | Description |
|---|---|
app/Http/Controllers/LandingPageTemplateController.php |
Synchronizes independent assignments and fallbacks. |
app/Http/Requests/StoreLandingPageTemplateRequest.php |
Removes GFZ assignment restriction. |
app/Http/Requests/UpdateLandingPageTemplateRequest.php |
Removes legacy assignment restrictions. |
app/Models/LandingPageTemplate.php |
Maps template types to assignment columns. |
app/Observers/DatacenterLandingPageTemplateAssignmentObserver.php |
Restores missing default assignments. |
app/Providers/AppServiceProvider.php |
Registers the observer. |
database/migrations/2026_09_08_000001_backfill_default_landing_page_template_assignments.php |
Backfills missing assignments. |
database/seeders/LandingPageTemplateSeeder.php |
Initializes missing defaults for all datacenters. |
docs/pre-release-testing.md |
Updates manual verification steps. |
resources/data/changelog.json |
Documents the assignment fix. |
resources/js/pages/docs.tsx |
Updates user guidance. |
resources/js/pages/landing-page-templates.tsx |
Supports moving and restoring assignments. |
tests/pest/Feature/DatacenterControllerTest.php |
Verifies defaults on creation. |
tests/pest/Feature/LandingPages/DatacenterTemplateAssignmentBackfillMigrationTest.php |
Tests migration behavior. |
tests/pest/Feature/LandingPages/DatacenterTemplateAssignmentTest.php |
Tests independent assignment workflows. |
tests/pest/Feature/LandingPages/LandingPagePublicControllerTest.php |
Tests GFZ template inheritance. |
tests/pest/Feature/LandingPages/LandingPageTemplateControllerTest.php |
Tests assignment-column mapping. |
tests/pest/Feature/Observers/DatacenterLandingPageTemplateAssignmentObserverTest.php |
Tests observer defaults. |
tests/pest/Feature/Seeders/IgsnDatacenterSeederTest.php |
Tests seeder assignment repair. |
tests/pest/Unit/Services/LandingPageTemplateResolverServiceTest.php |
Tests persisted defaults and defensive fallback. |
tests/vitest/pages/__tests__/landing-page-templates.test.tsx |
Tests updated assignment UI. |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Update `IgsnDatacenterSeederTest` to use direct `DB::table(...)` updates when nulling or tweaking template IDs, avoiding model-level side effects during setup. The test now explicitly clears template assignments before reseeding and uses raw updates in fallback scenarios, making the seeder behavior assertions deterministic.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core assignment and fallback behavior across backend logic, data backfilling, observers, and UI, so it warrants a final human pass despite strong test updates.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
app/Observers/DatacenterLandingPageTemplateAssignmentObserver.php:21
Schema::hasColumn(...)is executed every time this observer runs for a Datacenter with a null assignment; during seeding/imports this can result in many information_schema lookups. Consider memoizing the column-existence checks in static variables so the expensive schema check is performed at most once per request/process.
tests/vitest/pages/tests/landing-page-templates.test.tsx:498- The test asserts an exact ordering for
datacenter_ids([11, 10]), but the order is not semantically meaningful and can change based on UI selection order or future sorting, making this assertion brittle. Prefer an order-insensitive matcher (e.g.expect.arrayContaining) or sort before comparing.
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
Avoid repeated `Schema::hasColumn()` lookups when assigning default landing page templates to datacenters by caching column existence in the observer. The tests now cover the one-time-per-process schema check behavior, and the landing-page template frontend test was updated to accept datacenter IDs in any order while still asserting both assignments are sent.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new backfill migration currently hard-fails if built-in templates are missing, which can block migrations in recovery scenarios and should be made idempotent/self-healing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
The backfill migration now calls `LandingPageTemplate::ensureSystemTemplatesExist()` before resolving default template IDs, so missing built-in templates are recreated instead of causing a hard failure. The landing page migration test was updated accordingly to verify a deleted IGSN default template is restored and datacenter template assignments are backfilled with the expected IDs.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavior change is well-covered by updated/new tests, with only a minor docblock wording nit identified.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
app/Http/Controllers/LandingPageTemplateController.php:490
- Docblock says assignments are handled "within its resource scope", but syncDatacenters is type-agnostic (Resource vs IGSN) and uses the template type to select the datacenter foreign key. Updating the wording avoids misleading future readers.
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
Clarifies the landing page template controller documentation for how datacenters are assigned within a template type slot. This update makes the intended behavior explicit: reusing a datacenter moves it to the current template, while removing it from a custom template restores the built-in template default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request significantly refactors how datacenter assignments to landing page templates are managed, making assignments for Resource and IGSN templates fully independent and ensuring that every datacenter always has a persistent default assignment for both types. The changes also simplify validation logic, improve reliability through an observer and migration, and update documentation and user guidance to reflect the new behavior.
Datacenter Assignment Logic Overhaul:
syncDatacentersinLandingPageTemplateControllerto decouple Resource and IGSN template assignments, ensuring that removing a datacenter from a custom template restores the built-in default for that type, and that assignments for the two types are independent. Custom templates can now serve zero, one, or many datacenters. [1] [2]LandingPageTemplate::datacenterAssignmentColumnForTypeto resolve the correct foreign key for each template type, simplifying assignment logic.Validation and Observer Updates:
StoreLandingPageTemplateRequestandUpdateLandingPageTemplateRequestthat previously prevented GFZ from being assigned to custom templates, reflecting the new independent assignment model. [1] [2] [3]DatacenterLandingPageTemplateAssignmentObserverto ensure that every datacenter always retains both built-in template assignments as persistent fallbacks. [1] [2] [3]Database Consistency and Seeding:
Documentation and User Guidance:
Changelog: