[Lens][Visualize][Inspector][Reporting] Unified check for CSV cells for known formula characters (and value escaping more in general) #105221

dej611 · 2021-07-12T13:52:22Z

Summary

This PR unifies the escaping logic for both reporting and client side CSV export.
The escape_value module has moved to the data plugin, which now exposes a new escapeFormulaValues flag to enable the new formula check.

In addition on top of this refactor a new tableHasFormulas function has been created which is used to provide visual feedback to the user when the exported table contains formula special characters.

Tested downloads:

Lens
Visualize data table
Inspector download
Reporting (already in place prior to this PR)

Note

The download button on the Dashboard contextual menu does not have a warning tooltip: should it be added?
Is tooltip on context menu buttons a valid UI pattern?
It's already used for Visualize data table but unsure about general approach here.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-07-13T10:52:30Z

Pinging @elastic/kibana-app (Team:KibanaApp)

ThomThomson

Code review only, presentation team changes LGTM!

wylieconlon

So I've tested with xpack.reporting.csv.escapeFormulaValues on and found that it does correctly escape when using the CSV Reporting feature, but not when using the other CSV export options. I also saw that the tooltips are correctly warning about CSV formulas. So even if this is working as expected, I'd like a followup to see how we want to deal with formula escaping across all CSVs.

wylieconlon · 2021-07-13T19:24:07Z

src/plugins/vis_type_table/public/components/table_vis_controls.tsx

