Fix error handling for deleting backups (#1434)

app/Filament/Server/Resources/BackupResource.php

@@ -14,6 +14,7 @@ use App\Repositories\Daemon\DaemonBackupRepository;
 use App\Services\Backups\DownloadLinkService;
 use App\Filament\Components\Tables\Columns\BytesColumn;
 use App\Filament\Components\Tables\Columns\DateTimeColumn;
+use App\Services\Backups\DeleteBackupService;
 use Filament\Facades\Filament;
 use Filament\Forms\Components\Checkbox;
 use Filament\Forms\Components\Placeholder;
@@ -30,6 +31,7 @@ use Filament\Tables\Columns\IconColumn;
 use Filament\Tables\Columns\TextColumn;
 use Filament\Tables\Table;
 use Illuminate\Database\Eloquent\Model;
+use Illuminate\Http\Client\ConnectionException;
 use Illuminate\Http\Request;
 
 class BackupResource extends Resource
@@ -142,7 +144,7 @@ class BackupResource extends Resource
                                     ->send();
                             }
 
-                            if (!$backup->is_successful && is_null($backup->completed_at)) { //TODO Change to Notifications
+                            if (!$backup->is_successful && is_null($backup->completed_at)) {
                                 return Notification::make()
                                     ->danger()
                                     ->title('Backup Restore Failed')
@@ -175,9 +177,26 @@ class BackupResource extends Resource
                         ->visible(fn (Backup $backup) => $backup->status === BackupStatus::Successful),
                     DeleteAction::make('delete')
                         ->disabled(fn (Backup $backup) => $backup->is_locked)
-                        ->modalDescription(fn (Backup $backup) => 'Do you wish to delete, ' . $backup->name . '?')
+                        ->modalDescription(fn (Backup $backup) => 'Do you wish to delete ' . $backup->name . '?')
                         ->modalSubmitActionLabel('Delete Backup')
-                        ->action(fn (BackupController $backupController, Backup $backup, Request $request) => $backupController->delete($request, $server, $backup))
+                        ->action(function (Backup $backup, DeleteBackupService $deleteBackupService) {
+                            try {
+                                $deleteBackupService->handle($backup);
+                            } catch (ConnectionException) {
+                                Notification::make()
+                                    ->title('Could not delete backup')
+                                    ->body('Connection to node failed')
+                                    ->danger()
+                                    ->send();
+
+                                return;
+                            }
+
+                            Activity::event('server:backup.delete')
+                                ->subject($backup)
+                                ->property(['name' => $backup->name, 'failed' => !$backup->is_successful])
+                                ->log();
+                        })
                         ->visible(fn (Backup $backup) => $backup->status !== BackupStatus::InProgress),
                 ]),
             ]);

app/Services/Backups/DeleteBackupService.php

@@ -6,12 +6,11 @@ use App\Extensions\Filesystem\S3Filesystem;
 use Aws\S3\S3Client;
 use Illuminate\Http\Response;
 use App\Models\Backup;
-use GuzzleHttp\Exception\ClientException;
 use Illuminate\Database\ConnectionInterface;
 use App\Extensions\Backups\BackupManager;
 use App\Repositories\Daemon\DaemonBackupRepository;
 use App\Exceptions\Service\Backup\BackupLockedException;
-use Illuminate\Http\Client\ConnectionException;
+use Exception;
 
 class DeleteBackupService
 {
@@ -48,12 +47,10 @@ class DeleteBackupService
         $this->connection->transaction(function () use ($backup) {
             try {
                 $this->daemonBackupRepository->setServer($backup->server)->delete($backup);
-            } catch (ConnectionException $exception) {
+            } catch (Exception $exception) {
-                $previous = $exception->getPrevious();
-
                 // Don't fail the request if the Daemon responds with a 404, just assume the backup
                 // doesn't actually exist and remove its reference from the Panel as well.
-                if (!$previous instanceof ClientException || $previous->getResponse()->getStatusCode() !== Response::HTTP_NOT_FOUND) {
+                if ($exception->getCode() !== Response::HTTP_NOT_FOUND) {
                     throw $exception;
                 }
             }

tests/Integration/Services/Backups/DeleteBackupServiceTest.php

@@ -2,10 +2,8 @@
 
 namespace App\Tests\Integration\Services\Backups;
 
-use GuzzleHttp\Psr7\Request;
 use GuzzleHttp\Psr7\Response;
 use App\Models\Backup;
-use GuzzleHttp\Exception\ClientException;
 use App\Extensions\Backups\BackupManager;
 use App\Extensions\Filesystem\S3Filesystem;
 use App\Services\Backups\DeleteBackupService;
@@ -54,7 +52,7 @@ class DeleteBackupServiceTest extends IntegrationTestCase
         $backup = Backup::factory()->create(['server_id' => $server->id]);
 
         $mock = $this->mock(DaemonBackupRepository::class);
-        $mock->expects('setServer->delete')->with($backup)->andThrow(new ConnectionException(previous: new ClientException('', new Request('DELETE', '/'), new Response(404))));
+        $mock->expects('setServer->delete')->with($backup)->andThrow(new ConnectionException(code: 404));
 
         $this->app->make(DeleteBackupService::class)->handle($backup);
 
@@ -69,7 +67,7 @@ class DeleteBackupServiceTest extends IntegrationTestCase
         $backup = Backup::factory()->create(['server_id' => $server->id]);
 
         $mock = $this->mock(DaemonBackupRepository::class);
-        $mock->expects('setServer->delete')->with($backup)->andThrow(new ConnectionException(previous: new ClientException('', new Request('DELETE', '/'), new Response(500))));
+        $mock->expects('setServer->delete')->with($backup)->andThrow(new ConnectionException(code: 500));
 
         $this->expectException(ConnectionException::class);