Feat: Move Event action into standalone actions#51
Open
Dunkstormen wants to merge 5 commits intov2from
Open
Conversation
Reviewer's GuideRefactors event creation, update, and deletion logic out of EventService into three dedicated action classes (CreateEvent, UpdateEvent, DeleteEvent) and wires EventController to use these actions while keeping read-only/event-utility logic in EventService. Sequence diagram for creating an event with CreateEvent actionsequenceDiagram
actor User
participant EventController
participant CreateEvent
participant BannerUploadService
participant DB
participant Event
participant Log
User->>EventController: POST /events (StoreEventRequest)
EventController->>CreateEvent: __invoke(validatedData, authenticatedUser)
CreateEvent->>CreateEvent: validateRecurrenceRule(recurrence_rule?)
CreateEvent->>DB: beginTransaction()
alt banner is UploadedFile
CreateEvent->>BannerUploadService: upload(banner)
BannerUploadService-->>CreateEvent: bannerPath
else no banner
CreateEvent-->>CreateEvent: bannerPath = null
end
CreateEvent->>Event: create(data + banner_path + created_by)
Event-->>CreateEvent: event
CreateEvent->>Log: info(event created message)
CreateEvent->>DB: commit()
CreateEvent-->>EventController: event
EventController-->>User: redirect to events.show(event)
Sequence diagram for updating an event with UpdateEvent actionsequenceDiagram
actor User
participant EventController
participant UpdateEvent
participant BannerUploadService
participant DB
participant Event
participant Log
User->>EventController: PUT /events/{event} (UpdateEventRequest)
EventController->>EventController: authorize(update, event)
EventController->>UpdateEvent: __invoke(event, validatedData, authenticatedUser, bannerFile)
UpdateEvent->>UpdateEvent: validateRecurrenceRule(recurrence_rule?)
UpdateEvent->>DB: beginTransaction()
alt banner is UploadedFile
UpdateEvent->>BannerUploadService: upload(banner)
BannerUploadService-->>UpdateEvent: bannerPath
UpdateEvent->>Event: set banner_path
else no banner
UpdateEvent-->>Event: keep existing banner_path
end
UpdateEvent->>Event: update(data)
UpdateEvent->>Log: info(event updated message)
UpdateEvent->>DB: commit()
UpdateEvent-->>EventController: event
EventController-->>User: redirect to events.show(event)
Updated class diagram for Event actions and EventServiceclassDiagram
class EventController {
create(Request request)
store(StoreEventRequest request, CreateEvent createEvent)
edit(Request request, Event event, EventService eventService)
update(UpdateEventRequest request, Event event, UpdateEvent updateEvent)
destroy(Event event, DeleteEvent deleteEvent)
}
class EventService {
<<service>>
+__construct(BannerUploadService bannerUploadService)
+getEventSummary(Event event) array
+getEventDetails(Event event) array
+getManagementData(Event event) array
+generateUpcomingInstances(Event event, int limit, Carbon startDate) Collection
+toggleOccurrence(Event event, string date, bool cancel) void
+getLastEndedOccurrence(Event event) array
}
class CreateEvent {
<<action>>
+__construct(BannerUploadService bannerUploadService)
+__invoke(array data, User user) Event
-validateRecurrenceRule(string rule) void
}
class UpdateEvent {
<<action>>
+__construct(BannerUploadService bannerUploadService)
+__invoke(Event event, array data, User user, UploadedFile banner) Event
-validateRecurrenceRule(string rule) void
}
class DeleteEvent {
<<action>>
+__construct(BannerUploadService bannerUploadService)
+__invoke(Event event, User user) void
}
class BannerUploadService {
<<service>>
+upload(UploadedFile file) string
+delete(string path) void
}
class Event {
<<model>>
+int id
+string title
+string banner_path
+int created_by
+string recurrence_rule
+Carbon start_datetime
+array cancelled_occurrences
+create(array attributes) Event
+update(array attributes) void
+delete() void
}
class User {
<<model>>
+int id
+string vatsim_cid
}
EventController --> EventService : uses for read operations
EventController --> CreateEvent : uses for create
EventController --> UpdateEvent : uses for update
EventController --> DeleteEvent : uses for delete
CreateEvent --> BannerUploadService : uploads banner
UpdateEvent --> BannerUploadService : uploads banner
DeleteEvent --> BannerUploadService : deletes banner
CreateEvent --> Event : creates
UpdateEvent --> Event : updates
DeleteEvent --> Event : deletes
CreateEvent --> User : creator
UpdateEvent --> User : updater
DeleteEvent --> User : deleter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
EventController@store, the banner file is no longer passed to the action, butCreateEventexpectsbannerinside the$dataarray, so banner uploads on create will silently stop working unless you explicitly pass the uploaded file into the action or adjust how$data['banner']is populated. - The
UpdateEventaction signature__invoke(Event $event, array $data, User $user)does not match the controller call$updateEvent($event, $request->validated(), auth()->user(), $request->file('banner')), which will cause an argument count/type mismatch; either update the action signature to accept the banner file or move the banner handling back into the controller data. - When updating an event banner in
UpdateEvent, the previous implementation deleted the old banner file before uploading a new one, but the new action only uploads and overwritesbanner_pathwithout cleanup, which can leave orphaned files; consider preserving the delete behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `EventController@store`, the banner file is no longer passed to the action, but `CreateEvent` expects `banner` inside the `$data` array, so banner uploads on create will silently stop working unless you explicitly pass the uploaded file into the action or adjust how `$data['banner']` is populated.
- The `UpdateEvent` action signature `__invoke(Event $event, array $data, User $user)` does not match the controller call `$updateEvent($event, $request->validated(), auth()->user(), $request->file('banner'))`, which will cause an argument count/type mismatch; either update the action signature to accept the banner file or move the banner handling back into the controller data.
- When updating an event banner in `UpdateEvent`, the previous implementation deleted the old banner file before uploading a new one, but the new action only uploads and overwrites `banner_path` without cleanup, which can leave orphaned files; consider preserving the delete behavior.
## Individual Comments
### Comment 1
<location path="app/Http/Controllers/EventController.php" line_range="144" />
<code_context>
- );
+ $this->authorize('update', $event);
+
+ $event = $updateEvent($event, $request->validated(), auth()->user(), $request->file('banner'));
return redirect()->route('events.show', $event)
</code_context>
<issue_to_address>
**issue (bug_risk):** The UpdateEvent action is invoked with a mismatched signature, which will cause a runtime error.
`UpdateEvent::__invoke` is declared as `(Event $event, array $data, User $user): Event`, but it’s being called with four arguments (including the banner file). In PHP this will throw an argument count error. Either extend the action signature to accept the banner file or stop passing it and handle the banner within the existing parameters.
</issue_to_address>
### Comment 2
<location path="app/Actions/UpdateEvent.php" line_range="37-38" />
<code_context>
+
+ return DB::transaction(function () use ($event, $data, $user, $banner) {
+
+ $event->banner_path = $banner instanceof UploadedFile
+ ? $this->bannerUploadService->upload($banner)
+ : $event->banner_path;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Updating a banner does not delete the previous file, potentially leaving orphaned uploads.
Previously, `EventService::updateEvent` deleted the existing banner before uploading a new one. The new logic only overwrites `banner_path`, leaving the old file on disk. Please restore the cleanup step so that, when a new banner is uploaded and an old one exists, the old file is deleted before saving the new `banner_path`.
</issue_to_address>
### Comment 3
<location path="app/Actions/UpdateEvent.php" line_range="51" />
<code_context>
+ });
+ }
+
+ protected function validateRecurrenceRule(string $rule): void
+ {
+ try {
</code_context>
<issue_to_address>
**suggestion:** Recurrence rule validation logic is duplicated between CreateEvent and UpdateEvent.
Since both `CreateEvent` and `UpdateEvent` define the same `validateRecurrenceRule` method, please extract this into a shared helper/trait or validator so recurrence validation is maintained in a single place.
Suggested implementation:
```
});
}
}
```
To fully implement the refactor and avoid duplication:
1. **Create a shared trait** (e.g. `app/Actions/Concerns/ValidatesRecurrenceRule.php`):
- Namespace: `App\Actions\Concerns;`
- Define the `protected function validateRecurrenceRule(string $rule): void` method there, with the existing `Rule`/`ValidationException` logic moved into it.
2. **Update this `UpdateEvent` action class**:
- Add `use App\Actions\Concerns\ValidatesRecurrenceRule;` at the top of the file with the other imports.
- Inside the `UpdateEvent` class body, add `use ValidatesRecurrenceRule;`.
3. **Update the `CreateEvent` action class** (likely `app/Actions/CreateEvent.php`):
- Remove its local `validateRecurrenceRule` implementation, just like above.
- Import and `use` the same `ValidatesRecurrenceRule` trait inside the `CreateEvent` class.
These steps will centralize recurrence rule validation while keeping both actions using the same logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ); | ||
| $this->authorize('update', $event); | ||
|
|
||
| $event = $updateEvent($event, $request->validated(), auth()->user(), $request->file('banner')); |
There was a problem hiding this comment.
issue (bug_risk): The UpdateEvent action is invoked with a mismatched signature, which will cause a runtime error.
UpdateEvent::__invoke is declared as (Event $event, array $data, User $user): Event, but it’s being called with four arguments (including the banner file). In PHP this will throw an argument count error. Either extend the action signature to accept the banner file or stop passing it and handle the banner within the existing parameters.
Comment on lines
+37
to
+38
| $event->banner_path = $banner instanceof UploadedFile | ||
| ? $this->bannerUploadService->upload($banner) |
There was a problem hiding this comment.
issue (bug_risk): Updating a banner does not delete the previous file, potentially leaving orphaned uploads.
Previously, EventService::updateEvent deleted the existing banner before uploading a new one. The new logic only overwrites banner_path, leaving the old file on disk. Please restore the cleanup step so that, when a new banner is uploaded and an old one exists, the old file is deleted before saving the new banner_path.
| }); | ||
| } | ||
|
|
||
| protected function validateRecurrenceRule(string $rule): void |
There was a problem hiding this comment.
suggestion: Recurrence rule validation logic is duplicated between CreateEvent and UpdateEvent.
Since both CreateEvent and UpdateEvent define the same validateRecurrenceRule method, please extract this into a shared helper/trait or validator so recurrence validation is maintained in a single place.
Suggested implementation:
});
}
}
To fully implement the refactor and avoid duplication:
-
Create a shared trait (e.g.
app/Actions/Concerns/ValidatesRecurrenceRule.php):- Namespace:
App\Actions\Concerns; - Define the
protected function validateRecurrenceRule(string $rule): voidmethod there, with the existingRule/ValidationExceptionlogic moved into it.
- Namespace:
-
Update this
UpdateEventaction class:- Add
use App\Actions\Concerns\ValidatesRecurrenceRule;at the top of the file with the other imports. - Inside the
UpdateEventclass body, adduse ValidatesRecurrenceRule;.
- Add
-
Update the
CreateEventaction class (likelyapp/Actions/CreateEvent.php):- Remove its local
validateRecurrenceRuleimplementation, just like above. - Import and
usethe sameValidatesRecurrenceRuletrait inside theCreateEventclass.
- Remove its local
These steps will centralize recurrence rule validation while keeping both actions using the same logic.
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
1 participant
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.
Summary by Sourcery
Extract event creation, update, and deletion logic from EventService into dedicated action classes and wire controllers to use them while keeping occurrence-related utilities in EventService.
New Features:
Enhancements: