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

Job Info button in Reporting Listing #25421

Merged
merged 14 commits into from
Nov 27, 2018

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Nov 8, 2018

Proposal to have an "info" button in the Reporting listing

Screenshots: #25421 (comment)

@tsullivan tsullivan added review v5.6.0 v7.0.0 enhancement New value added to drive a business result labels Nov 8, 2018
@nreese
Copy link
Contributor

nreese commented Nov 8, 2018

@tsullivan Did you mean to give this PR a v5.6.0 label? Shouldn't that be v6.6.0?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan added v6.6.0 and removed v5.6.0 labels Nov 9, 2018
@tsullivan
Copy link
Member Author

@nreese yes :) thanks for the tip

@stacey-gammon
Copy link
Contributor

The proposal lgtm. Some of the info is duplicate from whats on the table but I don't think it matters much. Mostly I suspect this will be used for debugging, not necessarily information the user wants to see in the table or sort on.

return null;
}

const iGet = (path: string) => get(info, path, 'n/a');
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: this helper gets made everytime renderInfo is called. We could probably extract this around line 22 so the browser can do whatever optimizations it can in its JIT (likely small).

Copy link
Member Author

Choose a reason for hiding this comment

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

True, since it was enclosing info. I changed it to just use lodash directly from the JSX


const iGet = (path: string) => get(info, path, 'n/a');

// TODO browser type
Copy link
Contributor

Choose a reason for hiding this comment

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

How hard is this to plumb up? I can take that in a sub-task :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be worth the effort! Right now, browser type is not in the schema of data that gets indexed to manage the reporting jobs. It would need to be added to the fields of data and populated when jobs are created.

Copy link
Contributor

Choose a reason for hiding this comment

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

Coolio, don't want to block this from merging, so I'll do my own PR once this is in master

Copy link
Member Author

Choose a reason for hiding this comment

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

I put another todo of "queue method" in. It's another thing we don't have in the record today: whether the job was queued by clicking the button in the UI, or using the POST URL. It would be nice to have, as (I think) the code path between the options is a little different.

@tsullivan
Copy link
Member Author

Mostly I suspect this will be used for debugging

That the intention. For troubleshooting, people tend to change settings around and re-try with different options. This would help historically show the different status and results with different options.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan
Copy link
Member Author

Going to re-visit this to replace the tooltip with a EUI flyout

@tsullivan tsullivan added WIP Work in progress and removed review labels Nov 19, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan
Copy link
Member Author

PDF Processing

screen shot 2018-11-19 at 2 31 12 pm

PDF Completed

screen shot 2018-11-19 at 2 34 00 pm

PDF Failed

screen shot 2018-11-19 at 2 12 31 pm

CSV Completed

screen shot 2018-11-19 at 2 34 07 pm

CSV Failed

screen shot 2018-11-19 at 2 46 35 pm

@tsullivan
Copy link
Member Author

@joelgriffith @stacey-gammon I changed the UI to use a Flyout instead of a popover, as suggested by @cjcenizal. Mind taking another look?

cc @cjcenizal

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan added review and removed WIP Work in progress labels Nov 19, 2018
@joelgriffith
Copy link
Contributor

joelgriffith commented Nov 19, 2018

Looks good @tsullivan !

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan force-pushed the reporting/listing-info-callout branch from 3a129b7 to 81b3e24 Compare November 20, 2018 20:15
@tsullivan
Copy link
Member Author

Thanks @joelgriffith ! I'm going to work on adding Jest tests w/ snapshots while trying to get a 2nd approval on this. I might need to wait until more people are back from vacation

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joelgriffith
Copy link
Contributor

Once this is in I'd like to add some more information regarding the browser. Not sure if there's security concerns with revealing that we're running headless chrome under-the-hood... but would definitely be nice in diagnosing

@tsullivan tsullivan merged commit 55a9b22 into elastic:master Nov 27, 2018
@tsullivan tsullivan deleted the reporting/listing-info-callout branch November 27, 2018 16:57
tsullivan added a commit to tsullivan/kibana that referenced this pull request Nov 27, 2018
* Job Info button in Reporting Listing

* use lodash directly

* start of flyout use

* description list in flyout

* capitalize

* undefined guard

* expire info on close

* add jest test

* better at error handling + messaging
tsullivan added a commit that referenced this pull request Nov 27, 2018
* Job Info button in Reporting Listing

* use lodash directly

* start of flyout use

* description list in flyout

* capitalize

* undefined guard

* expire info on close

* add jest test

