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: Allocation file system explorer: Routing #5866

Merged
merged 4 commits into from
Jun 21, 2019
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 ui/app/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ Router.map(function() {
this.route('allocation', { path: '/:allocation_id' }, function() {
this.route('task', { path: '/:name' }, function() {
this.route('logs');
this.route('fs', function() {
this.route('path', { path: '/*path' });
});
});
});
});
Expand Down
15 changes: 15 additions & 0 deletions ui/app/routes/allocations/allocation/task/fs/path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import Route from '@ember/routing/route';

export default Route.extend({
model({ path }) {
return {
path: decodeURIComponent(path),
task: this.modelFor('allocations.allocation.task'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little suggestion, could you just call the task property model? Then you don't need to rename it below?

setupController(controller, model) {
  this._super(...arguments);
  controller.setProperties(model);
}

It's clear from the modelFor argument that it's a task

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know that this quite reaches convention status, but I’ve referred to this before as a “compound model”. I haven’t seen many conventions around the naming of its components apart from them being named the things that they are. I think it would be somewhat confusing to have the model hook be loading something that contains a path and a model.

I hope some day there’s a more first-class way to do this kind of data loading!

Copy link
Contributor

@johncowen johncowen Jun 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other option would be to keep task and not rename it to model in setupController. I think my main point was more not having to rename it than deciding what to call it. I know that some folks prefer having a model in the templates, assumed this was why it was being renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I wouldn't have to do model hook shenanigans to pass that param to the controller, but if the param changes without the model changing then the setupController hook doesn't refire.

But I also wouldn't get caught up on this. I expect this route to more or less be entirely rewritten when @backspace works on the file explorer component. Hard to say now what the best way to provide context to the controller is.

Copy link
Contributor

@johncowen johncowen Jun 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet yeah, gotcha, was just a tiny thing I noticed, no biggie.

};
},

setupController(controller, { path, task }) {
this._super(...arguments);
controller.setProperties({ path, model: task });
},
});
1 change: 1 addition & 0 deletions ui/app/templates/allocations/allocation/task/fs.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{{outlet}}
14 changes: 14 additions & 0 deletions ui/app/templates/allocations/allocation/task/fs/index.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{{partial "allocations/allocation/task/subnav"}}
Copy link
Contributor

@backspace backspace Jun 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something for the future, do you have any interest in removing the use of this partial? As it looks like partials will be deprecated. It seems like it could be replaced by a simple component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite lackadaisical when it comes to dealing with deprecations. Of course it makes sense for these to become components eventually, and I won't stand in anyone's way who wants to do that work now. I just tend to wait until I have to 😛

<section class="section full-width-section">
<h1 class="title">Index fs route</h1>
{{#if model.isRunning}}
Task is running, show file explorer.
{{else}}
<div data-test-not-running class="empty-message">
<h3 data-test-not-running-headline class="empty-message-headline">Task is not Running</h3>
<p data-test-not-running-body class="empty-message-body">
Cannot access files of a task that is not running.
</p>
</div>
{{/if}}
</section>
4 changes: 4 additions & 0 deletions ui/app/templates/allocations/allocation/task/fs/path.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{{partial "allocations/allocation/task/subnav"}}
<section class="section full-width-section">
<h1 class="title">Path fs route <code>{{path}}</code></h1>
</section>
1 change: 1 addition & 0 deletions ui/app/templates/allocations/allocation/task/subnav.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
<ul>
<li>{{#link-to "allocations.allocation.task.index" model.allocation model activeClass="is-active"}}Overview{{/link-to}}</li>
<li>{{#link-to "allocations.allocation.task.logs" model.allocation model activeClass="is-active"}}Logs{{/link-to}}</li>
<li>{{#link-to "allocations.allocation.task.fs" model.allocation model activeClass="is-active"}}Files{{/link-to}}</li>
</ul>
</div>
42 changes: 42 additions & 0 deletions ui/tests/acceptance/task-fs-path-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { currentURL } from '@ember/test-helpers';
import { Promise } from 'rsvp';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import setupMirage from 'ember-cli-mirage/test-support/setup-mirage';
import Path from 'nomad-ui/tests/pages/allocations/task/fs/path';

let allocation;
let task;

module('Acceptance | task fs path', function(hooks) {
setupApplicationTest(hooks);
setupMirage(hooks);

hooks.beforeEach(async function() {
server.create('agent');
server.create('node', 'forceIPv4');
const job = server.create('job', { createAllocations: false });

allocation = server.create('allocation', { jobId: job.id, clientStatus: 'running' });
task = server.schema.taskStates.where({ allocationId: allocation.id }).models[0];
});

test('visiting /allocations/:allocation_id/:task_name/fs/:path', async function(assert) {
const paths = ['some-file.log', 'a/deep/path/to/a/file.log', '/', 'Unicode™®'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah I love the Unicode


const testPath = async filePath => {
await Path.visit({ id: allocation.id, name: task.name, path: filePath });
assert.equal(
currentURL(),
`/allocations/${allocation.id}/${task.name}/fs/${encodeURIComponent(filePath)}`,
'No redirect'
);
assert.ok(Path.tempTitle.includes(filePath), `Temp title includes path, ${filePath}`);
};

await paths.reduce(async (prev, filePath) => {
await prev;
return testPath(filePath);
}, Promise.resolve());
});
});
40 changes: 40 additions & 0 deletions ui/tests/acceptance/task-fs-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { currentURL } from '@ember/test-helpers';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import setupMirage from 'ember-cli-mirage/test-support/setup-mirage';
import FS from 'nomad-ui/tests/pages/allocations/task/fs';

let allocation;
let task;

module('Acceptance | task fs', function(hooks) {
setupApplicationTest(hooks);
setupMirage(hooks);

hooks.beforeEach(async function() {
server.create('agent');
server.create('node', 'forceIPv4');
const job = server.create('job', { createAllocations: false });

allocation = server.create('allocation', { jobId: job.id, clientStatus: 'running' });
task = server.schema.taskStates.where({ allocationId: allocation.id }).models[0];
});

test('visiting /allocations/:allocation_id/:task_name/fs', async function(assert) {
await FS.visit({ id: allocation.id, name: task.name });
assert.equal(currentURL(), `/allocations/${allocation.id}/${task.name}/fs`, 'No redirect');
});

test('when the task is not running, an empty state is shown', async function(assert) {
task.update({
finishedAt: new Date(),
});

await FS.visit({ id: allocation.id, name: task.name });
assert.ok(FS.hasEmptyState, 'Non-running task has no files');
assert.ok(
FS.emptyState.headline.includes('Task is not Running'),
'Empty state explains the condition'
);
});
});
12 changes: 12 additions & 0 deletions ui/tests/pages/allocations/task/fs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { create, isPresent, text, visitable } from 'ember-cli-page-object';

export default create({
visit: visitable('/allocations/:id/:name/fs'),

hasFiles: isPresent('[data-test-file-explorer]'),

hasEmptyState: isPresent('[data-test-not-running]'),
emptyState: {
headline: text('[data-test-not-running-headline]'),
},
});
7 changes: 7 additions & 0 deletions ui/tests/pages/allocations/task/fs/path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { create, text, visitable } from 'ember-cli-page-object';

export default create({
visit: visitable('/allocations/:id/:name/fs/:path'),

tempTitle: text('h1.title'),
});