Added Notifications for Friend Request#23
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR introduces a FriendService to manage friend-related notifications and a webhook endpoint that processes friendship database changes, triggering appropriate notifications for friend requests and acceptances via push tokens while respecting user notification preferences. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Webhook as Webhook Endpoint
participant FriendService
participant Database as Supabase
participant Cache as CacheService
participant Notification as NotificationService
Client->>Webhook: POST /webhooks/friends (INSERT)
Webhook->>Webhook: Validate table name & payload
Webhook->>FriendService: send_friend_request_notification(friendship)
FriendService->>FriendService: Validate friendship fields
FriendService->>Database: Fetch sender profile
Database-->>FriendService: Profile data
FriendService->>Cache: Get recipient notification settings
Cache-->>FriendService: friend_requests setting
FriendService->>Cache: Get recipient push tokens
Cache-->>FriendService: Token list
FriendService->>Notification: Enqueue notification (title, body, tokens, metadata)
Notification-->>FriendService: success/failure
FriendService-->>Webhook: Return status
Webhook-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
backend/tests/test_friend_service_integration.py (1)
1-5: Remove unused imports.
AsyncMock(line 4) andjson(line 5) are imported but not used in this test file.🧹 Suggested cleanup
import os import sys import pytest -from unittest.mock import MagicMock, patch, AsyncMock -import json +from unittest.mock import MagicMock, patchbackend/routers/webhooks.py (2)
27-32: Consider consolidating duplicate payload models.
FriendWebhookPayloadhas identical structure toEntryWebhookPayload. Consider creating a shared base model or using a singleWebhookPayloadclass.♻️ Suggested consolidation
-class EntryWebhookPayload(BaseModel): - """Payload structure for entry webhook.""" - type: str # 'INSERT', 'UPDATE', 'DELETE' - table: str - record: Optional[Dict[str, Any]] = None - old_record: Optional[Dict[str, Any]] = None - -class FriendWebhookPayload(BaseModel): - """Payload structure for friend webhook.""" +class WebhookPayload(BaseModel): + """Payload structure for Supabase webhooks.""" type: str # 'INSERT', 'UPDATE', 'DELETE' table: str record: Optional[Dict[str, Any]] = None old_record: Optional[Dict[str, Any]] = None
299-306: Improve exception chaining for better debugging.When re-raising exceptions, use
raise ... from eto preserve the exception chain. This applies to both webhook handlers.♻️ Suggested fix
except Exception as e: - logger.error(f"Error processing friend webhook: {str(e)}", exc_info=True) + logger.error("Error processing friend webhook: %s", e, exc_info=True) raise HTTPException( status_code=500, - detail=f"Internal server error: {str(e)}" - ) + detail=f"Internal server error: {e!s}" + ) from ebackend/tests/test_friend_service.py (1)
1-14: Minor: Unused import.
AsyncMockis imported but not used.🧹 Suggested cleanup
import os import sys import pytest -from unittest.mock import MagicMock, patch, AsyncMock +from unittest.mock import MagicMock, patchbackend/services/friend_service.py (3)
10-17: Type aliases provide no additional type safety.
FriendshipDictandProfileDictextendingDict[str, Any]don't add type checking benefits. Consider usingTypedDictfor actual compile-time key/type checking, or remove these aliases.♻️ TypedDict alternative for better type safety
from typing import TypedDict, Optional class FriendshipDict(TypedDict, total=False): id: str user_id: str friend_id: str status: str class ProfileDict(TypedDict, total=False): id: str username: Optional[str] full_name: Optional[str] email: Optional[str]
280-304: Fail-open design for notification settings is intentional but warrants documentation.The
_filter_recipients_by_notification_settingsmethod defaults to sending notifications when:
- User has no settings stored (line 292-294)
- An error occurs fetching settings (line 303-304)
This fail-open approach prioritizes notification delivery over respecting potentially-unknown preferences. Consider adding a comment explaining this design choice, as it could surprise future maintainers.
261-263: Consider usinglogger.exception()in catch blocks.In the helper methods, using
logger.exception()instead oflogger.error()would automatically include the stack trace, which aids debugging.♻️ Example fix for _get_user_profile
except Exception as e: - logger.error(f"Error fetching user profile {user_id}: {str(e)}") + logger.exception("Error fetching user profile %s", user_id) return NoneAlso applies to: 301-302, 331-332
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/routers/webhooks.pybackend/services/friend_service.pybackend/services/posthog_client.pybackend/tests/test_friend_service.pybackend/tests/test_friend_service_integration.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/tests/test_friend_service.py (3)
backend/services/cache_service.py (2)
get_notification_settings_batch(71-131)get_push_tokens_batch(172-238)backend/services/notification_service.py (1)
enqueue_notification(52-122)backend/services/friend_service.py (2)
send_friend_request_notification(39-139)send_request_accept_notification(141-244)
backend/tests/test_friend_service_integration.py (3)
backend/services/cache_service.py (2)
get_notification_settings_batch(71-131)get_push_tokens_batch(172-238)backend/services/notification_service.py (1)
enqueue_notification(52-122)backend/services/friend_service.py (2)
send_friend_request_notification(39-139)send_request_accept_notification(141-244)
backend/services/posthog_client.py (1)
frontend/constants/posthog.ts (1)
posthog(36-38)
backend/services/friend_service.py (3)
backend/services/notification_service.py (1)
enqueue_notification(52-122)backend/services/supabase_client.py (1)
get_supabase_client(4-9)backend/services/cache_service.py (2)
get_notification_settings_batch(71-131)get_push_tokens_batch(172-238)
🪛 Ruff (0.14.10)
backend/routers/webhooks.py
185-185: Unused function argument: request
(ARG001)
186-186: Unused function argument: x_supabase_signature
(ARG001)
212-212: Abstract raise to an inner function
(TRY301)
244-244: Abstract raise to an inner function
(TRY301)
281-281: Abstract raise to an inner function
(TRY301)
294-297: Abstract raise to an inner function
(TRY301)
302-302: Use explicit conversion flag
Replace with conversion flag
(RUF010)
303-306: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
305-305: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/services/friend_service.py
135-135: Consider moving this statement to an else block
(TRY300)
138-138: Use explicit conversion flag
Replace with conversion flag
(RUF010)
240-240: Consider moving this statement to an else block
(TRY300)
243-243: Use explicit conversion flag
Replace with conversion flag
(RUF010)
260-260: Consider moving this statement to an else block
(TRY300)
261-261: Do not catch blind exception: Exception
(BLE001)
262-262: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
262-262: Use explicit conversion flag
Replace with conversion flag
(RUF010)
299-299: Consider moving this statement to an else block
(TRY300)
301-301: Do not catch blind exception: Exception
(BLE001)
302-302: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
302-302: Use explicit conversion flag
Replace with conversion flag
(RUF010)
329-329: Consider moving this statement to an else block
(TRY300)
331-331: Do not catch blind exception: Exception
(BLE001)
332-332: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
332-332: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (8)
backend/services/posthog_client.py (1)
1-12: LGTM - Singleton Posthog client accessor.The implementation follows a standard lazy initialization pattern. While not strictly thread-safe (race condition possible on first access), this is typically acceptable in Python due to the GIL, and the worst case is creating the client twice during startup—not a correctness issue.
Note: This module appears unused in the current PR changes. Verify it's needed or intended for future use.
backend/tests/test_friend_service_integration.py (2)
16-65: Well-structured test fixtures.The fixtures properly mock the Supabase client's chainable API and inject mocked dependencies into FriendService. The nested patching approach ensures isolation.
68-289: Comprehensive test coverage for FriendService flows.The tests effectively cover:
- Success paths with proper notification content verification
- Fallback name handling when username/full_name are absent
- Enqueue failure scenarios
The assertions on metadata fields (
friendship_id,sender_id,recipient_id,notification_type,page_url) ensure the notification payloads are correctly structured.backend/routers/webhooks.py (2)
242-277: UPDATE handler correctly guards against missing old_record.The ternary on line 248 properly handles the case where
old_recordis None. The logic for detectingpending → acceptedtransitions is sound.
182-240: Friend webhook INSERT handler looks good.The handler correctly:
- Validates the table name
- Checks for pending status before sending notifications
- Returns appropriate status codes for success/partial_success scenarios
- Logs processing details
backend/tests/test_friend_service.py (1)
67-414: Thorough unit test coverage.The tests effectively validate:
- Success paths with correct notification payloads
- Validation failures (missing required fields)
- Status-based filtering (pending/accepted checks)
- Profile lookup failures
- User preference respecting (notifications disabled returns True without sending)
- Edge case of no push tokens
The pattern of asserting
result is Truewithassert_not_called()for preference-based skips correctly documents the expected behavior.backend/services/friend_service.py (2)
39-139: Friend request notification flow is well-structured.The method correctly:
- Validates all required fields upfront
- Short-circuits for non-pending status (returning True)
- Gracefully handles missing profiles (returning False)
- Respects user notification preferences
- Handles missing push tokens without error
The distinction between returning
False(actual errors) vsTrue(valid skip scenarios) is clear and well-documented.
141-244: Accept notification method correctly mirrors request notification.The parallel structure with appropriate differences:
- Uses
friend_activitysetting (vsfriend_requests)- Metadata uses
requester_id/accepter_id(vssender_id/recipient_id)- Notification targets the original requester
Good design consistency.
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.