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

Prettier Job Information Page #3340

Merged
merged 10 commits into from Dec 20, 2016

Conversation

Projects
None yet
4 participants
@erasche
Copy link
Member

commented Dec 19, 2016

Changes:

  • split one giant table into multiple tables based on section
  • nicer headers (using h2/h3s for semantic meaning)
  • terminal themed command line block to allow for easy visual acquisition by admins
  • moved giant job metrics section down a bit
  • Fixed left column width of metrics table and removed redundant (runtime environment variable)
  • word-break: break-word; all the things

First half of job info page
Second half of job info page

@erasche erasche added this to the 17.01 milestone Dec 19, 2016

@erasche erasche requested a review from martenson Dec 19, 2016



<h3>Command Line</h3>
%if job and job.command_line and trans.user_is_admin():

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 20, 2016

Member

I'm going to open a PR with these changes - but I think this if should be up a bit so you don't show the heading of an empty block for non-admins.

This comment has been minimized.

Copy link
@erasche

erasche Dec 20, 2016

Author Member

Fixed

<% job_metrics = trans.app.job_metrics %>
%for metric in sorted(job.metrics, key=lambda x:x.metric_name):
<% metric_title, metric_value = job_metrics.format( metric.plugin, metric.metric_name, metric.metric_value ) %>
<% metric_title = metric_title.replace(' (runtime environment variable)', '') %>

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 20, 2016

Member

I'm not on-board for this change I don't think. Even if I agree this shouldn't be appended to the environment variable - we need to drop it in the metric formatter since its only job is to produce the title that is presented here.

This comment has been minimized.

Copy link
@erasche

erasche Dec 20, 2016

Author Member

Sure, absolutely. I couldn't figure out if it was used anywhere else (EnvFormatter/EnvPlugin classes both fail the grep test), so I didn't want to make a potentially unhelpful change and opted for the most conservative option.

Will fix this

erasche added some commits Dec 20, 2016

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

@jmchilton just now I found myself a bit worried that by removing (runtime environment variable) from the display, we were losing something of value. So, I split into several tables instead, one for each plugin:

utvalg_050

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

It's a bit visually noisy, removed redundant table headers + "Plugin:" prefix.

utvalg_051

@natefoo

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

+1, this looks great!

erasche and others added some commits Dec 20, 2016

Initial tool form and dataset detail selenium testing.
- Test parameters and tables appear on dataset details page after running a tool.
- Test a simple tool execution with an integer field.
- Test re-run a tool.
- Test a tool with a data drop down.
- Improved select2 abstractions.
@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

Pushed two commits into this branch - one that adds selenium functional tests for running tools and checking this dataset details page (was actually working on this when the PR came it so I'm hijacking your PR) and one to sort the plugins based on the displayed title instead of the plugin key (not displayed).

% if tool:
Tool: ${tool.name | h}
% else:
Unknown Tool

This comment has been minimized.

Copy link
@erasche

erasche Dec 20, 2016

Author Member

This section showing the tool name was moved up into an h2 above, so this is now duplicated.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 20, 2016

Member

Opps - bad merge with my previous changes. Sorry - I'll fix that.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

I don't have time to look at this right now, but it may be a good idea at resolving also #2954 and #3252 while you guys are working on this ;)

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

@jmchilton Nice! Always great to have more tests.

@nsoranzo you're not wrong. I'll see what I can do. Moving this to WIP

@erasche erasche added status/WIP and removed status/review labels Dec 20, 2016

@erasche erasche changed the title Prettier Job Information Page [WIP] Prettier Job Information Page and Metrics for Non-admins Dec 20, 2016

@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

@erasche Noooooooo WIP - we should merge as is and follow up if there is time. This is a solid atomic improvement that has two +1s. Those are good issues to address and work on them would be greatly appreciated - but I hijacked your PR to make faster progress on tool and workflow testing.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

@jmchilton if you're happy to merge as-is, I'll remove the tag and just keep working on my branch.

@erasche erasche removed the request for review from martenson Dec 20, 2016

@erasche erasche removed the status/WIP label Dec 20, 2016

@erasche erasche changed the title [WIP] Prettier Job Information Page and Metrics for Non-admins Prettier Job Information Page Dec 20, 2016

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

@nsoranzo / @jmchilton opened #3344 to address the other issues since I was at it

@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

I rush it and then my own linting error is going to prevent me from merging it now and you've already solved the other problem. I'm sure a moron some days.

@jmchilton jmchilton merged commit af90b42 into galaxyproject:dev Dec 20, 2016

1 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
framework test Test scheduled.
Details
toolshed test Test scheduled.
Details
api test Build finished. 244 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

Thanks for the awesome upgrade @erasche and thanks for letting me hijack it!

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

Happy to help @jmchilton, thanks for the review!

@bgruening bgruening deleted the erasche:nicer_info_page branch Dec 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.