UI: Specialized Job Detail Pages #3829

DingoEatingFuzz · 2018-02-02T18:14:29Z

Tailored job detail pages based on the type of job. Features include:

Periodic launches and parameterized dispatches are no long in the root job list
Periodic and parameterized jobs are labeled as such in the root job list
The summary chart in the root job list now charts children rather than allocations for periodic and parameterized jobs
Periodic detail page lists all periodic launches
Parameterized detail page lists all dispatches
Breadcrumbs for child jobs link back to parent jobs
Parameterized child detail page includes the dispatch payload
Periodic child detail page has a "force launch" button for quickly running the job out of band of the cron setting

alisdair

Looks really good! 🎉 I clicked around the UI locally and it all worked nicely. Didn't try too hard to break anything. (Wow, the dev Mirage server sure takes a while to start up, huh?)

I really like the multi-component structure. It's much clearer than the multiple-nested-if alternative. Test coverage seems great too.

Left a few questions & comments while reading through the code, but nothing important.

alisdair · 2018-02-02T22:23:54Z

ui/app/components/children-status-bar.js

+import { computed } from '@ember/object';
+import DistributionBar from './distribution-bar';
+
+export default DistributionBar.extend({


I might be misreading this, but would it be possible to use composition instead of inheritance here? That is, have this component (and the other subclass) render {{distribution-bar data=data}}?

It would mean rendering another component, but it might be clearer to understand the code that way. It took me a while to figure out how this subclass interacted with the parent.

I definitely agree it would be clearer and more idiomatic. I just still have a gut aversion to making extra components. Especially when those components are in lists.

There are no lists in Nomad long enough to cause component allocation to drag the app though, so I shouldn't prematurely optimize.

alisdair · 2018-02-02T22:38:37Z

ui/app/components/job-page/abstract.js

+        args: ['jobs.job', job],
+      },
+    ];
+  }),


Again wondering if composition is an option here. All of the templates have the same breadcrumbs markup, so if this logic was pulled into a component, there might be no need for an abstract parent class.

Refactoring all of breadcrumbs is in my top three things to do. I'd prefer to leave this the way it is, understanding full well that it is strange and out of place, and clean it up when I refactor all of breadcrumbs.

alisdair · 2018-02-02T22:48:55Z

ui/app/models/job.js

@@ -49,6 +108,12 @@ export default Model.extend({
  runningChildren: attr('number'),
  deadChildren: attr('number'),

+  childrenList: collect('pendingChildren', 'runningChildren', 'deadChildren'),
+
+  totalChildren: sum('childrenList'),


Fancy! 💅

alisdair · 2018-02-02T22:50:09Z

ui/app/serializers/job.js

+      hash.ParentID = JSON.stringify([hash.ParentID, hash.NamespaceID || 'default']);
+    }
+
+    // Periodic is a boolean on list and an object on single


alisdair · 2018-02-03T14:18:36Z

ui/mirage/factories/job.js

+      Enabled: true,
+      ProhibitOverlap: true,
+      Spec: '*/5 * * * * *',
+      SpecType: 'ctron',


Not important, but should this be 'cron'?

alisdair · 2018-02-03T14:20:29Z

ui/tests/acceptance/job-detail-test.js

-const sum = (list, key) => list.reduce((sum, item) => sum + get(item, key), 0);
+moduleForJob('Acceptance | job detail (batch)', () => server.create('job', { type: 'batch' }));
+moduleForJob('Acceptance | job detail (system)', () => server.create('job', { type: 'system' }));
+moduleForJob('Acceptance | job detail (periodic)', () => server.create('job', 'periodic'));


This is a really interesting way to test very similar pages! Will be stealing this for some upcoming TFE tests.

It works, but I'd prefer something that is a little more qunit-y. I sorta fell back into my mocha comfort zone when writing this and nested related tests in the moduleForJob function rather than setting some global context with moduleForJob and calling the tests right below it.

alisdair · 2018-02-03T14:24:20Z

ui/app/templates/components/job-page/parts/task-groups.hbs

+            onClick=(action gotoTaskGroup row.model)}}
+        {{/t.body}}
+      {{/list-table}}
+    {{/list-pagination}}


I don't see any pagination controls being rendered for this list-pagination instance. Why is it being used?

Good eyes. I must have copied this from the children component which does actually have pagination.

alisdair · 2018-02-05T15:22:57Z

ui/tests/integration/job-page/periodic-test.js

+            .filterBy('method', 'POST')
+            .find(req => req.url === `/v1/job/${id}/periodic/force?namespace=${namespace}`),
+          'POST request was made'
+        );


I often prefer asserting about the state of the Mirage DB over looking at logged requests. It helps me be more sure that the request body was correct (and that my Mirage handler was exercised). For example, in this case I think you could assert that a periodicChild record was created for the job.

Good call! This is the first POST in the UI, so I haven't had to write a test like this with Mirage yet.

Paths now start from package.json location, not project root.

This is a composite of scheduler type, batch variations, and children v. template

This is to later compose job detail page variations