* better at error handling + messaging
sebelga added a commit that referenced this pull request Nov 28, 2018
* [APM] Fix horizontal scrollbar being visible in windows 8.1 (#25988)

* [APM] Changed 'Response Time' to 'Duration' in transactions screens (#25990)

* translate InfraOps visualization component (Part 3) (#25213)

* translate InfraOps visualization component (Part 3 - part of folder components)

* update translation of Infra Ops vizualization component (Part 3)

* update translation of Infra Ops vizualization component (Part 3)

* change some ids and add pluralization

* update Infra Ops Part 3 - change some ids, change some intl.formatMessage() to <FormattedMessage> and directly wrap some classes by injectI18n()

* update Infra-III - add static to displayName

* [i18n] Translate Agg_types(part_3) (#26118)

* Translate agg_types - metrics

* Fix issues

* [ML] Aggregate anomalies table data using configured Kibana timezone (#26192)

* [ML] Aggregate anomalies table data using configured Kibana timezone

* [ML] Move dataFormatTz prop out of controller scope

* [ML] Fix alignment of filter icons in anomalies table (#26253)

* [ML] Fix alignment of filter icons in anomalies table

* [ML] Teak y position of icons in expanded row of table

* translate sample data (#26069)

translate sample data

* [ML] Wrap controller initialization in assertions. (#26265)

- The controller tests introduced in #25382 had a flaw: If a controller initialization would fail and throw an error, that test suite wouldn't be able to clean up any stubs. So tests using the same stubs would report and error because the stubs couldn't be wrapped again.
- This PR wraps every controller initialization inside an assertion and catches those errors properly as part of the test.

* [APM] fixes #20145 by displaying span.context.http.url in the span details flyout (#26238)

* Fix spaces license check (#26270)

## Summary

Allows the public spaces API to work with a gold license

Resolves #26271

* Job Info button in Reporting Listing (#25421)

* Job Info button in Reporting Listing

* use lodash directly

* start of flyout use

* description list in flyout

* capitalize

* undefined guard

* expire info on close

* add jest test

* better at error handling + messaging

* Add description for vis types (#26243)

* [migrations] Throw error if reindex task fails (#26062)

Signed-off-by: Tyler Smalley <[email protected]>

* [Reporting] Better logging for waitForSelector failure (#25762)

* [Reporting] Better logging for waitForSelector failure

* break waitForSelector

* experimental changes

* cleanup/consistency

* fix some test report title strings

* test disable chromium

* roll back test code

* take out non-working phantom changes

* roll back disable chromium test

* allow logger to use curried tags

* temporarily re-do report failure-causing change for test

* replace newline with escaped for single log line

* undo test change

* remove obsolete test

* [kbn/pm] allow packages to define extra paths to clean (#26132)

I noticed some discussion about how kbn clean should probably clear out the `.eslintcache` file, since it doesn't handle changes in related modules (for things like the import plugin) and it would be nice if `yarn kbn clean` took care of the issue. I figured it's not a bad idea, but adding `.eslintcache` directly to `@kbn/pm` felt wrong, so instead I've added another config options that can go in the package.json file, `clean.extraPatterns`. This array of patterns is passed into `del()` so that it can include things like negation.

As the name suggests this doesn't override the initial paths, just adds some extras that will be checked and cleared when `yarn kbn clean` is run.

* [config] fix logging.useUTC deprecation unset (#26053)

* Extend precommit hook script to support git GUI apps (#25883)

* feat(NA): extend support from precommit hook to git GUI apps.

* docs(NA): more descriptive error message.

* [DOCS] Clarify monitoring dependencies (#26229)

* apm: add ECS fields to index pattern (#26214)

* support standard license (#26294)

* [kbn-pm] update build

* [eslint] use disallow license header rule (#26309)

Fixes #26295

There are several places where we have accidentally added new license headers with linters but failed to remove old license headers manually. This prevents that by applying the an inverted version of the license headers rule that removed invalid license headers when files are moved.

* Bump node to 8.14.0 (#26313)

Signed-off-by: Tyler Smalley <[email protected]>

* Watch optimizer cache invalidation  (#24172)

* chore(NA): cherry pick work from spencer on impleting the cache invalidation system and merging it with current master.

* feat(NA): add support for dlls bundle into the cache state invalidation system.

* chore(NA): merge with master.

* feat(NA): first working version for the watch cache.

* feat(NA): added logger, correct cache delete and removed last todos.

* feat(NA): remove some useless features for the time being.

* refact(NA): just pass kibanaHapiServer.log function directly instead of an anonimous function that calls the kibanaHapiServer.log one.

* refact(NA): move everything to async.

* refact(NA): remove dll mentions.

* chore(NA): removed types/mkdirp as we dont use mkdirp into typescript.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result review v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants