Conversation
📝 WalkthroughWalkthroughThis PR adds a notification system for backup completion events. It introduces a BackupCompleted event fired when backups finish, a listener that triggers database and email notifications, and a dedicated email notification class. A new configuration option controls email notification behavior. The PR also modifies PluginService to conditionally handle build failures based on plugin/theme context. Changes
Sequence DiagramsequenceDiagram
participant Controller as BackupStatusController
participant Event as BackupCompleted Event
participant Listener as BackupCompletedListener
participant FilamentNotif as Filament Notification
participant EmailNotif as Email Notification
participant User
Controller->>Event: Fire BackupCompleted(backup, server, owner)
Event->>Listener: handle(event)
Listener->>FilamentNotif: Create Database Notification
FilamentNotif->>User: Display in Panel
Listener->>EmailNotif: Dispatch (config-driven)
EmailNotif->>User: Send Email with Backup Details
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ 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. Comment |
|
Welp, I guess this supersedes #2041? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@app/Listeners/Backup/BackupCompletedListener.php`:
- Around line 15-18: The call to loadMissing('language') on $event->owner is
incorrect because language is a scalar attribute, not a relationship; remove the
$event->owner->loadMissing('language') line from BackupCompletedListener (where
$event->backup->loadMissing('server') and $event->owner->loadMissing('language')
are used) so the code simply relies on $event->owner->language (and the existing
null-coalescing) to determine $locale.
- Line 8: The import for the Action class is incorrect: replace the current use
of Filament\Actions\Action with the notification-specific class
Filament\Notifications\Actions\Action in BackupCompletedListener (update the use
statement for the Action symbol so notification actions resolve correctly);
ensure any references to Action in the listener continue to refer to the new
import.
In `@tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php`:
- Around line 75-103: The test
test_panel_notification_created_on_backup_completion incorrectly calls
Event::fake([]) which still fakes all events; remove the Event::fake([]) call
(or replace it with Event::fakeExcept([BackupCompleted::class]) if you intend to
fake others) so that the BackupCompleted event and subsequent notification flow
are actually dispatched; update the test to rely on real events and keep the
rest of the assertion logic unchanged.
- Around line 105-135: The test method
test_email_notification_sent_when_config_enabled is erroneously preventing real
event dispatch by calling Event::fake([]); remove that Event::fake([]) call (or
stop faking events) so the BackupCompleted event can be dispatched to its
listener and trigger BackupCompletedNotification; keep Notification::fake()
as-is and run the test to verify the notification is sent via the real
event/listener flow.
- Around line 137-162: The test method
test_email_notification_not_sent_when_config_disabled() incorrectly calls
Event::fake([]), preventing the backup completion event from being dispatched
and causing Notification::assertNothingSent() to give a false positive; remove
the Event::fake([]) (or replace it with a more targeted fake such as
Event::fakeExcept([...]) if other tests require faking) so the real event is
dispatched when posting to the /api/remote/backups/{$backup->uuid} endpoint, and
keep Notification::assertNothingSent() to verify no notifications were sent when
Config::set('panel.email.send_backup_notification', false).
In `@tests/Unit/Listeners/Backup/BackupCompletedListenerTest.php`:
- Around line 36-44: The test incorrectly calls $user->setRelation('language',
'en') treating 'language' as a relationship; remove that setRelation call and
ensure the User factory sets the language attribute (e.g.,
User::factory()->make([... 'language' => 'en' ...])) so the listener can read
$event->owner->language as a plain attribute; update the test to rely on the
factory-provided 'language' attribute and delete the erroneous setRelation
invocation.
- Around line 143-152: The test is calling shouldReceive('loadMissing') on a
real model instance returned by Backup::factory()->make(), which fails because
make() returns an Eloquent model not a Mockery mock; replace the factory-made
Backup with a Mockery partial mock (e.g.,
Mockery::mock(Backup::class)->makePartial()) or use your test helper partialMock
for Backup, then set the id/name/bytes attributes (or setRelation('server',
$server)) on that mock and stub loadMissing to returnSelf so
shouldReceive('loadMissing')->andReturnSelf() is invoked on a mock rather than a
real model.
- Around line 95-103: The test is calling $backup->shouldReceive('loadMissing')
on an instance created by Backup::factory()->make(), which isn't a Mockery mock
and will fail; replace the factory-made model with a partial mock or spy: create
a Mockery partial mock of Backup (e.g., m::mock(Backup::class)->makePartial())
and set up the expected method stubs (loadMissing and any attribute getters) on
that mock, or alternatively avoid shouldReceive and use the factory-produced
model but assert behavior without stubbing loadMissing; update references to the
Backup instance where loadMissing is stubbed (the $backup variable and the
shouldReceive('loadMissing')->with('server')->andReturnSelf() call) accordingly.
🧹 Nitpick comments (4)
tests/Unit/Events/Backup/BackupCompletedTest.php (1)
46-51: Consider usingclass_uses_recursivefor robustness.
class_uses()only returns traits directly used by the class, not inherited traits. While this works correctly here sinceBackupCompleteddirectly usesSerializesModels, using Laravel'sclass_uses_recursive()helper would be more resilient to future refactoring.♻️ Suggested improvement
public function test_event_uses_serializes_models_trait(): void { - $traits = class_uses(BackupCompleted::class); + $traits = class_uses_recursive(BackupCompleted::class); $this->assertContains('Illuminate\Queue\SerializesModels', $traits); }tests/Unit/Listeners/Backup/BackupCompletedListenerTest.php (1)
182-219: Good coverage for null language handling.The test correctly verifies that the listener handles a null language gracefully by falling back to 'en'. However, the same
setRelation('language', null)issue exists on line 195 — it should be removed since the factory already sets the attribute.tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php (2)
22-23: Unused$uservariable from destructuring.The static analysis correctly flags the unused
$uservariable. Since only the server is needed, you can use a placeholder to suppress the warning:[, $server] = $this->generateTestAccount();Also applies to: 30-30, 58-58, 80-80, 111-111, 143-143, 168-168
131-131: Unused$channelsparameter in closure.The static analysis correctly flags the unused
$channelsparameter. It can be removed:function ($notification) use ($backup) { return $notification->backup->id === $backup->id; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/Events/Backup/BackupCompleted.phpapp/Http/Controllers/Api/Remote/Backups/BackupStatusController.phpapp/Listeners/Backup/BackupCompletedListener.phpapp/Notifications/BackupCompleted.phpapp/Services/Helpers/PluginService.phpconfig/panel.phplang/en/notifications.phptests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.phptests/Unit/Events/Backup/BackupCompletedTest.phptests/Unit/Listeners/Backup/BackupCompletedListenerTest.phptests/Unit/Notifications/BackupCompletedTest.php
🧰 Additional context used
🧬 Code graph analysis (8)
app/Listeners/Backup/BackupCompletedListener.php (3)
app/Models/Backup.php (1)
Backup(36-107)app/Events/Backup/BackupCompleted.php (1)
BackupCompleted(11-19)app/Notifications/BackupCompleted.php (1)
BackupCompleted(13-35)
app/Events/Backup/BackupCompleted.php (2)
app/Models/Backup.php (1)
Backup(36-107)app/Events/Event.php (1)
Event(5-5)
app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php (3)
app/Models/Backup.php (1)
Backup(36-107)app/Events/Backup/BackupCompleted.php (1)
BackupCompleted(11-19)app/Notifications/BackupCompleted.php (1)
BackupCompleted(13-35)
app/Notifications/BackupCompleted.php (3)
app/Models/Backup.php (1)
Backup(36-107)app/Events/Backup/BackupCompleted.php (2)
BackupCompleted(11-19)__construct(18-18)app/helpers.php (1)
convert_bytes_to_readable(36-49)
tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php (2)
app/Events/Backup/BackupCompleted.php (1)
BackupCompleted(11-19)app/Notifications/BackupCompleted.php (1)
BackupCompleted(13-35)
tests/Unit/Listeners/Backup/BackupCompletedListenerTest.php (6)
app/Models/Backup.php (1)
Backup(36-107)app/Events/Backup/BackupCompleted.php (1)
BackupCompleted(11-19)app/Notifications/BackupCompleted.php (1)
BackupCompleted(13-35)app/Listeners/Backup/BackupCompletedListener.php (2)
BackupCompletedListener(11-40)handle(13-39)app/Models/Server.php (1)
Server(135-552)app/Models/User.php (1)
User(97-506)
tests/Unit/Events/Backup/BackupCompletedTest.php (3)
app/Models/Backup.php (1)
Backup(36-107)app/Events/Backup/BackupCompleted.php (1)
BackupCompleted(11-19)app/Notifications/BackupCompleted.php (1)
BackupCompleted(13-35)
app/Services/Helpers/PluginService.php (1)
app/Models/Plugin.php (1)
isTheme(227-230)
🪛 PHPMD (2.15.0)
tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php
22-22: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
30-30: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
58-58: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
80-80: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
111-111: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
131-131: Avoid unused parameters such as '$channels'. (undefined)
(UnusedFormalParameter)
143-143: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
168-168: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (14)
app/Services/Helpers/PluginService.php (3)
260-283: LGTM!The new
$throwparameter provides good flexibility for callers to control error behavior. The improved error message "Could not install yarn dependencies" is more specific, and the logic to throw when either$throwis true or dev mode is active is appropriate.Minor style note: the parentheses in
throw ($exception)at line 276 are unnecessary but harmless.
299-299: LGTM!Passing
$plugin->isTheme()ensures that theme installations fail loudly when assets don't build correctly, which is appropriate since themes directly affect the UI. Regular plugins can tolerate soft failures.
346-346: Verify intended behavior:buildAssets()call missing theme flag.The
installPluginmethod now passes$plugin->isTheme()tobuildAssets(), butuninstallPluginstill callsbuildAssets()without the flag. This appears inconsistent—if a theme is being uninstalled and the asset rebuild fails, the panel could be left in a broken visual state.Was this intentional (uninstall failures should always be soft), or should this also pass the theme flag for consistency?
Suggested fix if the flag should be passed
- $this->buildAssets(); + $this->buildAssets($plugin->isTheme());config/panel.php (1)
51-52: LGTM!The new configuration entry follows the established pattern for notification settings in this file, with consistent naming and a sensible default value.
app/Events/Backup/BackupCompleted.php (1)
11-19: LGTM!The event class follows Laravel conventions with proper use of
SerializesModelsfor queue serialization and clean PHP 8+ promoted constructor properties.app/Notifications/BackupCompleted.php (2)
13-17: LGTM on the notification structure.The notification correctly implements
ShouldQueuefor async processing and uses theQueueabletrait. The promoted constructor property is clean.
25-34: This concern has already been addressed in the listener.The
BackupCompletedListeneralready eager-loads the server relationship on line 15 ($event->backup->loadMissing('server');) before dispatching the notification on line 37. This prevents the N+1 query issue when the notification is dequeued and$this->backup->server->nameis accessed during mail rendering.Likely an incorrect or invalid review comment.
tests/Unit/Events/Backup/BackupCompletedTest.php (1)
3-9: The namespaceApp\Tests\Unit\Events\Backupis correct and matches the established project convention. All test files in the repository consistently use theApp\Tests\*pattern, including Unit, Integration, Feature, Traits, and Seeders directories.lang/en/notifications.php (1)
9-11: > Likely an incorrect or invalid review comment.app/Listeners/Backup/BackupCompletedListener.php (1)
20-38: Notification and email dispatch logic looks correct.The Filament database notification is properly constructed with localized strings, a view action linking to the backups page, and correctly sends to the owner. The conditional email notification respects the config toggle. The use of
config()->get()with a default value provides a safe fallback.tests/Unit/Listeners/Backup/BackupCompletedListenerTest.php (1)
74-77: Config mock may not interceptconfig()->get()calls.The listener uses
config()->get('panel.email.send_backup_notification', true), which resolves the config repository via theconfig()helper, not theConfigfacade directly. MockingConfig::shouldReceive('get')may not intercept this call. Consider usingConfig::set()before invoking the listener instead:Config::set('panel.email.send_backup_notification', false);app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php (1)
83-86: Event dispatch after successful backup looks correct.The
BackupCompletedevent is properly dispatched outside the transaction (ensuring data is committed first) and only for successful backups. The event carries the backup model, server, and server owner as expected by the event constructor and listener.One consideration:
$server->usermay trigger a lazy-load query. If performance is a concern for high-volume backup scenarios, consider eager-loading the user relationship or passing it explicitly.tests/Unit/Notifications/BackupCompletedTest.php (1)
13-157: Comprehensive and well-structured notification tests.The test suite provides thorough coverage of the
BackupCompletednotification:
- Verifies instantiation and property accessibility
- Confirms
ShouldQueueimplementation andQueueabletrait usage- Validates mail channel configuration
- Checks mail message content (greeting, backup details, action button)
The tests are properly isolated using factory-made models with manually set relationships.
tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php (1)
195-205: Helper method for node authorization is well-implemented.The
withNodeAuthorizationmethod properly constructs the Bearer token from the node's daemon credentials and returns$thisfor method chaining.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| $event->backup->loadMissing('server'); | ||
| $event->owner->loadMissing('language'); | ||
|
|
||
| $locale = $event->owner->language ?? 'en'; |
There was a problem hiding this comment.
loadMissing('language') is incorrect — language is a scalar attribute, not a relationship.
Looking at the User model, language is defined as a string attribute ('language' => 'en' in $attributes), not a relationship. Calling loadMissing on it has no effect and is misleading. The null-coalescing on line 18 handles the case correctly already.
♻️ Proposed fix — remove the invalid loadMissing call
public function handle(BackupCompleted $event): void
{
$event->backup->loadMissing('server');
- $event->owner->loadMissing('language');
$locale = $event->owner->language ?? 'en';🤖 Prompt for AI Agents
In `@app/Listeners/Backup/BackupCompletedListener.php` around lines 15 - 18, The
call to loadMissing('language') on $event->owner is incorrect because language
is a scalar attribute, not a relationship; remove the
$event->owner->loadMissing('language') line from BackupCompletedListener (where
$event->backup->loadMissing('server') and $event->owner->loadMissing('language')
are used) so the code simply relies on $event->owner->language (and the existing
null-coalescing) to determine $locale.
| public function test_panel_notification_created_on_backup_completion(): void | ||
| { | ||
| // Don't fake events for this test - we want the full flow | ||
| Event::fake([]); | ||
|
|
||
| [$user, $server] = $this->generateTestAccount(); | ||
|
|
||
| $backup = Backup::factory()->create([ | ||
| 'server_id' => $server->id, | ||
| 'is_successful' => false, | ||
| 'completed_at' => null, | ||
| ]); | ||
|
|
||
| $this->withNodeAuthorization($server->node) | ||
| ->postJson("/api/remote/backups/{$backup->uuid}", [ | ||
| 'successful' => true, | ||
| 'checksum' => 'abc123', | ||
| 'checksum_type' => 'sha256', | ||
| 'size' => 1024000, | ||
| ]) | ||
| ->assertNoContent(); | ||
|
|
||
| // Verify notification exists in database | ||
| $this->assertDatabaseHas('notifications', [ | ||
| 'notifiable_type' => 'App\Models\User', | ||
| 'notifiable_id' => $server->user->id, | ||
| 'type' => 'Filament\Notifications\DatabaseNotification', | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Event::fake([]) does not allow real event dispatch.
The comment on line 77 says "Don't fake events for this test - we want the full flow", but Event::fake([]) still enables event faking (with an empty exclusion list). This means events are faked but none are excluded from faking — which is the opposite of the intent.
To allow real event dispatching, either:
- Don't call
Event::fake()at all, or - Use
Event::fakeExcept([BackupCompleted::class])if you want to fake other events but allow this one
Since this test wants the full notification flow, simply remove the Event::fake([]) call.
🐛 Proposed fix
public function test_panel_notification_created_on_backup_completion(): void
{
- // Don't fake events for this test - we want the full flow
- Event::fake([]);
-
[$user, $server] = $this->generateTestAccount();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function test_panel_notification_created_on_backup_completion(): void | |
| { | |
| // Don't fake events for this test - we want the full flow | |
| Event::fake([]); | |
| [$user, $server] = $this->generateTestAccount(); | |
| $backup = Backup::factory()->create([ | |
| 'server_id' => $server->id, | |
| 'is_successful' => false, | |
| 'completed_at' => null, | |
| ]); | |
| $this->withNodeAuthorization($server->node) | |
| ->postJson("/api/remote/backups/{$backup->uuid}", [ | |
| 'successful' => true, | |
| 'checksum' => 'abc123', | |
| 'checksum_type' => 'sha256', | |
| 'size' => 1024000, | |
| ]) | |
| ->assertNoContent(); | |
| // Verify notification exists in database | |
| $this->assertDatabaseHas('notifications', [ | |
| 'notifiable_type' => 'App\Models\User', | |
| 'notifiable_id' => $server->user->id, | |
| 'type' => 'Filament\Notifications\DatabaseNotification', | |
| ]); | |
| } | |
| public function test_panel_notification_created_on_backup_completion(): void | |
| { | |
| [$user, $server] = $this->generateTestAccount(); | |
| $backup = Backup::factory()->create([ | |
| 'server_id' => $server->id, | |
| 'is_successful' => false, | |
| 'completed_at' => null, | |
| ]); | |
| $this->withNodeAuthorization($server->node) | |
| ->postJson("/api/remote/backups/{$backup->uuid}", [ | |
| 'successful' => true, | |
| 'checksum' => 'abc123', | |
| 'checksum_type' => 'sha256', | |
| 'size' => 1024000, | |
| ]) | |
| ->assertNoContent(); | |
| // Verify notification exists in database | |
| $this->assertDatabaseHas('notifications', [ | |
| 'notifiable_type' => 'App\Models\User', | |
| 'notifiable_id' => $server->user->id, | |
| 'type' => 'Filament\Notifications\DatabaseNotification', | |
| ]); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
80-80: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php`
around lines 75 - 103, The test
test_panel_notification_created_on_backup_completion incorrectly calls
Event::fake([]) which still fakes all events; remove the Event::fake([]) call
(or replace it with Event::fakeExcept([BackupCompleted::class]) if you intend to
fake others) so that the BackupCompleted event and subsequent notification flow
are actually dispatched; update the test to rely on real events and keep the
rest of the assertion logic unchanged.
| public function test_email_notification_sent_when_config_enabled(): void | ||
| { | ||
| Notification::fake(); | ||
| Event::fake([]); // Clear event fakes to allow real dispatch | ||
| Config::set('panel.email.send_backup_notification', true); | ||
|
|
||
| [$user, $server] = $this->generateTestAccount(); | ||
|
|
||
| $backup = Backup::factory()->create([ | ||
| 'server_id' => $server->id, | ||
| 'is_successful' => false, | ||
| 'completed_at' => null, | ||
| ]); | ||
|
|
||
| $this->withNodeAuthorization($server->node) | ||
| ->postJson("/api/remote/backups/{$backup->uuid}", [ | ||
| 'successful' => true, | ||
| 'checksum' => 'abc123', | ||
| 'checksum_type' => 'sha256', | ||
| 'size' => 1024000, | ||
| ]) | ||
| ->assertNoContent(); | ||
|
|
||
| Notification::assertSentTo( | ||
| [$server->user], | ||
| BackupCompletedNotification::class, | ||
| function ($notification, $channels) use ($backup) { | ||
| return $notification->backup->id === $backup->id; | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same issue: Event::fake([]) prevents real event dispatch.
Lines 107-108 have the same problem. This test wants to verify email notifications are sent through the real event/listener flow, but Event::fake([]) prevents the BackupCompleted event from being dispatched to its listener. The test will likely fail or pass incorrectly.
Remove Event::fake([]) to allow the real event dispatch and listener execution.
🐛 Proposed fix
public function test_email_notification_sent_when_config_enabled(): void
{
Notification::fake();
- Event::fake([]); // Clear event fakes to allow real dispatch
Config::set('panel.email.send_backup_notification', true);🧰 Tools
🪛 PHPMD (2.15.0)
111-111: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
131-131: Avoid unused parameters such as '$channels'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In `@tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php`
around lines 105 - 135, The test method
test_email_notification_sent_when_config_enabled is erroneously preventing real
event dispatch by calling Event::fake([]); remove that Event::fake([]) call (or
stop faking events) so the BackupCompleted event can be dispatched to its
listener and trigger BackupCompletedNotification; keep Notification::fake()
as-is and run the test to verify the notification is sent via the real
event/listener flow.
| public function test_email_notification_not_sent_when_config_disabled(): void | ||
| { | ||
| Notification::fake(); | ||
| Event::fake([]); // Clear event fakes to allow real dispatch | ||
| Config::set('panel.email.send_backup_notification', false); | ||
|
|
||
| [$user, $server] = $this->generateTestAccount(); | ||
|
|
||
| $backup = Backup::factory()->create([ | ||
| 'server_id' => $server->id, | ||
| 'is_successful' => false, | ||
| 'completed_at' => null, | ||
| ]); | ||
|
|
||
| $this->withNodeAuthorization($server->node) | ||
| ->postJson("/api/remote/backups/{$backup->uuid}", [ | ||
| 'successful' => true, | ||
| 'checksum' => 'abc123', | ||
| 'checksum_type' => 'sha256', | ||
| 'size' => 1024000, | ||
| ]) | ||
| ->assertNoContent(); | ||
|
|
||
| // Email notification should not be sent when config is disabled | ||
| Notification::assertNothingSent(); | ||
| } |
There was a problem hiding this comment.
Same Event::fake([]) issue and potential false positive on assertNothingSent.
- Line 140 has the same
Event::fake([])problem — it prevents real dispatch. - Line 161 uses
Notification::assertNothingSent(), which only checks Laravel notifications. Since the event won't dispatch due toEvent::fake([]), this assertion would pass regardless of the config setting, making it a false positive.
🐛 Proposed fix
public function test_email_notification_not_sent_when_config_disabled(): void
{
Notification::fake();
- Event::fake([]); // Clear event fakes to allow real dispatch
Config::set('panel.email.send_backup_notification', false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function test_email_notification_not_sent_when_config_disabled(): void | |
| { | |
| Notification::fake(); | |
| Event::fake([]); // Clear event fakes to allow real dispatch | |
| Config::set('panel.email.send_backup_notification', false); | |
| [$user, $server] = $this->generateTestAccount(); | |
| $backup = Backup::factory()->create([ | |
| 'server_id' => $server->id, | |
| 'is_successful' => false, | |
| 'completed_at' => null, | |
| ]); | |
| $this->withNodeAuthorization($server->node) | |
| ->postJson("/api/remote/backups/{$backup->uuid}", [ | |
| 'successful' => true, | |
| 'checksum' => 'abc123', | |
| 'checksum_type' => 'sha256', | |
| 'size' => 1024000, | |
| ]) | |
| ->assertNoContent(); | |
| // Email notification should not be sent when config is disabled | |
| Notification::assertNothingSent(); | |
| } | |
| public function test_email_notification_not_sent_when_config_disabled(): void | |
| { | |
| Notification::fake(); | |
| Config::set('panel.email.send_backup_notification', false); | |
| [$user, $server] = $this->generateTestAccount(); | |
| $backup = Backup::factory()->create([ | |
| 'server_id' => $server->id, | |
| 'is_successful' => false, | |
| 'completed_at' => null, | |
| ]); | |
| $this->withNodeAuthorization($server->node) | |
| ->postJson("/api/remote/backups/{$backup->uuid}", [ | |
| 'successful' => true, | |
| 'checksum' => 'abc123', | |
| 'checksum_type' => 'sha256', | |
| 'size' => 1024000, | |
| ]) | |
| ->assertNoContent(); | |
| // Email notification should not be sent when config is disabled | |
| Notification::assertNothingSent(); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
143-143: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/Integration/Api/Remote/Backups/BackupCompletionNotificationTest.php`
around lines 137 - 162, The test method
test_email_notification_not_sent_when_config_disabled() incorrectly calls
Event::fake([]), preventing the backup completion event from being dispatched
and causing Notification::assertNothingSent() to give a false positive; remove
the Event::fake([]) (or replace it with a more targeted fake such as
Event::fakeExcept([...]) if other tests require faking) so the real event is
dispatched when posting to the /api/remote/backups/{$backup->uuid} endpoint, and
keep Notification::assertNothingSent() to verify no notifications were sent when
Config::set('panel.email.send_backup_notification', false).
| $backup = Backup::factory()->make([ | ||
| 'id' => 1, | ||
| 'name' => 'test-backup.tar.gz', | ||
| 'bytes' => 1024000, | ||
| ]); | ||
|
|
||
| // Set up relationships | ||
| $backup->setRelation('server', $server); | ||
| $user->setRelation('language', 'en'); |
There was a problem hiding this comment.
Incorrect mock setup for setRelation with scalar value.
On line 44, $user->setRelation('language', 'en') attempts to set a string as a relationship, but language is a string attribute on the User model, not a relationship. This mirrors the incorrect assumption in the listener code. Since the listener accesses $event->owner->language as an attribute, you should use the factory's attribute assignment instead:
$user = User::factory()->make(['id' => 1, 'language' => 'en']);Line 34 already does this correctly, so line 44 can be removed.
🤖 Prompt for AI Agents
In `@tests/Unit/Listeners/Backup/BackupCompletedListenerTest.php` around lines 36
- 44, The test incorrectly calls $user->setRelation('language', 'en') treating
'language' as a relationship; remove that setRelation call and ensure the User
factory sets the language attribute (e.g., User::factory()->make([... 'language'
=> 'en' ...])) so the listener can read $event->owner->language as a plain
attribute; update the test to rely on the factory-provided 'language' attribute
and delete the erroneous setRelation invocation.
| $backup = Backup::factory()->make([ | ||
| 'id' => 1, | ||
| 'name' => 'test-backup.tar.gz', | ||
| 'bytes' => 1024000, | ||
| ]); | ||
|
|
||
| // Set up relationships | ||
| $backup->setRelation('server', $server); | ||
| $backup->shouldReceive('loadMissing')->with('server')->andReturnSelf(); |
There was a problem hiding this comment.
Cannot use shouldReceive on a factory-made model.
Line 103 calls $backup->shouldReceive('loadMissing'), but $backup is created via Backup::factory()->make(), not as a Mockery mock. This will fail at runtime. Either create the backup as a partial mock like the user:
$backup = m::mock(Backup::class)->makePartial();
$backup->shouldReceive('getAttribute')->with('name')->andReturn('test-backup.tar.gz');
// ... other setupOr use the spy/fake approach consistently.
🤖 Prompt for AI Agents
In `@tests/Unit/Listeners/Backup/BackupCompletedListenerTest.php` around lines 95
- 103, The test is calling $backup->shouldReceive('loadMissing') on an instance
created by Backup::factory()->make(), which isn't a Mockery mock and will fail;
replace the factory-made model with a partial mock or spy: create a Mockery
partial mock of Backup (e.g., m::mock(Backup::class)->makePartial()) and set up
the expected method stubs (loadMissing and any attribute getters) on that mock,
or alternatively avoid shouldReceive and use the factory-produced model but
assert behavior without stubbing loadMissing; update references to the Backup
instance where loadMissing is stubbed (the $backup variable and the
shouldReceive('loadMissing')->with('server')->andReturnSelf() call) accordingly.
| $server = Server::factory()->make(['id' => 1, 'name' => 'Test Server']); | ||
| $backup = Backup::factory()->make([ | ||
| 'id' => 1, | ||
| 'name' => 'test-backup.tar.gz', | ||
| 'bytes' => 1024000, | ||
| ]); | ||
|
|
||
| // Set up relationships | ||
| $backup->setRelation('server', $server); | ||
| $backup->shouldReceive('loadMissing')->with('server')->andReturnSelf(); |
There was a problem hiding this comment.
Same issue: shouldReceive called on factory-made model.
Lines 151-152 repeat the same problem — calling $backup->shouldReceive('loadMissing') on a factory-made Backup model. The test will fail because Backup::factory()->make() returns a real Eloquent model, not a Mockery mock.
🤖 Prompt for AI Agents
In `@tests/Unit/Listeners/Backup/BackupCompletedListenerTest.php` around lines 143
- 152, The test is calling shouldReceive('loadMissing') on a real model instance
returned by Backup::factory()->make(), which fails because make() returns an
Eloquent model not a Mockery mock; replace the factory-made Backup with a
Mockery partial mock (e.g., Mockery::mock(Backup::class)->makePartial()) or use
your test helper partialMock for Backup, then set the id/name/bytes attributes
(or setRelation('server', $server)) on that mock and stub loadMissing to
returnSelf so shouldReceive('loadMissing')->andReturnSelf() is invoked on a mock
rather than a real model.
3 participants
Resolves #1742