From 7c6154749a3a9e05ddf849529084f70d1674ca12 Mon Sep 17 00:00:00 2001 From: arc53-machine <232052973+arc53-machine@users.noreply.github.com> Date: Mon, 28 Sep 2026 20:59:36 +0100 Subject: [PATCH] Address review: model metadata, serialized admin saves, member account choice - models.py: connector_policies.enabled is the nullable column; the earlier edit had loosened user_custom_models.enabled instead. A test pins both against their migrations. - Admin > Connectors saves one change at a time, so an older snapshot can no longer overwrite a newer one. - In member mode, a member with several connected accounts of the same service uses the one they used (or connected) most recently, not the oldest. --- docsgpt/connectors/resolve.py | 36 ++++++++++++++++++------ docsgpt/storage/db/models.py | 6 ++-- frontend/src/admin/Connectors.test.tsx | 37 +++++++++++++++++++++++++ frontend/src/admin/Connectors.tsx | 24 ++++++++++------ tests/connectors/test_runtime.py | 17 ++++++++++++ tests/storage/db/test_migration_0038.py | 11 ++++++++ 6 files changed, 110 insertions(+), 21 deletions(-) diff --git a/docsgpt/connectors/resolve.py b/docsgpt/connectors/resolve.py index 2c90917a..fd9f47f9 100644 --- a/docsgpt/connectors/resolve.py +++ b/docsgpt/connectors/resolve.py @@ -9,6 +9,7 @@ credentials; callers read them from the resolved row through from __future__ import annotations +from datetime import datetime, timezone import logging from dataclasses import dataclass from typing import Optional @@ -107,17 +108,34 @@ def resolve_connection(resource: dict, invoker_user_id: Optional[str]) -> Option def _member_connection(repo: ConnectorSessionsRepository, owned: Optional[dict], invoker: str) -> Optional[dict]: - """The invoker's own connection to the service the owner's connection is for.""" + """The invoker's own connection to the service the owner's connection is for. + + A member with several connected accounts of that service gets the one + they used most recently (then the most recently connected): the account + they are working in, and the one a "Connect to continue" just added. + """ if owned is None: return None - for row in repo.list_for_user(invoker): - if row.get("provider") != owned.get("provider"): - continue - if (row.get("server_url") or "") != (owned.get("server_url") or ""): - continue - if service.normalize_status(row) == service.STATUS_CONNECTED: - return row - return None + candidates = [ + row for row in repo.list_for_user(invoker) + if row.get("provider") == owned.get("provider") + and (row.get("server_url") or "") == (owned.get("server_url") or "") + and service.normalize_status(row) == service.STATUS_CONNECTED + ] + if not candidates: + return None + + def when(value) -> datetime: + if isinstance(value, str): + value = datetime.fromisoformat(value) + if not isinstance(value, datetime): + return datetime.min.replace(tzinfo=timezone.utc) + return value if value.tzinfo else value.replace(tzinfo=timezone.utc) + + def recency(row: dict) -> tuple: + return tuple(when(row.get(field)) for field in ("last_used_at", "updated_at", "created_at")) + + return max(candidates, key=recency) def audit_delegation(resolved: ResolvedConnection, *, invoker: Optional[str], resource_type: str, diff --git a/docsgpt/storage/db/models.py b/docsgpt/storage/db/models.py index ff682541..d366a4f0 100644 --- a/docsgpt/storage/db/models.py +++ b/docsgpt/storage/db/models.py @@ -419,8 +419,7 @@ user_custom_models_table = Table( # docsgpt.security.encryption.encrypt_credentials. Column("api_key_encrypted", Text, nullable=False), Column("capabilities", JSONB, nullable=False, server_default="{}"), - # NULL: on when the connector has its server settings (see connectors.service). - Column("enabled", Boolean), + Column("enabled", Boolean, nullable=False, server_default="true"), Column("created_at", DateTime(timezone=True), nullable=False, server_default=func.now()), Column("updated_at", DateTime(timezone=True), nullable=False, server_default=func.now()), ) @@ -616,7 +615,8 @@ connector_policies_table = Table( "connector_policies", metadata, Column("connector_key", Text, primary_key=True), - Column("enabled", Boolean, nullable=False, server_default="true"), + # NULL: on when the connector has its server settings (see connectors.service). + Column("enabled", Boolean), Column("credential_mode", Text, nullable=False, server_default="choose"), Column("updated_by", Text), Column("updated_at", DateTime(timezone=True), nullable=False, server_default=func.now()), diff --git a/frontend/src/admin/Connectors.test.tsx b/frontend/src/admin/Connectors.test.tsx index 5643546f..7f5d56c3 100644 --- a/frontend/src/admin/Connectors.test.tsx +++ b/frontend/src/admin/Connectors.test.tsx @@ -165,6 +165,43 @@ describe('Admin Connectors', () => { expect(container.querySelectorAll('tbody tr')).toHaveLength(2); }); + it('saves one change at a time so a late response cannot win', async () => { + getAdmin.mockResolvedValue(payload()); + const pending: ((value: unknown) => void)[] = []; + updateAdmin.mockImplementation( + () => new Promise((resolve) => pending.push(resolve)), + ); + await render(); + const toggle = () => + container.querySelector( + '[aria-label="Google Drive enabled"]', + )!; + await act(async () => toggle().click()); + await act(async () => + container.querySelector('#allow-custom-mcp')!.click(), + ); + // The second save waits for the first. + expect(updateAdmin).toHaveBeenCalledTimes(1); + await act(async () => + pending[0](payload({ connectors: [connector({ enabled: false })] })), + ); + expect(updateAdmin).toHaveBeenCalledTimes(2); + await act(async () => + pending[1]( + payload({ + connectors: [connector({ enabled: false })], + allow_custom_mcp: false, + }), + ), + ); + expect(toggle().getAttribute('aria-checked')).toBe('false'); + expect( + container + .querySelector('#allow-custom-mcp')! + .getAttribute('aria-checked'), + ).toBe('false'); + }); + it('offers a retry when loading fails', async () => { getAdmin.mockRejectedValue(new Error('offline')); await render(); diff --git a/frontend/src/admin/Connectors.tsx b/frontend/src/admin/Connectors.tsx index 40ead063..bffb55a9 100644 --- a/frontend/src/admin/Connectors.tsx +++ b/frontend/src/admin/Connectors.tsx @@ -1,5 +1,5 @@ import { ExternalLink, Info, TriangleAlert } from 'lucide-react'; -import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import connectorsService from '../api/services/connectorsService'; @@ -177,7 +177,10 @@ export default function Connectors() { load(); }, [load]); - const save = async (body: { + // Saves run one after another: each response is a full snapshot, so an + // older one arriving last would otherwise put back a stale policy. + const saveQueue = useRef>(Promise.resolve()); + const save = (body: { policies?: Record; allow_custom_mcp?: boolean; }) => { @@ -188,13 +191,16 @@ export default function Connectors() { message: 'Could not save the change.', }), ); - try { - const next = await connectorsService.updateAdmin(body, token); - if (next?.success) setData(next); - else failed(); - } catch { - failed(); - } + saveQueue.current = saveQueue.current.then(async () => { + try { + const next = await connectorsService.updateAdmin(body, token); + if (next?.success) setData(next); + else failed(); + } catch { + failed(); + } + }); + return saveQueue.current; }; if (data === null && loading) return ; diff --git a/tests/connectors/test_runtime.py b/tests/connectors/test_runtime.py index e1b9d0e2..bdaca042 100644 --- a/tests/connectors/test_runtime.py +++ b/tests/connectors/test_runtime.py @@ -86,6 +86,23 @@ class TestResolution: assert resolved.connection_id == bobs assert resolved.delegated is False + def test_member_with_several_accounts_uses_the_one_last_used(self, pg_conn): + from docsgpt.connectors.resolve import resolve_connection + + owner = _connection(pg_conn) + older = _connection(pg_conn, user="bob") + pg_conn.execute(text( + "UPDATE connector_sessions SET account_label = 'bob-home', last_used_at = now() - interval '1 day' " + "WHERE id = CAST(:i AS uuid)" + ), {"i": older}) + newer = _connection(pg_conn, user="bob") + pg_conn.execute(text( + "UPDATE connector_sessions SET last_used_at = now() WHERE id = CAST(:i AS uuid)" + ), {"i": newer}) + with _service_db(pg_conn): + resolved = resolve_connection(_tool(owner, mode="member"), "bob") + assert resolved.available and resolved.connection_id == newer + def test_member_mode_without_own_connection_is_unavailable(self, pg_conn): from docsgpt.connectors.resolve import resolve_connection diff --git a/tests/storage/db/test_migration_0038.py b/tests/storage/db/test_migration_0038.py index 66123837..42f62eb3 100644 --- a/tests/storage/db/test_migration_0038.py +++ b/tests/storage/db/test_migration_0038.py @@ -384,3 +384,14 @@ class TestMigration0038Credentials: ).scalar() assert read_secrets(drive)["token_info"]["access_token"] == "plain-at" assert count == 1 + + +def test_model_metadata_matches_the_policy_and_custom_model_columns(): + """The SQLAlchemy tables mirror the migrations for both ``enabled`` columns.""" + from docsgpt.storage.db.models import connector_policies_table, user_custom_models_table + + policy_enabled = connector_policies_table.c.enabled + assert policy_enabled.nullable is True and policy_enabled.server_default is None + model_enabled = user_custom_models_table.c.enabled + assert model_enabled.nullable is False + assert str(model_enabled.server_default.arg) == "true"