chore(connections): align multiple connections saving behavior with single connections mode COMPASS-8133 (#6097)

* chore(connections): align multiple connections saving behavior with single connection

* chore(connections): run storage tests for both feature flag values

Author: gribnoysup

packages/compass-connections/src/stores/connections-store.spec.tsx

@@ -304,95 +304,55 @@ describe('useConnections', function () {
       });
     });
 
-    describe('saving connections during connect in single connection mode', function () {
-      it('should NOT update existing connection with new props when existing connection is successfull', async function () {
-        await preferences.savePreferences({
-          // We're testing multiple connections by default
-          enableNewMultipleConnectionSystem: false,
+    for (const multipleConnectionsEnabled of [true, false]) {
+      describe(`when multiple connections ${
+        multipleConnectionsEnabled ? 'enabled' : 'disabled'
+      }`, function () {
+        it('should NOT update existing connection with new props when existing connection is successfull', async function () {
+          await preferences.savePreferences({
+            enableNewMultipleConnectionSystem: multipleConnectionsEnabled,
+          });
+
+          const connections = renderHookWithContext();
+          const saveSpy = sinon.spy(mockConnectionStorage, 'save');
+
+          await connections.current.connect({
+            ...mockConnections[0],
+            favorite: { name: 'foobar' },
+          });
+
+          // Only once on success so that we're not updating existing connections if
+          // they failed
+          expect(saveSpy).to.have.been.calledOnce;
+          expect(saveSpy.getCall(0)).to.have.nested.property(
+            'args[0].connectionInfo.favorite.name',
+            'turtles'
+          );
         });
 
-        const connections = renderHookWithContext();
-        const saveSpy = sinon.spy(mockConnectionStorage, 'save');
+        it('should not update existing connection if connection failed', async function () {
+          await preferences.savePreferences({
+            enableNewMultipleConnectionSystem: multipleConnectionsEnabled,
+          });
 
-        await connections.current.connect({
-          ...mockConnections[0],
-          favorite: { name: 'foobar' },
-        });
-
-        // Only once on success so that we're not updating existing connections if
-        // they failed
-        expect(saveSpy).to.have.been.calledOnce;
-        expect(saveSpy.getCall(0)).to.have.nested.property(
-          'args[0].connectionInfo.favorite.name',
-          'turtles'
-        );
-      });
-
-      it('should not update existing connection if connection failed', async function () {
-        await preferences.savePreferences({
-          enableNewMultipleConnectionSystem: false,
-        });
-
-        const saveSpy = sinon.spy(mockConnectionStorage, 'save');
-        const onConnectionFailed = sinon.spy();
-        const connections = renderHookWithContext({ onConnectionFailed });
-
-        sinon
-          .stub(connectionsManager, 'connect')
-          .rejects(new Error('Failed to connect'));
-
-        await connections.current.connect({
-          ...mockConnections[0],
-          favorite: { name: 'foobar' },
-        });
-
-        expect(onConnectionFailed).to.have.been.calledOnce;
-        expect(saveSpy).to.not.have.been.called;
-      });
-    });
+          const saveSpy = sinon.spy(mockConnectionStorage, 'save');
+          const onConnectionFailed = sinon.spy();
+          const connections = renderHookWithContext({ onConnectionFailed });
 
-    describe('saving connections during connect in multiple connections mode', function () {
-      it('should update existing connection with new props when connection is successfull', async function () {
-        const connections = renderHookWithContext();
-        const saveSpy = sinon.spy(mockConnectionStorage, 'save');
+          sinon
+            .stub(connectionsManager, 'connect')
+            .rejects(new Error('Failed to connect'));
 
-        await connections.current.connect({
-          ...mockConnections[0],
-          favorite: { name: 'foobar' },
-        });
+          await connections.current.connect({
+            ...mockConnections[0],
+            favorite: { name: 'foobar' },
+          });
 
-        // Saved before and after in multiple connections mode
-        expect(saveSpy).to.have.been.calledTwice;
-        expect(saveSpy.getCall(0)).to.have.nested.property(
-          'args[0].connectionInfo.favorite.name',
-          'foobar'
-        );
-        expect(saveSpy.getCall(1)).to.have.nested.property(
-          'args[0].connectionInfo.favorite.name',
-          'foobar'
-        );
-      });
-
-      it('should always update existing connection even if conneciton will fail', async function () {
-        const saveSpy = sinon.spy(mockConnectionStorage, 'save');
-        const onConnectionFailed = sinon.spy();
-        const connections = renderHookWithContext({ onConnectionFailed });
-
-        sinon
-          .stub(connectionsManager, 'connect')
-          .rejects(new Error('Failed to connect'));
-
-        await connections.current.connect({
-          ...mockConnections[0],
-          connectionOptions: {
-            connectionString: 'mongodb://super-broken-new-url',
-          },
+          expect(onConnectionFailed).to.have.been.calledOnce;
+          expect(saveSpy).to.not.have.been.called;
         });
-
-        expect(onConnectionFailed).to.have.been.calledOnce;
-        expect(saveSpy).to.have.been.calledOnce;
       });
-    });
+    }
   });
 
   describe('#disconnect', function () {

packages/compass-connections/src/stores/connections-store.tsx

@@ -2,7 +2,7 @@ import { useCallback, useEffect, useReducer, useRef } from 'react';
 import type { DataService, connect } from 'mongodb-data-service';
 import { useConnectionsManagerContext } from '../provider';
 import { type ConnectionInfo } from '@mongodb-js/connection-storage/provider';
-import { cloneDeep, merge } from 'lodash';
+import { cloneDeep } from 'lodash';
 import { UUID } from 'bson';
 import { useToast } from '@mongodb-js/compass-components';
 import { createLogger } from '@mongodb-js/compass-logging';
@@ -687,11 +687,6 @@ export function useConnections({
         // means to returning this info as part of the connections list for now
         if (!isAutoconnectAttempt) {
           if (
-            // In single connection mode we only update existing connection when
-            // we successfully connected. In multiple connections we don't care
-            // if existing connection fails with errors and update it either way
-            // before we finished connecting
-            preferences.getPreferences().enableNewMultipleConnectionSystem ||
             // TODO(COMPASS-7397): The way the whole connection logic is set up
             // right now it is required that we save connection before starting
             // the connection process even if we don't need or want to so that
@@ -745,28 +740,16 @@ export function useConnections({
         });
 
         try {
-          const mergeConnectionInfo = preferences.getPreferences()
-            .persistOIDCTokens
-            ? { connectionOptions: await dataService.getUpdatedSecrets() }
-            : {};
-
           // Auto-connect info should never be saved
           if (!isAutoconnectAttempt) {
+            // After connection is established we only update lastUsed time and
+            // maybe an OIDC token if preferences allow
             await saveConnectionInfo({
-              ...merge(
-                // In single connection mode we only update the last used
-                // timestamp and maybe an OIDC token, everything else is kept
-                // as-is so that "Connect" and "Save" are distinct features (as
-                // the button labels in the connection form suggest). In
-                // multiple connections we update everything
-                preferences.getPreferences().enableNewMultipleConnectionSystem
-                  ? connectionInfo
-                  : // Existing connection info might be missing when connecting
-                    // to a new connection for the first time
-                    existingConnectionInfo ?? connectionInfo,
-                mergeConnectionInfo
-              ),
+              id: connectionInfo.id,
               lastUsed: new Date(),
+              ...(preferences.getPreferences().persistOIDCTokens
+                ? { connectionOptions: await dataService.getUpdatedSecrets() }
+                : {}),
             });
           }
         } catch (err) {

packages/connection-form/src/components/connection-form-actions.spec.tsx

@@ -38,7 +38,7 @@ describe('<ConnectionFormModalActions />', function () {
           onSaveAndConnect={onSaveAndConnectSpy}
         ></ConnectionFormModalActions>
       );
-      const saveButton = screen.getByText('Save & Connect');
+      const saveButton = screen.getByText('Connect');
       fireEvent(
         saveButton,
         new MouseEvent('click', {

packages/connection-form/src/components/connection-form-actions.tsx

@@ -184,7 +184,7 @@ export function ConnectionFormModalActions({
           variant={ButtonVariant.Primary}
           onClick={onSaveAndConnect}
         >
-          Save & Connect
+          Connect
         </Button>
       </div>
     </div>