DingoEatingFuzz · 2018-02-06T18:56:31Z

I addressed everything I said I would, @alisdair. I'm not sure if I like using composition over inheritance for the chart components now that I've gone through the exercise of writing it. It ends up being pretty repetitive and burdened by its own gotchas (e.g., plumbing the class and any other attributes down to the inner distribution-bar).

Those changes are all in the last commit if you want to exam it directly.

alisdair · 2018-02-06T19:07:18Z

Yeah, agreed, I'm not sure the composition approach is much cleaner. I didn't notice you were conditionally rendering the legend or yielding, that makes it much more difficult.

I don't have any ideas for improving it further. Sorry the suggestion wasn't useful!

DingoEatingFuzz · 2018-02-06T22:46:48Z

I'm glad you brought it up! It was worth trying despite it not panning out.

github-actions · 2023-03-13T02:14:32Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

DingoEatingFuzz added the theme/ui label Feb 2, 2018

alisdair reviewed Feb 5, 2018

View reviewed changes

DingoEatingFuzz added 7 commits February 5, 2018 20:13

Filter child jobs out of the root jobs list

37c39a9

Model the parent/child relationship in jobs

0c50472

Specialized children-status-bar variant of the allocation-status-bar

552b958

Use the children summary instead of alloc summary when applicable

eae275f

Fix lint-staged paths

525312d

Paths now start from package.json location, not project root.

Computed a template type for a job

c6369a6

This is a composite of scheduler type, batch variations, and children v. template

Handle the difference between parameterized on single and list responses

a47835f

DingoEatingFuzz force-pushed the f-ui-specialized-job-pages branch from 832d6d2 to 2414f48 Compare February 6, 2018 17:00

DingoEatingFuzz added 20 commits February 6, 2018 10:53

Deconstruct the existing job detail page into common parts

ec35b5a

This is to later compose job detail page variations

Recreate the service job detail page using job part components

bb1e0d6

Fleshing out job page parts and differences

0ea1d0d

New job page components for parent jobs and batch jobs

b4bdc61

Paginated and sortable table for job launches/children jobs

ab22e95

Bring payload in from the job api response

81273dc

Breadcrumbs for the periodic child job page

fbd166b

Elastic mode for cli window component

ec49a72

Payload details for the parameterized child job detail page

3fc0910

For now, the system job is identical to the service job

d7b9283

Add ability to force a periodic job launch

5238052

Differentiate between no search matches and no allocs on task group page

60cb1ac

Clean up force launch button

dc0fa16

State periodic or parameterized as the job type when applicable

d264c43

Trim the parent job prefix off the child job names when displaying them

35f388a

Update job factory to use traits for specifying job type

98b0068

Integration tests for the body job part

3907f39

Job part children tests

e8606f7

Job part evaluations test

88f1349

Running deployment job page part tests

80c2acd

DingoEatingFuzz added 7 commits February 6, 2018 10:53

Tests for the placement failures job part

3ca0d64

Tests for the summary job page part

82de007

Tests for the task groups job page part

1671786

New module-for-job for acceptance testing job detail differences

6e30864

Integration test for periodic job force launch

b7f57ec

Inject system so namespace shows up on job detail components

de27385

Always shutdown the mirage server

9e606f3

DingoEatingFuzz force-pushed the f-ui-specialized-job-pages branch from 2414f48 to e818757 Compare February 6, 2018 18:54

DingoEatingFuzz force-pushed the f-ui-specialized-job-pages branch from e818757 to 9e606f3 Compare February 6, 2018 22:47

DingoEatingFuzz merged commit ea9ff98 into master Feb 7, 2018

DingoEatingFuzz deleted the f-ui-specialized-job-pages branch February 7, 2018 01:48

github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Specialized Job Detail Pages #3829

UI: Specialized Job Detail Pages #3829

DingoEatingFuzz commented Feb 2, 2018

alisdair left a comment

alisdair Feb 2, 2018

DingoEatingFuzz Feb 6, 2018

alisdair Feb 2, 2018

DingoEatingFuzz Feb 6, 2018

alisdair Feb 2, 2018

alisdair Feb 2, 2018

alisdair Feb 3, 2018

DingoEatingFuzz Feb 6, 2018

alisdair Feb 3, 2018

DingoEatingFuzz Feb 6, 2018

alisdair Feb 3, 2018

DingoEatingFuzz Feb 6, 2018

alisdair Feb 5, 2018

DingoEatingFuzz Feb 6, 2018

DingoEatingFuzz commented Feb 6, 2018

alisdair commented Feb 6, 2018

DingoEatingFuzz commented Feb 6, 2018

github-actions bot commented Mar 13, 2023

UI: Specialized Job Detail Pages #3829

UI: Specialized Job Detail Pages #3829

Conversation

DingoEatingFuzz commented Feb 2, 2018

alisdair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DingoEatingFuzz commented Feb 6, 2018

alisdair commented Feb 6, 2018

DingoEatingFuzz commented Feb 6, 2018

github-actions bot commented Mar 13, 2023