[Table Vis] move format table logic to table render and consolidate types #3397

ananzh · 2023-02-08T00:59:08Z

Description

Currently, table data is formatted by a until function convertToFormattedData in TableVisComponent. In this PR, we moved the formatting data process to table_vis_response_handler.ts to combine with other data process logics. In this way, component render and data handling logics are completely isolated. This PR also solidate some types.

Issue Resolved:

fixes #3395

Check List

codecov-commenter · 2023-02-08T01:56:49Z

Codecov Report

Merging #3397 (11f0e49) into main (e4fccfc) will decrease coverage by 0.04%.
The diff coverage is 91.89%.

❗ Current head 11f0e49 differs from pull request most recent head e9e9f46. Consider uploading reports for the commit e9e9f46 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3397      +/-   ##
==========================================
- Coverage   66.44%   66.40%   -0.04%     
==========================================
  Files        3208     3226      +18     
  Lines       61744    61998     +254     
  Branches     9537     9585      +48     
==========================================
+ Hits        41023    41167     +144     
- Misses      18431    18525      +94     
- Partials     2290     2306      +16

Flag	Coverage Δ
Linux	`66.40% <91.89%> (+0.01%)`	⬆️
Windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...le/public/components/table_vis_component_group.tsx	`100.00% <ø> (ø)`
...table/public/components/table_vis_grid_columns.tsx	`3.84% <0.00%> (ø)`
src/plugins/vis_type_table/public/table_vis_fn.ts	`100.00% <ø> (ø)`
...pe_table/public/utils/convert_to_formatted_data.ts	`73.46% <60.00%> (ø)`
...is_type_table/public/components/table_vis_cell.tsx	`91.66% <91.66%> (ø)`
...vis_type_table/public/components/table_vis_app.tsx	`100.00% <100.00%> (ø)`
...pe_table/public/components/table_vis_component.tsx	`100.00% <100.00%> (ø)`
...is_type_table/public/table_vis_response_handler.ts	`95.23% <100.00%> (ø)`
.../vis_type_table/public/utils/add_percentage_col.ts	`100.00% <100.00%> (ø)`
...vis_type_table/public/utils/convert_to_csv_data.ts	`80.00% <100.00%> (ø)`
... and 2 more

... and 17 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

AMoo-Miki · 2023-03-01T18:52:14Z

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

+    return uiState.sort.colIndex !== undefined &&
      columns[uiState.sort.colIndex].id &&
      uiState.sort.direction


Suggested change

return uiState.sort.colIndex !== undefined &&

columns[uiState.sort.colIndex].id &&

uiState.sort.direction

return columns[uiState.sort.colIndex]?.id !== undefined &&

uiState.sort.direction

... since checking uiState.sort.colIndex proves nothing and all we care for is columns[uiState.sort.colIndex].id to exist. if uiState.sort.colIndex = 999 and there is not such index, columns[uiState.sort.colIndex].id would throw exception.

uiState.sort.colIndex is optional... shouldn't we request it to be not undefined?
if not check this, there is a type error

AMoo-Miki · 2023-03-01T18:52:53Z

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

+    return uiState.sort.colIndex &&
      dataGridColumns[uiState.sort.colIndex].id &&


Suggested change

return uiState.sort.colIndex &&

dataGridColumns[uiState.sort.colIndex].id &&

return dataGridColumns[uiState.sort.colIndex]?.id !== undefined &&

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

src/plugins/vis_type_table/public/table_vis_response_handler.ts

joshuarrrr

A couple minor questions and comments, otherwise I agree with Miki's suggestions.

CHANGELOG.md

src/plugins/vis_type_table/public/table_vis_response_handler.ts

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

joshuarrrr

@ananzh This is looking great so far! I've only reviewed the utils, but I figured I'd share my comments so far.

joshuarrrr · 2023-04-05T21:59:50Z

src/plugins/vis_type_table/public/utils/use_pagination.ts

