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
hashicorp · May 13, 2022 · 19e2e5c · 19e2e5c
1 parent 7c1c117
commit 19e2e5c
Show file tree

Hide file tree

Showing 9 changed files with 26 additions and 17 deletions.
diff --git a/.changelog/13012.txt 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -104,15 +104,15 @@ 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'
     );
 
     await Task.visit({ id: allocation.id, name: task.name });
     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
@@ -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();