[Reporting] Relax save requirement for CSV reports #99313

jloleysens · 2021-05-05T09:46:43Z

Summary

Part one of addressing this issue: #18439

Release note

We relaxed the requirement to first create a saved search when generating a CSV report via the share menu in Discover.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

…arches

tsullivan · 2021-05-06T05:09:13Z

There should be a default title (with translations applied) so the when you see the report in the management app, it has some kind of title (maybe "New search"):

tsullivan · 2021-05-06T05:21:42Z

x-pack/plugins/reporting/public/components/reporting_panel_content.tsx

@@ -85,7 +90,7 @@ class ReportingPanelContentUi extends Component<Props, State> {
  }

  public render() {
-    if (this.isNotSaved() || this.props.isDirty || this.state.isStale) {
+    if (this.mustSaveReport()) {


I think we can simply add a prop to the ReportingPanelContent component called requiresSavedState or something. In x-pack/plugins/reporting/public/share_context_menu/register_csv_reporting.tsx#L109, the prop is hardcoded to false and in x-pack/plugins/reporting/public/components/screen_capture_panel_content.tsx#L45 it is hardcoded to true.

Suggested change

if (this.mustSaveReport()) {

if (this.props.requiresSavedState() && this.isNotSaved() || this.props.isDirty || this.state.isStale) {

tsullivan · 2021-05-06T05:25:00Z

I recalled an issue that was closed as a duplicate of #18439

Create a dashboard and add a saved search panel that has expanded columns
Re-arrange the columns
Download a CSV export

The columns in the export should match the state set by the user in the dashboard panel

Here's the original issue: #43977

jloleysens · 2021-05-06T08:41:57Z

@elasticmachine merge upstream

jloleysens · 2021-05-06T10:59:43Z

@tsullivan Thanks for providing context and extra things to consider!

The columns in the export should match the state set by the user in the dashboard panel

This sounds like something that we should look into addressing in another PR if there is a specific issue. I tested with discover and it looks like field order is preserved

It has some kind of title (maybe "New search")

Great point! I came up with the following

My reasoning behind the current implementation is that reporting should not have to guess what the title should be based on object type, rather users of reporting should take the responsibility of ensuring that an appropriate title is provided. In this way titles can contain a bit more context. Let me know if you think reporting should also have a fallback for titles, though.

elasticmachine · 2021-05-06T11:00:57Z

Pinging @elastic/kibana-app-services (Team:AppServices)

jloleysens · 2021-05-10T07:47:55Z

@elasticmachine merge upstream

jloleysens · 2021-05-11T12:01:26Z

@elasticmachine merge upstream

kertal · 2021-05-11T13:44:02Z

src/plugins/discover/public/application/components/top_nav/get_top_nav_links.ts

+            savedSearch.title ||
+            i18n.translate('discover.localMenu.fallbackReportTitle', {
+              defaultMessage: 'Discover search [{date}]',
+              values: { date: moment().toISOString() },


Without true it would return UTC time, but I think it would be more useful to provide an ISO string with offset to UTC

Suggested change

values: { date: moment().toISOString() },

values: { date: moment().toISOString(true) },

BTW great improvement! love it that you no longer have to save a search to export!

jloleysens · 2021-05-11T15:15:57Z

@elasticmachine merge upstream

tsullivan

LGTM

kibanamachine · 2021-05-11T17:09:57Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	410.4KB	410.6KB	+206.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`reporting`	84.3KB	84.4KB	+77.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`crossClusterReplication`	8	6	-2
`fleet`	4	2	-2
`globalSearch`	4	2	-2
`licensing`	18	15	-3
total			-9

History

💚 Build #125324 succeeded c4927b0
💔 Build #125274 failed 8239492
💚 Build #124961 succeeded 3aae4a3
💚 Build #124675 succeeded 3efeff8
💚 Build #124665 succeeded 865eaad

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kertal

Kibana app owned code LGTM, thx for this little big change! Tested locally, timestamp in the CSV title now is also helpful for non-UTC people 🕐

* update requirement to save csv report in ui * update expectation that CSV reporting is disabled for new discover searches * update test expectations (again) * refactor to using props-driven approach * provide a fallback title * refine title a bit more * added component level test * return ISO string with offset Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* update requirement to save csv report in ui * update expectation that CSV reporting is disabled for new discover searches * update test expectations (again) * refactor to using props-driven approach * provide a fallback title * refine title a bit more * added component level test * return ISO string with offset Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

update requirement to save csv report in ui

94d6e29

jloleysens requested a review from tsullivan May 5, 2021 09:46

jloleysens added 2 commits May 5, 2021 14:40

update expectation that CSV reporting is disabled for new discover se…

1c07517

…arches

update test expectations (again)

71649af

tsullivan reviewed May 6, 2021

View reviewed changes

kibanamachine and others added 5 commits May 6, 2021 04:42

Merge branch 'master' into reporting/relax-save-requirement-for-csv

f7ff59e

refactor to using props-driven approach

1386e76

provide a fallback title

865eaad

refine title a bit more

611a506

added component level test

3efeff8

jloleysens marked this pull request as ready for review May 6, 2021 10:59

jloleysens requested a review from a team May 6, 2021 10:59

jloleysens added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement Team:AppServices v7.14.0 v8.0.0 labels May 6, 2021

Merge branch 'master' into reporting/relax-save-requirement-for-csv

3aae4a3

Merge branch 'master' into reporting/relax-save-requirement-for-csv

8239492

kertal reviewed May 11, 2021

View reviewed changes

kibanamachine and others added 2 commits May 11, 2021 11:16

Merge branch 'master' into reporting/relax-save-requirement-for-csv

c4927b0

return ISO string with offset

a1d6011

tsullivan self-requested a review May 11, 2021 16:56

tsullivan approved these changes May 11, 2021

View reviewed changes

kertal approved these changes May 12, 2021

View reviewed changes

jloleysens merged commit c22d9fd into elastic:master May 12, 2021

jloleysens deleted the reporting/relax-save-requirement-for-csv branch May 12, 2021 08:32

jloleysens mentioned this pull request May 12, 2021

[7.x] [Reporting] Relax save requirement for CSV reports (#99313) #99888

Merged

tsullivan mentioned this pull request Apr 5, 2022

[Discover] Improve error message for CSV Reports menu option in readonly Discover state #129300

Merged

2 tasks

tsullivan mentioned this pull request Apr 20, 2022

[Reporting] Ability to export CSV without saved object #113694

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Relax save requirement for CSV reports #99313

[Reporting] Relax save requirement for CSV reports #99313

jloleysens commented May 5, 2021 •

edited

Loading

tsullivan commented May 6, 2021

tsullivan May 6, 2021

tsullivan commented May 6, 2021

jloleysens commented May 6, 2021

jloleysens commented May 6, 2021

elasticmachine commented May 6, 2021

jloleysens commented May 10, 2021

jloleysens commented May 11, 2021

kertal May 11, 2021

jloleysens commented May 11, 2021

tsullivan left a comment

kibanamachine commented May 11, 2021

References to deprecated APIs

kertal left a comment

	if (this.mustSaveReport()) {
	if (this.props.requiresSavedState() && this.isNotSaved() \|\| this.props.isDirty \|\| this.state.isStale) {

	values: { date: moment().toISOString() },
	values: { date: moment().toISOString(true) },

[Reporting] Relax save requirement for CSV reports #99313

[Reporting] Relax save requirement for CSV reports #99313

Conversation

jloleysens commented May 5, 2021 • edited Loading

Summary

Release note

Checklist

tsullivan commented May 6, 2021

tsullivan May 6, 2021

Choose a reason for hiding this comment

tsullivan commented May 6, 2021

jloleysens commented May 6, 2021

jloleysens commented May 6, 2021

elasticmachine commented May 6, 2021

jloleysens commented May 10, 2021

jloleysens commented May 11, 2021

kertal May 11, 2021

Choose a reason for hiding this comment

jloleysens commented May 11, 2021

tsullivan left a comment

Choose a reason for hiding this comment

kibanamachine commented May 11, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

References to deprecated APIs

History

kertal left a comment

Choose a reason for hiding this comment

jloleysens commented May 5, 2021 •

edited

Loading