@@ -20,20 +20,23 @@ export const usePagination = (visConfig: TableVisConfig, nRow: number) => {
  ]);

  useEffect(() => {
-    const perPage = visConfig.perPage || 10;
+    const perPage = visParams.perPage || 0;


Why the change in the fallback value? It seems like 0 items per page is an invalid page size, and 10 is a more reasonable default.

Is there also a way we can avoid doing this fallback twice (here and on L12)?

I think I never explain this thoroughly but keeping changing this value. My bad. The line const perPage = visParams.perPage || 0; is setting the default value for perPage to 0 if visParams.perPage is not set or is a falsy value (e.g., null, undefined, or 0). The reason for using 0 as the default value is because the pagination functionality is designed to be optional. When the value of perPage is set to 0, it indicates that pagination is not enabled, and the table should display all rows without pagination. If we set the default value to 10, it would mean that pagination would be enabled by default, displaying only 10 rows per page even if the user has not explicitly set a perPage value.

So setting const perPage = visParams.perPage || 0 makes logic sense but it makes the next line const maxiPageIndex = Math.ceil(nRow / perPage) - 1; vulnerable. There are two edge cases we should consider 1) visParams.perPage is 0. Well this is not possible. The Max rows per page option (src/plugins/vis_type_table/public/components/table_vis_options.tsx) forbids being set to 0. The NumberInputOption component for the Max rows per page option has a min property set to 1, which means the minimum allowed value for this input field is 1. 2) visParams.perPage = ''. This is possible if Max rows per page option is empty. This would set maxiPageIndex to Infinity. The setPagination function will be called, but since maxiPageIndex is Infinity, the condition p.pageIndex > maxiPageIndex will always be false, and the pageIndex will not be updated. This is also reflected in the unit test:

it('should not set pagination if perPage is empty string', () => { visParams.perPage = ''; const { result } = renderHook(() => usePagination(visParams, 20)); expect(result.current).toEqual(undefined); });

However, the code still does not break. Even though maxiPageIndex becomes Infinity, the setPagination function will update the pageIndex value to be the same as the current pageIndex value, which means that the pagination state remains unchanged.

Not necessary for approval, but I think this is a case where it just makes sense to abstract it out as a function, that can have its own test cases and comment explanations:

