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

Feature: Dev Tools Query Export/Import #3810

Merged

Conversation

kishor82
Copy link
Contributor

@kishor82 kishor82 commented Apr 8, 2023

Description

Add support for exporting and restoring the history of commands in Dev Tools #3767

Screenshot

Screen.Recording.2023-09-07.at.6.37.35.PM.mov

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

@joshuarrrr
Copy link
Member

@kishor82 Thanks! I see this involves some new UI changes. To help our UX designers review those flows, can you please add some screenshots or screencasts that demo the new feature?

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2023

Codecov Report

Merging #3810 (680fca8) into main (43fe5d5) will increase coverage by 0.04%.
The diff coverage is 92.70%.

@@            Coverage Diff             @@
##             main    #3810      +/-   ##
==========================================
+ Coverage   66.45%   66.49%   +0.04%     
==========================================
  Files        3399     3402       +3     
  Lines       64915    65011      +96     
  Branches    10387    10401      +14     
==========================================
+ Hits        43141    43232      +91     
- Misses      19210    19211       +1     
- Partials     2564     2568       +4     
Flag Coverage Δ
Linux_1 34.80% <ø> (ø)
Linux_2 55.31% <ø> (ø)
Linux_3 44.61% <92.70%> (+0.14%) ⬆️
Linux_4 34.89% <ø> (ø)
Windows_1 34.81% <ø> (ø)
Windows_2 55.27% <ø> (ø)
Windows_3 44.61% <92.70%> (+0.13%) ⬆️
Windows_4 34.89% <ø> (ø)

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

Files Changed Coverage Δ
...blic/application/contexts/services_context.mock.ts 100.00% <ø> (ø)
...e/public/application/contexts/services_context.tsx 77.77% <ø> (ø)
...lic/application/components/import_mode_control.tsx 92.30% <92.30%> (ø)
...le/public/application/components/import_flyout.tsx 92.50% <92.50%> (ø)
.../public/application/components/overwrite_modal.tsx 100.00% <100.00%> (ø)
...c/application/hooks/use_data_init/use_data_init.ts 8.00% <100.00%> (+3.83%) ⬆️

... and 1 file with indirect coverage changes

@kishor82
Copy link
Contributor Author

@joshuarrrr Please let me know if you have any further feedback or suggestions. Thank you for taking the time to review my work.

dev-tool.mp4

@kishor82 kishor82 force-pushed the feature/console-query-export branch from 460c061 to 5f90ef5 Compare April 11, 2023 05:33
@joshuarrrr
Copy link
Member

@KrooshalUX or @kgcreative can you review the new UI in the screencast above?

@joshuarrrr joshuarrrr added the ux / ui Improvements or additions to user experience, flows, components, UI elements label Apr 13, 2023
@kishor82 kishor82 force-pushed the feature/console-query-export branch from 5f90ef5 to f132c60 Compare April 13, 2023 05:30
@kishor82 kishor82 force-pushed the feature/console-query-export branch from f132c60 to 8e08282 Compare April 14, 2023 16:55
@kishor82 kishor82 force-pushed the feature/console-query-export branch 2 times, most recently from e4a357f to 1a6b993 Compare April 17, 2023 21:18
@kgcreative
Copy link
Member

LGTM - @KrooshalUX -- any recommendations or concerns?

@kishor82 kishor82 force-pushed the feature/console-query-export branch 4 times, most recently from 938c365 to 89aa76a Compare April 20, 2023 17:59
@kishor82
Copy link
Contributor Author

kishor82 commented Sep 7, 2023

@ashwin-pc, the link checker step is failing for my PR, and I'm not sure what's causing it to fail. Can you help me troubleshoot this issue?

@kishor82
Copy link
Contributor Author

kishor82 commented Sep 7, 2023

@ashwin-pc, the link checker step is failing for my PR, and I'm not sure what's causing it to fail. Can you help me troubleshoot this issue?

Ohh, I think it's failing because https://www.circl.lu/doc/misp/ is not reachable right now.

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.

Really nice job with the PR. The changes look good to me and nice coverage with tests. The only real concern i had is with the name of the export but that can be addressed in a followup PR too.

Note: I'd suggest adding a screen recording or screenshots in future PR's to speed up the review proces since without them I ususally have to pull down the change and validate the change myself. Tagging @kgcreative to take a look at the UX changes from this PR, just to make sure there arent any issues with the new UX.

const results = await objectStorageClient.text.findAll();
const senseData = results.sort((a, b) => a.createdAt - b.createdAt)[0];
const blob = new Blob([JSON.stringify(senseData || {})], { type: 'application/json' });
saveAs(blob, 'sense.json');
Copy link
Member

Choose a reason for hiding this comment

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

