feat: user form#118
Conversation
… gamefication, and providers
WalkthroughReworks the User admin UI by adding a tabbed EditUser form and removing four Filament RelationManager classes and their tests; introduces Provider enum casting and factory changes; updates seeders and authentication to persist ProviderEnum; adjusts multiple tests and message UI to use ProviderEnum values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant EditUser as EditUser Page
participant Tabs as Tabs Form
participant Backend as Backend / Models
note over Tabs,Backend `#E6F5FF`: New tabbed form persists nested relations
Admin->>EditUser: Open EditUser
EditUser->>Tabs: Build form() (General, Address, Information, Gamefication, Providers)
Admin->>Tabs: Fill fields & submit
Tabs->>Backend: Persist User + nested relations (providers repeater)
Backend->>Backend: Provider attribute cast/stored as ProviderEnum
Backend-->>Tabs: Save result
Tabs-->>Admin: Show success or validation errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app-modules/user/src/Filament/Admin/Resources/Users/Schemas/UserForm.php (1)
16-41: User field schema is clear; watch for duplication withEditUser::formThe field setup (labels, placeholders, validation) looks good in isolation, but these four inputs are now also defined separately in
EditUser::form(General tab) with different rules (e.g.,name/requiredthere, no min/max lengths, different UX). This divergence can easily drift and cause inconsistent validation between create and edit. Consider centralizing these field definitions (or at least sharing the validation rules) soUserFormandEditUserstay in sync, and double‑check that makingpasswordrequired here matches where this schema is actually used (create vs edit).app-modules/provider/database/factories/ProviderFactory.php (1)
26-26: Factory provider values now match validation; consider deriving fromProviderEnumIncluding both
'discord'and'twitch'here is consistent withCreateProviderRequestandProviderEnum, but the literals are now duplicated in three places. To avoid future drift, consider pulling from the enum, e.g.:+use He4rt\Provider\Enums\ProviderEnum; ... - 'provider' => fake()->randomElement(['discord', 'twitch']), + 'provider' => fake()->randomElement( + array_map(fn (ProviderEnum $case) => $case->value, ProviderEnum::cases()), + ),This keeps the factory automatically in sync if new providers are added.
app-modules/user/src/Filament/Admin/Resources/Users/Pages/EditUser.php (3)
25-40: General tab duplicates core user fields with weaker validation thanUserFormHere you redefine
username,name,passwordinstead of reusing the configuration inUserForm::configure(). That leads to several practical differences on edit vs create (e.g., onlyusernameis required here, no min/max length constraints, no localized labels/placeholders), which can create inconsistent behavior and UX across pages. It may be worth either pulling the shared config fromUserFormor at least mirroring the same validation rules and labels on this General tab, and explicitly deciding whetherpasswordshould behave differently on edit (typically nullable and only persisted when filled, to avoid forcing resets or mishandling hashed values).
70-85: Gamefication tab read‑only fields and date formattingUsing
TextEntryto present character/tenant data read‑only in the edit form makes sense for a “profile overview” style tab. Two small points to consider:
- The tab label
"Gamefication"looks like a typo of"Gamification"if that’s what you intend user‑facing.- For
character.daily_bonus_claimed_at, confirm that$stateis always a valueDate::parse()can handle (e.g., string/Carbon ornull); if it can ever be an unexpected type, you may want a stricter guard or explicit cast before parsing.
86-103: Providers tab is effectively read‑only; confirm that’s intentional and that the API matches your Filament/Schemas versionThe Providers tab uses a
Repeaterbound to theprovidersrelationship but only containsTextEntrycomponents and has both adding and deleting disabled, which makes this section a read‑only listing rather than a management UI. That’s fine if provider creation/updates are handled elsewhere (e.g., dedicated provider flows), but if admins are expected to manage providers here, you’ll likely want editable form components instead. Also, ensure thataddable(false)/deletable(false)and embeddingTextEntryinside aRepeaterare supported patterns for the Filament/Schemas version you’re on, as those APIs differ slightly between releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app-modules/provider/database/factories/ProviderFactory.php(1 hunks)app-modules/user/src/Filament/Admin/Resources/Users/Pages/EditUser.php(1 hunks)app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/AddressRelationManager.php(0 hunks)app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/CharacterRelationManager.php(0 hunks)app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/InformationRelationManager.php(0 hunks)app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/ProvidersRelationManager.php(0 hunks)app-modules/user/src/Filament/Admin/Resources/Users/Schemas/UserForm.php(1 hunks)app-modules/user/src/Filament/Admin/Resources/Users/UserResource.php(0 hunks)app-modules/user/tests/Feature/Filament/Admin/Resources/User/RelationManagers/AddressRelationManagerTest.php(0 hunks)app-modules/user/tests/Feature/Filament/Admin/Resources/User/RelationManagers/CharacterRelationManagerTest.php(0 hunks)app-modules/user/tests/Feature/Filament/Admin/Resources/User/RelationManagers/InformationRelationManagerTest.php(0 hunks)database/seeders/BaseSeeder.php(2 hunks)
💤 Files with no reviewable changes (8)
- app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/ProvidersRelationManager.php
- app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/InformationRelationManager.php
- app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/CharacterRelationManager.php
- app-modules/user/tests/Feature/Filament/Admin/Resources/User/RelationManagers/AddressRelationManagerTest.php
- app-modules/user/src/Filament/Admin/Resources/Users/RelationManagers/AddressRelationManager.php
- app-modules/user/tests/Feature/Filament/Admin/Resources/User/RelationManagers/InformationRelationManagerTest.php
- app-modules/user/tests/Feature/Filament/Admin/Resources/User/RelationManagers/CharacterRelationManagerTest.php
- app-modules/user/src/Filament/Admin/Resources/Users/UserResource.php
🧰 Additional context used
🧬 Code graph analysis (3)
database/seeders/BaseSeeder.php (4)
app-modules/provider/src/Models/Provider.php (2)
Provider(26-85)newFactory(59-62)database/migrations/2023_01_18_210724_create_providers_table.php (2)
Blueprint(17-24)up(14-26)app-modules/provider/tests/Feature/NewAccountByProviderTest.php (2)
provider(34-51)provider(8-33)app-modules/feedback/tests/Feature/CreateFeedbackTest.php (1)
providerSender(8-27)
app-modules/provider/database/factories/ProviderFactory.php (4)
app-modules/provider/src/Enums/ProviderEnum.php (1)
ProviderEnum(7-11)app-modules/authentication/src/Enums/OAuthProviderEnum.php (1)
OAuthProviderEnum(10-20)app-modules/provider/src/Http/Requests/CreateProviderRequest.php (1)
rules(16-23)tests/Unit/Message/Application/NewMessageTest.php (1)
fn () => [(16-28)
app-modules/user/src/Filament/Admin/Resources/Users/Pages/EditUser.php (1)
app-modules/user/src/Filament/Admin/Resources/Users/UserResource.php (2)
UserResource(20-48)form(30-33)
🔇 Additional comments (1)
database/seeders/BaseSeeder.php (1)
10-11: Provider seeding is consistent with the factory setupUsing
recycle($user)andrecycle($tenant)against the currentProviderFactorydefinition correctly ties the 4 seeded providers back to the existing admin user and tenant without creating extra records. Overriding$user->emailis fine for baseline seed data, assuming there’s no unique constraint onproviders.email.Also applies to: 75-79
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app-modules/provider/src/Models/Provider.php (1)
9-9: Enum cast is appropriate; align PHPDoc and double‑check downstream usagesThe new enum cast for
providerlooks correct and consistent withProviderEnumand the updated factory/seeder. Two follow‑ups to consider:
- The docblock still declares
@property string $provider(Line 25). With the cast in place,$provider->providerwill now be aProviderEnuminstance. Updating the annotation will help IDEs and static analysis:- * @property string $provider + * @property ProviderEnum $provider
- Verify any existing code that treats
provideras a raw string (comparisons, type hints, JSON casting outside Eloquent, etc.) to ensure it either expects an enum or calls$provider->provider->valuewhere a string is required.Also applies to: 25-25, 82-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app-modules/provider/database/factories/ProviderFactory.php(2 hunks)app-modules/provider/src/Models/Provider.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app-modules/provider/database/factories/ProviderFactory.php
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/provider/src/Models/Provider.php (2)
app-modules/provider/src/DTO/NewProviderDTO.php (1)
NewProviderDTO(10-24)app-modules/provider/src/Enums/ProviderEnum.php (1)
ProviderEnum(7-11)
…rovider assignment
…ovider value assertion
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app-modules/message/tests/Feature/NewMessageTest.php (1)
128-128: Missing.valueon enum causes inconsistency.Line 128 passes
$provider->provider(the enum object) to the route, while lines 43 and 83 correctly pass$provider->provider->value(the string value). Routes typically expect string values, not enum objects. This inconsistency could cause runtime errors or unexpected behavior.Apply this diff to fix the inconsistency:
- ->postJson(route('messages.create', ['provider' => $provider->provider]), $payload) + ->postJson(route('messages.create', ['provider' => $provider->provider->value]), $payload)
🧹 Nitpick comments (4)
app-modules/message/src/Filament/Admin/Resources/Messages/Tables/MessagesTable.php (1)
19-23: Provider column now shows related provider instead of FK, which is goodUsing
TextColumn::make('provider.provider')with->badge()is a nice improvement over showingprovider_id; it aligns with theProvidermodel’s enum cast and keeps sort/search behavior on the provider value. If you prefer showing a more human-friendly label (e.g.,ProviderEnum::getLabel()as used inMessageForm), you might wrap this in aformatStateUsingcallback to render the enum label instead of the raw value, but that’s optional.app-modules/message/src/Filament/Admin/Resources/Messages/Schemas/MessageForm.php (1)
12-24: Enum-based option labels for provider Select are correctImporting
Providerand usinggetOptionLabelFromRecordUsing(fn (Provider $record) => $record->provider->getLabel())cleanly leverages the enum cast onproviderto show a friendly label in the dropdown. This looks consistent with the rest of the enum integration; just ensure allProviderEnumcases implementgetLabel()so this never hits a missing-method/error path. If you expect many providers, you might optionally re-add->searchable()for UX, but it’s not required.app-modules/message/tests/Feature/NewVoiceMessageTest.php (1)
7-40: Voice message test aligns correctly with enum-backed providersUsing
ProviderEnum::Discordwhen seeding and$provider->provider->valuefor the payload andvoices.createroute ensures the test follows the enum-castedprovidercolumn. This is consistent with the rest of the refactor; you may optionally update shared helpers likeactingAsAdmin()to also rely onProviderEnumto avoid any future mismatch between hard-coded strings and enum values.app-modules/authentication/src/Actions/AuthenticateAction.php (1)
9-62: ProviderEnum usage in registration matches model casts; ensure DTO provider type/values stay alignedSwitching
updateOrCreateto use'provider' => ProviderEnum::from($userDTO->provider->value)correctly aligns persisted providers with theProvidermodel’s enum cast. This assumes that$userDTO->provideris a backed enum (or equivalent) whosevaluematches theProviderEnumbacking values. SincewithOAuth()still performs the lookup via->where('provider', $user->provider), it’s worth double‑checking that the DTO’sprovidertype / value is consistent in both places so an already-registered provider can be found instead of re-created.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app-modules/authentication/src/Actions/AuthenticateAction.php(2 hunks)app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php(1 hunks)app-modules/character/tests/Feature/ClaimCharacterBadgeTest.php(3 hunks)app-modules/message/src/Filament/Admin/Resources/Messages/Schemas/MessageForm.php(2 hunks)app-modules/message/src/Filament/Admin/Resources/Messages/Tables/MessagesTable.php(1 hunks)app-modules/message/tests/Feature/Filament/Admin/Message/CreateMessageTest.php(2 hunks)app-modules/message/tests/Feature/NewMessageTest.php(9 hunks)app-modules/message/tests/Feature/NewVoiceMessageTest.php(3 hunks)app-modules/provider/database/factories/ProviderFactory.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app-modules/provider/database/factories/ProviderFactory.php
🧰 Additional context used
🧬 Code graph analysis (5)
app-modules/message/tests/Feature/NewMessageTest.php (1)
app-modules/provider/src/Models/Provider.php (1)
Provider(27-93)
app-modules/message/tests/Feature/Filament/Admin/Message/CreateMessageTest.php (4)
app-modules/provider/src/Models/Provider.php (1)
Provider(27-93)app-modules/message/src/Models/Message.php (3)
tenant(46-49)provider(38-41)season(54-57)app-modules/tenant/src/Models/Tenant.php (1)
Tenant(22-94)app-modules/season/src/Models/Season.php (1)
Season(29-85)
app-modules/authentication/src/Actions/AuthenticateAction.php (2)
app-modules/provider/src/Models/Provider.php (1)
Provider(27-93)app-modules/provider/src/Models/Token.php (1)
provider(37-40)
app-modules/character/tests/Feature/ClaimCharacterBadgeTest.php (1)
app-modules/provider/src/Models/Provider.php (1)
Provider(27-93)
app-modules/message/tests/Feature/NewVoiceMessageTest.php (4)
app-modules/provider/src/Models/Provider.php (1)
Provider(27-93)app-modules/message/src/Models/Message.php (1)
provider(38-41)app-modules/message/src/Models/Voice.php (1)
provider(27-30)tests/TestCase.php (1)
actingAsAdmin(15-32)
🔇 Additional comments (7)
app-modules/character/tests/Feature/ClaimCharacterBadgeTest.php (1)
7-41: Badge-claim test is now consistent with ProviderEnum castingSwitching the factory to
'provider' => ProviderEnum::Discordand using$provider->provider->valuefor both route parameter andX-He4rt-Providerheader matches theProvidermodel’s enum cast and keeps everything using a single source of truth for the provider identifier. Just ensure the enum’s backing value remains the same slug the route expects (e.g.'discord').app-modules/message/tests/Feature/Filament/Admin/Message/CreateMessageTest.php (1)
7-23: Factory setup for tenant / provider / season looks good; just confirm recycle wiringCreating the
Tenantfirst and then usingProvider::factory()->recycle($tenant)andSeason::factory()->recycle($tenant)is a clean way to ensure related records share the same tenant, and setting'provider' => ProviderEnum::Discordkeeps this test aligned with enum casting. Please just confirm thatProviderFactoryandSeasonFactoryare configured to respectrecycle($tenant)(e.g., viaforeignIdFor(Tenant::class)or appropriatefor()relations) sotenant_idis actually populated as intended.app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php (1)
68-68: LGTM! Correctly accesses the enum value.The change properly adapts to the
providerfield being cast toProviderEnumin the Provider model. Accessing->valueretrieves the underlying string value of the enum.app-modules/message/tests/Feature/NewMessageTest.php (4)
7-7: LGTM! Correct import for the enum.The import of
ProviderEnumis necessary for the enum-based provider handling introduced in this PR.
20-20: LGTM! Factory calls correctly use the enum.The provider factories now properly use
ProviderEnum::Discordinstead of the string literal'discord', aligning with the enum-based provider handling.Also applies to: 61-61, 99-99
33-33: LGTM! Payload construction correctly accesses enum values.The payload construction properly uses
$provider->provider->valueto extract the string value from theProviderEnum, which is necessary since the Provider model now casts theproviderfield to an enum.Also applies to: 73-73, 118-118
43-43: LGTM! Route parameters correctly use enum values.The route generation correctly accesses
$provider->provider->valueto pass the string representation of the provider enum to the route.Also applies to: 83-83
4 participants
This pull request refactors the Filament user admin resource by removing the individual relation manager classes for user-related models (address, character, information, providers) and consolidating their functionality into a new tabbed form interface within the
EditUserpage. Additionally, it updates user form field configurations and expands provider seeding. These changes simplify the codebase, improve maintainability, and enhance the user editing experience.Refactoring and UI Consolidation:
AddressRelationManager,CharacterRelationManager,InformationRelationManager,ProvidersRelationManager) with a single tabbed form inEditUser.php, grouping user-related data (General, Address, Information, Gamefication, Providers) into tabs for easier navigation and editing.getRelationsmethod fromUserResource.php, eliminating the registration of the now-deleted relation managers.User Form Improvements:
UserForm.phpto provide clearer labels, placeholders, and validation rules for username, name, email, and password fields.Provider Model and Seeding:
ProviderFactory.phpto include both 'discord' and 'twitch'.These changes streamline the admin resource, improve usability, and reduce maintenance overhead.
Summary by CodeRabbit
New Features
Refactor
Removed
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.