const getPerPageCount = (perPageConfig, nRow) => { // if no perPageConfig specified, set to 0 to disable pagination const perPage = perPageConfig || 0; return Math.min(perPage, nRow); }

src/plugins/vis_type_table/public/utils/use_pagination.test.ts

src/plugins/vis_type_table/public/utils/ui_state_management.ts

src/plugins/vis_type_table/public/utils/ui_state_management.test.ts

src/plugins/vis_type_table/public/utils/add_percentage_col.test.ts

joshuarrrr · 2023-04-14T00:03:38Z

src/plugins/vis_type_table/public/utils/add_percentage_col.test.ts

+          sumTotal: 5,
+        },
+        {
+          title: 'count percentages',


Out of scope, but it's a little odd that this function allows the title of the percentage column to be set independently from the title of the source column.

joshuarrrr

Only some minor questions and suggestions, but nothing blocking.

src/plugins/vis_type_table/public/table_vis_response_handler.ts

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

src/plugins/vis_type_table/public/components/table_vis_component_group.test.tsx

stale

joshuarrrr · 2023-04-14T00:58:01Z

all checks pass, but needs changelog conflict resolution.

joshuarrrr · 2023-04-14T23:04:44Z

@ananzh You should merge or rebase on main to resolve the conflicts.

joshuarrrr · 2023-04-14T23:58:47Z

@ananzh The visbuilder tablevis functional test is failing:

 0 passing (1m)
  1 failing

  1) Vis Builder: Table Chart
       Basic test:

      Timed out retrying after 60000ms
      + expected - actual

      -'CatRow: 1, Column: 1:2,500Row: 1, Column: 2:'
      +'Dog'
      
      at testMetric (http://localhost:5601/__cypress/tests?p=cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/vis_builder/vis_types/table.spec.js:147:91)
      at testSplitRows (http://localhost:5601/__cypress/tests?p=cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/vis_builder/vis_types/table.spec.js:154:5)
      at Context.eval (http://localhost:5601/__cypress/tests?p=cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/vis_builder/vis_types/table.spec.js:130:7)

joshuarrrr · 2023-04-17T19:20:13Z

src/plugins/vis_type_table/public/components/table_vis_app.scss

 .visTable {
  display: flex;
  flex-direction: column;
  flex: 1 0 0;
  overflow: auto;
+
+  @include euiScrollBar;


Interesting - fine for this PR, but this usage is something we'll want to document/revisit in OUI - I think we'd prefer to avoid these stylings.

I am using a consistent @include euiScrollBar; as shown in other scss files in our repo. Why we want to avoid it?

See #3803 (comment) - we're aiming for a state where there are no/minimal SASS files in OSD. One difference is that EUI encouraged direct usage of styles (as done here), whereas OUI prefers only using React components as a way to access those styles.

But don't worry about it here - I'll open an issue.

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

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

joshuarrrr · 2023-04-17T19:47:40Z

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

+    const sortColumnId =
+      sort.colIndex !== null && sort.colIndex !== undefined
+        ? formattedColumns[sort.colIndex]?.id
+        : undefined;


nit - this ternary is unnecessary:

Suggested change

const sortColumnId =

sort.colIndex !== null && sort.colIndex !== undefined

? formattedColumns[sort.colIndex]?.id

: undefined;

const sortColumnId = formattedColumns[sort.colIndex]?.id;

Simple demo:

const fc = [{id: 'foo'}, {id: 'bar'}]; > undefined const s = {} > undefined fc[s.colIndex]?.id > undefined s.colIndex = 1 > 1 fc[s.colIndex]?.id > "bar"

Yeah I think I tried before. But it will give a type error

(property) ColumnSort.colIndex?: number | undefined Type 'undefined' cannot be used as an index type.ts(2538)

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

joshuarrrr · 2023-04-17T19:53:15Z

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

+      sort.colIndex !== null &&
+      sort.colIndex !== undefined &&
+      dataGridColumns[sort.colIndex].id &&
+      sort.direction


nit

Suggested change

sort.colIndex !== null &&

sort.colIndex !== undefined &&

dataGridColumns[sort.colIndex].id &&

sort.direction

dataGridColumns[sort.colIndex]?.id !== undefined &&

sort.direction

Yah I think I wrote to solve the type error Type 'undefined' cannot be used as an index type.ts(2538)

joshuarrrr · 2023-04-17T20:02:02Z

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

+        <EuiTitle size="xs" className="visTable__component__title">
          <h3>{title}</h3>
        </EuiTitle>


nit - There's a preferred way to center a title with OUI components:

Suggested change

<EuiTitle size="xs" className="visTable__component__title">

<h3>{title}</h3>

</EuiTitle>

<EuiTitle size="xs">

<EuiTextAlign textAlign="center">

<h3>{title}</h3>

</EuiTextAlign>

</EuiTitle>

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

joshuarrrr · 2023-04-17T20:28:11Z

src/plugins/vis_type_table/public/utils/use_pagination.ts

@@ -20,20 +20,23 @@ export const usePagination = (visConfig: TableVisConfig, nRow: number) => {
  ]);

  useEffect(() => {
-    const perPage = visConfig.perPage || 10;
+    const perPage = visParams.perPage || 0;


Not necessary for approval, but I think this is a case where it just makes sense to abstract it out as a function, that can have its own test cases and comment explanations:

const getPerPageCount = (perPageConfig, nRow) => { // if no perPageConfig specified, set to 0 to disable pagination const perPage = perPageConfig || 0; return Math.min(perPage, nRow); }

joshuarrrr

more nits - but nothing blocking.

Currently, table data is formatted by a until function convertToFormattedData in TableVisComponent. In this PR, we moved the formatting data process to table_vis_response_handler.ts to combine with other data process logics. In this way, component render and data handling logics are completely isolated. This PR also solidate some types. Issue Resolved: opensearch-project#3395 opensearch-project#2856 Signed-off-by: Anan Zhuang <ananzh@amazon.com>

joshuarrrr · 2023-04-17T22:01:07Z

src/plugins/vis_type_table/public/components/table_vis_app.scss

 .visTable {
  display: flex;
  flex-direction: column;
  flex: 1 0 0;
  overflow: auto;
+
+  @include euiScrollBar;


But don't worry about it here - I'll open an issue.

opensearch-trigger-bot · 2023-04-17T22:56:50Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3397-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fc195ea580844dea47e21c4854ac7767f2513531
# Push it to GitHub
git push --set-upstream origin backport/backport-3397-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3397-to-2.x.

…d unit tests Currently, table data is formatted by a until function convertToFormattedData in TableVisComponent. In this PR, we moved the formatting data process to table_vis_response_handler.ts to combine with other data process logics. In this way, component render and data handling logics are completely isolated. This PR also solidate some types. Backport PR: opensearch-project#3397 Issue Resolved: opensearch-project#3395 opensearch-project#2856 Signed-off-by: Anan Zhuang <ananzh@amazon.com>

…d unit tests (#3869) Currently, table data is formatted by a until function convertToFormattedData in TableVisComponent. In this PR, we moved the formatting data process to table_vis_response_handler.ts to combine with other data process logics. In this way, component render and data handling logics are completely isolated. This PR also solidate some types. Backport PR: #3397 Issue Resolved: #3395 #2856 Signed-off-by: Anan Zhuang <ananzh@amazon.com>

ananzh requested a review from a team as a code owner February 8, 2023 00:59

ananzh force-pushed the table-polish branch from 8f7ceab to 2783572 Compare February 8, 2023 01:00

ananzh added tableVis table visualization v2.6.0 labels Feb 8, 2023

ananzh requested a review from joshuarrrr February 9, 2023 21:47

ananzh mentioned this pull request Feb 16, 2023

[Table Visualization] Add unit tests via testing-library #3435

Closed

8 tasks

joshuarrrr added the backport 2.x label Feb 17, 2023

ananzh added v2.7.0 and removed v2.6.0 labels Feb 17, 2023

ananzh force-pushed the table-polish branch from e9c5838 to f027d3f Compare February 20, 2023 18:21

AMoo-Miki previously requested changes Mar 1, 2023

View reviewed changes

ananzh changed the title ~~[Table Vis] move format table logic to table render and solidate types~~ [Table Vis] move format table logic to table render and consolidate types Mar 7, 2023

ashwin-pc assigned joshuarrrr Mar 7, 2023

joshuarrrr reviewed Mar 20, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/plugins/vis_type_table/public/table_vis_response_handler.ts Outdated Show resolved Hide resolved

src/plugins/vis_type_table/public/components/table_vis_component_group.tsx Outdated Show resolved Hide resolved

joshuarrrr mentioned this pull request Mar 23, 2023

2.7.0 Platform Upgrades for Plugins (O11y, MDS, Geospatial,+) #3674

Closed

23 tasks

ananzh force-pushed the table-polish branch 2 times, most recently from 1a04a50 to 2611e06 Compare April 5, 2023 19:52

ananzh mentioned this pull request Apr 7, 2023

Fix: Update button position when number of rows per page is large #3797

Merged

8 tasks

joshuarrrr reviewed Apr 14, 2023

View reviewed changes

joshuarrrr previously approved these changes Apr 14, 2023

View reviewed changes

joshuarrrr assigned AMoo-Miki and ananzh Apr 14, 2023

ananzh dismissed joshuarrrr’s stale review via 49ca86c April 14, 2023 22:47

ananzh force-pushed the table-polish branch from 2611e06 to 49ca86c Compare April 14, 2023 22:47

ananzh requested review from kavilla, seanneumann and ashwin-pc as code owners April 14, 2023 22:47

ananzh requested review from abbyhu2000, zengyan-amazon, kristenTian, zhongnansu and manasvinibs as code owners April 14, 2023 22:47

ananzh force-pushed the table-polish branch from 49ca86c to 2e44c2f Compare April 14, 2023 23:16

joshuarrrr mentioned this pull request Apr 17, 2023

[Table Visualization][Test] Add unit tests #2856

Closed

joshuarrrr reviewed Apr 17, 2023

View reviewed changes

joshuarrrr previously approved these changes Apr 17, 2023

View reviewed changes

ananzh dismissed joshuarrrr’s stale review via 11f0e49 April 17, 2023 21:39

ananzh force-pushed the table-polish branch from 11f0e49 to e9e9f46 Compare April 17, 2023 21:42

joshuarrrr approved these changes Apr 17, 2023

View reviewed changes

ananzh assigned abbyhu2000 and ashwin-pc Apr 17, 2023

abbyhu2000 approved these changes Apr 17, 2023

View reviewed changes

joshuarrrr merged commit fc195ea into opensearch-project:main Apr 17, 2023

ananzh mentioned this pull request Apr 17, 2023

[Backport 2.x][Table Vis] move format table logic to table render and consolidate types #3869

Merged

8 tasks

joshuarrrr mentioned this pull request Apr 18, 2023

Fix: Data Table rendering only in a small scrollable portion of the screen #3816

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table Vis] move format table logic to table render and consolidate types #3397

[Table Vis] move format table logic to table render and consolidate types #3397

ananzh commented Feb 8, 2023 •

edited by joshuarrrr

Loading

codecov-commenter commented Feb 8, 2023 •

edited

Loading

AMoo-Miki Mar 1, 2023 •

edited

Loading

ananzh Mar 1, 2023

AMoo-Miki Mar 1, 2023

joshuarrrr left a comment

joshuarrrr left a comment

joshuarrrr Apr 5, 2023

ananzh Apr 14, 2023

ananzh Apr 14, 2023

joshuarrrr Apr 17, 2023

joshuarrrr Apr 14, 2023

joshuarrrr left a comment

joshuarrrr commented Apr 14, 2023

joshuarrrr commented Apr 14, 2023

joshuarrrr commented Apr 14, 2023

joshuarrrr Apr 17, 2023

ananzh Apr 17, 2023

joshuarrrr Apr 17, 2023

joshuarrrr Apr 17, 2023

joshuarrrr Apr 17, 2023

ananzh Apr 17, 2023

joshuarrrr Apr 17, 2023

ananzh Apr 17, 2023

joshuarrrr Apr 17, 2023

joshuarrrr Apr 17, 2023

joshuarrrr left a comment

joshuarrrr Apr 17, 2023

opensearch-trigger-bot bot commented Apr 17, 2023

		return uiState.sort.colIndex &&
		dataGridColumns[uiState.sort.colIndex].id &&

	return uiState.sort.colIndex &&
	dataGridColumns[uiState.sort.colIndex].id &&
	return dataGridColumns[uiState.sort.colIndex]?.id !== undefined &&

[Table Vis] move format table logic to table render and consolidate types #3397

[Table Vis] move format table logic to table render and consolidate types #3397

Conversation

ananzh commented Feb 8, 2023 • edited by joshuarrrr Loading

Description

Issue Resolved:

Check List

codecov-commenter commented Feb 8, 2023 • edited Loading

Codecov Report

AMoo-Miki Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr 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

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr commented Apr 14, 2023

joshuarrrr commented Apr 14, 2023

joshuarrrr commented Apr 14, 2023

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

joshuarrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Apr 17, 2023

ananzh commented Feb 8, 2023 •

edited by joshuarrrr

Loading

codecov-commenter commented Feb 8, 2023 •

edited

Loading

AMoo-Miki Mar 1, 2023 •

edited

Loading