Fix QA review findings: source_id, N+1 queries, event bubbling, type mismatches
Critical fixes: - C-01: Add receiver_umbral_name/receiver_preferred_name to frontend ConnectionRequest type - C-02: Flush connection request before notification to populate source_id - C-03: Add umbral_name to ProfileResponse/UserProfile, use in Settings Social card - C-04: Remove dead code in sharing-overrides endpoint, merge instead of replace Warning fixes: - W-01/W-02: Batch-fetch settings in incoming/outgoing/list connection endpoints (N+1 fix) - W-04: Add _purge_resolved_requests job for rejected/cancelled requests (30-day retention) - W-10: Add e.stopPropagation() to notification mark-read and delete buttons Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
3d22568b9c
commit
337b50c7ce
@ -26,6 +26,7 @@ from app.models.project import Project
|
|||||||
from app.models.ntfy_sent import NtfySent
|
from app.models.ntfy_sent import NtfySent
|
||||||
from app.models.totp_usage import TOTPUsage
|
from app.models.totp_usage import TOTPUsage
|
||||||
from app.models.session import UserSession
|
from app.models.session import UserSession
|
||||||
|
from app.models.connection_request import ConnectionRequest
|
||||||
from app.services.ntfy import send_ntfy_notification
|
from app.services.ntfy import send_ntfy_notification
|
||||||
from app.services.ntfy_templates import (
|
from app.services.ntfy_templates import (
|
||||||
build_event_notification,
|
build_event_notification,
|
||||||
@ -275,6 +276,18 @@ async def _purge_old_notifications(db: AsyncSession) -> None:
|
|||||||
await db.commit()
|
await db.commit()
|
||||||
|
|
||||||
|
|
||||||
|
async def _purge_resolved_requests(db: AsyncSession) -> None:
|
||||||
|
"""Remove rejected/cancelled connection requests older than 30 days."""
|
||||||
|
cutoff = datetime.now() - timedelta(days=30)
|
||||||
|
await db.execute(
|
||||||
|
delete(ConnectionRequest).where(
|
||||||
|
ConnectionRequest.status.in_(["rejected", "cancelled"]),
|
||||||
|
ConnectionRequest.resolved_at < cutoff,
|
||||||
|
)
|
||||||
|
)
|
||||||
|
await db.commit()
|
||||||
|
|
||||||
|
|
||||||
# ── Entry point ───────────────────────────────────────────────────────────────
|
# ── Entry point ───────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
async def run_notification_dispatch() -> None:
|
async def run_notification_dispatch() -> None:
|
||||||
@ -317,6 +330,7 @@ async def run_notification_dispatch() -> None:
|
|||||||
await _purge_totp_usage(db)
|
await _purge_totp_usage(db)
|
||||||
await _purge_expired_sessions(db)
|
await _purge_expired_sessions(db)
|
||||||
await _purge_old_notifications(db)
|
await _purge_old_notifications(db)
|
||||||
|
await _purge_resolved_requests(db)
|
||||||
|
|
||||||
except Exception:
|
except Exception:
|
||||||
# Broad catch: job failure must never crash the scheduler or the app
|
# Broad catch: job failure must never crash the scheduler or the app
|
||||||
|
|||||||
@ -183,6 +183,7 @@ async def send_connection_request(
|
|||||||
receiver_id=target.id,
|
receiver_id=target.id,
|
||||||
)
|
)
|
||||||
db.add(conn_request)
|
db.add(conn_request)
|
||||||
|
await db.flush() # populate conn_request.id for source_id
|
||||||
|
|
||||||
# Create in-app notification for receiver
|
# Create in-app notification for receiver
|
||||||
sender_settings = await _get_settings_for_user(db, current_user.id)
|
sender_settings = await _get_settings_for_user(db, current_user.id)
|
||||||
@ -196,7 +197,7 @@ async def send_connection_request(
|
|||||||
message=f"{sender_display} wants to connect with you",
|
message=f"{sender_display} wants to connect with you",
|
||||||
data={"sender_umbral_name": current_user.umbral_name},
|
data={"sender_umbral_name": current_user.umbral_name},
|
||||||
source_type="connection_request",
|
source_type="connection_request",
|
||||||
source_id=None, # Will be set after flush
|
source_id=conn_request.id,
|
||||||
)
|
)
|
||||||
|
|
||||||
await log_audit_event(
|
await log_audit_event(
|
||||||
@ -246,10 +247,18 @@ async def get_incoming_requests(
|
|||||||
)
|
)
|
||||||
requests = result.scalars().all()
|
requests = result.scalars().all()
|
||||||
|
|
||||||
|
# Fetch current user's settings once, batch-fetch sender settings
|
||||||
|
receiver_settings = await _get_settings_for_user(db, current_user.id)
|
||||||
|
sender_ids = [req.sender_id for req in requests]
|
||||||
|
if sender_ids:
|
||||||
|
settings_result = await db.execute(select(Settings).where(Settings.user_id.in_(sender_ids)))
|
||||||
|
settings_by_user = {s.user_id: s for s in settings_result.scalars().all()}
|
||||||
|
else:
|
||||||
|
settings_by_user = {}
|
||||||
|
|
||||||
responses = []
|
responses = []
|
||||||
for req in requests:
|
for req in requests:
|
||||||
sender_settings = await _get_settings_for_user(db, req.sender_id)
|
sender_settings = settings_by_user.get(req.sender_id)
|
||||||
receiver_settings = await _get_settings_for_user(db, current_user.id)
|
|
||||||
responses.append(_build_request_response(req, req.sender, sender_settings, current_user, receiver_settings))
|
responses.append(_build_request_response(req, req.sender, sender_settings, current_user, receiver_settings))
|
||||||
|
|
||||||
return responses
|
return responses
|
||||||
@ -279,10 +288,18 @@ async def get_outgoing_requests(
|
|||||||
)
|
)
|
||||||
requests = result.scalars().all()
|
requests = result.scalars().all()
|
||||||
|
|
||||||
|
# Fetch current user's settings once, batch-fetch receiver settings
|
||||||
|
sender_settings = await _get_settings_for_user(db, current_user.id)
|
||||||
|
receiver_ids = [req.receiver_id for req in requests]
|
||||||
|
if receiver_ids:
|
||||||
|
settings_result = await db.execute(select(Settings).where(Settings.user_id.in_(receiver_ids)))
|
||||||
|
settings_by_user = {s.user_id: s for s in settings_result.scalars().all()}
|
||||||
|
else:
|
||||||
|
settings_by_user = {}
|
||||||
|
|
||||||
responses = []
|
responses = []
|
||||||
for req in requests:
|
for req in requests:
|
||||||
sender_settings = await _get_settings_for_user(db, current_user.id)
|
receiver_settings = settings_by_user.get(req.receiver_id)
|
||||||
receiver_settings = await _get_settings_for_user(db, req.receiver_id)
|
|
||||||
responses.append(_build_request_response(req, current_user, sender_settings, req.receiver, receiver_settings))
|
responses.append(_build_request_response(req, current_user, sender_settings, req.receiver, receiver_settings))
|
||||||
|
|
||||||
return responses
|
return responses
|
||||||
@ -366,6 +383,8 @@ async def respond_to_request(
|
|||||||
db.add(conn_a)
|
db.add(conn_a)
|
||||||
db.add(conn_b)
|
db.add(conn_b)
|
||||||
|
|
||||||
|
await db.flush() # populate conn_a.id for source_id
|
||||||
|
|
||||||
# Notification to sender
|
# Notification to sender
|
||||||
receiver_display = (receiver_settings.preferred_name if receiver_settings else None) or current_user.umbral_name
|
receiver_display = (receiver_settings.preferred_name if receiver_settings else None) or current_user.umbral_name
|
||||||
await create_notification(
|
await create_notification(
|
||||||
@ -376,7 +395,7 @@ async def respond_to_request(
|
|||||||
message=f"{receiver_display} accepted your connection request",
|
message=f"{receiver_display} accepted your connection request",
|
||||||
data={"connected_umbral_name": current_user.umbral_name},
|
data={"connected_umbral_name": current_user.umbral_name},
|
||||||
source_type="user_connection",
|
source_type="user_connection",
|
||||||
source_id=None,
|
source_id=conn_b.id,
|
||||||
)
|
)
|
||||||
|
|
||||||
await log_audit_event(
|
await log_audit_event(
|
||||||
@ -436,9 +455,17 @@ async def list_connections(
|
|||||||
)
|
)
|
||||||
connections = result.scalars().all()
|
connections = result.scalars().all()
|
||||||
|
|
||||||
|
# Batch-fetch settings for connected users
|
||||||
|
connected_ids = [conn.connected_user_id for conn in connections]
|
||||||
|
if connected_ids:
|
||||||
|
settings_result = await db.execute(select(Settings).where(Settings.user_id.in_(connected_ids)))
|
||||||
|
settings_by_user = {s.user_id: s for s in settings_result.scalars().all()}
|
||||||
|
else:
|
||||||
|
settings_by_user = {}
|
||||||
|
|
||||||
responses = []
|
responses = []
|
||||||
for conn in connections:
|
for conn in connections:
|
||||||
conn_settings = await _get_settings_for_user(db, conn.connected_user_id)
|
conn_settings = settings_by_user.get(conn.connected_user_id)
|
||||||
responses.append(ConnectionResponse(
|
responses.append(ConnectionResponse(
|
||||||
id=conn.id,
|
id=conn.id,
|
||||||
connected_user_id=conn.connected_user_id,
|
connected_user_id=conn.connected_user_id,
|
||||||
@ -525,21 +552,7 @@ async def update_sharing_overrides(
|
|||||||
current_user: User = Depends(get_current_user),
|
current_user: User = Depends(get_current_user),
|
||||||
):
|
):
|
||||||
"""Update what YOU share with a specific connection."""
|
"""Update what YOU share with a specific connection."""
|
||||||
# Find the connection where the OTHER user connects to YOU
|
# Get our connection to know who the counterpart is
|
||||||
result = await db.execute(
|
|
||||||
select(UserConnection).where(
|
|
||||||
UserConnection.connected_user_id == current_user.id,
|
|
||||||
UserConnection.user_id != current_user.id,
|
|
||||||
)
|
|
||||||
)
|
|
||||||
# We need the reverse connection (where we are the connected_user)
|
|
||||||
# Actually, we need to find the connection from the counterpart's perspective
|
|
||||||
# The connection_id is OUR connection. The sharing overrides go on the
|
|
||||||
# counterpart's connection row (since they determine what they see from us).
|
|
||||||
# Wait — per the plan, sharing overrides control what WE share with THEM.
|
|
||||||
# So they go on their connection row pointing to us.
|
|
||||||
|
|
||||||
# First, get our connection to know who the counterpart is
|
|
||||||
our_conn = await db.execute(
|
our_conn = await db.execute(
|
||||||
select(UserConnection).where(
|
select(UserConnection).where(
|
||||||
UserConnection.id == connection_id,
|
UserConnection.id == connection_id,
|
||||||
@ -561,14 +574,17 @@ async def update_sharing_overrides(
|
|||||||
if not reverse_conn:
|
if not reverse_conn:
|
||||||
raise HTTPException(status_code=404, detail="Reverse connection not found")
|
raise HTTPException(status_code=404, detail="Reverse connection not found")
|
||||||
|
|
||||||
# Build validated overrides dict — only SHAREABLE_FIELDS keys
|
# Merge validated overrides — only SHAREABLE_FIELDS keys
|
||||||
overrides = {}
|
existing = dict(reverse_conn.sharing_overrides or {})
|
||||||
update_data = body.model_dump(exclude_unset=True)
|
update_data = body.model_dump(exclude_unset=True)
|
||||||
for key, value in update_data.items():
|
for key, value in update_data.items():
|
||||||
if key in SHAREABLE_FIELDS:
|
if key in SHAREABLE_FIELDS:
|
||||||
overrides[key] = value
|
if value is None:
|
||||||
|
existing.pop(key, None)
|
||||||
|
else:
|
||||||
|
existing[key] = value
|
||||||
|
|
||||||
reverse_conn.sharing_overrides = overrides if overrides else None
|
reverse_conn.sharing_overrides = existing if existing else None
|
||||||
|
|
||||||
await db.commit()
|
await db.commit()
|
||||||
return {"message": "Sharing overrides updated"}
|
return {"message": "Sharing overrides updated"}
|
||||||
|
|||||||
@ -199,6 +199,7 @@ class ProfileResponse(BaseModel):
|
|||||||
model_config = ConfigDict(from_attributes=True)
|
model_config = ConfigDict(from_attributes=True)
|
||||||
|
|
||||||
username: str
|
username: str
|
||||||
|
umbral_name: str
|
||||||
email: str | None
|
email: str | None
|
||||||
first_name: str | None
|
first_name: str | None
|
||||||
last_name: str | None
|
last_name: str | None
|
||||||
|
|||||||
@ -176,7 +176,7 @@ export default function NotificationsPage() {
|
|||||||
<div className="flex items-center gap-0.5 opacity-0 group-hover:opacity-100 transition-opacity">
|
<div className="flex items-center gap-0.5 opacity-0 group-hover:opacity-100 transition-opacity">
|
||||||
{!notification.is_read && (
|
{!notification.is_read && (
|
||||||
<button
|
<button
|
||||||
onClick={() => handleMarkRead(notification.id)}
|
onClick={(e) => { e.stopPropagation(); handleMarkRead(notification.id); }}
|
||||||
className="p-1 rounded hover:bg-accent/10 text-muted-foreground hover:text-accent transition-colors"
|
className="p-1 rounded hover:bg-accent/10 text-muted-foreground hover:text-accent transition-colors"
|
||||||
title="Mark as read"
|
title="Mark as read"
|
||||||
>
|
>
|
||||||
@ -184,7 +184,7 @@ export default function NotificationsPage() {
|
|||||||
</button>
|
</button>
|
||||||
)}
|
)}
|
||||||
<button
|
<button
|
||||||
onClick={() => handleDelete(notification.id)}
|
onClick={(e) => { e.stopPropagation(); handleDelete(notification.id); }}
|
||||||
className="p-1 rounded hover:bg-destructive/10 text-muted-foreground hover:text-destructive transition-colors"
|
className="p-1 rounded hover:bg-destructive/10 text-muted-foreground hover:text-destructive transition-colors"
|
||||||
title="Delete"
|
title="Delete"
|
||||||
>
|
>
|
||||||
|
|||||||
@ -733,11 +733,11 @@ export default function SettingsPage() {
|
|||||||
<Label>Umbral Name</Label>
|
<Label>Umbral Name</Label>
|
||||||
<div className="flex items-center gap-3">
|
<div className="flex items-center gap-3">
|
||||||
<Input
|
<Input
|
||||||
value={profileQuery.data?.username ?? ''}
|
value={profileQuery.data?.umbral_name ?? ''}
|
||||||
disabled
|
disabled
|
||||||
className="opacity-70 cursor-not-allowed"
|
className="opacity-70 cursor-not-allowed"
|
||||||
/>
|
/>
|
||||||
<CopyableField value={profileQuery.data?.username ?? ''} label="Umbral name" />
|
<CopyableField value={profileQuery.data?.umbral_name ?? ''} label="Umbral name" />
|
||||||
</div>
|
</div>
|
||||||
<p className="text-sm text-muted-foreground">
|
<p className="text-sm text-muted-foreground">
|
||||||
How other Umbra users find you
|
How other Umbra users find you
|
||||||
|
|||||||
@ -371,6 +371,7 @@ export interface UpcomingResponse {
|
|||||||
|
|
||||||
export interface UserProfile {
|
export interface UserProfile {
|
||||||
username: string;
|
username: string;
|
||||||
|
umbral_name: string;
|
||||||
email: string | null;
|
email: string | null;
|
||||||
first_name: string | null;
|
first_name: string | null;
|
||||||
last_name: string | null;
|
last_name: string | null;
|
||||||
@ -418,6 +419,8 @@ export interface ConnectionRequest {
|
|||||||
id: number;
|
id: number;
|
||||||
sender_umbral_name: string;
|
sender_umbral_name: string;
|
||||||
sender_preferred_name: string | null;
|
sender_preferred_name: string | null;
|
||||||
|
receiver_umbral_name: string;
|
||||||
|
receiver_preferred_name: string | null;
|
||||||
status: 'pending' | 'accepted' | 'rejected' | 'cancelled';
|
status: 'pending' | 'accepted' | 'rejected' | 'cancelled';
|
||||||
created_at: string;
|
created_at: string;
|
||||||
}
|
}
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user