feat: agenda Jobs admin page#40699
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
12de5e9 to
2b14da9
Compare
9a73a91 to
ab78bd0
Compare
There was a problem hiding this comment.
6 issues found across 27 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/cronJobs.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/cronJobs.ts:1">
P2: Substantial duplicated endpoint logic across cron.enable, cron.disable, and cron.trigger increases maintenance drift risk. All three share identical auth/permission/body/response configuration and identical action handler error-handling structure, differing only in the service method called. Consider consolidating via a shared handler factory.</violation>
</file>
<file name="apps/meteor/server/services/cron-jobs/service.ts">
<violation number="1" location="apps/meteor/server/services/cron-jobs/service.ts:12">
P2: Duplicate query, mapping, and response-building logic between `getCoreJobs` and `getAppJobs`</violation>
<violation number="2" location="apps/meteor/server/services/cron-jobs/service.ts:16">
P2: Paginated core/app job listing lacks explicit sort, making pagination order potentially unstable.</violation>
</file>
<file name="packages/core-typings/src/ICronJobItem.ts">
<violation number="1" location="packages/core-typings/src/ICronJobItem.ts:18">
P2: `status` is typed as unconstrained `string` instead of the finite union `CronJobStatus` ('running' | 'scheduled' | 'failed' | 'disabled' | 'completed') that the backend derives and returns.</violation>
</file>
<file name="apps/meteor/tests/end-to-end/api/cron-jobs.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/cron-jobs.ts:1">
P2: Missing happy-path coverage for the cron.trigger / cron.enable / cron.disable endpoints; the test file only checks failure cases and never verifies the successful response path.</violation>
<violation number="2" location="apps/meteor/tests/end-to-end/api/cron-jobs.ts:39">
P2: Missing assertions for pagination metadata in cron.appjobs and cron.history success tests allow regressions in count/offset/total to slip through.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,232 @@ | |||
| import { CronJobsSvc } from '@rocket.chat/core-services'; | |||
There was a problem hiding this comment.
P2: Substantial duplicated endpoint logic across cron.enable, cron.disable, and cron.trigger increases maintenance drift risk. All three share identical auth/permission/body/response configuration and identical action handler error-handling structure, differing only in the service method called. Consider consolidating via a shared handler factory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/cronJobs.ts:
<comment>Substantial duplicated endpoint logic across cron.enable, cron.disable, and cron.trigger increases maintenance drift risk. All three share identical auth/permission/body/response configuration and identical action handler error-handling structure, differing only in the service method called. Consider consolidating via a shared handler factory.</comment>
<file context>
@@ -0,0 +1,232 @@
+import { CronJobsSvc } from '@rocket.chat/core-services';
+import type { ICronJobItem, ICronHistoryItem } from '@rocket.chat/core-typings';
+import { ajv, ajvQuery, validateUnauthorizedErrorResponse, validateBadRequestErrorResponse } from '@rocket.chat/rest-typings';
+
+import type { ExtractRoutesFromAPI } from '../ApiClass';
+import { API } from '../api';
+import { getPaginationItems } from '../helpers/getPaginationItems';
+
+const isCronJobsListParams = ajvQuery.compile<{
</file context>
| export class CronJobsService extends ServiceClassInternal implements ICronJobsService { | ||
| protected name = 'cron-jobs'; | ||
|
|
||
| async getCoreJobs(pagination?: { |
There was a problem hiding this comment.
P2: Duplicate query, mapping, and response-building logic between getCoreJobs and getAppJobs
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/services/cron-jobs/service.ts, line 12:
<comment>Duplicate query, mapping, and response-building logic between `getCoreJobs` and `getAppJobs`</comment>
<file context>
@@ -0,0 +1,105 @@
+export class CronJobsService extends ServiceClassInternal implements ICronJobsService {
+ protected name = 'cron-jobs';
+
+ async getCoreJobs(pagination?: {
+ offset?: number;
+ count?: number;
</file context>
| offset?: number; | ||
| count?: number; | ||
| }): Promise<{ jobs: ICronJobItem[]; count: number; offset: number; total: number }> { | ||
| const { cursor, totalCount } = CronJobs.findPaginated( |
There was a problem hiding this comment.
P2: Paginated core/app job listing lacks explicit sort, making pagination order potentially unstable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/services/cron-jobs/service.ts, line 16:
<comment>Paginated core/app job listing lacks explicit sort, making pagination order potentially unstable.</comment>
<file context>
@@ -0,0 +1,105 @@
+ offset?: number;
+ count?: number;
+ }): Promise<{ jobs: ICronJobItem[]; count: number; offset: number; total: number }> {
+ const { cursor, totalCount } = CronJobs.findPaginated(
+ {
+ name: { $not: /^Apps-/ },
</file context>
| failCount?: number; | ||
| lastModifiedBy?: string; | ||
| data?: Record<string, any>; | ||
| status?: string; |
There was a problem hiding this comment.
P2: status is typed as unconstrained string instead of the finite union CronJobStatus ('running' | 'scheduled' | 'failed' | 'disabled' | 'completed') that the backend derives and returns.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core-typings/src/ICronJobItem.ts, line 18:
<comment>`status` is typed as unconstrained `string` instead of the finite union `CronJobStatus` ('running' | 'scheduled' | 'failed' | 'disabled' | 'completed') that the backend derives and returns.</comment>
<file context>
@@ -0,0 +1,19 @@
+ failCount?: number;
+ lastModifiedBy?: string;
+ data?: Record<string, any>;
+ status?: string;
+}
\ No newline at end of file
</file context>
| @@ -0,0 +1,204 @@ | |||
| import { expect } from 'chai'; | |||
There was a problem hiding this comment.
P2: Missing assertions for pagination metadata in cron.appjobs and cron.history success tests allow regressions in count/offset/total to slip through.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/cron-jobs.ts, line 39:
<comment>Missing assertions for pagination metadata in cron.appjobs and cron.history success tests allow regressions in count/offset/total to slip through.</comment>
<file context>
@@ -0,0 +1,204 @@
+ .expect(200)
+ .expect((res) => {
+ expect(res.body).to.have.property('success', true);
+ expect(res.body).to.have.property('jobs').and.to.be.an('array');
+ expect(res.body).to.have.property('offset');
+ expect(res.body).to.have.property('total');
</file context>
| @@ -0,0 +1,204 @@ | |||
| import { expect } from 'chai'; | |||
There was a problem hiding this comment.
P2: Missing happy-path coverage for the cron.trigger / cron.enable / cron.disable endpoints; the test file only checks failure cases and never verifies the successful response path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/cron-jobs.ts:
<comment>Missing happy-path coverage for the cron.trigger / cron.enable / cron.disable endpoints; the test file only checks failure cases and never verifies the successful response path.</comment>
<file context>
@@ -0,0 +1,204 @@
+import { expect } from 'chai';
+import { before, describe, it, after } from 'mocha';
+
+import { getCredentials, api, request, credentials } from '../../data/api-data';
+import { updatePermission } from '../../data/permissions.helper';
+
+describe('[Cron Jobs API]', () => {
+ before((done) => getCredentials(done));
+
</file context>
| export const OmnichannelEEService = proxify<IOmnichannelEEService>('omnichannel-ee'); | ||
| export const Import = proxify<IImportService>('import'); | ||
| export const OmnichannelAnalytics = proxify<IOmnichannelAnalyticsService>('omnichannel-analytics'); | ||
| export const CronJobsSvc = proxify<ICronJobsService>('cron-jobs'); |
There was a problem hiding this comment.
Don't name services svc. Its implied from the package where you're importing them.
Proposed changes
Agenda Jobs Admin Page
This PR will contain the work for the Agenda Jobs Administration Page.
Progress (Backend):
GET /v1/cron.jobsEndpoint:GET /v1/cron.appjobsEndpointGET /v1/cron.historyendpointIssue(s)
GSoC 2026 Agenda Job Admin Page