From 45dc1cfd581a5230af389d58a4322fb15fee007c Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 17:01:27 -0400 Subject: [PATCH] 12986 UI fails to load job when there is an "@" in job name in nomad 130 (#13012) * LastIndexOf and always append a namespace on job links * Confirmed the volume equivalent and simplified idWIthNamespace logic * Changelog added * PR comments addressed * Drop the redirect for the time being * Tests updated to reflect namespace on links * Task detail test default namespace link for test --- .changelog/13012.txt | 3 +++ ui/app/models/job.js | 8 +------- ui/app/models/volume.js | 2 -- ui/app/routes/jobs/job.js | 12 +++++++++++- ui/tests/acceptance/allocation-detail-test.js | 2 +- ui/tests/acceptance/jobs-list-test.js | 4 ++-- ui/tests/acceptance/regions-test.js | 6 +++++- ui/tests/acceptance/task-detail-test.js | 4 ++-- ui/tests/acceptance/topology-test.js | 2 +- 9 files changed, 26 insertions(+), 17 deletions(-) create mode 100644 .changelog/13012.txt diff --git a/.changelog/13012.txt b/.changelog/13012.txt new file mode 100644 index 000000000..dd646bec5 --- /dev/null +++ b/.changelog/13012.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: fixed a bug where links to jobs with "@" in their name would mis-identify namespace and 404 +``` diff --git a/ui/app/models/job.js b/ui/app/models/job.js index a232128e0..d248eafd1 100644 --- a/ui/app/models/job.js +++ b/ui/app/models/job.js @@ -37,13 +37,7 @@ export default class Job extends Model { @computed('plainId') get idWithNamespace() { - const namespaceId = this.belongsTo('namespace').id(); - - if (!namespaceId || namespaceId === 'default') { - return this.plainId; - } else { - return `${this.plainId}@${namespaceId}`; - } + return `${this.plainId}@${this.belongsTo('namespace').id() ?? 'default'}`; } @computed('periodic', 'parameterized', 'dispatched') diff --git a/ui/app/models/volume.js b/ui/app/models/volume.js index c230cfda8..26a0b2b83 100644 --- a/ui/app/models/volume.js +++ b/ui/app/models/volume.js @@ -42,8 +42,6 @@ export default class Volume extends Model { @computed('plainId') get idWithNamespace() { - // does this handle default namespace -- I think the backend handles this for us - // but the client would always need to recreate that logic return `${this.plainId}@${this.belongsTo('namespace').id()}`; } diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index e5a50d939..dba817d7b 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -9,13 +9,23 @@ export default class JobRoute extends Route { @service can; @service store; @service token; + @service router; serialize(model) { return { job_name: model.get('idWithNamespace') }; } model(params) { - const [name, namespace = 'default'] = params.job_name.split('@'); + let name, + namespace = 'default'; + const { job_name } = params; + const delimiter = job_name.lastIndexOf('@'); + if (delimiter !== -1) { + name = job_name.slice(0, delimiter); + namespace = job_name.slice(delimiter + 1); + } else { + name = job_name; + } const fullId = JSON.stringify([name, namespace]); diff --git a/ui/tests/acceptance/allocation-detail-test.js b/ui/tests/acceptance/allocation-detail-test.js index 9a55aa2a9..fecebd6e5 100644 --- a/ui/tests/acceptance/allocation-detail-test.js +++ b/ui/tests/acceptance/allocation-detail-test.js @@ -503,7 +503,7 @@ module('Acceptance | allocation detail (preemptions)', function (hooks) { await Allocation.preempter.visitJob(); assert.equal( currentURL(), - `/jobs/${preempterJob.id}`, + `/jobs/${preempterJob.id}@default`, 'Clicking the preempter job link navigates to the preempter job page' ); diff --git a/ui/tests/acceptance/jobs-list-test.js b/ui/tests/acceptance/jobs-list-test.js index 6614258dd..59ea143a3 100644 --- a/ui/tests/acceptance/jobs-list-test.js +++ b/ui/tests/acceptance/jobs-list-test.js @@ -64,7 +64,7 @@ module('Acceptance | jobs list', function (hooks) { assert.equal(jobRow.name, job.name, 'Name'); assert.notOk(jobRow.hasNamespace); - assert.equal(jobRow.link, `/ui/jobs/${job.id}`, 'Detail Link'); + assert.equal(jobRow.link, `/ui/jobs/${job.id}@default`, 'Detail Link'); assert.equal(jobRow.status, job.status, 'Status'); assert.equal(jobRow.type, typeForJob(job), 'Type'); assert.equal(jobRow.priority, job.priority, 'Priority'); @@ -78,7 +78,7 @@ module('Acceptance | jobs list', function (hooks) { await JobsList.visit(); await JobsList.jobs.objectAt(0).clickName(); - assert.equal(currentURL(), `/jobs/${job.id}`); + assert.equal(currentURL(), `/jobs/${job.id}@default`); }); test('the new job button transitions to the new job page', async function (assert) { diff --git a/ui/tests/acceptance/regions-test.js b/ui/tests/acceptance/regions-test.js index fc3ba7026..d81af2c08 100644 --- a/ui/tests/acceptance/regions-test.js +++ b/ui/tests/acceptance/regions-test.js @@ -54,7 +54,11 @@ module('Acceptance | regions (only one)', function (hooks) { const jobId = JobsList.jobs.objectAt(0).id; await JobsList.jobs.objectAt(0).clickRow(); - assert.equal(currentURL(), `/jobs/${jobId}`, 'No region query param'); + assert.equal( + currentURL(), + `/jobs/${jobId}@default`, + 'No region query param' + ); await ClientsList.visit(); assert.equal(currentURL(), '/clients', 'No region query param'); diff --git a/ui/tests/acceptance/task-detail-test.js b/ui/tests/acceptance/task-detail-test.js index ab3f8c167..5fb181e56 100644 --- a/ui/tests/acceptance/task-detail-test.js +++ b/ui/tests/acceptance/task-detail-test.js @@ -104,7 +104,7 @@ module('Acceptance | task detail', function (hooks) { await Layout.breadcrumbFor('jobs.job.index').visit(); assert.equal( currentURL(), - `/jobs/${job.id}`, + `/jobs/${job.id}@default`, 'Job breadcrumb links correctly' ); @@ -112,7 +112,7 @@ module('Acceptance | task detail', function (hooks) { await Layout.breadcrumbFor('jobs.job.task-group').visit(); assert.equal( currentURL(), - `/jobs/${job.id}/${taskGroup}`, + `/jobs/${job.id}@default/${taskGroup}`, 'Task Group breadcrumb links correctly' ); diff --git a/ui/tests/acceptance/topology-test.js b/ui/tests/acceptance/topology-test.js index 31548c338..e4f5ba81f 100644 --- a/ui/tests/acceptance/topology-test.js +++ b/ui/tests/acceptance/topology-test.js @@ -184,7 +184,7 @@ module('Acceptance | topology', function (hooks) { await reset(); await Topology.allocInfoPanel.visitJob(); - assert.equal(currentURL(), `/jobs/${job.id}`); + assert.equal(currentURL(), `/jobs/${job.id}@default`); await reset();