From f012e7acb081eb534c37a7eaaaf4edbcfcfc8da8 Mon Sep 17 00:00:00 2001 From: Hiroshi Nishio Date: Sun, 22 Feb 2026 18:06:40 -0800 Subject: [PATCH] Set created_by and updated_by on all database insert/update/upsert operations Audited every Supabase operation and fixed 7 missing audit trail fields: - insert_usage: added created_by (user_id:user_name format) - insert_installation: added created_by - upsert_user: fixed created_by format from str(user_id) to user_id:user_name - update_issue_url: added updated_by parameter - update_stripe_customer_id: added updated_by parameter - clear_old_content: added updated_by="system" - chat_with_claude/insert_llm_request: made created_by required and passed through --- services/chat_with_agent.py | 1 + services/claude/chat_with_claude.py | 2 + services/claude/test_chat_with_claude.py | 4 + .../coverages/test_update_issue_url.py | 19 +++- .../supabase/coverages/update_issue_url.py | 8 +- services/supabase/create_user_request.py | 1 + .../installations/insert_installation.py | 3 + .../installations/test_insert_installation.py | 35 ++++++- .../llm_requests/clear_old_content.py | 2 +- .../llm_requests/insert_llm_request.py | 2 +- .../llm_requests/test_clear_old_content.py | 4 +- .../llm_requests/test_insert_llm_request.py | 1 + .../owners/test_update_stripe_customer_id.py | 93 +++++++++++-------- .../owners/update_stripe_customer_id.py | 8 +- services/supabase/test_create_user_request.py | 1 + services/supabase/usage/insert_usage.py | 3 + services/supabase/usage/test_insert_usage.py | 8 ++ services/supabase/users/test_upsert_user.py | 6 +- services/supabase/users/upsert_user.py | 2 +- services/test_chat_with_agent.py | 40 ++++---- services/webhook/handle_installation.py | 2 + services/webhook/new_pr_handler.py | 3 +- services/webhook/schedule_handler.py | 1 + services/webhook/setup_handler.py | 1 + services/webhook/test_handle_installation.py | 6 ++ services/webhook/test_new_pr_handler.py | 2 +- 26 files changed, 178 insertions(+), 80 deletions(-) diff --git a/services/chat_with_agent.py b/services/chat_with_agent.py index 310048ad1..d0015a3c2 100644 --- a/services/chat_with_agent.py +++ b/services/chat_with_agent.py @@ -73,6 +73,7 @@ async def chat_with_agent( tools=tools, model_id=current_model, usage_id=usage_id, + created_by=f"{base_args['sender_id']}:{base_args['sender_name']}", ) break diff --git a/services/claude/chat_with_claude.py b/services/claude/chat_with_claude.py index 3b7899e92..a7d08c6e9 100644 --- a/services/claude/chat_with_claude.py +++ b/services/claude/chat_with_claude.py @@ -37,6 +37,7 @@ def chat_with_claude( tools: list[ToolUnionParam], model_id: ClaudeModelId, usage_id: int, + created_by: str, ): # https://docs.anthropic.com/en/api/client-sdks # Apply message optimization functions to save tokens @@ -131,6 +132,7 @@ def chat_with_claude( output_message=assistant_message, output_tokens=token_output, response_time_ms=response_time_ms, + created_by=created_by, ) return ( diff --git a/services/claude/test_chat_with_claude.py b/services/claude/test_chat_with_claude.py index 948957e27..36bc0ac01 100644 --- a/services/claude/test_chat_with_claude.py +++ b/services/claude/test_chat_with_claude.py @@ -28,6 +28,7 @@ def test_chat_with_claude_success(mock_claude, mock_insert_llm_request): tools=tools, model_id=ClaudeModelId.SONNET_4_6, usage_id=123, + created_by="4:test-user", ) assistant_message, tool_calls, token_input, token_output = result @@ -84,6 +85,7 @@ def test_chat_with_claude_with_tool_use(mock_claude, mock_insert_llm_request): tools=tools, model_id=ClaudeModelId.SONNET_4_6, usage_id=456, + created_by="4:test-user", ) _, tool_calls, _, _ = result @@ -111,6 +113,7 @@ def test_chat_with_claude_no_usage_response(mock_claude, mock_insert_llm_request tools=[], model_id=ClaudeModelId.SONNET_4_6, usage_id=789, + created_by="4:test-user", ) _, _, _, token_output = result @@ -145,6 +148,7 @@ def test_chat_with_claude_calls_optimization_functions( tools=[], model_id=ClaudeModelId.SONNET_4_6, usage_id=101, + created_by="4:test-user", ) mock_remove_outdated_apply_diff_to_file_attempts_and_results.assert_called_once() diff --git a/services/supabase/coverages/test_update_issue_url.py b/services/supabase/coverages/test_update_issue_url.py index 3c3aefa7d..23a78636b 100644 --- a/services/supabase/coverages/test_update_issue_url.py +++ b/services/supabase/coverages/test_update_issue_url.py @@ -56,12 +56,13 @@ def test_update_issue_url_success(self, mock_supabase_success): repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert mock_supabase.table.assert_called_once_with("coverages") mock_table.update.assert_called_once_with( - {"github_issue_url": github_issue_url} + {"github_issue_url": github_issue_url, "updated_by": "system"} ) mock_table.eq.assert_any_call("repo_id", repo_id) mock_table.eq.assert_any_call("full_path", file_path) @@ -82,12 +83,13 @@ def test_update_issue_url_with_empty_response(self, mock_supabase_empty_response repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert mock_supabase.table.assert_called_once_with("coverages") mock_table.update.assert_called_once_with( - {"github_issue_url": github_issue_url} + {"github_issue_url": github_issue_url, "updated_by": "system"} ) mock_table.eq.assert_any_call("repo_id", repo_id) mock_table.eq.assert_any_call("full_path", file_path) @@ -108,6 +110,7 @@ def test_update_issue_url_with_zero_repo_id(self, mock_supabase_success): repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert @@ -128,6 +131,7 @@ def test_update_issue_url_with_negative_repo_id(self, mock_supabase_success): repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert @@ -148,6 +152,7 @@ def test_update_issue_url_with_empty_file_path(self, mock_supabase_success): repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert @@ -168,10 +173,13 @@ def test_update_issue_url_with_empty_github_url(self, mock_supabase_success): repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert - mock_table.update.assert_called_once_with({"github_issue_url": ""}) + mock_table.update.assert_called_once_with( + {"github_issue_url": "", "updated_by": "system"} + ) assert result == [ {"id": 1, "github_issue_url": "https://github.com/owner/repo/issues/123"} ] @@ -190,6 +198,7 @@ def test_update_issue_url_with_long_file_path(self, mock_supabase_success): repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert @@ -212,6 +221,7 @@ def test_update_issue_url_with_special_characters_in_path( repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert @@ -232,6 +242,7 @@ def test_update_issue_url_with_large_repo_id(self, mock_supabase_success): repo_id=repo_id, file_path=file_path, github_issue_url=github_issue_url, + updated_by="system", ) # Assert @@ -252,6 +263,7 @@ def test_update_issue_url_exception_handling(self): repo_id=123456, file_path="src/main.py", github_issue_url="https://github.com/owner/repo/issues/123", + updated_by="system", ) # Assert - The handle_exceptions decorator should return None on error @@ -268,6 +280,7 @@ def test_update_issue_url_method_chaining(self, mock_supabase_success): repo_id=123456, file_path="src/main.py", github_issue_url="https://github.com/owner/repo/issues/123", + updated_by="system", ) # Assert - Verify the method chaining sequence diff --git a/services/supabase/coverages/update_issue_url.py b/services/supabase/coverages/update_issue_url.py index 19981cb62..49b115c84 100644 --- a/services/supabase/coverages/update_issue_url.py +++ b/services/supabase/coverages/update_issue_url.py @@ -4,11 +4,15 @@ @handle_exceptions(default_return_value=None, raise_on_error=False) def update_issue_url( - owner_id: int, repo_id: int, file_path: str, github_issue_url: str + owner_id: int, + repo_id: int, + file_path: str, + github_issue_url: str, + updated_by: str, ): result = ( supabase.table("coverages") - .update({"github_issue_url": github_issue_url}) + .update({"github_issue_url": github_issue_url, "updated_by": updated_by}) .eq("owner_id", owner_id) .eq("repo_id", repo_id) .eq("full_path", file_path) diff --git a/services/supabase/create_user_request.py b/services/supabase/create_user_request.py index d661bb683..31a320865 100644 --- a/services/supabase/create_user_request.py +++ b/services/supabase/create_user_request.py @@ -33,6 +33,7 @@ def create_user_request( repo_name=repo_name, pr_number=pr_number, user_id=user_id, + user_name=user_name, installation_id=installation_id, source=source, trigger=trigger, diff --git a/services/supabase/installations/insert_installation.py b/services/supabase/installations/insert_installation.py index e1f8b1b61..c113377cd 100644 --- a/services/supabase/installations/insert_installation.py +++ b/services/supabase/installations/insert_installation.py @@ -8,6 +8,8 @@ def insert_installation( owner_id: int, owner_type: str, owner_name: str, + user_id: int, + user_name: str, ): supabase.table(table_name="installations").insert( json={ @@ -15,5 +17,6 @@ def insert_installation( "owner_id": owner_id, "owner_type": owner_type, "owner_name": owner_name, + "created_by": f"{user_id}:{user_name}", } ).execute() diff --git a/services/supabase/installations/test_insert_installation.py b/services/supabase/installations/test_insert_installation.py index fa9ac7cb4..bf44e44d3 100644 --- a/services/supabase/installations/test_insert_installation.py +++ b/services/supabase/installations/test_insert_installation.py @@ -36,6 +36,8 @@ def valid_installation_data(): "owner_id": TEST_OWNER_ID, "owner_type": TEST_OWNER_TYPE, "owner_name": TEST_OWNER_NAME, + "user_id": 11111, + "user_name": "test-sender", } @@ -70,6 +72,8 @@ def test_insert_installation_with_minimal_data(mock_supabase_client): "owner_id": 67890, "owner_type": "User", "owner_name": "test-user", + "user_id": 11111, + "user_name": "test-sender", } result = insert_installation(**minimal_data) @@ -94,6 +98,8 @@ def test_insert_installation_with_organization_owner(mock_supabase_client): "owner_id": 11111, "owner_type": "Organization", "owner_name": "test-org", + "user_id": 22222, + "user_name": "test-sender", } result = insert_installation(**org_data) @@ -125,6 +131,8 @@ def test_insert_installation_with_various_data_types( owner_id=owner_id, owner_type=owner_type, owner_name=owner_name, + user_id=11111, + user_name="test-sender", ) assert result is None @@ -145,6 +153,8 @@ def test_insert_installation_supabase_chain_calls(mock_supabase_client): owner_id=TEST_OWNER_ID, owner_type=TEST_OWNER_TYPE, owner_name=TEST_OWNER_NAME, + user_id=11111, + user_name="test-sender", ) # Verify the method chain: supabase.table().insert().execute() @@ -168,8 +178,15 @@ def test_insert_installation_function_signature(): sig = inspect.signature(insert_installation) # Assert parameter count and names - assert len(sig.parameters) == 4 - expected_params = ["installation_id", "owner_id", "owner_type", "owner_name"] + assert len(sig.parameters) == 6 + expected_params = [ + "installation_id", + "owner_id", + "owner_type", + "owner_name", + "user_id", + "user_name", + ] for param in expected_params: assert param in sig.parameters @@ -178,6 +195,8 @@ def test_insert_installation_function_signature(): assert sig.parameters["owner_id"].annotation is int assert sig.parameters["owner_type"].annotation is str assert sig.parameters["owner_name"].annotation is str + assert sig.parameters["user_id"].annotation is int + assert sig.parameters["user_name"].annotation is str def test_insert_installation_with_supabase_exception(): @@ -197,6 +216,8 @@ def test_insert_installation_with_supabase_exception(): owner_id=TEST_OWNER_ID, owner_type=TEST_OWNER_TYPE, owner_name=TEST_OWNER_NAME, + user_id=11111, + user_name="test-sender", ) @@ -225,6 +246,8 @@ def test_insert_installation_with_http_error(): owner_id=TEST_OWNER_ID, owner_type=TEST_OWNER_TYPE, owner_name=TEST_OWNER_NAME, + user_id=11111, + user_name="test-sender", ) @@ -246,6 +269,8 @@ def test_insert_installation_with_special_characters_in_owner_name( owner_id=TEST_OWNER_ID, owner_type=TEST_OWNER_TYPE, owner_name=owner_name, + user_id=11111, + user_name="test-sender", ) # Verify the special characters are preserved @@ -267,6 +292,8 @@ def test_insert_installation_with_large_ids(mock_supabase_client): owner_id=large_owner_id, owner_type=TEST_OWNER_TYPE, owner_name=TEST_OWNER_NAME, + user_id=11111, + user_name="test-sender", ) insert_call_args = mock_supabase_client.table.return_value.insert.call_args @@ -288,6 +315,8 @@ def test_insert_installation_with_empty_owner_name(mock_supabase_client): owner_id=TEST_OWNER_ID, owner_type=TEST_OWNER_TYPE, owner_name="", + user_id=11111, + user_name="test-sender", ) insert_call_args = mock_supabase_client.table.return_value.insert.call_args @@ -306,6 +335,8 @@ def test_insert_installation_with_different_owner_types(mock_supabase_client): owner_id=TEST_OWNER_ID, owner_type=owner_type, owner_name=TEST_OWNER_NAME, + user_id=11111, + user_name="test-sender", ) insert_call_args = mock_supabase_client.table.return_value.insert.call_args diff --git a/services/supabase/llm_requests/clear_old_content.py b/services/supabase/llm_requests/clear_old_content.py index 73380de2f..eeea7c013 100644 --- a/services/supabase/llm_requests/clear_old_content.py +++ b/services/supabase/llm_requests/clear_old_content.py @@ -27,7 +27,7 @@ def clear_old_content(retention_days: int = 14): ids = [row["id"] for row in batch.data] supabase.table("llm_requests").update( - {"input_content": "", "output_content": ""} + {"input_content": "", "output_content": "", "updated_by": "system"} ).in_("id", ids).execute() total_cleared += len(ids) diff --git a/services/supabase/llm_requests/insert_llm_request.py b/services/supabase/llm_requests/insert_llm_request.py index ba5947d8a..1f60bf63d 100644 --- a/services/supabase/llm_requests/insert_llm_request.py +++ b/services/supabase/llm_requests/insert_llm_request.py @@ -16,9 +16,9 @@ def insert_llm_request( input_tokens: int, output_message: MessageParam, output_tokens: int, + created_by: str, response_time_ms: int | None = None, error_message: str | None = None, - created_by: str | None = None, ): # Convert messages to JSON strings input_content = json.dumps(input_messages, ensure_ascii=False) diff --git a/services/supabase/llm_requests/test_clear_old_content.py b/services/supabase/llm_requests/test_clear_old_content.py index 917751a34..f8453122d 100644 --- a/services/supabase/llm_requests/test_clear_old_content.py +++ b/services/supabase/llm_requests/test_clear_old_content.py @@ -46,7 +46,9 @@ def test_clear_old_content_single_batch(mock_datetime, mock_supabase): mock_select.lt.assert_called_with("created_at", expected_cutoff) mock_lt_select.neq.assert_called_with("input_content", "") mock_neq.limit.assert_called_with(SUPABASE_BATCH_SIZE) - mock_table.update.assert_called_with({"input_content": "", "output_content": ""}) + mock_table.update.assert_called_with( + {"input_content": "", "output_content": "", "updated_by": "system"} + ) mock_update.in_.assert_called_with("id", [1, 2, 3]) diff --git a/services/supabase/llm_requests/test_insert_llm_request.py b/services/supabase/llm_requests/test_insert_llm_request.py index de04e54c9..7ea5582e0 100644 --- a/services/supabase/llm_requests/test_insert_llm_request.py +++ b/services/supabase/llm_requests/test_insert_llm_request.py @@ -71,6 +71,7 @@ def test_insert_llm_request_database_error(mock_supabase): input_tokens=10, output_message={"role": "assistant", "content": "response"}, output_tokens=5, + created_by="4:test-user", ) assert result is None diff --git a/services/supabase/owners/test_update_stripe_customer_id.py b/services/supabase/owners/test_update_stripe_customer_id.py index 964b834c2..3f7e20286 100644 --- a/services/supabase/owners/test_update_stripe_customer_id.py +++ b/services/supabase/owners/test_update_stripe_customer_id.py @@ -28,13 +28,13 @@ def test_update_stripe_customer_id_success(mock_supabase): """Test successful update of stripe customer ID.""" # Execute result = update_stripe_customer_id( - owner_id=123456, stripe_customer_id="cus_test123" + owner_id=123456, stripe_customer_id="cus_test123", updated_by="42:testuser" ) # Assert mock_supabase.table.assert_called_once_with("owners") mock_supabase.table().update.assert_called_once_with( - {"stripe_customer_id": "cus_test123"} + {"stripe_customer_id": "cus_test123", "updated_by": "42:testuser"} ) mock_supabase.table().update().eq.assert_called_once_with("owner_id", 123456) mock_supabase.table().update().eq().execute.assert_called_once() @@ -44,11 +44,15 @@ def test_update_stripe_customer_id_success(mock_supabase): def test_update_stripe_customer_id_with_empty_string(mock_supabase): """Test update with empty string stripe customer ID.""" # Execute - result = update_stripe_customer_id(owner_id=789, stripe_customer_id="") + result = update_stripe_customer_id( + owner_id=789, stripe_customer_id="", updated_by="1:user" + ) # Assert mock_supabase.table.assert_called_once_with("owners") - mock_supabase.table().update.assert_called_once_with({"stripe_customer_id": ""}) + mock_supabase.table().update.assert_called_once_with( + {"stripe_customer_id": "", "updated_by": "1:user"} + ) mock_supabase.table().update().eq.assert_called_once_with("owner_id", 789) mock_supabase.table().update().eq().execute.assert_called_once() assert result is None @@ -60,12 +64,12 @@ def test_update_stripe_customer_id_with_long_customer_id(mock_supabase): # Execute result = update_stripe_customer_id( - owner_id=456, stripe_customer_id=long_customer_id + owner_id=456, stripe_customer_id=long_customer_id, updated_by="1:user" ) # Assert mock_supabase.table().update.assert_called_once_with( - {"stripe_customer_id": long_customer_id} + {"stripe_customer_id": long_customer_id, "updated_by": "1:user"} ) mock_supabase.table().update().eq.assert_called_once_with("owner_id", 456) assert result is None @@ -77,12 +81,12 @@ def test_update_stripe_customer_id_with_special_characters(mock_supabase): # Execute result = update_stripe_customer_id( - owner_id=999, stripe_customer_id=special_customer_id + owner_id=999, stripe_customer_id=special_customer_id, updated_by="1:user" ) # Assert mock_supabase.table().update.assert_called_once_with( - {"stripe_customer_id": special_customer_id} + {"stripe_customer_id": special_customer_id, "updated_by": "1:user"} ) mock_supabase.table().update().eq.assert_called_once_with("owner_id", 999) assert result is None @@ -94,12 +98,12 @@ def test_update_stripe_customer_id_with_unicode_characters(mock_supabase): # Execute result = update_stripe_customer_id( - owner_id=111, stripe_customer_id=unicode_customer_id + owner_id=111, stripe_customer_id=unicode_customer_id, updated_by="1:user" ) # Assert mock_supabase.table().update.assert_called_once_with( - {"stripe_customer_id": unicode_customer_id} + {"stripe_customer_id": unicode_customer_id, "updated_by": "1:user"} ) mock_supabase.table().update().eq.assert_called_once_with("owner_id", 111) assert result is None @@ -108,7 +112,9 @@ def test_update_stripe_customer_id_with_unicode_characters(mock_supabase): def test_update_stripe_customer_id_with_zero_owner_id(mock_supabase): """Test update with zero owner ID.""" # Execute - result = update_stripe_customer_id(owner_id=0, stripe_customer_id="cus_zero123") + result = update_stripe_customer_id( + owner_id=0, stripe_customer_id="cus_zero123", updated_by="1:user" + ) # Assert mock_supabase.table().update().eq.assert_called_once_with("owner_id", 0) @@ -119,7 +125,7 @@ def test_update_stripe_customer_id_with_negative_owner_id(mock_supabase): """Test update with negative owner ID.""" # Execute result = update_stripe_customer_id( - owner_id=-1, stripe_customer_id="cus_negative123" + owner_id=-1, stripe_customer_id="cus_negative123", updated_by="1:user" ) # Assert @@ -133,7 +139,9 @@ def test_update_stripe_customer_id_with_large_owner_id(mock_supabase): # Execute result = update_stripe_customer_id( - owner_id=large_owner_id, stripe_customer_id="cus_large123" + owner_id=large_owner_id, + stripe_customer_id="cus_large123", + updated_by="1:user", ) # Assert @@ -151,7 +159,9 @@ def test_update_stripe_customer_id_supabase_exception_handled(mock_supabase): ) # Execute - should not raise exception due to handle_exceptions decorator - result = update_stripe_customer_id(owner_id=666, stripe_customer_id="cus_error123") + result = update_stripe_customer_id( + owner_id=666, stripe_customer_id="cus_error123", updated_by="1:user" + ) # Assert - should return None due to handle_exceptions decorator assert result is None @@ -161,7 +171,9 @@ def test_update_stripe_customer_id_supabase_exception_handled(mock_supabase): def test_update_stripe_customer_id_table_method_called_correctly(mock_supabase): """Test that the correct table name is used.""" # Execute - update_stripe_customer_id(owner_id=111, stripe_customer_id="cus_table123") + update_stripe_customer_id( + owner_id=111, stripe_customer_id="cus_table123", updated_by="1:user" + ) # Assert mock_supabase.table.assert_called_once_with("owners") @@ -170,7 +182,9 @@ def test_update_stripe_customer_id_table_method_called_correctly(mock_supabase): def test_update_stripe_customer_id_method_chaining(mock_supabase): """Test that the Supabase method chaining works correctly.""" # Execute - update_stripe_customer_id(owner_id=888, stripe_customer_id="cus_chain123") + update_stripe_customer_id( + owner_id=888, stripe_customer_id="cus_chain123", updated_by="1:user" + ) # Assert the complete chain mock_supabase.table.assert_called_once_with("owners") @@ -184,7 +198,7 @@ def test_update_stripe_customer_id_attribute_error_handled(mock_supabase): mock_supabase.table.return_value.update.return_value.eq.return_value.execute.side_effect = AttributeError( "Attribute error" ) - result = update_stripe_customer_id(7001, "cus_error123") + result = update_stripe_customer_id(7001, "cus_error123", "1:user") assert result is None @@ -193,7 +207,7 @@ def test_update_stripe_customer_id_key_error_handled(mock_supabase): mock_supabase.table.return_value.update.return_value.eq.return_value.execute.side_effect = KeyError( "Key error" ) - result = update_stripe_customer_id(7003, "cus_error123") + result = update_stripe_customer_id(7003, "cus_error123", "1:user") assert result is None @@ -202,7 +216,7 @@ def test_update_stripe_customer_id_type_error_handled(mock_supabase): mock_supabase.table.return_value.update.return_value.eq.return_value.execute.side_effect = TypeError( "Type error" ) - result = update_stripe_customer_id(7005, "cus_error123") + result = update_stripe_customer_id(7005, "cus_error123", "1:user") assert result is None @@ -211,20 +225,12 @@ def test_update_stripe_customer_id_generic_exception_handled(mock_supabase): mock_supabase.table.return_value.update.return_value.eq.return_value.execute.side_effect = Exception( "Generic error" ) - result = update_stripe_customer_id(7007, "cus_error123") + result = update_stripe_customer_id(7007, "cus_error123", "1:user") assert result is None def test_update_stripe_customer_id_decorator_configuration(): """Test that the handle_exceptions decorator is configured correctly.""" - # This test verifies the decorator configuration by checking the function attributes - # The decorator should be configured with default_return_value=None and raise_on_error=False - - # Import the function to check its decorator configuration - from services.supabase.owners.update_stripe_customer_id import ( - update_stripe_customer_id, - ) - # The function should have the decorator applied assert hasattr(update_stripe_customer_id, "__wrapped__") @@ -239,12 +245,14 @@ def test_update_stripe_customer_id_whitespace_handling(mock_supabase): # Execute result = update_stripe_customer_id( - owner_id=5001, stripe_customer_id=customer_id_with_spaces + owner_id=5001, + stripe_customer_id=customer_id_with_spaces, + updated_by="1:user", ) # Assert - whitespace should be preserved as-is mock_supabase.table().update.assert_called_once_with( - {"stripe_customer_id": customer_id_with_spaces} + {"stripe_customer_id": customer_id_with_spaces, "updated_by": "1:user"} ) assert result is None @@ -266,12 +274,14 @@ def test_update_stripe_customer_id_parametrized( """Test function with various parameter combinations using parametrize.""" # Execute result = update_stripe_customer_id( - owner_id=owner_id, stripe_customer_id=stripe_customer_id + owner_id=owner_id, + stripe_customer_id=stripe_customer_id, + updated_by="1:user", ) # Assert mock_supabase.table().update.assert_called_once_with( - {"stripe_customer_id": stripe_customer_id} + {"stripe_customer_id": stripe_customer_id, "updated_by": "1:user"} ) mock_supabase.table().update().eq.assert_called_once_with("owner_id", owner_id) assert result is None @@ -280,19 +290,16 @@ def test_update_stripe_customer_id_parametrized( def test_update_stripe_customer_id_function_signature(): """Test that the function signature matches expectations.""" import inspect - from services.supabase.owners.update_stripe_customer_id import ( - update_stripe_customer_id, - ) # Get function signature sig = inspect.signature(update_stripe_customer_id) params = list(sig.parameters.keys()) # Assert expected parameters are present - expected_params = ["owner_id", "stripe_customer_id"] + expected_params = ["owner_id", "stripe_customer_id", "updated_by"] assert params == expected_params - # Check that parameters don't have default values (both are required) + # Check that parameters don't have default values (all are required) for param_name in expected_params: param = sig.parameters[param_name] assert param.default == inspect.Parameter.empty @@ -302,7 +309,7 @@ def test_update_stripe_customer_id_return_value_on_success(mock_supabase): """Test that update_stripe_customer_id returns None on successful execution.""" # Execute result = update_stripe_customer_id( - owner_id=3001, stripe_customer_id="cus_return123" + owner_id=3001, stripe_customer_id="cus_return123", updated_by="1:user" ) # Assert - function should return None (implicit return) @@ -314,11 +321,17 @@ def test_update_stripe_customer_id_update_data_structure(mock_supabase): stripe_customer_id = "cus_structure123" # Execute - update_stripe_customer_id(owner_id=4001, stripe_customer_id=stripe_customer_id) + update_stripe_customer_id( + owner_id=4001, + stripe_customer_id=stripe_customer_id, + updated_by="42:testuser", + ) # Assert - verify the exact structure of the update data call_args = mock_supabase.table().update.call_args[0][0] assert isinstance(call_args, dict) - assert len(call_args) == 1 + assert len(call_args) == 2 assert "stripe_customer_id" in call_args assert call_args["stripe_customer_id"] == stripe_customer_id + assert "updated_by" in call_args + assert call_args["updated_by"] == "42:testuser" diff --git a/services/supabase/owners/update_stripe_customer_id.py b/services/supabase/owners/update_stripe_customer_id.py index 35121a741..597fee473 100644 --- a/services/supabase/owners/update_stripe_customer_id.py +++ b/services/supabase/owners/update_stripe_customer_id.py @@ -3,7 +3,7 @@ @handle_exceptions(default_return_value=None, raise_on_error=False) -def update_stripe_customer_id(owner_id: int, stripe_customer_id: str): - supabase.table("owners").update({"stripe_customer_id": stripe_customer_id}).eq( - "owner_id", owner_id - ).execute() +def update_stripe_customer_id(owner_id: int, stripe_customer_id: str, updated_by: str): + supabase.table("owners").update( + {"stripe_customer_id": stripe_customer_id, "updated_by": updated_by} + ).eq("owner_id", owner_id).execute() diff --git a/services/supabase/test_create_user_request.py b/services/supabase/test_create_user_request.py index e637d1bd1..df53f51d4 100644 --- a/services/supabase/test_create_user_request.py +++ b/services/supabase/test_create_user_request.py @@ -60,6 +60,7 @@ def test_create_user_request_returns_usage_id( repo_name="test_repo", pr_number=123, user_id=12345, + user_name="test_user", installation_id=67890, source="github", trigger="dashboard", diff --git a/services/supabase/usage/insert_usage.py b/services/supabase/usage/insert_usage.py index b9e4aadfd..d73858caf 100644 --- a/services/supabase/usage/insert_usage.py +++ b/services/supabase/usage/insert_usage.py @@ -14,6 +14,7 @@ def insert_usage( repo_name: str, pr_number: int, user_id: int, + user_name: str, installation_id: int, source: str, trigger: Trigger, @@ -21,6 +22,7 @@ def insert_usage( lambda_log_stream: str | None = None, lambda_request_id: str | None = None, ): + created_by = f"{user_id}:{user_name}" result = ( supabase.table(table_name="usage") .insert( @@ -38,6 +40,7 @@ def insert_usage( "pr_number": pr_number, "is_test_passed": False, "is_merged": False, + "created_by": created_by, "lambda_log_group": lambda_log_group, "lambda_log_stream": lambda_log_stream, "lambda_request_id": lambda_request_id, diff --git a/services/supabase/usage/test_insert_usage.py b/services/supabase/usage/test_insert_usage.py index 7b6970c5c..27bb9fb20 100644 --- a/services/supabase/usage/test_insert_usage.py +++ b/services/supabase/usage/test_insert_usage.py @@ -31,6 +31,7 @@ def test_insert_usage_success_with_all_parameters(): repo_name="test_repo", pr_number=3, user_id=4, + user_name="test_user", installation_id=5, source="test", trigger="dashboard", @@ -58,6 +59,7 @@ def test_insert_usage_success_minimal_parameters(): repo_name="test_repo", pr_number=3, user_id=4, + user_name="test_user", installation_id=5, source="test", trigger="dashboard", @@ -84,6 +86,7 @@ def test_insert_usage_with_zero_values(): repo_name="test_repo", pr_number=0, user_id=0, + user_name="test_user", installation_id=0, source="test", trigger="dashboard", @@ -118,6 +121,7 @@ def test_insert_usage_with_different_triggers(): repo_name="test_repo", pr_number=3, user_id=4, + user_name="test_user", installation_id=5, source="test", trigger=trigger, @@ -141,6 +145,7 @@ def test_insert_usage_with_exception(): repo_name="test_repo", pr_number=3, user_id=4, + user_name="test_user", installation_id=5, source="test", trigger="dashboard", @@ -171,6 +176,7 @@ def test_insert_usage_with_http_error(): repo_name="test_repo", pr_number=3, user_id=4, + user_name="test_user", installation_id=5, source="test", trigger="dashboard", @@ -194,6 +200,7 @@ def test_insert_usage_with_json_decode_error(): repo_name="test_repo", pr_number=3, user_id=4, + user_name="test_user", installation_id=5, source="test", trigger="dashboard", @@ -220,6 +227,7 @@ def test_insert_usage_execute_exception(): repo_name="test_repo", pr_number=3, user_id=4, + user_name="test_user", installation_id=5, source="test", trigger="dashboard", diff --git a/services/supabase/users/test_upsert_user.py b/services/supabase/users/test_upsert_user.py index 825e2aad0..37f270bc2 100644 --- a/services/supabase/users/test_upsert_user.py +++ b/services/supabase/users/test_upsert_user.py @@ -50,7 +50,7 @@ def test_upsert_user_with_valid_email(mock_supabase, mock_check_email_is_valid): "user_id": user_id, "user_name": user_name, "email": email, - "created_by": str(user_id), + "created_by": f"{user_id}:{user_name}", }, on_conflict="user_id", ) @@ -75,7 +75,7 @@ def test_upsert_user_with_invalid_email(mock_supabase, mock_check_email_is_valid json={ "user_id": user_id, "user_name": user_name, - "created_by": str(user_id), + "created_by": f"{user_id}:{user_name}", }, on_conflict="user_id", ) @@ -100,7 +100,7 @@ def test_upsert_user_with_none_email(mock_supabase, mock_check_email_is_valid): json={ "user_id": user_id, "user_name": user_name, - "created_by": str(user_id), + "created_by": f"{user_id}:{user_name}", }, on_conflict="user_id", ) diff --git a/services/supabase/users/upsert_user.py b/services/supabase/users/upsert_user.py index 786824d47..2ea15f355 100644 --- a/services/supabase/users/upsert_user.py +++ b/services/supabase/users/upsert_user.py @@ -14,7 +14,7 @@ def upsert_user(user_id: int, user_name: str, email: str | None): "user_id": user_id, "user_name": user_name, **({"email": email} if email else {}), - "created_by": str(user_id), # Because created_by is text + "created_by": f"{user_id}:{user_name}", }, on_conflict="user_id", ).execute() diff --git a/services/test_chat_with_agent.py b/services/test_chat_with_agent.py index 74ec69447..fb704b1ea 100644 --- a/services/test_chat_with_agent.py +++ b/services/test_chat_with_agent.py @@ -24,7 +24,7 @@ async def test_chat_with_agent_passes_usage_id_to_claude( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) await chat_with_agent( messages=[{"role": "user", "content": "test"}], @@ -55,7 +55,7 @@ async def test_chat_with_agent_returns_token_counts( 15, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) result = await chat_with_agent( messages=[{"role": "user", "content": "test"}], @@ -103,7 +103,7 @@ async def test_get_local_file_content_start_line_end_line_logging( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__getitem__.return_value = Mock(return_value="file content") @@ -163,7 +163,7 @@ async def test_delete_file_logging( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__getitem__.return_value = Mock( @@ -230,7 +230,7 @@ async def test_move_file_logging( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__getitem__.return_value = Mock( @@ -295,7 +295,7 @@ async def test_replace_remote_file_content_handles_new_content_arg_name( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_function = Mock(return_value="Content replaced successfully") @@ -344,11 +344,7 @@ async def test_unavailable_tool_sends_slack_notification( 10, ) - base_args = Mock() - base_args.get.side_effect = lambda key, default=None: { - "owner": "test-owner", - "repo": "test-repo", - }.get(key, default) + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user", "owner": "test-owner", "repo": "test-repo"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__contains__.return_value = False @@ -408,7 +404,7 @@ async def test_restrict_edit_to_target_test_file_only_blocks_non_target_test( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__contains__.return_value = True @@ -465,6 +461,8 @@ async def test_verify_task_is_complete_with_pr_changes_returns_is_completed_true "repo": "test-repo", "pr_number": 123, "token": "test-token", + "sender_id": 1, + "sender_name": "test-user", }, ) @@ -519,6 +517,8 @@ async def test_verify_task_is_complete_without_pr_changes_returns_is_completed_f "repo": "test-repo", "pr_number": 123, "token": "test-token", + "sender_id": 1, + "sender_name": "test-user", }, ) @@ -569,7 +569,7 @@ async def test_regular_tool_returns_is_completed_false( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__getitem__.return_value = Mock(return_value="file content") @@ -604,7 +604,7 @@ async def test_no_tool_call_returns_is_completed_false( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) result = await chat_with_agent( messages=[{"role": "user", "content": "test"}], @@ -652,7 +652,7 @@ async def test_file_write_result_success_includes_formatted_content( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__contains__.return_value = True @@ -718,7 +718,7 @@ async def test_file_write_result_failure_returns_message_only( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__contains__.return_value = True @@ -784,7 +784,7 @@ async def test_file_move_result_returns_message( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__contains__.return_value = True @@ -847,7 +847,7 @@ async def test_full_file_read_calls_replace_with_is_full_file_read_true( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__getitem__.return_value = Mock( @@ -908,7 +908,7 @@ async def test_partial_file_read_calls_replace_with_is_full_file_read_false( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: mock_tools.__getitem__.return_value = Mock( @@ -980,7 +980,7 @@ async def test_multiple_parallel_tool_calls( 10, ) - base_args = Mock() + base_args = cast(BaseArgs, {"sender_id": 1, "sender_name": "test-user"}) with patch("services.chat_with_agent.tools_to_call") as mock_tools: call_count = 0 diff --git a/services/webhook/handle_installation.py b/services/webhook/handle_installation.py index 63d2e7ce6..590de9b71 100644 --- a/services/webhook/handle_installation.py +++ b/services/webhook/handle_installation.py @@ -53,6 +53,8 @@ async def handle_installation_created(payload: InstallationPayload): owner_id=owner_id, owner_type=owner_type, owner_name=owner_name, + user_id=user_id, + user_name=sender_name, ) upsert_user(user_id=user_id, user_name=sender_name, email=email) diff --git a/services/webhook/new_pr_handler.py b/services/webhook/new_pr_handler.py index 5c67fc45f..b5c6ff6a6 100644 --- a/services/webhook/new_pr_handler.py +++ b/services/webhook/new_pr_handler.py @@ -162,7 +162,8 @@ async def handle_new_pr( user_name=sender_name if sender_name else "unknown", ) if stripe_customer_id: - update_stripe_customer_id(owner_id, stripe_customer_id) + updated_by = f"{sender_id}:{sender_name}" if sender_id else "system" + update_stripe_customer_id(owner_id, stripe_customer_id, updated_by) # Now check availability (stripe_customer_id will exist or be None if creation failed) availability_status = check_availability( diff --git a/services/webhook/schedule_handler.py b/services/webhook/schedule_handler.py index 49ca950fd..0d8ccee05 100644 --- a/services/webhook/schedule_handler.py +++ b/services/webhook/schedule_handler.py @@ -404,6 +404,7 @@ def schedule_handler(event: EventBridgeSchedulerEvent): repo_id=repo_id, file_path=target_path, github_issue_url=pr_url, + updated_by=user_name, ) msg = f"created PR for {target_path}: {pr_url}" diff --git a/services/webhook/setup_handler.py b/services/webhook/setup_handler.py index d5d0ed0d1..0cf760fdb 100644 --- a/services/webhook/setup_handler.py +++ b/services/webhook/setup_handler.py @@ -139,6 +139,7 @@ async def setup_handler( repo_name=repo_name, pr_number=pr_number, user_id=0, + user_name=sender_name, installation_id=installation_id, source="setup_handler", trigger="setup", diff --git a/services/webhook/test_handle_installation.py b/services/webhook/test_handle_installation.py index 0d2c01874..16f1b7084 100644 --- a/services/webhook/test_handle_installation.py +++ b/services/webhook/test_handle_installation.py @@ -204,6 +204,8 @@ async def test_handle_installation_created_new_owner_with_grant( owner_id=67890, owner_type="Organization", owner_name="test-owner", + user_id=11111, + user_name="test-sender", ) # Verify user upsert @@ -258,6 +260,8 @@ async def test_handle_installation_created_existing_owner_with_existing_grant( owner_id=67890, owner_type="Organization", owner_name="test-owner", + user_id=11111, + user_name="test-sender", ) # Verify user upsert still happens @@ -367,6 +371,8 @@ async def test_handle_installation_created_with_user_type_owner( owner_id=67890, owner_type="User", owner_name="test-owner", + user_id=11111, + user_name="test-sender", ) async def test_handle_installation_created_with_none_email( diff --git a/services/webhook/test_new_pr_handler.py b/services/webhook/test_new_pr_handler.py index 7ce8ab9e9..3658185bb 100644 --- a/services/webhook/test_new_pr_handler.py +++ b/services/webhook/test_new_pr_handler.py @@ -180,7 +180,7 @@ async def test_stripe_customer_id_update( trigger="dashboard", ) - mock_update_stripe.assert_called_once_with(456, "cus_new123") + mock_update_stripe.assert_called_once_with(456, "cus_new123", "888:test_sender") @pytest.mark.asyncio