From 740047e21d300d5e9954e5e88ec040e14427ad36 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 13:46:30 -0400 Subject: [PATCH 1/7] LastIndexOf and always append a namespace on job links --- ui/app/models/job.js | 2 +- ui/app/routes/jobs/job.js | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/ui/app/models/job.js b/ui/app/models/job.js index a232128e0898..f94d5bd2b665 100644 --- a/ui/app/models/job.js +++ b/ui/app/models/job.js @@ -39,7 +39,7 @@ export default class Job extends Model { get idWithNamespace() { const namespaceId = this.belongsTo('namespace').id(); - if (!namespaceId || namespaceId === 'default') { + if (!namespaceId) { return this.plainId; } else { return `${this.plainId}@${namespaceId}`; diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index e5a50d939b21..9b9bafe56ae9 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -15,7 +15,17 @@ export default class JobRoute extends Route { } 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; + namespace = 'default'; + } const fullId = JSON.stringify([name, namespace]); From 73c10f0c5d7d56787fffbe18176f289d153ba378 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 13:51:36 -0400 Subject: [PATCH 2/7] Confirmed the volume equivalent and simplified idWIthNamespace logic --- ui/app/models/job.js | 8 +------- ui/app/models/volume.js | 2 -- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/ui/app/models/job.js b/ui/app/models/job.js index f94d5bd2b665..e8f797c6aebd 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) { - return this.plainId; - } else { - return `${this.plainId}@${namespaceId}`; - } + return `${this.plainId}@${this.belongsTo('namespace').id()}`; } @computed('periodic', 'parameterized', 'dispatched') diff --git a/ui/app/models/volume.js b/ui/app/models/volume.js index c230cfda8331..26a0b2b8389a 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()}`; } From fe959c76502c2b93f8e19d1f025f213a6fee9325 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 13:54:42 -0400 Subject: [PATCH 3/7] Changelog added --- .changelog/13012.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/13012.txt diff --git a/.changelog/13012.txt b/.changelog/13012.txt new file mode 100644 index 000000000000..dd646bec560d --- /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 +``` From b4220d40530d460f8181581b5d430101a0888a68 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 14:43:49 -0400 Subject: [PATCH 4/7] PR comments addressed --- ui/app/models/job.js | 2 +- ui/app/routes/jobs/job.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ui/app/models/job.js b/ui/app/models/job.js index e8f797c6aebd..d248eafd1ff2 100644 --- a/ui/app/models/job.js +++ b/ui/app/models/job.js @@ -37,7 +37,7 @@ export default class Job extends Model { @computed('plainId') get idWithNamespace() { - return `${this.plainId}@${this.belongsTo('namespace').id()}`; + return `${this.plainId}@${this.belongsTo('namespace').id() ?? 'default'}`; } @computed('periodic', 'parameterized', 'dispatched') diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index 9b9bafe56ae9..8d85da9c8690 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -9,6 +9,7 @@ export default class JobRoute extends Route { @service can; @service store; @service token; + @service router; serialize(model) { return { job_name: model.get('idWithNamespace') }; @@ -24,7 +25,7 @@ export default class JobRoute extends Route { namespace = job_name.slice(delimiter + 1); } else { name = job_name; - namespace = 'default'; + this.router.transitionTo('jobs.job.index', `${name}@${namespace}`); } const fullId = JSON.stringify([name, namespace]); From 93b9c39eb36d89b1601a4748689e4ac5ebd34cf5 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 15:38:25 -0400 Subject: [PATCH 5/7] Drop the redirect for the time being --- ui/app/routes/jobs/job.js | 1 - 1 file changed, 1 deletion(-) diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index 8d85da9c8690..dba817d7bc06 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -25,7 +25,6 @@ export default class JobRoute extends Route { namespace = job_name.slice(delimiter + 1); } else { name = job_name; - this.router.transitionTo('jobs.job.index', `${name}@${namespace}`); } const fullId = JSON.stringify([name, namespace]); From 1efdb977ae5a9afc4394cb792a76fbd7d2c1abb9 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 16:04:43 -0400 Subject: [PATCH 6/7] Tests updated to reflect namespace on links --- 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 | 2 +- ui/tests/acceptance/topology-test.js | 2 +- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/ui/tests/acceptance/allocation-detail-test.js b/ui/tests/acceptance/allocation-detail-test.js index 9a55aa2a9eb3..fecebd6e53e5 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 6614258ddd84..59ea143a31bc 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 fc3ba70268fa..d81af2c08513 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 ab3f8c16766c..37b6c7fdead1 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' ); diff --git a/ui/tests/acceptance/topology-test.js b/ui/tests/acceptance/topology-test.js index 31548c338142..e4f5ba81ff45 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(); From 8ee9fde2a63aa4e3b8345eebfcbe8753565e623f Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 13 May 2022 16:30:26 -0400 Subject: [PATCH 7/7] Task detail test default namespace link for test --- ui/tests/acceptance/task-detail-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/tests/acceptance/task-detail-test.js b/ui/tests/acceptance/task-detail-test.js index 37b6c7fdead1..5fb181e56772 100644 --- a/ui/tests/acceptance/task-detail-test.js +++ b/ui/tests/acceptance/task-detail-test.js @@ -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' );