Test/consolidate test suite#59
Conversation
- Auto-approve group photos as public when no face matches any user - Add ExpireStaleApprovals SQL + service method to unblock photos where users never respond (default 7-day timeout via config) - Wire hourly expiry background task into FastAPI lifespan
This test validates the asynchronous workflow of the AI pipeline locally. It uploads a photo, triggers the NATS event, polls for the processing_job completion, and asserts that the photo status transitions to 'approved'. Test output: 1 passed in ~1.5s
fix(photo-approval): resolve group photo lifecycle gaps
# Conflicts: # app/infra/redis.py
fix(core): address code review findings
feat(auth): Implement Email OTP Verification
wailbentafat
left a comment
There was a problem hiding this comment.
Automated review: inline comments added on specific buggy lines.
| await auth_service.mobile_register_resend_otp(redis=mock_redis, email=email) | ||
|
|
||
| assert exc_info.value.status_code == 404 | ||
| assert "No pending registration found" in exc_info.value.detail |
There was a problem hiding this comment.
Bug: Wrong mock patch path — this test will always pass even when NATS publish never fires.
The patch targets app.infra.nats.NatsClient.publish but users.py imports NatsClient with from app.infra.nats import NatsClient, so the name is already bound in app.service.users. Patching the source module after the import has no effect.
The test right above (test_mobile_register_sends_otp) does it correctly.
# ❌ wrong
with patch("app.infra.nats.NatsClient.publish", new_callable=AsyncMock) as mock_publish:
# ✅ correct
with patch("app.service.users.NatsClient.publish", new_callable=AsyncMock) as mock_publish:| mock_redis.incr.return_value = 1 | ||
|
|
||
| # Act | ||
| with patch("app.infra.nats.NatsClient.publish", new_callable=AsyncMock) as mock_publish: |
There was a problem hiding this comment.
Bug: Wrong mock patch path — this test will always pass silently even when NATS publish never fires.
users.py imports NatsClient as from app.infra.nats import NatsClient, so the name is already bound in app.service.users at import time. Patching app.infra.nats.NatsClient.publish after that has no effect on the already-bound reference.
The test right above this one (test_mobile_register_sends_otp) uses the correct path.
# ❌ current — patch has no effect
with patch("app.infra.nats.NatsClient.publish", new_callable=AsyncMock) as mock_publish:
# ✅ fix
with patch("app.service.users.NatsClient.publish", new_callable=AsyncMock) as mock_publish:| await auth_service.add_embbed_user( | ||
| user_id=user_id, | ||
| image_payloads=[payload], | ||
| ) |
There was a problem hiding this comment.
Bug: Wrong field name — this will raise a TypeError at runtime and the test will never run.
FaceImagePayload is defined with fields filename, content_type, and bytes — not image_bytes.
# ❌ current — TypeError: unexpected keyword argument 'image_bytes'
payload = FaceImagePayload(image_bytes=b"fake-image", filename="face.jpg")
# ✅ fix
payload = FaceImagePayload(bytes=b"fake-image", filename="face.jpg", content_type="image/jpeg")| from db.generated import photo_approvals as approval_queries | ||
|
|
||
|
|
||
| # =========================================================================== |
There was a problem hiding this comment.
Bug: Typo in module name — causes ImportError, the entire file fails to collect.
stuff_user does not exist. This should be staff_user (or whatever the actual generated module name is).
# ❌ current — ImportError: No module named 'db.generated.stuff_user'
from db.generated import stuff_user as staff_queries
# ✅ fix
from db.generated import staff_user as staff_queries
Remaining Bugs (couldn't pin inline)
|
Test Coverage & Scope
1. Unit Tests (
tests/unit/)2. Integration Tests (
tests/integration/)3. End-to-End (E2E) Tests (
tests/e2e/)/registerendpoint, retrieving the OTP directly from Redis, validating via/register/verify, and successfully logging in.Verification