fix: user form#122
Conversation
WalkthroughRefactors Filament user form into modular schemas, relaxes ZIP code required validation, adjusts user stats query year/month filtering, updates related user tests to use hardcoded inputs, and adds an events test ensuring a Talk can be created when an overlapping Pending talk exists. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app-modules/events/tests/Feature/Filament/App/Talks/CreateTalkTest.php (1)
96-118: Fix typo in test description.Line 96 contains a duplicate word: "should should be able to" should read "should be able to". Otherwise, the test logic correctly validates that pending talks don't block new talk creation, and the added blank lines improve readability.
Apply this diff to fix the typo:
- it('should should be able to create a talk if there is already one but is not accepted yet', function (): void { + it('should be able to create a talk if there is already one but is not accepted yet', function (): void {
🧹 Nitpick comments (2)
app-modules/user/src/Filament/Admin/Resources/Users/Pages/EditUser.php (1)
50-88: Confirm that Gamefication and Providers are intended to be read‑onlyIn the “Gamefication” and “Providers” tabs you now use
TextEntrycomponents plus aRepeaterthat isaddable(false)anddeletable(false). That makes these areas effectively read‑only (no editing or managing providers from this form). If the intent is only to surface related data, this is fine; if users were previously able to edit character/progression or manage providers here, that capability has been removed.Also, consider correcting the label from
"Gamefication"to"Gamification"if you want a polished UI string.app-modules/user/tests/Feature/Filament/Admin/Resources/User/EditUserTest.php (1)
29-41: Deterministic update test looks good; consider asserting ZIP behavior if relevantUsing fixed values for
username/assertHasNoFormErrors()makes the update test clearer and more robust. Ifzip_codeis persisted on the same model (or a directly-related address model that this form saves), you may want to add an assertion for its stored value as well; if it’s only here to ensure validation no longer blocks the update, the current coverage is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app-modules/events/tests/Feature/Filament/App/Talks/CreateTalkTest.php(1 hunks)app-modules/user/src/Filament/Admin/Resources/Users/Pages/EditUser.php(2 hunks)app-modules/user/src/Filament/Shared/Schemas/UserAddressForm.php(0 hunks)app-modules/user/src/Filament/Shared/Widgets/UsersStatsOverview.php(1 hunks)app-modules/user/tests/Feature/Filament/Admin/Resources/User/EditUserTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- app-modules/user/src/Filament/Shared/Schemas/UserAddressForm.php
🔇 Additional comments (1)
app-modules/user/src/Filament/Admin/Resources/Users/Pages/EditUser.php (1)
31-46: Modular form schemas for General/Address/Information look goodDelegating the tab content to
UserForm::configure,UserAddressForm::form, andUserInformationForm::configureviaSection+Schemaclosures is a nice cleanup and should improve reuse and consistency across pages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app-modules/user/src/Filament/Shared/Widgets/UsersStatsOverview.php (1)
28-39: Cloning fix looks good; consider range-based month filter for better index usageUsing
(clone $query)for each aggregate correctly avoids the previous->clone()undefined-method issue and keeps the tenant scoping intact for all three counts. The new year+month filter also makes the “this month” stat explicit.If you want a small perf/clarity tweak, you could:
- Capture
now()once, and- Use a range on
created_atinstead ofwhereYear/whereMonth(which wrap the column in functions and can hinder index usage), e.g.:$now = now(); $totalUsersMonth = (clone $query) ->whereBetween('created_at', [$now->copy()->startOfMonth(), $now->copy()->endOfMonth()]) ->count();
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.