Address QA warnings W-02 through W-07
W-02: Purge accepted connection requests after 90 days (rejected/cancelled stay at 30) W-04: Rename shadowed `type` parameter to `notification_type` with alias W-05: Extract notification type string literals to constants in connection service W-06: Match notification list polling interval to unread count (15s when visible) W-07: Add filter_to_shareable defence-in-depth gate on resolve_shared_profile output W-03: Verified false positive — no double person lookup exists in accept flow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
3fe344c3a0
commit
20692632f2
@ -277,17 +277,24 @@ async def _purge_old_notifications(db: AsyncSession) -> None:
|
|||||||
|
|
||||||
|
|
||||||
async def _purge_resolved_requests(db: AsyncSession) -> None:
|
async def _purge_resolved_requests(db: AsyncSession) -> None:
|
||||||
"""Remove rejected/cancelled connection requests older than 30 days.
|
"""Remove resolved connection requests after retention period.
|
||||||
|
|
||||||
Note: resolved_at must be set when changing status to rejected/cancelled.
|
Rejected/cancelled: 30 days. Accepted: 90 days (longer for audit trail).
|
||||||
Rows with NULL resolved_at are preserved (comparison with NULL yields NULL).
|
resolved_at must be set when changing status. NULL resolved_at rows are
|
||||||
Any future cancel endpoint must set resolved_at = now on status change.
|
preserved (comparison with NULL yields NULL).
|
||||||
"""
|
"""
|
||||||
cutoff = datetime.now() - timedelta(days=30)
|
reject_cutoff = datetime.now() - timedelta(days=30)
|
||||||
|
accept_cutoff = datetime.now() - timedelta(days=90)
|
||||||
await db.execute(
|
await db.execute(
|
||||||
delete(ConnectionRequest).where(
|
delete(ConnectionRequest).where(
|
||||||
ConnectionRequest.status.in_(["rejected", "cancelled"]),
|
ConnectionRequest.status.in_(["rejected", "cancelled"]),
|
||||||
ConnectionRequest.resolved_at < cutoff,
|
ConnectionRequest.resolved_at < reject_cutoff,
|
||||||
|
)
|
||||||
|
)
|
||||||
|
await db.execute(
|
||||||
|
delete(ConnectionRequest).where(
|
||||||
|
ConnectionRequest.status == "accepted",
|
||||||
|
ConnectionRequest.resolved_at < accept_cutoff,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
|||||||
@ -40,6 +40,8 @@ from app.schemas.connection import (
|
|||||||
)
|
)
|
||||||
from app.services.audit import get_client_ip, log_audit_event
|
from app.services.audit import get_client_ip, log_audit_event
|
||||||
from app.services.connection import (
|
from app.services.connection import (
|
||||||
|
NOTIF_TYPE_CONNECTION_ACCEPTED,
|
||||||
|
NOTIF_TYPE_CONNECTION_REQUEST,
|
||||||
SHAREABLE_FIELDS,
|
SHAREABLE_FIELDS,
|
||||||
create_person_from_connection,
|
create_person_from_connection,
|
||||||
detach_umbral_contact,
|
detach_umbral_contact,
|
||||||
@ -229,11 +231,11 @@ async def send_connection_request(
|
|||||||
await create_notification(
|
await create_notification(
|
||||||
db,
|
db,
|
||||||
user_id=target.id,
|
user_id=target.id,
|
||||||
type="connection_request",
|
type=NOTIF_TYPE_CONNECTION_REQUEST,
|
||||||
title="New Connection Request",
|
title="New 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=NOTIF_TYPE_CONNECTION_REQUEST,
|
||||||
source_id=conn_request.id,
|
source_id=conn_request.id,
|
||||||
)
|
)
|
||||||
|
|
||||||
@ -505,7 +507,7 @@ async def _respond_to_request_inner(
|
|||||||
await create_notification(
|
await create_notification(
|
||||||
db,
|
db,
|
||||||
user_id=sender_id,
|
user_id=sender_id,
|
||||||
type="connection_accepted",
|
type=NOTIF_TYPE_CONNECTION_ACCEPTED,
|
||||||
title="Connection Accepted",
|
title="Connection Accepted",
|
||||||
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},
|
||||||
|
|||||||
@ -23,7 +23,7 @@ router = APIRouter()
|
|||||||
@router.get("/", response_model=NotificationListResponse)
|
@router.get("/", response_model=NotificationListResponse)
|
||||||
async def list_notifications(
|
async def list_notifications(
|
||||||
unread_only: bool = Query(False),
|
unread_only: bool = Query(False),
|
||||||
type: str | None = Query(None, max_length=50),
|
notification_type: str | None = Query(None, max_length=50, alias="type"),
|
||||||
page: int = Query(1, ge=1),
|
page: int = Query(1, ge=1),
|
||||||
per_page: int = Query(20, ge=1, le=100),
|
per_page: int = Query(20, ge=1, le=100),
|
||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
@ -34,8 +34,8 @@ async def list_notifications(
|
|||||||
|
|
||||||
if unread_only:
|
if unread_only:
|
||||||
base = base.where(Notification.is_read == False) # noqa: E712
|
base = base.where(Notification.is_read == False) # noqa: E712
|
||||||
if type:
|
if notification_type:
|
||||||
base = base.where(Notification.type == type)
|
base = base.where(Notification.type == notification_type)
|
||||||
|
|
||||||
# Total count
|
# Total count
|
||||||
count_q = select(func.count()).select_from(base.subquery())
|
count_q = select(func.count()).select_from(base.subquery())
|
||||||
|
|||||||
@ -18,6 +18,10 @@ from app.services.ntfy import send_ntfy_notification
|
|||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
# Notification type constants — keep in sync with notifications model CHECK constraint
|
||||||
|
NOTIF_TYPE_CONNECTION_REQUEST = "connection_request"
|
||||||
|
NOTIF_TYPE_CONNECTION_ACCEPTED = "connection_accepted"
|
||||||
|
|
||||||
# Single source of truth — only these fields can be shared via connections
|
# Single source of truth — only these fields can be shared via connections
|
||||||
SHAREABLE_FIELDS = frozenset({
|
SHAREABLE_FIELDS = frozenset({
|
||||||
"first_name", "last_name", "preferred_name", "email", "phone", "mobile",
|
"first_name", "last_name", "preferred_name", "email", "phone", "mobile",
|
||||||
@ -75,7 +79,12 @@ def resolve_shared_profile(
|
|||||||
elif field in _SETTINGS_FIELD_MAP and _SETTINGS_FIELD_MAP[field]:
|
elif field in _SETTINGS_FIELD_MAP and _SETTINGS_FIELD_MAP[field]:
|
||||||
result[field] = getattr(settings, _SETTINGS_FIELD_MAP[field], None)
|
result[field] = getattr(settings, _SETTINGS_FIELD_MAP[field], None)
|
||||||
|
|
||||||
return result
|
return filter_to_shareable(result)
|
||||||
|
|
||||||
|
|
||||||
|
def filter_to_shareable(profile: dict) -> dict:
|
||||||
|
"""Strip any keys not in SHAREABLE_FIELDS. Defence-in-depth gate."""
|
||||||
|
return {k: v for k, v in profile.items() if k in SHAREABLE_FIELDS}
|
||||||
|
|
||||||
|
|
||||||
def create_person_from_connection(
|
def create_person_from_connection(
|
||||||
|
|||||||
@ -64,7 +64,7 @@ export function NotificationProvider({ children }: { children: ReactNode }) {
|
|||||||
return data;
|
return data;
|
||||||
},
|
},
|
||||||
staleTime: 15_000,
|
staleTime: 15_000,
|
||||||
refetchInterval: () => (visibleRef.current ? 60_000 : false),
|
refetchInterval: () => (visibleRef.current ? 15_000 : false),
|
||||||
});
|
});
|
||||||
|
|
||||||
const markReadMutation = useMutation({
|
const markReadMutation = useMutation({
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user