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

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

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Feb 8, 2023

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

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@ananzh ananzh requested a review from a team as a code owner February 8, 2023 00:59
@ananzh ananzh added tableVis table visualization v2.6.0 labels Feb 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

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.

Comment on lines 38 to 40
return uiState.sort.colIndex !== undefined &&
columns[uiState.sort.colIndex].id &&
uiState.sort.direction
Copy link
Collaborator

@AMoo-Miki AMoo-Miki Mar 1, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines 68 to 69
return uiState.sort.colIndex &&
dataGridColumns[uiState.sort.colIndex].id &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@ananzh 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
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

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

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

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

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

useEffect(() => {
const perPage = visConfig.perPage || 10;
const perPage = visParams.perPage || 0;
Copy link
Member

Choose a reason for hiding this comment

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

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)?

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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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);
}

sumTotal: 5,
},
{
title: 'count percentages',
Copy link
Member

Choose a reason for hiding this comment

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

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
joshuarrrr previously approved these changes Apr 14, 2023
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Only some minor questions and suggestions, but nothing blocking.

@joshuarrrr
Copy link
Member

all checks pass, but needs changelog conflict resolution.

@joshuarrrr
Copy link
Member

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

@joshuarrrr
Copy link
Member

@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)

.visTable {
display: flex;
flex-direction: column;
flex: 1 0 0;
overflow: auto;

@include euiScrollBar;
Copy link
Member

Choose a reason for hiding this comment

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

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.

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 am using a consistent @include euiScrollBar; as shown in other scss files in our repo. Why we want to avoid it?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines +39 to +42
const sortColumnId =
sort.colIndex !== null && sort.colIndex !== undefined
? formattedColumns[sort.colIndex]?.id
: undefined;
Copy link
Member

Choose a reason for hiding this comment

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

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" 

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Comment on lines +60 to +63
sort.colIndex !== null &&
sort.colIndex !== undefined &&
dataGridColumns[sort.colIndex].id &&
sort.direction
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
sort.colIndex !== null &&
sort.colIndex !== undefined &&
dataGridColumns[sort.colIndex].id &&
sort.direction
dataGridColumns[sort.colIndex]?.id !== undefined &&
sort.direction

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines +107 to 109
<EuiTitle size="xs" className="visTable__component__title">
<h3>{title}</h3>
</EuiTitle>
Copy link
Member

Choose a reason for hiding this comment

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

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>

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

useEffect(() => {
const perPage = visConfig.perPage || 10;
const perPage = visParams.perPage || 0;
Copy link
Member

Choose a reason for hiding this comment

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

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
joshuarrrr previously approved these changes Apr 17, 2023
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

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>
.visTable {
display: flex;
flex-direction: column;
flex: 1 0 0;
overflow: auto;

@include euiScrollBar;
Copy link
Member

Choose a reason for hiding this comment

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

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

@joshuarrrr joshuarrrr merged commit fc195ea into opensearch-project:main Apr 17, 2023
@opensearch-trigger-bot
Copy link
Contributor

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.

ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 17, 2023
…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>
ashwin-pc pushed a commit that referenced this pull request Apr 18, 2023
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Table Visualization] move format table logic to table render to completely isolate data handling logic
6 participants