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.

Sign up for GitHub

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

Jump to bottom

[Visualize] Fix inspector download filename issue when saving in-place #72605

Merged
merged 5 commits into from
Jul 29, 2020
Merged

[Visualize] Fix inspector download filename issue when saving in-place #72605

merged 5 commits into from
Jul 29, 2020

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jul 21, 2020 •
edited
Loading

Summary

Fixes: #72406

This PR provides a fix for the specific scenario where a visualization is saved in place and the inspector is opened in the same session to download the data.

download_name_inspector

Note: in case the inspector is open and the visualization is saved, the inspector panel has to be closed and re-opened to pick-up the change. I could not find a "nice" way to update the panel with the title information

@dej611 dej611 added Feature:Inspector Inspector infrastructure and implementations Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.9.0 labels Jul 21, 2020
@dej611 dej611 self-assigned this Jul 21, 2020
@dej611 dej611 linked an issue Jul 21, 2020 that may be closed by this pull request
[Visualizations] The downloaded csv from inspector has not the saved vis title #72406
Closed
@apmmachine
Copy link
Contributor

apmmachine commented Jul 21, 2020 •
edited
Loading

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #72605 updated]

  • Start Time: 2020-07-21T14:43:17.293+0000

  • Duration: 5 min 50 sec

@dej611
Copy link
Contributor Author

dej611 commented Jul 21, 2020

@elasticmachine merge upstream

@dej611
Copy link
Contributor Author

dej611 commented Jul 21, 2020

@elasticmachine merge upstream

@dej611 dej611 marked this pull request as ready for review July 22, 2020 13:38
@dej611 dej611 requested a review from a team as a code owner July 22, 2020 13:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@dej611 dej611 requested a review from stratoula July 23, 2020 08:26
stratoula
stratoula approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Tested it in safari. Bug is fixed and haven't identified any regression. Code is LGTM

@stratoula
Copy link
Contributor

stratoula commented Jul 23, 2020 •
edited
Loading

@dej611 can you please add the issue that this PR closes on the description?

@dej611
Copy link
Contributor Author

dej611 commented Jul 28, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
visualizations 408.2KB +108.0B 408.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

lizozom
lizozom reviewed Jul 29, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

While testing, I noticed that if the inspector is opened before saving the visualization, the file is still downloaded with the wrong name. You have to close and re-open the inspector to get the correct filename when downloading.

You can see that the visualization is already saved, but the name doesn't appear in the inspector:
image

When I re-open the inspector, the name appears (and the file is downloaded correctly):
image

Did this work differently prior to 7.9?

@dej611
Copy link
Contributor Author

dej611 commented Jul 29, 2020

While testing, I noticed that if the inspector is opened before saving the visualization, the file is still downloaded with the wrong name. You have to close and re-open the inspector to get the correct filename when downloading.

You can see that the visualization is already saved, but the name doesn't appear in the inspector:
...

Yes, this was in the note in the first message. :)
The problem is that the inspect panel cannot receive updates while open: close and re-open will refresh the content.

When I re-open the inspector, the name appears (and the file is downloaded correctly):
...

Did this work differently prior to 7.9?

I've tried a 7.8 version of Kibana and the panel was on top of all modals preventing the user to face the issue. It was stil possible to save via tabbing, but once saved the whole page reloads.

download_name_inspector-78

In 7.9 the save modal manage to go on top of the inspect panel making the user able to save while inspecting, without reloading the whole page.
For this particular scenario I'd consider it more a new bug rather than a regression, but open to fix it if important.

lizozom
lizozom approved these changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Sorry, haven't seen your footnote, as I was distracted by the awesome gif :-D

LGTM.

@dej611 dej611 merged commit 2d1939b into elastic:master Jul 29, 2020
@dej611 dej611 deleted the fix/visualize-inspector-download-name branch July 29, 2020 10:53
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 29, 2020
@gmmorris
Merge branch 'master' into task-manager/fix-flaky-test
8764243 
* master: (126 commits)
  [ML] Disabling ML if license feature is disabled (elastic#73187)
  [ML] Fixing old _xpack style es endpoint paths (elastic#73667)
  [DOCS] [Lens] 7.9 docs refresh (elastic#72301)
  [ML] DF Analytics results: ensure `View` link is only enabled when job has successfully completed (elastic#73539)
  Set timeRange to default to trigger the error message (elastic#73629)
  [ML] Functional tests - stabilize DFA navigation and index pattern handling (elastic#73660)
  [ILM] Add links to "Snapshot and Restore" from ILM "wait for snapshot policy" (elastic#72473)
  [kbn-storybook] Update Storybook to 5.3.19 (elastic#73320)
  [Metrics UI] Fix hasData call to ensure it has data not just indices (elastic#72969)
  [Uptime] Use `service.name` to link from Uptime -> APM where available (elastic#73618)
  allow others to update `URL.revokeObjectURL` property if needed (elastic#73639)
  regen docs (elastic#73650)
  [Visualize] Fix inspector download filename issue when saving in-place (elastic#72605)
  [Data] Query Input String manager (elastic#72093)
  [Security Solutions] Add tooltips (elastic#73436)
  Do not render descriptionless actions within an EuiCard (elastic#73611)
  [Security Solution][Detections] Value Lists Modal supports multiple exports (elastic#73532)
  [Security Solution][Resolver] Handle disabled process collection (elastic#73592)
  [Security_Solution][Bug] Fix user name/domain to ECS structure (elastic#73530)
  [Security Solution][Exceptions] - Update rule.exceptions_list to include exception list list_id (elastic#73349)
  ...
@dej611 dej611 mentioned this pull request Jul 31, 2020
Merged
dej611 added a commit to dej611/kibana that referenced this pull request Jul 31, 2020
@dej611 @elasticmachine
[Visualize] Fix inspector download filename issue when saving in-place (
81457ea 
elastic#72605)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

10 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@dej611 dej611 added v7.9.1 and removed v7.9.0 labels Aug 17, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

10 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

dej611 added a commit that referenced this pull request Sep 2, 2020
@dej611 @elasticmachine
[7.9] [Visualize] Fix inspector download filename issue when saving i…
6dd1d45 
…n-place (#72605) (#73923)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 2, 2020
@LeeDr LeeDr added v7.9.2 and removed v7.9.1 labels Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@lizozom lizozom lizozom approved these changes

Assignees

@dej611 dej611

Labels
Feature:Inspector Inspector infrastructure and implementations release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.2 v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Visualizations] The downloaded csv from inspector has not the saved vis title
7 participants
@dej611 @apmmachine @elasticmachine @stratoula @kibanamachine @lizozom @LeeDr