feat: talks time#105
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds talk scheduling (starts_at, ends_at) across database, models, factories, forms, and tests; introduces schedule availability validation and a Rejected talk status; restructures Filament talk forms with a reusable StartEndFieldsSchema; and refactors seeders into BaseSeeder and ThreeDotsSeeder. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Form
participant Form as TalkForm
participant Rule as AvailableTalkSchedule
participant Event as EventModel
participant DB as Database
User->>Form: Submit talk (event_id, starts_at, ends_at, ...)
Form->>Rule: Invoke AvailableTalkSchedule(event_id, starts_at) with ends_at
Rule->>Event: availableHours(event_id, start, end)
Event->>DB: Query talks with status Accepted/Done overlapping window
alt No conflict
DB-->>Event: none found
Event-->>Rule: available
Rule-->>Form: passes
Form-->>DB: Persist talk
DB-->>Form: saved
Form-->>User: success
else Conflict found
DB-->>Event: conflicting talk
Event-->>Rule: not available
Rule-->>Form: fail (Portuguese message)
Form-->>User: validation error on ends_at
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 (1)
🧰 Additional context used🧬 Code graph analysis (1)app-modules/events/src/Models/EventModel.php (2)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
@coderabbitai generate improvements |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/database/migrations/2025_11_05_192008_create_events_talks_table.php (1)
36-39: Implement a proper rollback forevents_talks
down()currently doesn’t drop theevents_talkstable and still contains a commented reference toevents_c4p, so rolling back this migration is effectively impossible.A straightforward fix:
public function down(): void { - // Schema::dropIfExists('events_c4p'); + Schema::dropIfExists('events_talks'); }
🧹 Nitpick comments (5)
database/seeders/ThreeDotsSeeder.php (1)
22-31: Make admin user dependency explicit or self-containedThis seeder assumes an
adminuser withname = 'admin'exists; if run standalone (withoutBaseSeeder)$userwill benulland->for($user, 'owner')will fail.Consider either:
- Guarding for a missing admin and creating/fetching via
firstOrCreateon a stable key (e.g. email/username), or- Documenting that this seeder must always be invoked after
BaseSeederand tightening the lookup to match howBaseSeedercreates the user (e.g. byusernameordatabase/seeders/BaseSeeder.php (1)
25-62: Seeder is non-idempotent and may break on repeated runsThe hard-coded
adminuser and tenant (he4rt) are created unconditionally; on a DB with existing data or on repeateddb:seedruns this can easily hit unique constraints onusername/slug.If you expect
db:seedto be safe to re-run, consider switching tofirstOrCreate/updateOrCreatefor the admin user and tenant (and adjusting dependent records accordingly). Otherwise, a short comment noting that this seeder assumes a fresh database would help future maintainers.app-modules/events/database/migrations/2025_11_05_192008_create_events_talks_table.php (1)
27-28: Confirm whether scheduling fields must always be present
starts_atandends_atare non-nullable timestamps. If there are flows where a talk exists before it’s scheduled (e.g., proposal phase), inserts will fail unless those fields are populated.If unscheduled talks are valid in your domain, consider marking these columns as nullable and enforcing presence at the application/validation level instead.
app-modules/events/src/Filament/Shared/Schemas/StartEndFieldsSchema.php (2)
21-30: Consider caching the event lookup to avoid repeated queries.The callback fetches the event from the database each time it's invoked. If this callback is triggered multiple times during form rendering or validation, it could result in unnecessary queries.
Additionally, since
start_atis cast todatetimein EventModel (line 186), it should already be a Carbon instance, makingDate::parse()unnecessary.Consider one of these approaches:
Option 1: Remove unnecessary Date::parse()
- return Date::parse($event->start_at); + return $event->start_at;Option 2: Cache the event lookup (if the callback is invoked frequently)
->minDate(function (Get $get): ?Carbon { static $eventCache = []; $eventId = $get('event_id'); if (!isset($eventCache[$eventId])) { $eventCache[$eventId] = EventModel::query()->find($eventId); } return $eventCache[$eventId]?->start_at; })
33-43: Remove unnecessary Date::parse() and consider caching.Same concerns as the
starts_atfield:Date::parse()is unnecessary sinceend_atis cast todatetime, and repeated database queries could be inefficient.Apply this diff:
- return Date::parse($event->end_at); + return $event->end_at;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app-modules/events/database/factories/EventFactory.php(1 hunks)app-modules/events/database/factories/TalkFactory.php(1 hunks)app-modules/events/database/migrations/2025_11_05_192008_create_events_talks_table.php(1 hunks)app-modules/events/src/Enums/Talks/TalkStatusEnum.php(4 hunks)app-modules/events/src/Filament/Admin/Resources/Talks/Schemas/TalkForm.php(1 hunks)app-modules/events/src/Filament/App/Talks/Schemas/TalkForm.php(2 hunks)app-modules/events/src/Filament/Shared/Schemas/StartEndFieldsSchema.php(1 hunks)app-modules/events/src/Models/EventModel.php(3 hunks)app-modules/events/src/Models/Talk.php(2 hunks)app-modules/events/tests/Feature/Filament/Admin/Talk/CreateTalk.php(2 hunks)app-modules/events/tests/Feature/Filament/App/Talks/CreateTalkTest.php(3 hunks)app/Rules/AvailableTalkSchedule.php(1 hunks)database/seeders/BaseSeeder.php(1 hunks)database/seeders/DatabaseSeeder.php(1 hunks)database/seeders/ThreeDotsSeeder.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
app/Rules/AvailableTalkSchedule.php
25-25: Avoid unused parameters such as '$attribute'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (12)
app-modules/events/src/Enums/Talks/TalkStatusEnum.php (1)
15-47: Enum extension and mappings look consistentThe new
Rejectedstatus is correctly added to the enum and fully wired intogetColor,getIcon, andgetLabel, and theAcceptedlabel change is coherent with the naming. No issues spotted here.database/seeders/DatabaseSeeder.php (1)
15-20: Seeder delegation is clear and orderedDelegating to
BaseSeederandThreeDotsSeedervia$this->call([...])simplifiesDatabaseSeederand makes the seeding order explicit. Looks good.app-modules/events/tests/Feature/Filament/Admin/Talk/CreateTalk.php (1)
39-40: LGTM!The test properly includes scheduling fields with a valid 1-hour duration between
starts_atandends_at.app-modules/events/src/Models/Talk.php (1)
31-32: LGTM!The
starts_atandends_atfields are correctly added to the fillable array and cast as datetime, enabling proper mass assignment and type handling for talk scheduling.Also applies to: 63-64
app-modules/events/database/factories/EventFactory.php (1)
32-34: LGTM!The changes improve semantic clarity by setting
event_atto start-of-day andend_atto end-of-day, creating more realistic test data for full-day events.app-modules/events/tests/Feature/Filament/App/Talks/CreateTalkTest.php (2)
40-41: LGTM!Test properly includes scheduling fields with a valid duration.
73-117: Well-structured validation tests for talk scheduling.The test suite effectively covers the availability validation logic, ensuring that overlapping talks are rejected when status is Accepted, but allowed when status is Pending.
app-modules/events/src/Filament/Admin/Resources/Talks/Schemas/TalkForm.php (1)
22-71: LGTM! Well-structured form refactoring.The new Flex-based layout with logical sections ("Proposta da Palestra", "Horários da Palestra", "Detalhes e Conteúdo") significantly improves organization and user experience. The use of
StartEndFieldsSchemapromotes code reuse and consistency.app-modules/events/src/Filament/App/Talks/Schemas/TalkForm.php (1)
25-84: LGTM! Consistent form structure with proper tenant scoping.The refactored layout mirrors the Admin form while appropriately:
- Filtering events by tenant (line 37)
- Auto-setting user_id, tenant_id, and status via hidden fields
- Defaulting status to Pending for user-submitted talks
The use of
StartEndFieldsSchemamaintains consistency across the application.app-modules/events/src/Filament/Shared/Schemas/StartEndFieldsSchema.php (1)
45-61: Validation logic looks good.The conditional application of
AvailableTalkScheduleis correct—it only validates when bothevent_idandstarts_atare present. The type casting on line 54 should work, though DateTimePicker typically provides properly formatted datetime values already.app-modules/events/src/Models/EventModel.php (2)
146-149: LGTM!The
talks()relationship is correctly defined.
162-163: Event time boundary checks are correct.The logic correctly ensures the event's
start_atis before or equal to the requested start time, andend_atis after or equal to the requested end time, confirming the event spans the entire requested window.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 31314 @danielhe4rt Great work on implementing the talk scheduling feature! I've analyzed the code and identified several improvements across bugs, performance, validation, and testing. Here are my recommendations: 🐛 Critical Bugs1. Error Message Parameters ReversedFile: // Current (incorrect)
$fail(sprintf('O Horário %s até %s não está disponível.', $end, $start));
// Should be
$fail(sprintf('O Horário %s até %s não está disponível.', $start, $end));2. Logic Error in Conflict Detection QueryFile: The // Current (incorrect)
$talkQuery
->where('status', '=', TalkStatusEnum::Accepted->value)
->orWhere('status', '=', TalkStatusEnum::Done->value);
// Should be
$talkQuery->whereIn('status', [
TalkStatusEnum::Accepted->value,
TalkStatusEnum::Done->value,
]);3. Incorrect Relationship DefinitionFile: // Current (incorrect) - uses 'user_id' as first foreign key
public function speakers(): HasManyThrough
{
return $this->hasManyThrough(User::class, Talk::class, 'user_id');
}
// Should be
public function speakers(): HasManyThrough
{
return $this->hasManyThrough(
User::class,
Talk::class,
'event_id', // Foreign key on talks table
'id', // Foreign key on users table
'id', // Local key on events table
'user_id' // Local key on talks table
);
}⚡ Performance Issues4. N+1 Query Problem in Form SchemaFile: The event is fetched twice (once for public static function make(): array
{
return [
DateTimePicker::make('starts_at')
->label('Starts at')
->minDate(function (Get $get): ?Carbon {
$event = self::getEvent($get);
return $event ? Date::parse($event->start_at) : null;
})
->maxDate(function (Get $get): ?Carbon {
$event = self::getEvent($get);
return $event ? Date::parse($event->end_at) : null;
})
->required(),
DateTimePicker::make('ends_at')
->minDate(function (Get $get): ?Carbon {
$event = self::getEvent($get);
return $event ? Date::parse($event->start_at) : null;
})
->maxDate(function (Get $get): ?Carbon {
$event = self::getEvent($get);
return $event ? Date::parse($event->end_at) : null;
})
->after('starts_at')
->rules([/* ... */])
->label('Ends at')
->required(),
];
}
// Add helper method with memoization
private static function getEvent(Get $get): ?EventModel
{
static $cache = [];
$eventId = $get('event_id');
if (!$eventId) {
return null;
}
if (!isset($cache[$eventId])) {
$cache[$eventId] = EventModel::query()
->where('id', $eventId)
->first(['id', 'start_at', 'end_at']);
}
return $cache[$eventId];
}5. Missing Database IndexesFile: Add indexes for the new timestamp columns: public function up(): void
{
Schema::create('events_talks', function (Blueprint $table): void {
// ... existing columns ...
$table->timestamp('starts_at');
$table->timestamp('ends_at');
$table->timestamps();
// Add indexes for performance
$table->index(['event_id', 'starts_at', 'ends_at']);
$table->index('status');
});
}🔒 Validation Issues6. Edit Scenario Not HandledFile: The validation will block editing existing talks because it doesn't exclude the current record: class AvailableTalkSchedule implements ValidationRule
{
public function __construct(
private readonly string|int $eventId,
private readonly string $start_at,
private readonly ?int $excludeTalkId = null, // Add this
) {}
public function validate(string $attribute, mixed $value, Closure $fail): void
{
$start = Date::parse($this->start_at)->toDateTimeString();
$end = Date::parse($value)->toDateTimeString();
$query = EventModel::query()
->where('id', $this->eventId)
->availableHours(start: $start, end: $end);
// Exclude current talk when editing
if ($this->excludeTalkId) {
$query->whereDoesntHave('talks', function (Builder $q) {
$q->where('id', $this->excludeTalkId);
});
}
if (!$query->exists()) {
$fail(sprintf('O Horário %s até %s não está disponível.', $start, $end));
}
}
}Then update the usage in ->rules([
function (Get $get): array {
$eventId = $get('event_id');
$startsAt = $get('starts_at');
$recordId = $get('id'); // Get current record ID for edit
if ($eventId && $startsAt) {
return [
new AvailableTalkSchedule(
eventId: (int) $eventId,
start_at: (string) $startsAt,
excludeTalkId: $recordId ? (int) $recordId : null,
),
];
}
return [];
},
])7. Add Model-Level ValidationFile: Add validation to ensure data integrity: protected static function booted(): void
{
static::saving(function (Talk $talk) {
// Ensure start is before end
if ($talk->starts_at >= $talk->ends_at) {
throw new \InvalidArgumentException(
'Start time must be before end time'
);
}
// Ensure talk is within event boundaries
$event = $talk->event;
if ($event) {
if ($talk->starts_at < $event->start_at || $talk->ends_at > $event->end_at) {
throw new \InvalidArgumentException(
'Talk must be scheduled within event boundaries'
);
}
}
});
}🧪 Testing Improvements8. Add Edge Case TestsFile: Add tests for: it('should not allow talks that partially overlap', function (): void {
$existingStart = now()->addHour();
$existingEnd = now()->addHours(3);
Talk::factory()->recycle($this->event)->create([
'starts_at' => $existingStart,
'ends_at' => $existingEnd,
'status' => TalkStatusEnum::Accepted,
]);
// Start during existing talk
livewire(CreateTalk::class)
->fillForm([
'event_id' => $this->event->getKey(),
'starts_at' => $existingStart->copy()->addMinutes(30),
'ends_at' => $existingEnd->copy()->addHour(),
// ... other fields
])
->call('create')
->assertHasFormErrors(['ends_at']);
});
it('should allow back-to-back talks', function (): void {
$existingStart = now()->addHour();
$existingEnd = now()->addHours(2);
Talk::factory()->recycle($this->event)->create([
'starts_at' => $existingStart,
'ends_at' => $existingEnd,
'status' => TalkStatusEnum::Accepted,
]);
// Start exactly when previous ends
livewire(CreateTalk::class)
->fillForm([
'event_id' => $this->event->getKey(),
'starts_at' => $existingEnd,
'ends_at' => $existingEnd->copy()->addHour(),
// ... other fields
])
->call('create')
->assertHasNoFormErrors();
});
it('should not allow talks outside event boundaries', function (): void {
livewire(CreateTalk::class)
->fillForm([
'event_id' => $this->event->getKey(),
'starts_at' => $this->event->end_at->addHour(),
'ends_at' => $this->event->end_at->addHours(2),
// ... other fields
])
->call('create')
->assertHasFormErrors(['starts_at']);
});📚 Additional Enhancements9. Add Timezone HandlingConsider adding timezone support in the protected $casts = [
'status' => TalkStatusEnum::class,
'starts_at' => 'datetime:Y-m-d H:i:s',
'ends_at' => 'datetime:Y-m-d H:i:s',
];
// Add accessor for user's timezone
public function getStartsAtInTimezoneAttribute(): Carbon
{
return $this->starts_at->timezone(auth()->user()->timezone ?? 'America/Sao_Paulo');
}10. Add Duration ValidationAdd reasonable limits for talk duration: // In StartEndFieldsSchema.php
DateTimePicker::make('ends_at')
// ... existing rules ...
->rules([
function (Get $get): array {
$rules = [];
$startsAt = $get('starts_at');
if ($startsAt) {
// Max 4 hours
$rules[] = Rule::when($startsAt, function () use ($get) {
$start = Carbon::parse($get('starts_at'));
$maxEnd = $start->copy()->addHours(4);
return "before_or_equal:{$maxEnd}";
});
}
return $rules;
},
])These improvements will make the feature more robust, performant, and maintainable! Let me know if you'd like me to help implement any of these suggestions. |
2 participants
Summary by CodeRabbit
New Features
Tests
Chores