Correctly handle schedule task deletion and avoid errors; closes #2534

app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php

@@ -56,7 +56,8 @@ class ScheduleTaskController extends ClientApiController
             );
         }
 
-        $lastTask = $schedule->tasks->last();
+        /** @var \Pterodactyl\Models\Task|null $lastTask */
+        $lastTask = $schedule->tasks()->orderByDesc('sequence_id')->first();
 
         /** @var \Pterodactyl\Models\Task $task */
         $task = $this->repository->create([
@@ -102,13 +103,16 @@ class ScheduleTaskController extends ClientApiController
     }
 
     /**
-     * Determines if a user can delete the task for a given server.
+     * Delete a given task for a schedule. If there are subsequent tasks stored in the database
+     * for this schedule their sequence IDs are decremented properly.
      *
      * @param \Pterodactyl\Http\Requests\Api\Client\ClientApiRequest $request
      * @param \Pterodactyl\Models\Server $server
      * @param \Pterodactyl\Models\Schedule $schedule
      * @param \Pterodactyl\Models\Task $task
      * @return \Illuminate\Http\JsonResponse
+     *
+     * @throws \Exception
      */
     public function delete(ClientApiRequest $request, Server $server, Schedule $schedule, Task $task)
     {
@@ -120,8 +124,12 @@ class ScheduleTaskController extends ClientApiController
             throw new HttpForbiddenException('You do not have permission to perform this action.');
         }
 
-        $this->repository->delete($task->id);
+        $schedule->tasks()->where('sequence_id', '>', $task->sequence_id)->update([
+            'sequence_id' => $schedule->tasks()->getConnection()->raw('(sequence_id - 1)'),
+        ]);
 
-        return JsonResponse::create(null, Response::HTTP_NO_CONTENT);
+        $task->delete();
+
+        return new JsonResponse(null, Response::HTTP_NO_CONTENT);
     }
 }

app/Services/Schedules/ProcessScheduleService.php

