Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ui, bug] UI fails to load job when there is an "@" in job name #13012

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/13012.txt
Original file line number Diff line number Diff line change
@@ -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
```
8 changes: 1 addition & 7 deletions ui/app/models/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
2 changes: 0 additions & 2 deletions ui/app/models/volume.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()}`;
}

Expand Down
12 changes: 11 additions & 1 deletion ui/app/routes/jobs/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
philrenaud marked this conversation as resolved.
Show resolved Hide resolved

const fullId = JSON.stringify([name, namespace]);

Expand Down
2 changes: 1 addition & 1 deletion ui/tests/acceptance/allocation-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);

Expand Down
4 changes: 2 additions & 2 deletions ui/tests/acceptance/jobs-list-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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) {
Expand Down
6 changes: 5 additions & 1 deletion ui/tests/acceptance/regions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
4 changes: 2 additions & 2 deletions ui/tests/acceptance/task-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);

Expand Down
2 changes: 1 addition & 1 deletion ui/tests/acceptance/topology-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down