Why is this named sense.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the time, no other name really came to mind, and I had a good experience with the 'Sense' Chrome extension for Elasticsearch queries, so I went with 'sense.json'! 😄


it('should call onConfirm when clicking the "Overwrite" button', () => {
act(() => {
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why the ts-ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw a similar test in an existing plugin that used @ts-ignore. I tried various approaches but couldn't get it to work, so I added @ts-ignore. I'll keep trying to fix it.

@kgcreative
Copy link
Member

LGTM! I would eventually love to be able to save this as a local view, or as a saved object (similar to a discover saved search) -- but I think for now this works!

@ashwin-pc
Copy link
Member

@kishor82 I wouldn't worry about the link checker since it's unrelated to your change and is something thats probably already fixed in main by now.

Signed-off-by: kishor82 <[email protected]>
@kishor82 kishor82 force-pushed the feature/console-query-export branch from 00698cc to 680fca8 Compare September 8, 2023 18:20
@manasvinibs manasvinibs merged commit 56dd85b into opensearch-project:main Sep 11, 2023
104 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 11, 2023
* feat: Add export functionality for dev tool queries

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

* feat: Add import functionality for dev tool queries

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

* update: changelog.

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

* feat: updated license headers for new file.

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

* fix: typo:ImportFlyout and removed ImportModeControl prop (isLegacyFile)

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

* fix: default query export

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

* fix: added basic text validation for imported file.

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

* update: removed legacy variable export.

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

* test: Add unit tests for OverwriteModal component

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

* test: Add unit tests for ImportModeControl component

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

* test: Add unit tests for ImportFlyout component

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

* test: updated unit tests for ImportFlyout component

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

* test: Enhance async operation handling and complete async cycle in Enzyme test suites.

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

* test: Enhanced test coverage for ImportFlyout component unit tests

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

* fix: Add 'http' property to serviceContextMock and update test expectations

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

---------

Signed-off-by: kishor82 <[email protected]>
(cherry picked from commit 56dd85b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 11, 2023
* feat: Add export functionality for dev tool queries

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

* feat: Add import functionality for dev tool queries

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

* update: changelog.

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

* feat: updated license headers for new file.

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

* fix: typo:ImportFlyout and removed ImportModeControl prop (isLegacyFile)

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

* fix: default query export

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

* fix: added basic text validation for imported file.

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

* update: removed legacy variable export.

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

* test: Add unit tests for OverwriteModal component

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

* test: Add unit tests for ImportModeControl component

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

* test: Add unit tests for ImportFlyout component

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

* test: updated unit tests for ImportFlyout component

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

* test: Enhance async operation handling and complete async cycle in Enzyme test suites.

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

* test: Enhanced test coverage for ImportFlyout component unit tests

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

* fix: Add 'http' property to serviceContextMock and update test expectations

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

---------

Signed-off-by: kishor82 <[email protected]>
(cherry picked from commit 56dd85b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ananzh pushed a commit that referenced this pull request Sep 13, 2023
* feat: Add export functionality for dev tool queries
* feat: Add import functionality for dev tool queries
* update: changelog.
* feat: updated license headers for new file.
* fix: typo:ImportFlyout and removed ImportModeControl prop (isLegacyFile)
* fix: default query export
* fix: added basic text validation for imported file.
* update: removed legacy variable export.
* test: Add unit tests for OverwriteModal component
* test: Add unit tests for ImportModeControl component
* test: Add unit tests for ImportFlyout component
* test: updated unit tests for ImportFlyout component
* test: Enhance async operation handling and complete async cycle in Enzyme test suites.
* test: Enhanced test coverage for ImportFlyout component unit tests
* fix: Add 'http' property to serviceContextMock and update test expectations



---------


(cherry picked from commit 56dd85b)

Signed-off-by: kishor82 <[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>
ananzh pushed a commit that referenced this pull request Sep 13, 2023
* feat: Add export functionality for dev tool queries
* feat: Add import functionality for dev tool queries
* update: changelog.
* feat: updated license headers for new file.
* fix: typo:ImportFlyout and removed ImportModeControl prop (isLegacyFile)
* fix: default query export
* fix: added basic text validation for imported file.
* update: removed legacy variable export.
* test: Add unit tests for OverwriteModal component
* test: Add unit tests for ImportModeControl component
* test: Add unit tests for ImportFlyout component
* test: updated unit tests for ImportFlyout component
* test: Enhance async operation handling and complete async cycle in Enzyme test suites.
* test: Enhanced test coverage for ImportFlyout component unit tests
* fix: Add 'http' property to serviceContextMock and update test expectations


---------


(cherry picked from commit 56dd85b)

Signed-off-by: kishor82 <[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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x first-time-contributor ux / ui Improvements or additions to user experience, flows, components, UI elements v2.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants