backups: default is_successful to false (#3522)

* backups: default is_successful to false
* backups: properly query backups

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.');
         }
 

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',

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,

app/Repositories/Eloquent/BackupRepository.php

@@ -25,7 +25,10 @@ class BackupRepository extends EloquentRepository
         return $this->getBuilder()
             ->withTrashed()
             ->where('server_id', $server)
-            ->where('is_successful', true)
+            ->where(function ($query) {
+                $query->whereNull('completed_at')
+                    ->orWhere('is_successful', '=', true);
+            })
             ->where('created_at', '>=', Carbon::now()->subSeconds($seconds)->toDateTimeString())
             ->get()
             ->toBase();

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();
         }
 

app/Services/Backups/InitiateBackupService.php

@@ -132,8 +132,12 @@ 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()->where(function ($query) {
+            $query->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) {

database/migrations/2021_08_03_210600_change_successful_field_to_default_to_false_on_backups_table.php

@@ -0,0 +1,37 @@
+<?php
+
+use Illuminate\Support\Facades\Schema;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Database\Migrations\Migration;
+
+class ChangeSuccessfulFieldToDefaultToFalseOnBackupsTable extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::table('backups', function (Blueprint $table) {
+            $table->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();
+        });
+    }
+}

resources/scripts/components/server/backups/BackupRow.tsx

@@ -44,7 +44,7 @@ export default ({ backup, className }: Props) => {
         <GreyRowBox css={tw`flex-wrap md:flex-nowrap items-center`} className={className}>
             <div css={tw`flex items-center truncate w-full md:flex-1`}>
                 <div css={tw`mr-4`}>
-                    {backup.completedAt ?
+                    {backup.completedAt !== null ?
                         backup.isLocked ?
                             <FontAwesomeIcon icon={faLock} css={tw`text-yellow-500`}/>
                             :
@@ -55,7 +55,7 @@ export default ({ backup, className }: Props) => {
                 </div>
                 <div css={tw`flex flex-col truncate`}>
                     <div css={tw`flex items-center text-sm mb-1`}>
-                        {!backup.isSuccessful &&
+                        {backup.completedAt !== null && !backup.isSuccessful &&
                         <span css={tw`bg-red-500 py-px px-2 rounded-full text-white text-xs uppercase border border-red-600 mr-2`}>
                             Failed
                         </span>
@@ -63,7 +63,7 @@ export default ({ backup, className }: Props) => {
                         <p css={tw`break-words truncate`}>
                             {backup.name}
                         </p>
-                        {(backup.completedAt && backup.isSuccessful) &&
+                        {(backup.completedAt !== null && backup.isSuccessful) &&
                         <span css={tw`ml-3 text-neutral-300 text-xs font-extralight hidden sm:inline`}>{bytesToHuman(backup.bytes)}</span>
                         }
                     </div>