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

chore(my-queries): support for multiple connections for my-queries COMPASS-7904, COMPASS-7915 #5841

Merged
merged 28 commits into from
Jun 17, 2024

Conversation

himanshusinghs
Copy link
Contributor

@himanshusinghs himanshusinghs commented May 24, 2024

Description

This PR introduces following changes:

  • "My Queries" plugin work now with ConnectionsManager and InstancesManager instead of DataService and MongoDBInstance
  • We don't rely anymore on singleConnectionConnectionInfo that we introduced at some point to render "My Queries" plugin. Instead, we have taken a more optimistic route which is to use useActiveConnections hook to get a hold of current active connections and work with the very first connection that we find, to render "CompassSidebar", "CompassShell" and interaction in "My Queries" plugin for single connection mode

For the saved aggregations and queries these are the changes to be expected:

State What to expect Modal
Connected to just one connection, trying to open the query and the namespace exists Opens the query in the target namespace
Connected to just one connection, trying to open the query and the namespace does not exist Opens the modal asking for another namespace, connection is already selected and the dropdown is not shown image
Connected to multiple connections, trying to open the query and the namespace exists in only one of the connections Opens the query in the only connection with the namespace
Connected to multiple connections, trying to open the query and the namespace exists multiple connections Opens the modal to choose a connection from one of the connections that have the namespace image
Connected to multiple connections, trying to open the query and the namespace does not exist in any of the active connections Opens the modal asking for a connection and another namespace image

There is additionally a new context menu item called "Open in" added when in multiple connections mode to explicitly open a query in a different connection / namespace.

Entry What you see when connected to multiple connections What you see when connected to just one connection
image image image

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@himanshusinghs himanshusinghs added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label May 24, 2024
@himanshusinghs himanshusinghs marked this pull request as ready for review May 24, 2024 13:26
@@ -0,0 +1,52 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This modal is for a case that was not discussed when planning for this and hence I would say not final but I am hoping to keep this until Ben is back and we have a chance to discuss this.

Copy link
Contributor Author

@himanshusinghs himanshusinghs May 24, 2024

Choose a reason for hiding this comment

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

This file appears deleted but I actually renamed it to select-connection-and-namespace-modal.tsx. Most of the original content of the file is preserved and I have mostly modified the component props and its mapState

@lerouxb
Copy link
Contributor

lerouxb commented May 24, 2024

One question would be why you're using radio buttons rather than the selectbox in the "Select a Connection" modal.

@himanshusinghs
Copy link
Contributor Author

One question would be why you're using radio buttons rather than the selectbox in the "Select a Connection" modal.

No specific reason other than that it was in the design. But IIRC Ben was also re-contemplating the idea of using select box for this. I can check in with him once he's back and if it needs changing then I can quickly iterate over that?

@paula-stacho
Copy link
Contributor

I've seen this isAction approach in other places, can I ask what is the benefit of this against the switch?

@himanshusinghs
Copy link
Contributor Author

I noticed some inconsistencies in the reducer like accessing non-existent parameters on passed actions. This isAction helps to get around that as it validates wether a received action is of the expected type.

@lerouxb
Copy link
Contributor

lerouxb commented Jun 10, 2024

I'm thinking the rules are too complicated and would propose:

  • If you're not connected to anything, tell the user to connect to something first.
  • If you're just connected to one connection, proceed as we do in the single connection world.
  • If you're connected to multiple things, make the user select a connection and confirm the namespace within that connection.

If we're going to make an exception where we prefer the "active" connection I think there will be trouble because as we've seen even in the single connection world the sidebar easily scrolls and I don't think there's anything we can do that will make it super obvious enough which connection will be used. At most I'd pre-select the current selection but still show the modal and make the user chooses so that they confirm.

ie. we'd only have the two modals: Select a namespace (like we do today) and Select a Connection and Namespace. I don't know how/where we'd display to the user that you have to connect to something first. But I think we should try and avoid having another way of kicking off a new connection.

@himanshusinghs
Copy link
Contributor Author

So for the points that you mentioned Le Roux, we do follow the first and the second, and only for the third point we try to detect if namespace is available in only one or multiple of the active connections.

But that is also only to keep the user experience consistent for the simplest of the use-case which is - when we try to open the query and the namespace for the query exists only in one connection we simply open the query in that connection.

@paula-stacho
Copy link
Contributor

paula-stacho commented Jun 10, 2024

since we have the OpenedModal: no-active-connections-modal in redux, and for example openItem action handles most of the decision making on which modal to use -> wouldn't it make sense to move the if no connections -> no-active-connections-modal bit to redux as well?

@himanshusinghs
Copy link
Contributor Author

himanshusinghs commented Jun 10, 2024

since we have the OpenedModal: no-active-connections-modal in redux, and for example openItem action handles most of the decision making on which modal to use -> wouldn't it make sense to move the if no connections -> no-active-connections-modal bit to redux as well?

Yea I think we can do this - I will push a commit for that.
(pushed 81d2b66)

@himanshusinghs himanshusinghs force-pushed the COMPASS-7904-7915-my-queries-multi-connection branch from e96fca4 to 3acbbec Compare June 10, 2024 20:10
Copy link
Contributor

@paula-stacho paula-stacho left a comment

Choose a reason for hiding this comment

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

Nice! 🎉

@himanshusinghs himanshusinghs force-pushed the COMPASS-7904-7915-my-queries-multi-connection branch from 8f26ca6 to 58fad7d Compare June 14, 2024 15:23
Copy link
Contributor

@lerouxb lerouxb left a comment

Choose a reason for hiding this comment

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

I still have some concerns about the design, but I suppose we can deal with those during the feedback phase.

@himanshusinghs
Copy link
Contributor Author

@lerouxb yea - it would be amazing if you could add those to the findings doc so that we don't loose the thought of it.

@himanshusinghs himanshusinghs merged commit 69f7964 into main Jun 17, 2024
27 checks passed
@himanshusinghs himanshusinghs deleted the COMPASS-7904-7915-my-queries-multi-connection branch June 17, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature flagged PRs labeled with this label will not be included in the release notes of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants