Added additional facebook data extracted from webhook#7
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| logger.info("Processing location for user %s", sender_id) | ||
| coords = location.get("coordinates", {}) or {} | ||
| lat = coords.get("lat") | ||
| long_val = coords.get("long") or coords.get("lng") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This pull request adds functionality to extract and store user profile information from Facebook webhooks, including user names, location data, profile pictures, locale, and timezone. The collected data is then used to personalize AI agent responses by incorporating user context into the system prompts.
Changes:
- Added database schema and repository functions for storing user profiles linked to Facebook sender IDs
- Implemented Facebook Graph API integration to fetch basic user information (no consent required)
- Enhanced webhook handlers to process both text messages and location sharing events
- Extended AI agent service to render dynamic template blocks based on available user context
- Added comprehensive test coverage for new user profile models, repository operations, and webhook handlers
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
migrations/005_user_profiles.sql |
New database schema for user profiles with unique sender_id constraint, location fields, and triggers for interaction tracking |
src/models/user_models.py |
Pydantic models for user profiles including create, update, and Facebook API response models |
src/models/agent_models.py |
Added user_name and user_location fields to AgentContext |
src/db/repository.py |
Repository functions for CRUD operations on user profiles with upsert support |
src/services/facebook_service.py |
New get_user_info function to fetch user data from Facebook Graph API |
src/services/agent_service.py |
Template rendering logic for conditional user context blocks using regex |
src/api/webhook.py |
Enhanced webhook processing to fetch/store user profiles, handle location sharing, and add random name personalization |
prompts/agent_system_instructions.md |
Added conditional template blocks for user name and location context |
tests/unit/test_webhook.py |
Comprehensive tests for message and location processing handlers |
tests/unit/test_repository.py |
Tests for user profile repository functions |
tests/unit/test_models.py |
Tests for user profile Pydantic models |
tests/unit/test_facebook_service.py |
Tests for Facebook Graph API user info fetching |
Comments suppressed due to low confidence (3)
tests/unit/test_facebook_service.py:39
- This import of module json is redundant, as it was previously imported on line 3.
import json
tests/unit/test_facebook_service.py:61
- This import of module json is redundant, as it was previously imported on line 3.
import json
tests/unit/test_facebook_service.py:159
- This import of module json is redundant, as it was previously imported on line 3.
import json
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def test_process_message_new_user_fetches_profile( | ||
| self, | ||
| mock_get_bot, | ||
| mock_upsert, | ||
| mock_get_info, | ||
| mock_get_profile, | ||
| mock_get_ref, | ||
| mock_agent_cls, | ||
| mock_send, | ||
| mock_save, | ||
| mock_bot_config, | ||
| mock_ref_doc, | ||
| mock_user_profile, | ||
| ): |
There was a problem hiding this comment.
Missing test coverage for the random name personalization logic (lines 136-138 in webhook.py). This feature randomly adds "Hi {user_name}!" prefix 20% of the time when confidence is high, but there are no tests verifying this behavior works correctly or that the randomness is seeded/controlled properly.
| async def test_process_message_new_user_fetches_profile( | ||
| self, | ||
| mock_get_bot, | ||
| mock_upsert, | ||
| mock_get_info, | ||
| mock_get_profile, | ||
| mock_get_ref, | ||
| mock_agent_cls, | ||
| mock_send, | ||
| mock_save, | ||
| mock_bot_config, | ||
| mock_ref_doc, | ||
| mock_user_profile, | ||
| ): |
There was a problem hiding this comment.
Missing test coverage for the scenario where get_user_info fails (returns None) after finding no existing user profile. The code in webhook.py handles this case by not creating a profile, but the test at line 112-125 assumes get_user_info always succeeds. Add a test case where get_user_info returns None to verify the system still functions correctly.
| async def test_process_location_update_fails_no_ack( | ||
| self, mock_update, mock_get_bot, mock_send | ||
| ): |
There was a problem hiding this comment.
Missing test coverage for the error handling path when location update fails. The test test_process_location_update_fails_no_ack verifies that no acknowledgment is sent when update fails, but there's no test for what happens when update_user_profile raises an exception (caught by the try-except at line 175).
| if deps.user_name: | ||
| prompt = re.sub( | ||
| r"{% if user_name %}\s*(.*?)\s*{% endif %}", | ||
| lambda m: m.group(1).replace("{{ user_name }}", deps.user_name), | ||
| prompt, | ||
| count=1, | ||
| flags=re.DOTALL, | ||
| ) | ||
| else: | ||
| prompt = re.sub( | ||
| r"{% if user_name %}.*?{% endif %}", | ||
| "", | ||
| prompt, | ||
| count=1, | ||
| flags=re.DOTALL, | ||
| ) | ||
| if deps.user_location: | ||
| prompt = re.sub( | ||
| r"{% if user_location %}\s*(.*?)\s*{% endif %}", | ||
| lambda m: m.group(1).replace("{{ user_location }}", deps.user_location), | ||
| prompt, | ||
| count=1, | ||
| flags=re.DOTALL, | ||
| ) | ||
| else: | ||
| prompt = re.sub( | ||
| r"{% if user_location %}.*?{% endif %}", | ||
| "", | ||
| prompt, | ||
| count=1, | ||
| flags=re.DOTALL, | ||
| ) |
There was a problem hiding this comment.
Missing test coverage for the template rendering logic with user context. The tests in test_webhook.py verify that user_name and user_location are passed to the agent, but there are no tests that verify the conditional template blocks ({% if user_name %}, {% if user_location %}) in agent_service.py are correctly rendered with actual values or stripped when values are None.
| update user_profiles | ||
| set | ||
| last_interaction_at = now(), | ||
| total_messages = total_messages + 1 | ||
| where sender_id = new.sender_id; |
There was a problem hiding this comment.
The trigger function update_user_interaction() updates user_profiles by sender_id from the inserted message_history row, but if the user_profile doesn't exist (e.g., if a message was saved before the profile was created), this update will silently do nothing. While this may not cause errors, it could result in inaccurate total_messages count. Consider adding error handling or ensuring user profiles are always created before message history is saved.
| update user_profiles | |
| set | |
| last_interaction_at = now(), | |
| total_messages = total_messages + 1 | |
| where sender_id = new.sender_id; | |
| -- Ensure a user_profile exists for this sender and keep interaction metadata accurate. | |
| insert into user_profiles ( | |
| sender_id, | |
| page_id, | |
| first_interaction_at, | |
| last_interaction_at, | |
| total_messages | |
| ) | |
| values ( | |
| new.sender_id, | |
| new.page_id, | |
| now(), | |
| now(), | |
| 1 | |
| ) | |
| on conflict (sender_id) do update | |
| set | |
| last_interaction_at = now(), | |
| total_messages = user_profiles.total_messages + 1; |
| def upsert_user_profile(profile: UserProfileCreate) -> str | None: | ||
| """ | ||
| Create or update user profile (upsert on sender_id). | ||
|
|
||
| Returns: | ||
| User profile ID or None on error | ||
| """ | ||
| try: | ||
| client = get_supabase_client() | ||
| data = profile.model_dump(exclude_none=True) | ||
| result = ( | ||
| client.table("user_profiles") | ||
| .upsert(data, on_conflict="sender_id") | ||
| .execute() |
There was a problem hiding this comment.
The upsert operation uses on_conflict="sender_id", but this means that if a user interacts with a different page_id, their profile will be updated with the new page_id, potentially losing the association with the original page. This could cause issues in multi-page scenarios where the same user interacts with multiple Facebook pages managed by this system.
|
|
||
| import logging | ||
| from fastapi import APIRouter, Request, Response, BackgroundTasks | ||
| import random |
There was a problem hiding this comment.
The random module is used for personalization logic but is not seeded, which could make testing difficult. For production use with non-deterministic behavior this is fine, but consider documenting this as non-deterministic behavior or providing a way to seed the random generator for testing purposes.
| updates = UserProfileUpdate( | ||
| location_lat=float(lat), | ||
| location_long=float(long_val), |
There was a problem hiding this comment.
The float() conversion on lines 188-189 could raise a ValueError if lat or long_val contain non-numeric values. While the outer try-except will catch this, it would log a generic error without specific context about invalid coordinate data. Consider adding specific validation or a more descriptive error message for invalid coordinate types.
| updates = UserProfileUpdate( | |
| location_lat=float(lat), | |
| location_long=float(long_val), | |
| try: | |
| lat_float = float(lat) | |
| long_float = float(long_val) | |
| except (TypeError, ValueError): | |
| logger.warning( | |
| "Invalid coordinate types for user %s: lat=%r, long=%r", | |
| sender_id, | |
| lat, | |
| long_val, | |
| ) | |
| return | |
| updates = UserProfileUpdate( | |
| location_lat=lat_float, | |
| location_long=long_float, |
| updates = UserProfileUpdate( | ||
| location_lat=float(lat), | ||
| location_long=float(long_val), | ||
| location_title=title, | ||
| location_address=address, | ||
| ) | ||
| success = update_user_profile(sender_id, page_id, updates) |
There was a problem hiding this comment.
The function update_user_profile() will attempt to update a user profile that may not exist in the database. If a user shares their location without having sent a message first (which would create their profile), this update will silently fail and return False. Consider checking if the profile exists first, or using upsert_user_profile with a UserProfileCreate object that includes the location data.
| updates = UserProfileUpdate( | |
| location_lat=float(lat), | |
| location_long=float(long_val), | |
| location_title=title, | |
| location_address=address, | |
| ) | |
| success = update_user_profile(sender_id, page_id, updates) | |
| # Ensure a profile exists before attempting to update; if none exists, | |
| # create or upsert one with the provided location data. | |
| profile = get_user_profile(sender_id, page_id) | |
| if profile is not None: | |
| updates = UserProfileUpdate( | |
| location_lat=float(lat), | |
| location_long=float(long_val), | |
| location_title=title, | |
| location_address=address, | |
| ) | |
| success = update_user_profile(sender_id, page_id, updates) | |
| else: | |
| create_data = UserProfileCreate( | |
| user_id=sender_id, | |
| page_id=page_id, | |
| location_lat=float(lat), | |
| location_long=float(long_val), | |
| location_title=title, | |
| location_address=address, | |
| ) | |
| success = upsert_user_profile(create_data) |
| def get_user_profile(sender_id: str, page_id: str) -> dict | None: | ||
| """ | ||
| Get user profile by sender_id (unique per user). | ||
|
|
||
| page_id is accepted for API consistency but lookup is by sender_id only. | ||
|
|
||
| Returns: | ||
| User profile dict or None if not found | ||
| """ | ||
| try: | ||
| client = get_supabase_client() | ||
| result = ( | ||
| client.table("user_profiles") | ||
| .select("*") | ||
| .eq("sender_id", sender_id) | ||
| .execute() |
There was a problem hiding this comment.
According to the schema in migrations/005_user_profiles.sql, sender_id has a unique constraint. However, the function comment states "page_id is accepted for API consistency but lookup is by sender_id only." This design creates a potential issue: if a user (sender_id) interacts with multiple pages, only their first interaction will be stored. Consider whether sender_id should be unique per page (composite unique constraint on sender_id, page_id) or if this is intentional behavior.
jreakin
changed the base branch from
test-messaging-to-supa-integration
to
graphite-base/7
jreakin
force-pushed
the
facebook-user-input-addition
branch
from
3c8f5c7 to
1f2b464
Compare
No description provided.