From c46131e7adb6af7c862515b8ee68ecd89266afac Mon Sep 17 00:00:00 2001 From: Matthew Penner Date: Tue, 3 Aug 2021 15:46:08 -0600 Subject: [PATCH] backups: default is_successful to false --- .../Api/Client/Servers/BackupController.php | 2 +- .../Remote/ReportBackupCompleteRequest.php | 2 +- app/Models/Backup.php | 2 +- .../Eloquent/BackupRepository.php | 3 +- app/Services/Backups/DeleteBackupService.php | 2 +- .../Backups/InitiateBackupService.php | 5 ++- ...d_to_default_to_false_on_backups_table.php | 37 +++++++++++++++++++ .../components/server/backups/BackupRow.tsx | 6 +-- 8 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 database/migrations/2021_08_03_210600_change_successful_field_to_default_to_false_on_backups_table.php diff --git a/app/Http/Controllers/Api/Client/Servers/BackupController.php b/app/Http/Controllers/Api/Client/Servers/BackupController.php index 5bc58a813..fe9232e8e 100644 --- a/app/Http/Controllers/Api/Client/Servers/BackupController.php +++ b/app/Http/Controllers/Api/Client/Servers/BackupController.php @@ -225,7 +225,7 @@ class BackupController extends ClientApiController throw new BadRequestHttpException('This server is not currently in a state that allows for a backup to be restored.'); } - if (!$backup->is_successful && !$backup->completed_at) { + if (!$backup->is_successful && is_null($backup->completed_at)) { throw new BadRequestHttpException('This backup cannot be restored at this time: not completed or failed.'); } diff --git a/app/Http/Requests/Api/Remote/ReportBackupCompleteRequest.php b/app/Http/Requests/Api/Remote/ReportBackupCompleteRequest.php index a90a2b2b9..0c96b3f02 100644 --- a/app/Http/Requests/Api/Remote/ReportBackupCompleteRequest.php +++ b/app/Http/Requests/Api/Remote/ReportBackupCompleteRequest.php @@ -12,7 +12,7 @@ class ReportBackupCompleteRequest extends FormRequest public function rules() { return [ - 'successful' => 'present|boolean', + 'successful' => 'required|boolean', 'checksum' => 'nullable|string|required_if:successful,true', 'checksum_type' => 'nullable|string|required_if:successful,true', 'size' => 'nullable|numeric|required_if:successful,true', diff --git a/app/Models/Backup.php b/app/Models/Backup.php index 26dcc7242..9ad03fd32 100644 --- a/app/Models/Backup.php +++ b/app/Models/Backup.php @@ -64,7 +64,7 @@ class Backup extends Model * @var array */ protected $attributes = [ - 'is_successful' => true, + 'is_successful' => false, 'is_locked' => false, 'checksum' => null, 'bytes' => 0, diff --git a/app/Repositories/Eloquent/BackupRepository.php b/app/Repositories/Eloquent/BackupRepository.php index b53547e44..b8614ebb2 100644 --- a/app/Repositories/Eloquent/BackupRepository.php +++ b/app/Repositories/Eloquent/BackupRepository.php @@ -25,7 +25,8 @@ class BackupRepository extends EloquentRepository return $this->getBuilder() ->withTrashed() ->where('server_id', $server) - ->where('is_successful', true) + ->whereNull('completed_at') + ->orWhere('is_successful', '=', true) ->where('created_at', '>=', Carbon::now()->subSeconds($seconds)->toDateTimeString()) ->get() ->toBase(); diff --git a/app/Services/Backups/DeleteBackupService.php b/app/Services/Backups/DeleteBackupService.php index 80d6374b1..66eefe675 100644 --- a/app/Services/Backups/DeleteBackupService.php +++ b/app/Services/Backups/DeleteBackupService.php @@ -63,7 +63,7 @@ class DeleteBackupService // I also don't really see any reason you'd have a locked, failed backup to keep // around. The logic that updates the backup to the failed state will also remove // the lock, so this condition should really never happen. - if ($backup->is_locked && ($backup->completed_at && $backup->is_successful)) { + if ($backup->is_locked && ($backup->is_successful && !is_null($backup->completed_at))) { throw new BackupLockedException(); } diff --git a/app/Services/Backups/InitiateBackupService.php b/app/Services/Backups/InitiateBackupService.php index 347740dc1..02606bf16 100644 --- a/app/Services/Backups/InitiateBackupService.php +++ b/app/Services/Backups/InitiateBackupService.php @@ -132,8 +132,9 @@ class InitiateBackupService } } - // Check if the server has reached or exceeded it's backup limit. - $successful = $server->backups()->where('is_successful', true); + // Check if the server has reached or exceeded its backup limit. + // completed_at == null will cover any ongoing backups, while is_successful == true will cover any completed backups. + $successful = $server->backups()->whereNull('completed_at')->orWhere('is_successful', true); if (!$server->backup_limit || $successful->count() >= $server->backup_limit) { // Do not allow the user to continue if this server is already at its limit and can't override. if (!$override || $server->backup_limit <= 0) { diff --git a/database/migrations/2021_08_03_210600_change_successful_field_to_default_to_false_on_backups_table.php b/database/migrations/2021_08_03_210600_change_successful_field_to_default_to_false_on_backups_table.php new file mode 100644 index 000000000..1dddb7f3b --- /dev/null +++ b/database/migrations/2021_08_03_210600_change_successful_field_to_default_to_false_on_backups_table.php @@ -0,0 +1,37 @@ +boolean('is_successful')->after('uuid')->default(false)->change(); + }); + + // Convert currently processing backups to the new format so things don't break. + DB::table('backups')->select('id')->where('is_successful', 1)->whereNull('completed_at')->update([ + 'is_successful' => 0, + ]); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('backups', function (Blueprint $table) { + $table->boolean('is_successful')->after('uuid')->default(true)->change(); + }); + } +} diff --git a/resources/scripts/components/server/backups/BackupRow.tsx b/resources/scripts/components/server/backups/BackupRow.tsx index 139ce33ba..459a0e404 100644 --- a/resources/scripts/components/server/backups/BackupRow.tsx +++ b/resources/scripts/components/server/backups/BackupRow.tsx @@ -44,7 +44,7 @@ export default ({ backup, className }: Props) => {
- {backup.completedAt ? + {backup.completedAt !== null ? backup.isLocked ? : @@ -55,7 +55,7 @@ export default ({ backup, className }: Props) => {
- {!backup.isSuccessful && + {backup.completedAt !== null && !backup.isSuccessful && Failed @@ -63,7 +63,7 @@ export default ({ backup, className }: Props) => {

{backup.name}

- {(backup.completedAt && backup.isSuccessful) && + {(backup.completedAt !== null && backup.isSuccessful) && }