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

Adjusted source of QueryStringManager functions for flyout. #8864

Merged

Conversation

AWSHurneyt
Copy link
Contributor

@AWSHurneyt AWSHurneyt commented Nov 14, 2024

Description

  1. Addressed comment Fix template queries loading and update getSampleQuery interface #8848 (comment)
  2. Added try/catch block.

Issues Resolved

Screenshot

Testing the changes

Changelog

  • skip

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
Contributor

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 8864.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 47.05882% with 9 lines in your changes missing coverage. Please review.

Project coverage is 60.94%. Comparing base (c20c041) to head (c6e6441).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...ui/saved_query_flyouts/open_saved_query_flyout.tsx 47.05% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8864      +/-   ##
==========================================
+ Coverage   60.92%   60.94%   +0.01%     
==========================================
  Files        3800     3800              
  Lines       90841    90881      +40     
  Branches    14313    14323      +10     
==========================================
+ Hits        55343    55384      +41     
+ Misses      31978    31968      -10     
- Partials     3520     3529       +9     
Flag Coverage Δ
Linux_1 29.01% <ø> (-0.02%) ⬇️
Linux_2 56.39% <ø> (ø)
Linux_3 37.93% <47.05%> (+0.06%) ⬆️
Linux_4 29.00% <ø> (-0.90%) ⬇️
Windows_1 29.02% <ø> (-0.04%) ⬇️
Windows_2 56.34% <ø> (ø)
Windows_3 37.92% <47.05%> (+0.06%) ⬆️
Windows_4 29.00% <ø> (-0.90%) ⬇️

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.


🚨 Try these New Features:

Signed-off-by: AWSHurneyt <[email protected]>
Signed-off-by: AWSHurneyt <[email protected]>
Signed-off-by: AWSHurneyt <[email protected]>
Signed-off-by: AWSHurneyt <[email protected]>
@kavilla
Copy link
Member

kavilla commented Nov 14, 2024

thank you so much! one more thing, we use a changelog bot to generate changelog fragment and then release notes. do you mind updating the PR description. in this case i think it can be skip as this is fixing something that hasn't been released yet. but i'll leave that up to you. you might need to install the bot as well

#8864 (comment)

i think regardless it might fail the ci check because it's detecting commits t hat had a changelog fragment

Copy link
Contributor

❌ Changelog Entry Missing Hyphen

Changelog entries must begin with a hyphen (-).

@github-actions github-actions bot added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Nov 14, 2024
setSavedQueries(mutableSavedQueries);
const allQueries = await savedQueryService.getAllSavedQueries();
const mutableSavedQueries = allQueries.filter((q) => !q.attributes.isTemplate);
if (currentTabIdRef.current === 'mutable-saved-queries') {
Copy link
Member

Choose a reason for hiding this comment

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

is this required? line 89 should be covering it right?

Copy link
Contributor Author

@AWSHurneyt AWSHurneyt Nov 14, 2024

Choose a reason for hiding this comment

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

@jowg-amazon could you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

We were noticing that the getAllSavedQueries call can take a little while to return. If a user switches tabs before this call is finished, we were seeing that setSavedQueries(mutableSavedQueries) was overwriting the saved queries incorrectly. This check makes sure that we're still on the mutable saved queries tab.

Copy link
Member

Choose a reason for hiding this comment

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

instead of just have a state variable that sets a boolean if it hasTemplateQueries, why don't we just have a state variable for templateQueries and a state variable mutableSavedQueries.

Or even more so just set saved queries to all queries and then in the component if the selected tab is template-saved-queries you filter out all the saved queries not matching template-saved-queries etc.

Signed-off-by: AWSHurneyt <[email protected]>
if (currentTabIdRef.current === 'mutable-saved-queries') {
setSavedQueries(mutableSavedQueries);
const allQueries = await savedQueryService.getAllSavedQueries();
Copy link
Member

Choose a reason for hiding this comment

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

we should refactor this component. we shouldn't need to fire a request to get all saved queries if the user opened saved queries and then switched to template queries and then switch back to saved queries and fire a request. we already have their saved queries

Copy link
Member

Choose a reason for hiding this comment

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

Yes i think we already noticed the queries taking way longer to show up than they should

if (currentTabIdRef.current === 'mutable-saved-queries') {
setSavedQueries(mutableSavedQueries);
const allQueries = await savedQueryService.getAllSavedQueries();
Copy link
Member

Choose a reason for hiding this comment

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

Yes i think we already noticed the queries taking way longer to show up than they should

@AWSHurneyt
Copy link
Contributor Author

@kavilla @ashwin-pc could one of you merge the PR? I don't seem to have the permissions to do so.

@kavilla kavilla merged commit 5740259 into opensearch-project:main Nov 18, 2024
68 of 69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 18, 2024
* Change origin of query string management functions.

Signed-off-by: AWSHurneyt <[email protected]>

* Added try/catch block.

Signed-off-by: AWSHurneyt <[email protected]>

* Changed source of query string management functions.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed typo.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fix lint errors.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed lint error.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test mocks.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>
(cherry picked from commit 5740259)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
abbyhu2000 pushed a commit that referenced this pull request Nov 19, 2024
…8887)

* Change origin of query string management functions.



* Added try/catch block.



* Changed source of query string management functions.



* Fixed typo.



* Fixed import.



* Fix lint errors.



* Fixed lint error.



* Fixed test mocks.



---------


(cherry picked from commit 5740259)

Signed-off-by: AWSHurneyt <[email protected]>
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>
raintygao pushed a commit to raintygao/OpenSearch-Dashboards that referenced this pull request Nov 20, 2024
…ch-project#8864)

* Change origin of query string management functions.

Signed-off-by: AWSHurneyt <[email protected]>

* Added try/catch block.

Signed-off-by: AWSHurneyt <[email protected]>

* Changed source of query string management functions.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed typo.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fix lint errors.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed lint error.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test mocks.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>
silva-qa pushed a commit to silva-qa/OpenSearch-Dashboards that referenced this pull request Dec 12, 2024
…ch-project#8864)

* Change origin of query string management functions.

Signed-off-by: AWSHurneyt <[email protected]>

* Added try/catch block.

Signed-off-by: AWSHurneyt <[email protected]>

* Changed source of query string management functions.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed typo.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fix lint errors.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed lint error.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test mocks.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>
Signed-off-by: Federico Silva <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x discover for discover reinvent discover-next first-time-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants