Address code review feedback

- Return 400 instead of panicking on invalid organization_id
- Use generic error message for internal failures
- Drop duplicate validation from initiate handler (kept in tx)
- Make preserveConnectionFields mutate in place
- Remove as type assertions in GoogleWorkspaceConnector
- Use sort.Slice instead of sort.SliceStable
- Consistent error prefixes in Slack sender

Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>

apps/console/src/pages/iam/organizations/settings/_components/GoogleWorkspaceConnector.tsx

@@ -12,7 +12,7 @@
 // OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-import { formatError, type GraphQLError, sprintf } from "@probo/helpers";
+import { sprintf } from "@probo/helpers";
 import { useTranslate } from "@probo/i18n";
 import {
   Badge,
@@ -128,10 +128,7 @@ export function GoogleWorkspaceConnector(props: {
         if (errors?.length) {
           toast({
             title: __("Error"),
-            description: formatError(
-              __("Failed to disconnect Google Workspace"),
-              errors as GraphQLError[],
-            ),
+            description: errors.map((e) => e.message).join(", "),
             variant: "error",
           });
           return;
@@ -146,10 +143,7 @@ export function GoogleWorkspaceConnector(props: {
       onError(error) {
         toast({
           title: __("Error"),
-          description: formatError(
-            __("Failed to disconnect Google Workspace"),
-            error as GraphQLError,
-          ),
+          description: error.message,
           variant: "error",
         });
       },
@@ -179,10 +173,7 @@ export function GoogleWorkspaceConnector(props: {
         if (errors?.length) {
           toast({
             title: __("Error"),
-            description: formatError(
-              __("Failed to update excluded user names"),
-              errors as GraphQLError[],
-            ),
+            description: errors.map((e) => e.message).join(", "),
             variant: "error",
           });
           return;
@@ -196,10 +187,7 @@ export function GoogleWorkspaceConnector(props: {
       onError(error) {
         toast({
           title: __("Error"),
-          description: formatError(
-            __("Failed to update excluded user names"),
-            error as GraphQLError,
-          ),
+          description: error.message,
           variant: "error",
         });
       },

pkg/coredata/connector.go

@@ -147,7 +147,7 @@ func (c *Connector) LoadOneByOrganizationIDAndProvider(
 	}
 
 	// Widest-scope-wins, tiebreak by most recent updated_at.
-	sort.SliceStable(connectors, func(i, j int) bool {
+	sort.Slice(connectors, func(i, j int) bool {
 		ci, cj := connectorScopeCount(connectors[i]), connectorScopeCount(connectors[j])
 		if ci != cj {
 			return ci > cj

pkg/probo/connector_service.go

@@ -356,7 +356,8 @@ func (s *ConnectorService) Reconnect(
 				return fmt.Errorf("cannot reconnect connector: not an OAuth2 connector")
 			}
 
-			cnnctr.Connection = preserveConnectionFields(req.Connection, cnnctr.Connection)
+			preserveConnectionFields(req.Connection, cnnctr.Connection)
+			cnnctr.Connection = req.Connection
 			cnnctr.UpdatedAt = time.Now()
 
 			return cnnctr.Update(ctx, conn, s.svc.scope, s.svc.encryptionKey)
@@ -369,11 +370,11 @@ func (s *ConnectorService) Reconnect(
 	return cnnctr, nil
 }
 
-// preserveConnectionFields returns newConn with refresh token and Slack
-// webhook settings copied from oldConn when newConn omits them. Google
+// preserveConnectionFields copies refresh token and Slack webhook
+// settings from oldConn into newConn when newConn omits them. Google
 // omits refresh_token on incremental-auth reuse; a Slack access-review
 // reconnect with no incoming-webhook scope omits the webhook settings.
-func preserveConnectionFields(newConn, oldConn connector.Connection) connector.Connection {
+func preserveConnectionFields(newConn, oldConn connector.Connection) {
 	switch n := newConn.(type) {
 	case *connector.OAuth2Connection:
 		if o, ok := oldConn.(*connector.OAuth2Connection); ok {
@@ -393,5 +394,4 @@ func preserveConnectionFields(newConn, oldConn connector.Connection) connector.C
 			}
 		}
 	}
-	return newConn
 }

pkg/server/api/console/v1/connector_initiate.go

@@ -29,9 +29,7 @@ import (
 	"go.probo.inc/probo/pkg/server/api/authn"
 )
 
-var (
-	errInvalidReconnectConnector = errors.New("invalid reconnect connector")
-)
+var errInvalidReconnectConnector = errors.New("invalid reconnect connector")
 
 func handleConnectorInitiate(
 	logger *log.Logger,
@@ -53,7 +51,8 @@ func handleConnectorInitiate(
 
 		organizationID, err := gid.ParseGID(r.URL.Query().Get("organization_id"))
 		if err != nil {
-			panic(fmt.Errorf("cannot parse organization id: %w", err))
+			httpserver.RenderError(w, http.StatusBadRequest, fmt.Errorf("invalid organization_id parameter"))
+			return
 		}
 
 		if authn.APIKeyFromContext(r.Context()) != nil {
@@ -100,7 +99,7 @@ func handleConnectorInitiate(
 				return
 			}
 			logger.ErrorCtx(r.Context(), "cannot look up existing connector", log.Error(err))
-			httpserver.RenderError(w, http.StatusInternalServerError, fmt.Errorf("cannot look up existing connector"))
+			httpserver.RenderError(w, http.StatusInternalServerError, fmt.Errorf("internal error"))
 			return
 		}
 
@@ -146,9 +145,6 @@ func loadExistingConnector(
 		if err != nil {
 			return nil, err
 		}
-		if err := validateReconnectConnector(found, organizationID, provider); err != nil {
-			return nil, err
-		}
 		return found, nil
 	}
 
@@ -163,32 +159,3 @@ func loadExistingConnector(
 	return found, err
 }
 
-func validateReconnectConnector(
-	c *coredata.Connector,
-	organizationID gid.GID,
-	provider string,
-) error {
-	if c == nil {
-		return fmt.Errorf("%w: connector not found", errInvalidReconnectConnector)
-	}
-
-	var connectorProvider coredata.ConnectorProvider
-	if err := connectorProvider.Scan(provider); err != nil {
-		return fmt.Errorf("%w: unsupported provider: %w", errInvalidReconnectConnector, err)
-	}
-
-	if c.OrganizationID != organizationID {
-		return fmt.Errorf("%w: organization mismatch", errInvalidReconnectConnector)
-	}
-	if c.Provider != connectorProvider {
-		return fmt.Errorf("%w: provider mismatch", errInvalidReconnectConnector)
-	}
-	if c.Protocol != coredata.ConnectorProtocolOAuth2 {
-		return fmt.Errorf("%w: not an OAuth2 connector", errInvalidReconnectConnector)
-	}
-	if c.Connection == nil {
-		return fmt.Errorf("%w: connector has nil connection", errInvalidReconnectConnector)
-	}
-
-	return nil
-}

pkg/slack/sender.go

@@ -160,7 +160,7 @@ func (s *Sender) sendMessage(ctx context.Context, tx pg.Querier, message *coreda
 		if errors.Is(err, coredata.ErrResourceNotFound) {
 			return nil, nil, fmt.Errorf("cannot send slack message: no connector configured for organization")
 		}
-		return nil, nil, fmt.Errorf("cannot load slack connector: %w", err)
+		return nil, nil, fmt.Errorf("cannot send slack message: %w", err)
 	}
 
 	if c.Connection == nil {
@@ -284,7 +284,7 @@ func (s *Sender) updateMessage(ctx context.Context, tx pg.Querier, updateMessage
 		if errors.Is(err, coredata.ErrResourceNotFound) {
 			return fmt.Errorf("cannot update slack message: no connector configured for organization")
 		}
-		return fmt.Errorf("cannot load slack connector: %w", err)
+		return fmt.Errorf("cannot update slack message: %w", err)
 	}
 
 	if c.Connection == nil {