From b554ba71514879ba3d4cdce9b3397b82d30b7c54 Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Wed, 4 Mar 2026 09:05:46 +0800 Subject: [PATCH] Fix QA: IntegrityError handling, dict mutation, birthday sync, None guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - C-01: Wrap accept flow flush/commit in IntegrityError handling (409) - C-02: Use separate remote_timestamps dict instead of pop() on shared profile - W-01: Add birthday sync in Link conversion path (existing person → umbral) - W-02: Add None guard on max(updated_at) comparison in get_person Co-Authored-By: Claude Opus 4.6 --- backend/app/routers/connections.py | 23 +++++++++++++++++++---- backend/app/routers/people.py | 20 ++++++++++---------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/backend/app/routers/connections.py b/backend/app/routers/connections.py index f63107e..430c015 100644 --- a/backend/app/routers/connections.py +++ b/backend/app/routers/connections.py @@ -9,7 +9,7 @@ Security: - Audit logging for all connection events """ import asyncio -from datetime import datetime, timedelta, timezone +from datetime import date as date_type, datetime, timedelta, timezone from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Path, Query, Request from sqlalchemy import delete, select, func, and_, update @@ -415,7 +415,7 @@ async def respond_to_request( select(Person).where(Person.id == request_person_id) ) existing_person = existing_result.scalar_one_or_none() - # Re-validate at accept time (C-01, W-01): ownership must match sender, + # Re-validate at accept time: ownership must match sender, # and must not already be umbral (prevents double-conversion races) if existing_person and existing_person.user_id == sender_id and not existing_person.is_umbral_contact: # Convert existing standard contact to umbral @@ -433,6 +433,13 @@ async def respond_to_request( existing_person.address = receiver_shared.get("address") or existing_person.address existing_person.company = receiver_shared.get("company") or existing_person.company existing_person.job_title = receiver_shared.get("job_title") or existing_person.job_title + # Sync birthday from shared profile + birthday_str = receiver_shared.get("birthday") + if birthday_str: + try: + existing_person.birthday = date_type.fromisoformat(birthday_str) + except (ValueError, TypeError): + pass # Recompute display name full = ((first_name or '') + ' ' + (last_name or '')).strip() existing_person.name = full or current_user.umbral_name @@ -444,7 +451,11 @@ async def respond_to_request( ) db.add(person_for_sender) - await db.flush() # populate person IDs + try: + await db.flush() # populate person IDs + except IntegrityError: + await db.rollback() + raise HTTPException(status_code=409, detail="Connection already exists") # Create bidirectional connections conn_a = UserConnection( @@ -460,7 +471,11 @@ async def respond_to_request( db.add(conn_a) db.add(conn_b) - await db.flush() # populate conn_a.id for source_id + try: + await db.flush() # populate conn_a.id for source_id + except IntegrityError: + await db.rollback() + raise HTTPException(status_code=409, detail="Connection already exists") # Notification to sender receiver_display = (receiver_settings.preferred_name if receiver_settings else None) or current_user.umbral_name diff --git a/backend/app/routers/people.py b/backend/app/routers/people.py index 5c031ad..6420e52 100644 --- a/backend/app/routers/people.py +++ b/backend/app/routers/people.py @@ -91,8 +91,9 @@ async def get_people( for c in conns_result.scalars().all() } - # Build shared profiles + # Build shared profiles and track remote timestamps separately shared_profiles: dict[int, dict] = {} + remote_timestamps: dict[int, datetime] = {} for uid in linked_user_ids: user = users_by_id.get(uid) user_settings = settings_by_user.get(uid) @@ -102,19 +103,17 @@ async def get_people( ) # umbral_name is always visible (public identity), not a shareable field shared_profiles[uid]["umbral_name"] = user.umbral_name - shared_profiles[uid]["_updated_at"] = max( - user.updated_at, user_settings.updated_at - ) + if user.updated_at and user_settings.updated_at: + remote_timestamps[uid] = max(user.updated_at, user_settings.updated_at) # Attach to response responses = [] for p in people: resp = PersonResponse.model_validate(p) if p.linked_user_id and p.linked_user_id in shared_profiles: - profile = shared_profiles[p.linked_user_id] - resp.shared_fields = profile + resp.shared_fields = shared_profiles[p.linked_user_id] # Show the latest update time across local record and connected user's profile - remote_updated = profile.pop("_updated_at", None) + remote_updated = remote_timestamps.get(p.linked_user_id) if remote_updated and remote_updated > p.updated_at: resp.updated_at = remote_updated responses.append(resp) @@ -188,9 +187,10 @@ async def get_person( ) resp.shared_fields["umbral_name"] = linked_user.umbral_name # Show the latest update time across local record and connected user's profile - remote_updated = max(linked_user.updated_at, linked_settings.updated_at) - if remote_updated > person.updated_at: - resp.updated_at = remote_updated + if linked_user.updated_at and linked_settings.updated_at: + remote_updated = max(linked_user.updated_at, linked_settings.updated_at) + if remote_updated > person.updated_at: + resp.updated_at = remote_updated return resp