@@ -43,7 +43,7 @@ class ProcessScheduleService
     public function handle(Schedule $schedule, bool $now = false)
     {
         /** @var \Pterodactyl\Models\Task $task */
-        $task = $schedule->tasks()->where('sequence_id', 1)->first();
+        $task = $schedule->tasks()->orderBy('sequence_id', 'asc')->first();
 
         if (is_null($task)) {
             throw new DisplayException(

tests/Integration/Api/Client/Server/ScheduleTask/DeleteScheduleTaskTest.php

@@ -0,0 +1,84 @@
+<?php
+
+namespace Pterodactyl\Tests\Integration\Api\Client\Server\ScheduleTask;
+
+use Pterodactyl\Models\Task;
+use Pterodactyl\Models\User;
+use Illuminate\Http\Response;
+use Pterodactyl\Models\Schedule;
+use Pterodactyl\Models\Permission;
+use Pterodactyl\Tests\Integration\Api\Client\ClientApiIntegrationTestCase;
+
+class DeleteScheduleTaskTest extends ClientApiIntegrationTestCase
+{
+    /**
+     * Test that an error is returned if the schedule does not belong to the server.
+     */
+    public function testScheduleNotBelongingToServerReturnsError()
+    {
+        $server2 = $this->createServerModel();
+        [$user] = $this->generateTestAccount();
+
+        $schedule = factory(Schedule::class)->create(['server_id' => $server2->id]);
+        $task = factory(Task::class)->create(['schedule_id' => $schedule->id]);
+
+        $this->actingAs($user)->deleteJson($this->link($task))->assertNotFound();
+    }
+
+    /**
+     * Test that an error is returned if the task and schedule in the URL do not line up
+     * with eachother.
+     */
+    public function testTaskBelongingToDifferentScheduleReturnsError()
+    {
+        [$user, $server] = $this->generateTestAccount();
+
+        $schedule = factory(Schedule::class)->create(['server_id' => $server->id]);
+        $schedule2 = factory(Schedule::class)->create(['server_id' => $server->id]);
+        $task = factory(Task::class)->create(['schedule_id' => $schedule->id]);
+
+        $this->actingAs($user)->deleteJson("/api/client/servers/{$server->uuid}/schedules/{$schedule2->id}/tasks/{$task->id}")->assertNotFound();
+    }
+
+    /**
+     * Test that a user without the required permissions returns an error.
+     */
+    public function testUserWithoutPermissionReturnsError()
+    {
+        [$user, $server] = $this->generateTestAccount([Permission::ACTION_SCHEDULE_CREATE]);
+
+        $schedule = factory(Schedule::class)->create(['server_id' => $server->id]);
+        $task = factory(Task::class)->create(['schedule_id' => $schedule->id]);
+
+        $this->actingAs($user)->deleteJson($this->link($task))->assertForbidden();
+
+        $user2 = factory(User::class)->create();
+
+        $this->actingAs($user2)->deleteJson($this->link($task))->assertNotFound();
+    }
+
+    /**
+     * Test that a schedule task is deleted and items with a higher sequence ID are decremented
+     * properly in the database.
+     */
+    public function testScheduleTaskIsDeletedAndSubsequentTasksAreUpdated()
+    {
+        [$user, $server] = $this->generateTestAccount();
+
+        $schedule = factory(Schedule::class)->create(['server_id' => $server->id]);
+        $tasks = [
+            factory(Task::class)->create(['schedule_id' => $schedule->id, 'sequence_id' => 1]),
+            factory(Task::class)->create(['schedule_id' => $schedule->id, 'sequence_id' => 2]),
+            factory(Task::class)->create(['schedule_id' => $schedule->id, 'sequence_id' => 3]),
+            factory(Task::class)->create(['schedule_id' => $schedule->id, 'sequence_id' => 4]),
+        ];
+
+        $response = $this->actingAs($user)->deleteJson($this->link($tasks[1]));
+        $response->assertStatus(Response::HTTP_NO_CONTENT);
+
+        $this->assertDatabaseHas('tasks', ['id' => $tasks[0]->id, 'sequence_id' => 1]);
+        $this->assertDatabaseHas('tasks', ['id' => $tasks[2]->id, 'sequence_id' => 2]);
+        $this->assertDatabaseHas('tasks', ['id' => $tasks[3]->id, 'sequence_id' => 3]);
+        $this->assertDatabaseMissing('tasks', ['id' => $tasks[1]->id]);
+    }
+}

tests/Integration/Services/Schedules/ProcessScheduleServiceTest.php

@@ -81,6 +81,37 @@ class ProcessScheduleServiceTest extends IntegrationTestCase
         $this->assertDatabaseHas('tasks', ['id' => $task->id, 'is_queued' => true]);
     }
 
+    /**
+     * Test that even if a schedule's task sequence gets messed up the first task based on
+     * the ascending order of tasks is used.
+     *
+     * @see https://github.com/pterodactyl/panel/issues/2534
+     */
+    public function testFirstSequenceTaskIsFound()
+    {
+        $this->swap(Dispatcher::class, $dispatcher = Mockery::mock(Dispatcher::class));
+
+        $server = $this->createServerModel();
+        /** @var \Pterodactyl\Models\Schedule $schedule */
+        $schedule = factory(Schedule::class)->create(['server_id' => $server->id]);
+
+        /** @var \Pterodactyl\Models\Task $task */
+        $task2 = factory(Task::class)->create(['schedule_id' => $schedule->id, 'sequence_id' => 4]);
+        $task = factory(Task::class)->create(['schedule_id' => $schedule->id, 'sequence_id' => 2]);
+        $task3 = factory(Task::class)->create(['schedule_id' => $schedule->id, 'sequence_id' => 3]);
+
+        $dispatcher->expects('dispatch')->with(Mockery::on(function (RunTaskJob $job) use ($task) {
+            return $task->id === $job->task->id;
+        }));
+
+        $this->getService()->handle($schedule);
+
+        $this->assertDatabaseHas('schedules', ['id' => $schedule->id, 'is_processing' => true]);
+        $this->assertDatabaseHas('tasks', ['id' => $task->id, 'is_queued' => true]);
+        $this->assertDatabaseHas('tasks', ['id' => $task2->id, 'is_queued' => false]);
+        $this->assertDatabaseHas('tasks', ['id' => $task3->id, 'is_queued' => false]);
+    }
+
     /**
      * @return array
      */