From 02853d50ca0eadd607c1fd8efe99628901852dd2 Mon Sep 17 00:00:00 2001 From: dartcafe Date: Fri, 14 Nov 2025 19:52:14 +0100 Subject: [PATCH 1/8] fix and optimize job management Signed-off-by: dartcafe --- lib/Controller/AdminController.php | 2 +- lib/Cron/AutoReminderCron.php | 6 +- lib/Cron/JanitorCron.php | 7 +- lib/Cron/NotificationCron.php | 7 +- lib/Cron/TimedCronJob.php | 43 ------ lib/Service/SystemService.php | 37 ++--- src/Api/modules/admin.ts | 4 +- src/Api/modules/api.types.ts | 4 +- .../Settings/AdminSettings/AdminJobs.vue | 144 +++++++++--------- src/views/AdminSettingsPage.vue | 2 +- 10 files changed, 113 insertions(+), 143 deletions(-) delete mode 100644 lib/Cron/TimedCronJob.php diff --git a/lib/Controller/AdminController.php b/lib/Controller/AdminController.php index 62d02d3862..670266aaf5 100644 --- a/lib/Controller/AdminController.php +++ b/lib/Controller/AdminController.php @@ -71,7 +71,7 @@ public function getJobs(): JSONResponse { */ #[FrontpageRoute(verb: 'POST', url: '/administration/job/run')] public function runJob($job): JSONResponse { - return $this->response(fn () => $this->systemService->runJob($job)); + return $this->response(fn () => ['job' => $this->systemService->runJob($job)]); } /** diff --git a/lib/Cron/AutoReminderCron.php b/lib/Cron/AutoReminderCron.php index df3e53c450..2ce8f17bb9 100644 --- a/lib/Cron/AutoReminderCron.php +++ b/lib/Cron/AutoReminderCron.php @@ -13,6 +13,7 @@ use OCA\Polls\Attributes\ManuallyRunnableCronJob; use OCA\Polls\Service\MailService; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\BackgroundJob\TimedJob; use OCP\ISession; use Psr\Log\LoggerInterface; @@ -20,7 +21,7 @@ * @psalm-api */ #[ManuallyRunnableCronJob] -class AutoReminderCron extends TimedCronJob { +class AutoReminderCron extends TimedJob { public function __construct( protected ITimeFactory $time, protected LoggerInterface $logger, @@ -28,7 +29,8 @@ public function __construct( private ISession $session, protected bool $supportsManualRun = true, ) { - parent::setInterval(30); // run every 30 minutes + parent::__construct($time); + parent::setInterval(300); // run every 5 minutes } /** diff --git a/lib/Cron/JanitorCron.php b/lib/Cron/JanitorCron.php index 300879b45a..133f647135 100644 --- a/lib/Cron/JanitorCron.php +++ b/lib/Cron/JanitorCron.php @@ -19,6 +19,8 @@ use OCA\Polls\Db\V5\TableManager; use OCA\Polls\Db\VoteMapper; use OCA\Polls\Model\Settings\AppSettings; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\BackgroundJob\TimedJob; use OCP\ISession; use Psr\Log\LoggerInterface; @@ -26,10 +28,11 @@ * @psalm-api */ #[ManuallyRunnableCronJob] -class JanitorCron extends TimedCronJob { +class JanitorCron extends TimedJob { // private AppSettings $appSettings; public function __construct( + protected ITimeFactory $time, protected LoggerInterface $logger, private ISession $session, private CommentMapper $commentMapper, @@ -42,6 +45,8 @@ public function __construct( private AppSettings $appSettings, protected bool $supportsManualRun = true, ) { + parent::__construct($time); + parent::setInterval(86400); // run once a day } /** diff --git a/lib/Cron/NotificationCron.php b/lib/Cron/NotificationCron.php index 87115f14d5..7cfd6bd903 100644 --- a/lib/Cron/NotificationCron.php +++ b/lib/Cron/NotificationCron.php @@ -12,6 +12,8 @@ use OCA\Polls\AppConstants; use OCA\Polls\Attributes\ManuallyRunnableCronJob; use OCA\Polls\Service\MailService; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\BackgroundJob\TimedJob; use OCP\ISession; use Psr\Log\LoggerInterface; @@ -19,14 +21,15 @@ * @psalm-api */ #[ManuallyRunnableCronJob] -class NotificationCron extends TimedCronJob { +class NotificationCron extends TimedJob { public function __construct( + protected ITimeFactory $time, protected LoggerInterface $logger, private ISession $session, private MailService $mailService, protected bool $supportsManualRun = true, ) { - // parent::__construct($time); + parent::__construct($time); parent::setInterval(300); // run every 5 minutes } diff --git a/lib/Cron/TimedCronJob.php b/lib/Cron/TimedCronJob.php deleted file mode 100644 index 5f48b42d5c..0000000000 --- a/lib/Cron/TimedCronJob.php +++ /dev/null @@ -1,43 +0,0 @@ -getName(); - - $this->logger->error('{job} does not support manual execution', ['job' => $jobClassName]); - throw new Exception('Job does not support manual execution'); - } - -} diff --git a/lib/Service/SystemService.php b/lib/Service/SystemService.php index a77350137f..600a39ea8d 100644 --- a/lib/Service/SystemService.php +++ b/lib/Service/SystemService.php @@ -11,7 +11,6 @@ use Exception; use OCA\Polls\AppConstants; use OCA\Polls\Attributes\ManuallyRunnableCronJob; -use OCA\Polls\Cron\TimedCronJob; use OCA\Polls\Db\Share; use OCA\Polls\Db\ShareMapper; use OCA\Polls\Db\UserMapper; @@ -31,7 +30,6 @@ use OCP\Collaboration\Collaborators\ISearch; use OCP\L10N\IFactory; use OCP\Share\IShare; -use phpDocumentor\Reflection\Types\This; use Psr\Log\LoggerInterface; class SystemService { @@ -271,31 +269,34 @@ public function validatePublicUsernameByToken(string $userName, string $token): * @param class-string $className The class name of the cron job to run * @throws Exception */ - public function runJob($className): string { + public function runJob($className): array { $jobs = $this->jobList->getJobsIterator($className, null, 0); foreach ($jobs as $job) { $jobClassName = get_class($job); $reflection_class = new \ReflectionClass($jobClassName); - if ($reflection_class->getNamespaceName() !== 'OCA\\Polls\\Cron') { + if ( + $reflection_class->getNamespaceName() !== 'OCA\\Polls\\Cron' + || $jobClassName !== $className + ) { continue; } - if (get_class($job) === $className) { - /** @var TimedCronJob $job */ - $job->setLastRun(time()); + $this->logger->info('{job}: started manually.', ['job' => $reflection_class->getShortName()]); + $job->setLastRun(0); + $job->start($this->jobList); + $job->setLastRun(time()); - if ( - !empty($reflection_class->getAttributes(ManuallyRunnableCronJob::class)) - && $reflection_class->hasMethod('manuallyRun') - ) { - return $job->manuallyRun(); - } else { - $this->logger->error('Job {job} does not support manual execution', ['job' => $jobClassName]); - throw new Exception('Job does not support manual execution'); - } - } + return [ + 'id' => $job->getId(), + 'className' => get_class($job), + 'lastRun' => $job->getLastRun(), + 'argument' => $job->getArgument(), + 'nameSpace' => $reflection_class->getNamespaceName(), + 'name' => $reflection_class->getShortName(), + 'manuallyRunnable' => !empty($reflection_class->getAttributes(ManuallyRunnableCronJob::class)), + ]; } throw new Exception('Job not found'); } @@ -315,7 +316,7 @@ public function getCronJobs(): array { continue; } - $joblist[] = [ + $joblist[$reflection_class->getShortName()] = [ 'id' => $job->getId(), 'className' => get_class($job), 'lastRun' => $job->getLastRun(), diff --git a/src/Api/modules/admin.ts b/src/Api/modules/admin.ts index 3ccfda1249..cc228a67c8 100644 --- a/src/Api/modules/admin.ts +++ b/src/Api/modules/admin.ts @@ -8,7 +8,7 @@ import { httpInstance, createCancelTokenHandler } from './HttpApi' import type { Job, JobsList } from './api.types' const adminJobs = { - getJobsList(): Promise> { + getJobsList(): Promise> { return httpInstance.request({ method: 'GET', url: 'administration/jobs', @@ -19,7 +19,7 @@ const adminJobs = { }) }, - runJob(job: Job) { + runJob(job: Job): Promise> { return httpInstance.request({ method: 'POST', url: 'administration/job/run', diff --git a/src/Api/modules/api.types.ts b/src/Api/modules/api.types.ts index 721ad1db23..f335e195c9 100644 --- a/src/Api/modules/api.types.ts +++ b/src/Api/modules/api.types.ts @@ -18,9 +18,7 @@ export type Job = { name: string manuallyRunnable: boolean } -export type JobsList = { - jobs: Job[] -} +export type JobsList = Record export type ApiEmailAdressList = { displayName: string diff --git a/src/components/Settings/AdminSettings/AdminJobs.vue b/src/components/Settings/AdminSettings/AdminJobs.vue index 5952894404..725a19f328 100644 --- a/src/components/Settings/AdminSettings/AdminJobs.vue +++ b/src/components/Settings/AdminSettings/AdminJobs.vue @@ -9,54 +9,53 @@ import { t } from '@nextcloud/l10n' import { AdminAPI } from '../../../Api' import { onMounted, ref } from 'vue' -import type { Job } from '../../../Api/modules/api.types' +import type { Job, JobsList } from '../../../Api/modules/api.types' import { NcActionButton, NcListItem } from '@nextcloud/vue' import { DateTime } from 'luxon' import PlayIcon from 'vue-material-design-icons/Play.vue' import NotSupportedIcon from 'vue-material-design-icons/MinusCircleOutline.vue' import JobExecutedIcon from 'vue-material-design-icons/CheckCircleOutline.vue' -const jobsList = ref(null) +const jobsList = ref({}) const executedJobs = ref(new Set()) -async function loadJobs() { - try { - const response = await AdminAPI.getJobsList() - jobsList.value = response.data.jobs - } catch (error) { - jobsList.value = null - Logger.error('Error on fetching jobs list', { error }) - } -} -/** - * fetch the jobs list on component mount - */ -onMounted(() => { - loadJobs() -}) +const jobHintsOne: Array = [ + t( + 'polls', + 'Please understand, that the jobs were defined as asynchronous jobs by intention.', + ), + t( + 'polls', + 'Starting the jobs does not mean, that the rules for these actions are overridden.', + ), +] -type LastRunInfo = { - text: string - title: string -} +const jobHintsTwo: Array = [ + t( + 'polls', + 'Each job can only be run once. If you want to rerun them, you have to refresh the page.', + ), +] -type Description = { - name: string - description: string -} -const jobsDescription: Record = { - 'OCA\\Polls\\Cron\\AutoReminderCron': { +const jobsDescription: Record< + string, + { + name: string + description: string + } +> = { + AutoReminderCron: { name: 'Autoreminder', description: t('polls', 'Sends out pending automatic reminders.'), }, - 'OCA\\Polls\\Cron\\JanitorCron': { + JanitorCron: { name: 'Janitor', description: t( 'polls', 'Cleans up old or irrelevant data from the system to maintain optimal performance and organization.', ), }, - 'OCA\\Polls\\Cron\\NotificationCron': { + NotificationCron: { name: 'Notification', description: t( 'polls', @@ -67,81 +66,84 @@ const jobsDescription: Record = { function getDescription(job: Job): string { if (job.className in jobsDescription) { - return jobsDescription[job.className].description + return jobsDescription[job.name].description } return t('polls', 'Description is missing.') } -function getLastRun(job: Job): LastRunInfo { - return job.lastRun === null - ? { +function getLastRun(job: Job): { text: string; title: string } { + switch (job.lastRun) { + case -1: + return { + text: t('polls', 'Execution failed'), + title: t( + 'polls', + 'The last execution of this job has failed. Please check the logs for more details.', + ), + } + case undefined: + case 0: + case null: + return { text: t('polls', 'Not run yet'), title: t('polls', 'This job has never been run yet.'), } - : { + default: + return { text: DateTime.fromSeconds(job.lastRun).toRelative() as string, title: DateTime.fromSeconds(job.lastRun).toLocaleString( DateTime.DATETIME_SHORT, ), } -} - -const jobHints: Record = { - line_one: t( - 'polls', - 'Please understand, that the jobs were defined as asynchronous jobs by intention.', - ), - line_two: t( - 'polls', - 'Only use them, if it is absolutely neccessary (i.e. your cron does not work properly) or for testing.', - ), - line_three: t( - 'polls', - 'Starting the jobs does not mean, that the rules for these actions are overridden.', - ), - line_four: t( - 'polls', - 'The time information just displays the last regular cron job run, not the manually initiated runs.', - ), - line_five: t( - 'polls', - 'Each job can only be run once. If you want to rerun them, you have to refresh the page.', - ), - line_six: t('polls', 'If you want to see the result, please check the logs.'), + } } async function runJob(job: Job) { try { - executedJobs.value.add(job.id) - await AdminAPI.runJob(job) + executedJobs.value.add(job.name) + const response = await AdminAPI.runJob(job) + const executedJob = response.data.job + jobsList.value[executedJob.name] = executedJob } catch (error) { + jobsList.value[job.name].lastRun = -1 Logger.error('Error on executing job', { error, job }) } } +async function loadJobs() { + try { + const response = await AdminAPI.getJobsList() + jobsList.value = response.data.jobs + } catch (error) { + jobsList.value = {} + Logger.error('Error on fetching jobs list', { error }) + } +} +/** + * fetch the jobs list on component mount + */ +onMounted(() => { + loadJobs() +})