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

[OSCI][Fix][Discover]Prevent Adding Timefield to the Side Nav Selected Fields on Column Removal #5537

Merged
merged 11 commits into from
Dec 14, 2023

Conversation

MadaniKK
Copy link
Contributor

@MadaniKK MadaniKK commented Nov 27, 2023

Description

Issue #5527: Now the time field of an index pattern would not get wrongly added to the selected field when we click remove column on canvas.

Issue #5538: When doc_table:hideTimeColumn is enabled, removing the last columns from the canvas would automatically add "_source" to the selected fields in the left nav bar.

Issues Resolved

issue #5527 #5538

Screenshot

For #5527
image
image

For #5538

image image

Testing the changes

Check List

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

…Field if it was not previously chosen.

Signed-off-by: qiwen li <qiwen_li@brown.edu>
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a5c45a3) 66.98% compared to head (c1a322e) 67.02%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5537      +/-   ##
==========================================
+ Coverage   66.98%   67.02%   +0.03%     
==========================================
  Files        3294     3294              
  Lines       63296    63296              
  Branches    10066    10066              
==========================================
+ Hits        42398    42422      +24     
+ Misses      18500    18431      -69     
- Partials     2398     2443      +45     
Flag Coverage Δ
Linux_1 35.24% <ø> (ø)
Linux_2 55.17% <ø> (?)
Linux_3 43.87% <ø> (?)
Linux_4 35.34% <ø> (ø)
Windows_1 35.27% <ø> (ø)
Windows_2 55.14% <ø> (ø)
Windows_3 43.89% <ø> (ø)
Windows_4 35.34% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshuarrrr
Copy link
Member

@MadaniKK Can you add a changelog entry?

@MadaniKK
Copy link
Contributor Author

@MadaniKK Can you add a changelog entry?

Hi, Josh, I found another bug (issue #5538) when I wrote this PR, and I found out I could fix it within the same PR. should I make a new PR or fix these two issues in the same PR? ( I just needed to reorder two lines of codes that I wrote in this PR and it will fix issue #5538 too)

@ananzh
Copy link
Member

ananzh commented Dec 1, 2023

@MadaniKK love these fixes and thanks a lot. I will re-run the failed cypress tests and update you.

@ananzh
Copy link
Member

ananzh commented Dec 2, 2023

@MadaniKK, for #5538, your understanding seems not quite right.

This advanced setting doc_table:hideTimeColumn is to allow Discover and Saved Search in Dashboard to not display timestamp column (first column) when enabled for a time based index pattern . Without this setting, for time based index pattern, display timestamp column is default and user can't remove it.

For the legacy Discover, we could have a better understanding

  • after enabling doc_table:hideTimeColumn
Screenshot 2023-12-01 at 4 53 02 PM

The buildColumns fun itself will make sure when no columns return ['_source']:

export function buildColumns(columns: string[]) {
  if (columns.length > 1 && columns.indexOf('_source') !== -1) {
    return columns.filter((col) => col !== '_source');
  } else if (columns.length !== 0) {
    return columns;
  }
  return ['_source'];
}

Therefore, the problem is why enabling doc_table:hideTimeColumn will cause #5538 and how could fix it. I think we haven't implement the logic for this advanced settings.

@ananzh
Copy link
Member

ananzh commented Dec 2, 2023

@MadaniKK I would suggest targeting #5527 in this PR and re-check if all logics are needed if just fixing this specific issue.

Then start another PR to make sure we have time field removed when enabling doc_table:hideTimeColumn. What do you think?

Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

Would like to research on doc_table:hideTimeColumn and find out the root cause of the break. Thanks a lot for carefully checking these settings.

ananzh
ananzh previously approved these changes Dec 14, 2023
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

I think this is an acceptable change to fix all the bugs together. @MadaniKK could you select and add more explains here from public slack? Also there is a conflict on CHANGELOG. Thanks.

@ananzh ananzh added discover for discover reinvent bug Something isn't working labels Dec 14, 2023
Signed-off-by: qiwen li <qiwen_li@brown.edu>
Signed-off-by: qiwen li <qiwen_li@brown.edu>
This reverts commit ef5f062.

Signed-off-by: qiwen li <qiwen_li@brown.edu>

revert
Signed-off-by: Qiwen Li <qiwen_li@brown.edu>
@ananzh ananzh merged commit 0a33d4a into opensearch-project:main Dec 14, 2023
67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 14, 2023
…d Fields on Column Removal (#5537)

* added in a filter for columns before rendering the panel, remove timeField if it was not previously chosen.
* fix issue 5538 too
---------

Signed-off-by: qiwen li <qiwen_li@brown.edu>
Signed-off-by: Qiwen Li <qiwen_li@brown.edu>
(cherry picked from commit 0a33d4a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
manasvinibs pushed a commit that referenced this pull request Dec 15, 2023
…d Fields on Column Removal (#5537) (#5617)

* added in a filter for columns before rendering the panel, remove timeField if it was not previously chosen.
* fix issue 5538 too
---------

Signed-off-by: qiwen li <qiwen_li@brown.edu>
Signed-off-by: Qiwen Li <qiwen_li@brown.edu>
(cherry picked from commit 0a33d4a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 26, 2024
…d Fields on Column Removal (#5537)

* added in a filter for columns before rendering the panel, remove timeField if it was not previously chosen.
* fix issue 5538 too
---------

Signed-off-by: qiwen li <qiwen_li@brown.edu>
Signed-off-by: Qiwen Li <qiwen_li@brown.edu>
(cherry picked from commit 0a33d4a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
manasvinibs pushed a commit that referenced this pull request Jan 31, 2024
…d Fields on Column Removal (#5537)

* added in a filter for columns before rendering the panel, remove timeField if it was not previously chosen.
* fix issue 5538 too
---------

Signed-off-by: qiwen li <qiwen_li@brown.edu>
Signed-off-by: Qiwen Li <qiwen_li@brown.edu>
(cherry picked from commit 0a33d4a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
ananzh pushed a commit that referenced this pull request Jan 31, 2024
…d Fields on Column Removal (#5537) (#5745)

* added in a filter for columns before rendering the panel, remove timeField if it was not previously chosen.
* fix issue 5538 too
---------

Signed-off-by: qiwen li <qiwen_li@brown.edu>
Signed-off-by: Qiwen Li <qiwen_li@brown.edu>
(cherry picked from commit 0a33d4a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working discover for discover reinvent first-time-contributor v2.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants