diff --git a/app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php b/app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php index 0d613b6b..c6f8ee33 100644 --- a/app/Http/Controllers/Api/Client/Servers/ScheduleTaskController.php +++ b/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); } } diff --git a/app/Services/Schedules/ProcessScheduleService.php b/app/Services/Schedules/ProcessScheduleService.php index 1f810d6f..ec16bc36 100644 --- a/app/Services/Schedules/ProcessScheduleService.php +++ b/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( diff --git a/tests/Integration/Api/Client/Server/ScheduleTask/DeleteScheduleTaskTest.php b/tests/Integration/Api/Client/Server/ScheduleTask/DeleteScheduleTaskTest.php new file mode 100644 index 00000000..ef3e1dee --- /dev/null +++ b/tests/Integration/Api/Client/Server/ScheduleTask/DeleteScheduleTaskTest.php @@ -0,0 +1,84 @@ +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]); + } +} diff --git a/tests/Integration/Services/Schedules/ProcessScheduleServiceTest.php b/tests/Integration/Services/Schedules/ProcessScheduleServiceTest.php index 4941a9bd..a5a4f65f 100644 --- a/tests/Integration/Services/Schedules/ProcessScheduleServiceTest.php +++ b/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 */