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

[Fix] Lose OriginatingApp Connection on Save As #72725

Merged

Conversation

ThomThomson
Copy link
Contributor

Summary

Closes #72429 by restoring the flow from 7.8. When a user has redirected to an editor from a container, then used the 'save as' button and chosen not to return to the originating app, the connection to that app should be severed.

How to test

  • Edit a panel from a dashboard OR create a new panel from dashboard
  • Once you are in the editor, use the modal (save as), to save the visualization / lens visualization
  • Deselect add to dashboards after saving select save as new visualization then save.
  • The originatingApp connection should now be lost, and the top nav menu should only display save

Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] This was checked for keyboard-only and screenreader accessibility
- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
- [ ] This was checked for cross-browser compatibility

For maintainers

@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.10.0 v7.9.0 labels Jul 21, 2020
@ThomThomson ThomThomson requested a review from a team July 21, 2020 19:40
@elasticmachine
Copy link
Contributor

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

@ThomThomson ThomThomson changed the title Originating app persists after save as [Fix] Lose OriginatingApp Connection on Save As Jul 21, 2020
@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Firefox for both Lens and Visualize:

  • Create new from dashboard, save without returning
  • Edit from dashboard, save as new without returning

In both cases, the app behaves as if there is no dashboard connection afterwards.

I feel a bit weird about using a mutable local variable which just happens to get picked up by the next render cycle triggered by the subsequent push, but I can't think of a better way for now. Thanks for the functional tests here! We can address this when improving state management overall.

LGTM

@ThomThomson
Copy link
Contributor Author

As I looked over it again, I also felt a little weird mutating the variable in mounter.tsx, so I added originatingApp to the state of app.tsx, and used that instead. It will still have to be revisited later most likely, but it now feels much better!

@majagrubic
Copy link
Contributor

Tested in Chrome on Mac OS for following scenarios:

  • created a new visualization from dashboard, save and add to dashboard
  • created a new visualization from dashboard, saved it without adding to dashboard

I can confirm the regression is gone. Looking at the code now.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Code LGTM

Copy link
Contributor

@flash1293 flash1293 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 now something is broken. When I edit a Lens vis from the dashboard and use "Save as" without "return to dashboard", it will still show the "Return to dashboard" switch in the modal when I try to save again.

return_compressed

@ThomThomson
Copy link
Contributor Author

Good catch @flash1293, that was caused by a typo which has been fixed in the latest commit.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and works fine, LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
lens 24.9KB +199.0B 24.7KB
visualize 677.3KB +260.0B 677.0KB
total - +459.0B -

History

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

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Jul 23, 2020
Reset originatingApp in lens and visualize when return to dashboard on saving is false
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Jul 23, 2020
Reset originatingApp in lens and visualize when return to dashboard on saving is false
ThomThomson added a commit that referenced this pull request Jul 23, 2020
Reset originatingApp in lens and visualize when return to dashboard on saving is false
ThomThomson added a commit that referenced this pull request Jul 23, 2020
Reset originatingApp in lens and visualize when return to dashboard on saving is false
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 5, 2020
…onnection not to be lost properly after Create new
ThomThomson added a commit that referenced this pull request Aug 6, 2020
Fixed typo created in #72725 which caused the originatingApp connection not to be lost properly after Create new
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 6, 2020
…c#74420)

Fixed typo created in elastic#72725 which caused the originatingApp connection not to be lost properly after Create new
ThomThomson added a commit that referenced this pull request Aug 6, 2020
#74569)

Fixed typo created in #72725 which caused the originatingApp connection not to be lost properly after Create new
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 10, 2020
…c#74420)

Fixed typo created in elastic#72725 which caused the originatingApp connection not to be lost properly after Create new
ThomThomson added a commit that referenced this pull request Aug 10, 2020
#74692)

Fixed typo created in #72725 which caused the originatingApp connection not to be lost properly after Create new
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Weird flow when "Save To dashboards" is not selected
5 participants