From dbe14844eaf98d0fbae4371e5a7c8ef1b8002a9b Mon Sep 17 00:00:00 2001 From: Cameron Dawson Date: Tue, 30 Apr 2019 15:15:15 -0700 Subject: [PATCH] Fix circular dependency in JobModel/PushModel (#4929) --- tests/ui/unit/models/jobs_test.js | 12 +++------ ui/job-view/context/Pushes.jsx | 8 +++++- ui/job-view/details/tabs/SimilarJobsTab.jsx | 1 - ui/job-view/pushes/Push.jsx | 11 ++++++-- ui/models/job.js | 12 +++------ ui/models/push.js | 28 --------------------- 6 files changed, 23 insertions(+), 49 deletions(-) diff --git a/tests/ui/unit/models/jobs_test.js b/tests/ui/unit/models/jobs_test.js index 88c53051c..b5f5e21de 100644 --- a/tests/ui/unit/models/jobs_test.js +++ b/tests/ui/unit/models/jobs_test.js @@ -7,8 +7,6 @@ import paginatedJobListFixtureOne from '../../mock/job_list/pagination/page_1'; import paginatedJobListFixtureTwo from '../../mock/job_list/pagination/page_2'; describe('JobModel', () => { - const repoName = 'mozilla-inbound'; - afterEach(() => { fetchMock.reset(); }); @@ -19,7 +17,7 @@ describe('JobModel', () => { }); test('should return a promise', () => { - const result = JobModel.getList('mozilla-inbound'); + const result = JobModel.getList(); expect(result.then).toBeDefined(); }); }); @@ -37,17 +35,13 @@ describe('JobModel', () => { }); test('should return a page of results by default', async () => { - const jobList = await JobModel.getList(repoName, { count: 2 }); + const jobList = await JobModel.getList({ count: 2 }); expect(jobList).toHaveLength(2); }); test('should return all the pages when fetch_all==true', async () => { - const jobList = await JobModel.getList( - repoName, - { count: 2 }, - { fetch_all: true }, - ); + const jobList = await JobModel.getList({ count: 2 }, { fetch_all: true }); expect(jobList).toHaveLength(3); expect(jobList[2].id).toBe(3); diff --git a/ui/job-view/context/Pushes.jsx b/ui/job-view/context/Pushes.jsx index a2de4a2bf..9b6334718 100644 --- a/ui/job-view/context/Pushes.jsx +++ b/ui/job-view/context/Pushes.jsx @@ -373,7 +373,13 @@ export class PushesClass extends React.Component { } const pushIds = pushList.map(push => push.id); const lastModified = this.getLastModifiedJobTime(); - const jobList = await PushModel.getJobs(pushIds, { lastModified }); + const jobList = await JobModel.getList( + { + push_id__in: pushIds.join(','), + last_modified__gt: lastModified.toISOString().replace('Z', ''), + }, + { fetch_all: true }, + ); // break the jobs up per push const jobs = jobList.reduce((acc, job) => { const pushJobs = acc[job.push_id] ? [...acc[job.push_id], job] : [job]; diff --git a/ui/job-view/details/tabs/SimilarJobsTab.jsx b/ui/job-view/details/tabs/SimilarJobsTab.jsx index 6a523974c..3c6aa4e05 100644 --- a/ui/job-view/details/tabs/SimilarJobsTab.jsx +++ b/ui/job-view/details/tabs/SimilarJobsTab.jsx @@ -57,7 +57,6 @@ class SimilarJobsTab extends React.Component { }); const newSimilarJobs = await JobModel.getSimilarJobs( - repoName, selectedJob.id, options, ); diff --git a/ui/job-view/pushes/Push.jsx b/ui/job-view/pushes/Push.jsx index 9588d3789..5a222bdea 100644 --- a/ui/job-view/pushes/Push.jsx +++ b/ui/job-view/pushes/Push.jsx @@ -10,7 +10,7 @@ import { import { withPushes } from '../context/Pushes'; import { getGroupMapKey } from '../../helpers/aggregateId'; import { getAllUrlParams, getUrlParam } from '../../helpers/location'; -import PushModel from '../../models/push'; +import JobModel from '../../models/job'; import RunnableJobModel from '../../models/runnableJob'; import { withNotifications } from '../../shared/context/Notifications'; import { getRevisionTitle } from '../../helpers/revision'; @@ -139,7 +139,14 @@ class Push extends React.Component { fetchJobs = async () => { const { push } = this.props; - const jobs = await PushModel.getJobs(push.id); + const jobs = await JobModel.getList( + { + push_id: push.id, + count: 2000, + return_type: 'list', + }, + { fetch_all: true }, + ); this.mapPushJobs(jobs); }; diff --git a/ui/models/job.js b/ui/models/job.js index 283b57b62..7f21d5ebc 100644 --- a/ui/models/job.js +++ b/ui/models/job.js @@ -45,7 +45,7 @@ export default class JobModel { .join(' '); } - static getList(repoName, options, config) { + static getList(options, config) { // a static method to retrieve a list of JobModel config = config || {}; const fetch_all = config.fetch_all || false; @@ -67,11 +67,7 @@ export default class JobModel { const offset = parseInt(data.meta.offset, 10) + count; const newOptions = { ...options, offset, count }; - nextPagesJobs = await JobModel.getList( - repoName, - newOptions, - config, - ); + nextPagesJobs = await JobModel.getList(newOptions, config); } if ('job_property_names' in data) { // the results came as list of fields @@ -108,12 +104,12 @@ export default class JobModel { }); } - static getSimilarJobs(repoName, pk, options, config) { + static getSimilarJobs(pk, options, config) { config = config || {}; // The similar jobs endpoints returns the same type of objects as // the job list endpoint, so let's reuse the getList method logic. config.uri = `${uri}${pk}/similar_jobs/`; - return JobModel.getList(repoName, options, config); + return JobModel.getList(options, config); } static async retrigger(jobs, repoName, notify) { diff --git a/ui/models/push.js b/ui/models/push.js index 5fb98ccb0..f2de04142 100644 --- a/ui/models/push.js +++ b/ui/models/push.js @@ -5,7 +5,6 @@ import { getData } from '../helpers/http'; import { getProjectUrl, getUrlParam } from '../helpers/location'; import { createQueryParams, pushEndpoint } from '../helpers/url'; -import JobModel from './job'; import TaskclusterModel from './taskcluster'; const convertDates = function convertDates(locationParams) { @@ -60,33 +59,6 @@ export default class PushModel { return fetch(getProjectUrl(`${pushEndpoint}${pk}/`, repoName)); } - static getJobs(pushIds, options = {}) { - const { lastModified, repo } = options; - delete options.lastModified; - delete options.repo; - const params = { - return_type: 'list', - count: 2000, - ...options, - }; - - if (!Array.isArray(pushIds)) { - params.push_id = pushIds; - } else { - params.push_id__in = pushIds.join(','); - } - if (lastModified) { - // XXX: should never happen, but maybe sometimes does? see bug 1287501 - if (!(lastModified instanceof Date)) { - throw Error( - `Invalid parameter passed to get job updates: ${lastModified}. Please reload treeherder`, - ); - } - params.last_modified__gt = lastModified.toISOString().replace('Z', ''); - } - return JobModel.getList(repo, params, { fetch_all: true }); - } - static triggerMissingJobs(decisionTaskId) { return TaskclusterModel.load(decisionTaskId).then(results => { const actionTaskId = slugid();