+        position="top"
+        content={i18n.translate('visTypeTable.vis.controls.exportButtonFormulasWarning', {
+          defaultMessage:
+            'Your CSV contains characters which spreadsheet application can interpret as formulas',


Looks like the same typo in all the text you've added:

Suggested change

'Your CSV contains characters which spreadsheet application can interpret as formulas',

'Your CSV contains characters which spreadsheet applications can interpret as formulas',

wylieconlon · 2021-07-13T19:31:39Z

src/plugins/data/common/exports/export_csv.tsx

  formatFactory: FormatFactory;
  raw?: boolean;
 }

 export function datatableToCSV(
  { columns, rows }: Datatable,
-  { csvSeparator, quoteValues, formatFactory, raw }: CSVOptions
+  { csvSeparator, quoteValues, formatFactory, raw, escapeFormulaValues = false }: CSVOptions


Can we make this a required argument instead? I found it hard to trace the code that is using the default value here vs actually providing a value. It looks like only the Reporting integration has a way to escape formulas, with the xpack.reporting.csv.escapeFormulaValues setting, and I was expecting that other places in the code would also use escaping.

dej611 · 2021-07-14T13:47:05Z

So I've tested with xpack.reporting.csv.escapeFormulaValues on and found that it does correctly escape when using the CSV Reporting feature, but not when using the other CSV export options. I also saw that the tooltips are correctly warning about CSV formulas. So even if this is working as expected, I'd like a followup to see how we want to deal with formula escaping across all CSVs.

You are right, the formula escape is not currently enabled outside Reporting.
Next step here is to figure out how to expose the escape option to the user and enable the feature, I'll create an issue about that.

jloleysens · 2021-07-15T14:43:17Z

@elasticmachine merge upstream

jloleysens

Code changes LGTM!

Did a smoke test for reporting locally, worked as expected.

jloleysens · 2021-07-15T12:00:18Z

src/plugins/data/public/utils/table_inspector_view/components/download_options.tsx

+    const detectedFormulasInTables = this.props.datatables.some(({ columns, rows }) =>
+      tableHasFormulas(columns, rows)
+    );


It might be useful to memoize the result of this check and only re-run it when the datatables prop has changed. WDYT?

I did add the memoization on this specific instance. Or perhaps you meant something more generic like on the tableHasFormulas function?

kibanamachine · 2021-07-15T17:18:16Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	588	591	+3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	3259	3273	+14

Async chunks

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

id	before	after	diff
`data`	164.4KB	165.1KB	+718.0B
`lens`	1.5MB	1.5MB	+479.0B
`visTypeTable`	102.2KB	102.7KB	+492.0B
total			+1.6KB

Page load bundle

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

id	before	after	diff
`dashboard`	329.2KB	329.2KB	+23.0B
`data`	840.0KB	841.4KB	+1.4KB
total			+1.5KB

Unknown metric groups

API count

id	before	after	diff
`data`	3825	3839	+14

History

💚 Build #138544 succeeded b8d2add
💛 Build #138486 was flaky 14f2401
💚 Build #138085 succeeded a6feca1
💔 Build #137912 failed 1f921ce
💔 Build #137841 failed 2601e6a

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

…or known formula characters (and value escaping more in general) (elastic#105221) * ✨ Unify escaping logic for csv export * 📝 Update api doc * ✅ Fix test with new escape logic * 👌 First batch of feedback * 💬 Fix typo * 👌 Memoize function Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-07-16T08:58:48Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…or known formula characters (and value escaping more in general) (#105221) (#105925) * ✨ Unify escaping logic for csv export * 📝 Update api doc * ✅ Fix test with new escape logic * 👌 First batch of feedback * 💬 Fix typo * 👌 Memoize function Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Marco Liberati <[email protected]>

✨ Unify escaping logic for csv export

fd94375

dej611 added 3 commits July 12, 2021 16:26

📝 Update api doc

2601e6a

✅ Fix test with new escape logic

1f921ce

Merge remote-tracking branch 'upstream/master' into fix/56081

a6feca1

dej611 added the release_note:fix label Jul 13, 2021

dej611 marked this pull request as ready for review July 13, 2021 10:52

dej611 requested a review from a team July 13, 2021 10:52

dej611 requested review from a team as code owners July 13, 2021 10:52

ThomThomson approved these changes Jul 13, 2021

View reviewed changes

wylieconlon approved these changes Jul 13, 2021

View reviewed changes

dej611 added 3 commits July 14, 2021 17:32

👌 First batch of feedback

53bc2b8

💬 Fix typo

14f2401

Merge remote-tracking branch 'upstream/master' into fix/56081

b8d2add

Merge branch 'master' into fix/56081

d65d858

jloleysens approved these changes Jul 15, 2021

View reviewed changes

👌 Memoize function

756e252

dej611 merged commit 2e5e4ca into elastic:master Jul 16, 2021

dej611 deleted the fix/56081 branch July 16, 2021 08:55

dej611 mentioned this pull request Jul 16, 2021

[Lens][Dashboard][Inspector] Escape formulas option for CSV #105924

Open

kibanamachine mentioned this pull request Jul 16, 2021

[7.x] [Lens][Visualize][Inspector][Reporting] Unified check for CSV cells for known formula characters (and value escaping more in general) (#105221) #105925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens][Visualize][Inspector][Reporting] Unified check for CSV cells for known formula characters (and value escaping more in general) #105221

[Lens][Visualize][Inspector][Reporting] Unified check for CSV cells for known formula characters (and value escaping more in general) #105221

dej611 commented Jul 12, 2021

elasticmachine commented Jul 13, 2021

ThomThomson left a comment

wylieconlon left a comment

wylieconlon Jul 13, 2021

wylieconlon Jul 13, 2021

dej611 commented Jul 14, 2021

jloleysens commented Jul 15, 2021

jloleysens left a comment

jloleysens Jul 15, 2021

dej611 Jul 15, 2021

kibanamachine commented Jul 15, 2021

API count

kibanamachine commented Jul 16, 2021

	'Your CSV contains characters which spreadsheet application can interpret as formulas',
	'Your CSV contains characters which spreadsheet applications can interpret as formulas',

[Lens][Visualize][Inspector][Reporting] Unified check for CSV cells for known formula characters (and value escaping more in general) #105221

[Lens][Visualize][Inspector][Reporting] Unified check for CSV cells for known formula characters (and value escaping more in general) #105221

Conversation

dej611 commented Jul 12, 2021

Summary

Note

Checklist

For maintainers

elasticmachine commented Jul 13, 2021

ThomThomson left a comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Jul 13, 2021

Choose a reason for hiding this comment

wylieconlon Jul 13, 2021

Choose a reason for hiding this comment

dej611 commented Jul 14, 2021

jloleysens commented Jul 15, 2021

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Jul 15, 2021

Choose a reason for hiding this comment

dej611 Jul 15, 2021

Choose a reason for hiding this comment

kibanamachine commented Jul 15, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kibanamachine commented Jul 16, 2021

💚 Backport successful