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

[Datasource selector] Sort datasource option list alphabetically #5719

Conversation

mengweieric
Copy link
Collaborator

Description

Sort datasource option list in each group alphabetically.

Issues Resolved

#5609

Screenshot

Testing the changes

Check List

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

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b5d39b6) 67.03% compared to head (8c0ab6a) 67.03%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5719   +/-   ##
=======================================
  Coverage   67.03%   67.03%           
=======================================
  Files        3296     3296           
  Lines       63339    63343    +4     
  Branches    10087    10087           
=======================================
+ Hits        42459    42465    +6     
- Misses      18430    18432    +2     
+ Partials     2450     2446    -4     
Flag Coverage Δ
Linux_1 35.23% <0.00%> (-0.01%) ⬇️
Linux_2 55.18% <ø> (ø)
Linux_3 43.92% <100.00%> (+<0.01%) ⬆️
Linux_4 35.33% <0.00%> (-0.01%) ⬇️
Windows_1 35.26% <0.00%> (-0.01%) ⬇️
Windows_2 55.15% <ø> (ø)
Windows_3 43.93% <100.00%> (+<0.01%) ⬆️
Windows_4 35.33% <0.00%> (-0.01%) ⬇️

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.

Copy link
Member

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

is it possible to have backend to include sort in the query?

return {
...dsGroup,
options: [...dsGroup.options].sort((ds1, ds2) => {
return ds1.label.localeCompare(ds2.label, undefined, { sensitivity: 'base' });
Copy link
Member

Choose a reason for hiding this comment

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

base was the original behavior in 2.9?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It uses lodash's sortBy without any addition settings which I believe is not using 'base' as I tested that for example 'a' vs 'A' will be sorted in different groups. For index patterns it's fine as there will be no Uppercase allowed plus a list of restrictions, but since we also introduced non index pattern datasource with some different, flexible naming restrictions, I figured this way should have the better support.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

This looks good to me. Think we are good to merge this once we have a unit test for this

@mengweieric
Copy link
Collaborator Author

is it possible to have backend to include sort in the query?

It's possible but currently we are pulling datasources through different APIs and then consolidate them from front end. With this implementation we will need to compose different queries to include sorting for different datasources. For now I think to sort them after consolidations will be relatively easier.

@ananzh ananzh merged commit 058dfbc into opensearch-project:main Jan 25, 2024
76 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 25, 2024
* sort datasource option list in each group alphabetically
* add test for sorting
* remove one datasource per test

---------

Signed-off-by: Eric <[email protected]>
(cherry picked from commit 058dfbc)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 25, 2024
* sort datasource option list in each group alphabetically
* add test for sorting
* remove one datasource per test

---------

Signed-off-by: Eric <[email protected]>
(cherry picked from commit 058dfbc)
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
…) (#5735)

* sort datasource option list in each group alphabetically
* add test for sorting
* remove one datasource per test

---------

Signed-off-by: Eric <[email protected]>
(cherry picked from commit 058dfbc)
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>
Co-authored-by: Anan Zhuang <[email protected]>
ananzh pushed a commit that referenced this pull request Jan 31, 2024
…) (#5734)

* sort datasource option list in each group alphabetically
* add test for sorting
* remove one datasource per test

---------

Signed-off-by: Eric <[email protected]>
(cherry picked from commit 058dfbc)
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>
yujin-emma pushed a commit to yujin-emma/OpenSearch-Dashboards that referenced this pull request Feb 5, 2024
…nsearch-project#5719)

* sort datasource option list in each group alphabetically
* add test for sorting
* remove one datasource per test

---------

Signed-off-by: Eric <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants