Skip to content

refactor: convert FQCN morph types to aliases#187

Merged
gvieira18 merged 11 commits into4.xfrom
refactor/morph-types-to-aliases
Mar 21, 2026
Merged

refactor: convert FQCN morph types to aliases#187
gvieira18 merged 11 commits into4.xfrom
refactor/morph-types-to-aliases

Conversation

@danielhe4rt
Copy link
Contributor

@danielhe4rt danielhe4rt commented Mar 21, 2026

Summary

  • Add migrations to deduplicate external identities and convert morph types to aliases
  • Update all modules (identity, events, gamification, bot-discord, economy, activity) to use morph aliases instead of FQCN

Changes

  • database: 3 new migrations for morph type normalization
  • identity: update polymorphic morph types to use aliases
  • events/gamification: use morph aliases for polymorphic relations
  • bot-discord/economy/activity: update morph types across modules

Summary by CodeRabbit

  • Bug Fixes

    • Deduplicated external identities, merged related messages and experience, and removed orphaned user accounts
    • Normalized polymorphic type identifiers to prevent provider-resolution mismatches
  • Chores

    • Registered explicit polymorphic mappings and standardized morph-type handling across the app
    • Switched test/runtime setup to PostgreSQL and updated CI/workflow/test scripts
    • Added migrations to convert morph types and adjust schema; bumped tooling/package versions
  • Other

    • Enforced unique usernames in test data generation and minor README text update

@coderabbitai
Copy link

coderabbitai bot commented Mar 21, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5ab81d93-95fc-4f2e-a3c6-4c260888e340

📥 Commits

Reviewing files that changed from the base of the PR and between 911a805 and 539fcbe.

📒 Files selected for processing (1)
  • README.md
✅ Files skipped from review due to trivial changes (1)
  • README.md

📝 Walkthrough

Walkthrough

Replaced stored and queried polymorphic model type FQCNs with Eloquent morph aliases by using (new Model)->getMorphClass() across actions, factories, events, commands, tests, and seeders. Registered explicit morph maps and enabled requireMorphMap in service providers. Converted two enum cases to alias strings. Added migrations to normalize morph types to aliases, deduplicate external identities, and remove/merge orphan users. Switched test/database tooling to PostgreSQL (CI workflow, phpunit, .env.testing.example), updated test running targets (Makefile, composer scripts), and bumped several dependency versions.

Possibly related PRs

Suggested reviewers

  • danielhe4rt
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor: convert FQCN morph types to aliases' accurately and specifically describes the main change: converting fully-qualified class names to aliases for polymorphic relations across the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can suggest fixes for GitHub Check annotations.

Configure the reviews.tools.github-checks setting to adjust the time to wait for GitHub Checks to complete.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (4)
database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php (2)

62-77: Consider wrapping the migration in a transaction for atomicity.

The migration updates multiple tables sequentially. If it fails midway, the database will be left in a partially migrated state where some tables have aliases and others still have FQCNs. This could cause runtime errors since the application code expects consistent morph types.

♻️ Proposed fix to add transaction
     public function up(): void
     {
+        DB::transaction(function (): void {
         foreach (self::TABLES as $table => $column) {
             if (! Schema::hasTable($table)) {
                 continue;
             }

             foreach (self::MORPH_MAP as $alias => $fqcns) {
                 foreach ($fqcns as $fqcn) {
                     DB::table($table)
                         ->where($column, $fqcn)
                         ->update([$column => $alias]);
                 }
             }
         }
+        });
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php`
around lines 62 - 77, Wrap the entire up() migration work (the loops over
self::TABLES and self::MORPH_MAP) in a DB transaction so the multi-table updates
are atomic: either use DB::transaction(...) around the existing logic or
explicitly call DB::beginTransaction(), run the
DB::table(...)->where(...)->update(...) loops, then DB::commit(), and on
exception DB::rollBack() and rethrow; keep identifiers TABLES, MORPH_MAP, up(),
Schema::hasTable and DB::table(...) unchanged so the same update logic runs
inside the transaction.

79-102: The down() method should also be wrapped in a transaction for consistency.

Same reasoning as the up() method—partial rollback would leave inconsistent morph types.

♻️ Proposed fix
     public function down(): void
     {
+        DB::transaction(function (): void {
         $reverseMap = [
             'user' => User::class,
             // ... rest of the map
         ];

         foreach (self::TABLES as $table => $column) {
             // ...
         }
+        });
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php`
around lines 79 - 102, Wrap the down() migration in a DB transaction to avoid
partial updates: open a DB::transaction closure around the existing loop that
iterates self::TABLES and applies the $reverseMap updates so all updates either
commit or rollback together. Ensure you keep the Schema::hasTable checks and the
inner DB::table(...)->where(...)->update(...) calls unchanged inside the
transaction; reference the down() method, self::TABLES, $reverseMap,
Schema::hasTable and DB::table to locate where to wrap the transaction.
database/migrations/2026_03_21_200000_cleanup_orphan_users.php (1)

10-115: Wrap the entire migration in a transaction for data integrity.

This migration performs complex multi-phase operations across numerous tables. A failure in Phase 2 or 3 would leave the database in an inconsistent state where some data has been merged/reassigned but orphan records remain partially deleted.

♻️ Proposed fix
     public function up(): void
     {
+        DB::transaction(function (): void {
         // Build orphan→kept user mapping from dedup pairs
         $mappings = DB::select('
             ...
         ');
         
         // ... rest of migration logic ...
         
         DB::table('users')
             ->whereIn('id', $orphanUserIds)
             ->delete();
+        });
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@database/migrations/2026_03_21_200000_cleanup_orphan_users.php` around lines
10 - 115, The up() migration must run inside a DB transaction to ensure all
phases are atomic; wrap the existing logic in up() in a transaction (either by
returning DB::transaction(function () { ... }) around the current body or by
calling DB::beginTransaction(), running the existing steps, DB::commit() and
DB::rollBack() in a try/catch that rethrows on error). Ensure you reference the
same function up() and keep all current DB::* calls (e.g., DB::select,
DB::table(...)->update/delete) inside the transactional block so any exception
rolls back all changes.
database/migrations/2026_03_21_192332_deduplicate_external_identities.php (1)

14-29: Length-based duplicate detection works correctly but relies on implicit assumptions.

The query correctly identifies old vs new identity rows for the User model type (double-backslash variant = 25 characters, single-backslash = 22 characters, mathematically verified). However, the approach is fragile because it:

  1. Uses magic numbers (25, 22) without validation that only these lengths exist in the data
  2. Lacks explicit documentation of the FQCN pattern assumption
  3. Would break if the User namespace depth changed

Since external_identities currently only stores User model types, the detection is accurate in practice. Consider adding a comment documenting the expected FQCN variants and their lengths, or refactoring to match explicit patterns like 'He4rt%User%Models%User' for better maintainability.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@database/migrations/2026_03_21_192332_deduplicate_external_identities.php`
around lines 14 - 29, The SELECT in the migration uses magic lengths (25/22) to
distinguish User FQCN variants—replace the fragile length checks with explicit
pattern matching or document the assumption: update the query that populates
$duplicates (in this migration) to use explicit model_type patterns (e.g. LIKE
patterns matching the current FQCN variants for User) instead of LENGTH(...) and
add a short inline comment explaining the expected FQCN variants and why those
patterns are used so future namespace changes won’t break silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php`:
- Around line 51-59: The tenant lookup using ExternalIdentity->first() can
return null and the subsequent dereference of $this->tenantProvider->tenant_id
in the memberProvider query will crash; update the tenant query to either call
firstOrFail() on ExternalIdentity (so it throws a clear exception) or explicitly
check for null after the first() call and handle it (e.g., throw a descriptive
exception or return early) before building $this->memberProvider; refer to the
ExternalIdentity query that assigns $this->tenantProvider and the subsequent
$this->memberProvider assignment when applying the fix.

In
`@app-modules/identity/database/migrations/2025_11_02_171945_add_polymorphic_fields_to_providers_table.php`:
- Line 18: The conversion migration that backfills class aliases must also
convert existing values in the providers.model_type column (not just handle
fresh installs) — update the alias conversion migration to include the providers
table and run a safe UPDATE on providers setting model_type = <alias> where
model_type = <FQCN> (handle NULLs and only touch known mappings), referencing
the migration that adds polymorphic fields
(add_polymorphic_fields_to_providers_table) and the providers.model_type column
so you update legacy FQCN entries to their alias equivalents as part of the
migration.

In `@app/Providers/AppServiceProvider.php`:
- Around line 44-48: The configureDatabase call enables
Relation::requireMorphMap but the Economy module never registers polymorphic
types, causing ClassMorphViolationException for models like Wallet and
Transaction; in the Economy service provider (or where models are bootstrapped)
add a Relation::morphMap registration that maps the morph keys to the concrete
classes (e.g., map 'wallet' => Wallet::class and 'transaction' =>
Transaction::class), and audit polymorphic usages (morphTo, morphMany(owner),
etc.) to ensure each morph name used by Wallet, Transaction or related models
has a corresponding entry in the morphMap so Relation::requireMorphMap is
satisfied.

In `@database/migrations/2026_03_21_192332_deduplicate_external_identities.php`:
- Around line 44-48: The increment on the characters table can silently no-op if
the kept user has no character row; instead, before calling
DB::table('characters')->where('user_id',
$dup->new_user_id)->increment('experience', $oldXp), check for an existing
character for $dup->new_user_id (e.g., query for a row or use an upsert), and if
none exists create a new character record with the appropriate initial fields
including 'experience' set to $oldXp, otherwise increment as before; also emit a
warning/log entry referencing $dup->new_user_id and $oldXp when creating the
missing character so reviewers can audit the migration.

In `@database/migrations/2026_03_21_200000_cleanup_orphan_users.php`:
- Around line 32-39: The PostgreSQL-only cast 'users.id::text' inside the
whereNotExists closure causes portability issues; update the comparison in the
closure used to build $orphanUserIds to remove the ::text cast and compare
columns directly (e.g., use whereColumn('external_identities.model_id',
'users.id') or DB::raw('users.id')) so the query is grammar-agnostic, and ensure
external_identities.model_id is treated as a string at the PHP/model level if
necessary.

---

Nitpick comments:
In `@database/migrations/2026_03_21_192332_deduplicate_external_identities.php`:
- Around line 14-29: The SELECT in the migration uses magic lengths (25/22) to
distinguish User FQCN variants—replace the fragile length checks with explicit
pattern matching or document the assumption: update the query that populates
$duplicates (in this migration) to use explicit model_type patterns (e.g. LIKE
patterns matching the current FQCN variants for User) instead of LENGTH(...) and
add a short inline comment explaining the expected FQCN variants and why those
patterns are used so future namespace changes won’t break silently.

In `@database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php`:
- Around line 62-77: Wrap the entire up() migration work (the loops over
self::TABLES and self::MORPH_MAP) in a DB transaction so the multi-table updates
are atomic: either use DB::transaction(...) around the existing logic or
explicitly call DB::beginTransaction(), run the
DB::table(...)->where(...)->update(...) loops, then DB::commit(), and on
exception DB::rollBack() and rethrow; keep identifiers TABLES, MORPH_MAP, up(),
Schema::hasTable and DB::table(...) unchanged so the same update logic runs
inside the transaction.
- Around line 79-102: Wrap the down() migration in a DB transaction to avoid
partial updates: open a DB::transaction closure around the existing loop that
iterates self::TABLES and applies the $reverseMap updates so all updates either
commit or rollback together. Ensure you keep the Schema::hasTable checks and the
inner DB::table(...)->where(...)->update(...) calls unchanged inside the
transaction; reference the down() method, self::TABLES, $reverseMap,
Schema::hasTable and DB::table to locate where to wrap the transaction.

In `@database/migrations/2026_03_21_200000_cleanup_orphan_users.php`:
- Around line 10-115: The up() migration must run inside a DB transaction to
ensure all phases are atomic; wrap the existing logic in up() in a transaction
(either by returning DB::transaction(function () { ... }) around the current
body or by calling DB::beginTransaction(), running the existing steps,
DB::commit() and DB::rollBack() in a try/catch that rethrows on error). Ensure
you reference the same function up() and keep all current DB::* calls (e.g.,
DB::select, DB::table(...)->update/delete) inside the transactional block so any
exception rolls back all changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9991e42a-4fdf-47b9-92e6-84d5931fb48f

📥 Commits

Reviewing files that changed from the base of the PR and between 6f151f3 and 59bb39a.

📒 Files selected for processing (22)
  • app-modules/activity/src/Actions/NewMessage.php
  • app-modules/bot-discord/src/Events/MessageReceivedEvent.php
  • app-modules/bot-discord/src/Events/WelcomeMember.php
  • app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php
  • app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php
  • app-modules/economy/database/factories/WalletFactory.php
  • app-modules/economy/tests/Feature/EconomyFlowTest.php
  • app-modules/events/database/factories/EventAgendaFactory.php
  • app-modules/events/src/Enums/SchedulableTypeEnum.php
  • app-modules/events/src/Providers/EventsServiceProvider.php
  • app-modules/gamification/src/Providers/GamificationServiceProvider.php
  • app-modules/identity/database/factories/ExternalIdentityFactory.php
  • app-modules/identity/database/migrations/2025_11_02_171945_add_polymorphic_fields_to_providers_table.php
  • app-modules/identity/src/ExternalIdentity/Actions/CreateAccountByExternalIdentity.php
  • app-modules/identity/src/Providers/IdentityServiceProvider.php
  • app-modules/identity/src/User/Actions/UpdateProfile.php
  • app-modules/identity/tests/Feature/Auth/AuthenticateActionTest.php
  • app/Providers/AppServiceProvider.php
  • database/migrations/2026_03_21_192332_deduplicate_external_identities.php
  • database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php
  • database/migrations/2026_03_21_200000_cleanup_orphan_users.php
  • database/seeders/ThreeDotsSeeder.php

gvieira18
gvieira18 previously approved these changes Mar 21, 2026
Clintonrocha98
Clintonrocha98 previously approved these changes Mar 21, 2026
Copy link
Collaborator

@Clintonrocha98 Clintonrocha98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

- Add PostgreSQL 18 service with dynamic port mapping
- Create test_he4rtbot database automatically
- Inject DB_PORT per-step to override phpunit.xml default
- Add pdo_pgsql to PHP extensions for CI runner
- Switch from SQLite :memory: to production-parity testing
- Update phpunit.xml to use pgsql connection
- Configure PostgreSQL credentials: test_he4rtbot database
- Update .env.testing.example to match docker-compose setup
- Ensure production parity for test migrations and queries
- Remove --parallel flag from test/test-unit/test-feature in Makefile
- Remove --parallel flag from test scripts in composer.json
- Add setup-test-db target for manual database creation
- Parallel testing reserved for CI only (--ci --parallel)
- Local runs use serial mode for simpler debugging
@gvieira18 gvieira18 dismissed stale reviews from Clintonrocha98 and themself via 393e2fe March 21, 2026 20:21
- Bump Filament packages to v5.4.1
- Update Laravel framework to v12.55.1 and Nightwatch to v1.24.4
- Upgrade pestphp/pest and related testing tools
- Update webmozart/assert to v2.1.6
- Bump various JS dependencies (tailwindcss, prettier-plugin-blade)
- Refresh composer.lock with new dependency versions
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
Makefile (1)

96-98: Consider validating the file argument.

If make import-db is invoked without the file variable, pg_restore will fail with an unclear error. A guard would improve usability.

🔧 Proposed validation
 .PHONY: import-db
 import-db: ## Import a PostgreSQL dump file (usage: make import-db file=path/to/dump)
+ifndef file
+	$(error Usage: make import-db file=path/to/dump)
+endif
 	`@PGHOST`=localhost PGUSER=postgres PGPASSWORD=postgres pg_restore -x -O -cC -j 8 -d postgres $(file)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 96 - 98, The import-db Make target invokes pg_restore
with $(file) but doesn't validate that the file variable was provided; add a
guard at the start of the import-db recipe to check that the $(file) variable is
non-empty and fail with a clear message if missing (e.g., echo "Usage: make
import-db file=path/to/dump" && exit 1), so update the import-db target (which
runs pg_restore) to validate $(file) before calling pg_restore and print a
helpful error when it's not supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@Makefile`:
- Around line 96-98: The import-db Make target invokes pg_restore with $(file)
but doesn't validate that the file variable was provided; add a guard at the
start of the import-db recipe to check that the $(file) variable is non-empty
and fail with a clear message if missing (e.g., echo "Usage: make import-db
file=path/to/dump" && exit 1), so update the import-db target (which runs
pg_restore) to validate $(file) before calling pg_restore and print a helpful
error when it's not supplied.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d87b07f1-b3bd-4faa-adf7-528bab3d554e

📥 Commits

Reviewing files that changed from the base of the PR and between 59bb39a and 393e2fe.

📒 Files selected for processing (6)
  • .env.testing.example
  • .github/workflows/_pest.yml
  • .github/workflows/continuous-integration.yml
  • Makefile
  • composer.json
  • phpunit.xml

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@composer.json`:
- Around line 15-16: The composer.json contains package constraints that don't
exist on Packagist; update the version constraints for the specific packages
named in the diff: change "pestphp/pest" to an existing release (e.g. ^4.4.2),
change "filament/spatie-laravel-media-library-plugin" to the published v5.4.0
(e.g. ^5.4.0), and change "owenvoke/blade-fontawesome" to the latest available
on Packagist (e.g. ^3.2.0) or remove/hold that dependency until the upstream
release is available, then run composer update to verify installs succeed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8cc67b73-99e6-4934-9b6c-a00efeec6188

📥 Commits

Reviewing files that changed from the base of the PR and between 393e2fe and 3131a6d.

⛔ Files ignored due to path filters (2)
  • composer.lock is excluded by !**/*.lock
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • composer.json
  • package.json
✅ Files skipped from review due to trivial changes (1)
  • package.json

- Set $keyType to 'string' in Badge and Sponsor models for UUID support
- Update tests to use tenant model keys instead of hardcoded IDs
- Allow badgeId to be string or int in PersistClaimedBadge handler
- Adjust event attendee pivot updates to use attendee keys
- Ensure unique usernames in UserFactory
- Add migration to relax country/state column lengths in user_address
@gvieira18
Copy link
Member

Lessons learned from 911a805: "Never use SQLite as a test database unless the production database is also SQLite."

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php (2)

120-122: ⚠️ Potential issue | 🟡 Minor

Inconsistent use of ->id vs ->getKey().

Line 98 was updated to use auth()->user()->getKey(), but line 122 still uses auth()->user()->id. For consistency and UUID support, update this to use getKey().

Proposed fix
     expect($event->attendees()->count())->toBe(5)
         ->and($event->fresh()->waitlist_count)->toBe(1)
-        ->and($event->isParticipating(auth()->user()->id))->toBeTrue();
+        ->and($event->isParticipating(auth()->user()->getKey()))->toBeTrue();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php`
around lines 120 - 122, The assertion uses auth()->user()->id which is
inconsistent with the earlier change to auth()->user()->getKey(); update the
call passed into Event::isParticipating to use auth()->user()->getKey() instead
of ->id so UUIDs work and calls are consistent—locate the assertion chain
containing
expect($event->attendees()->count())->toBe(5)->and($event->fresh()->waitlist_count)->toBe(1)->and($event->isParticipating(...))
and replace the inner auth()->user()->id with auth()->user()->getKey().

140-141: ⚠️ Potential issue | 🟡 Minor

Same inconsistency — use getKey() for consistency.

Proposed fix
     expect($event->attendees()->count())->toBe(4)
-        ->and($event->isParticipating(auth()->user()->id))->tobeFalse();
+        ->and($event->isParticipating(auth()->user()->getKey()))->toBeFalse();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php`
around lines 140 - 141, The assertion uses auth()->user()->id inconsistently;
change the call to use getKey() for consistency by replacing auth()->user()->id
with auth()->user()->getKey() in the assertion that calls isParticipating() (the
line chaining
expect($event->attendees()->count())->toBe(4)->and($event->isParticipating(...))->tobeFalse()).
Ensure only the parameter passed to isParticipating() is updated to
auth()->user()->getKey().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@app-modules/identity/database/migrations/2026_03_21_000002_fix_user_address_column_lengths.php`:
- Around line 14-15: The migration widens the DB columns country/state but the
Filament form still restricts them to maxLength(4); update
app-modules/identity/src/Filament/Shared/Schemas/UserAddressForm.php to increase
or remove the maxLength constraint for the country and state fields (references:
UserAddressForm, the country and state field declarations that call
maxLength(4)) so UI validation matches the migration and users can enter longer
values; ensure any client/server validators using the same symbols are updated
consistently.
- Around line 22-23: The migration currently shrinks the country and state
columns to length 4 which makes rollback unsafe if existing values exceed 4
characters; before calling $table->string('country', 4)->nullable()->change()
and $table->string('state', 4)->nullable()->change() (in this migration file),
add a safe normalization step that truncates or cleans existing values to <=4
characters (e.g., update rows to substring(country,1,4) / substring(state,1,4)
or set overly-long values to NULL) inside a DB transaction, and/or add a guard
that aborts the schema change if any rows exceed length 4 so the schema change
cannot fail during rollback. Ensure the normalization runs in the same migration
(or in down()/up() as appropriate) and target the country and state column names
referenced above.

---

Outside diff comments:
In `@app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php`:
- Around line 120-122: The assertion uses auth()->user()->id which is
inconsistent with the earlier change to auth()->user()->getKey(); update the
call passed into Event::isParticipating to use auth()->user()->getKey() instead
of ->id so UUIDs work and calls are consistent—locate the assertion chain
containing
expect($event->attendees()->count())->toBe(5)->and($event->fresh()->waitlist_count)->toBe(1)->and($event->isParticipating(...))
and replace the inner auth()->user()->id with auth()->user()->getKey().
- Around line 140-141: The assertion uses auth()->user()->id inconsistently;
change the call to use getKey() for consistency by replacing auth()->user()->id
with auth()->user()->getKey() in the assertion that calls isParticipating() (the
line chaining
expect($event->attendees()->count())->toBe(4)->and($event->isParticipating(...))->tobeFalse()).
Ensure only the parameter passed to isParticipating() is updated to
auth()->user()->getKey().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d0a6a458-8df8-4102-9225-bf61d79f9b6b

📥 Commits

Reviewing files that changed from the base of the PR and between 3131a6d and 911a805.

📒 Files selected for processing (9)
  • app-modules/events/src/Models/Sponsor.php
  • app-modules/events/tests/Feature/Filament/Admin/Event/CreateEventTest.php
  • app-modules/events/tests/Feature/Filament/Admin/Sponsors/CreateSponsorTest.php
  • app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php
  • app-modules/gamification/src/Badge/Models/Badge.php
  • app-modules/gamification/src/Character/Actions/PersistClaimedBadge.php
  • app-modules/identity/database/factories/UserFactory.php
  • app-modules/identity/database/migrations/2026_03_21_000002_fix_user_address_column_lengths.php
  • app-modules/identity/tests/Feature/FindProfileTest.php
✅ Files skipped from review due to trivial changes (1)
  • app-modules/identity/database/factories/UserFactory.php

gvieira18
gvieira18 previously approved these changes Mar 21, 2026
Copy link

@thalesmengue thalesmengue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@gvieira18 gvieira18 merged commit 226ad51 into 4.x Mar 21, 2026
6 checks passed
@gvieira18 gvieira18 deleted the refactor/morph-types-to-aliases branch March 21, 2026 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants