Skip to content

Conversation

@bokelley
Copy link
Contributor

@bokelley bokelley commented Nov 6, 2025

Problem

The dashboard was incorrectly showing "Inventory Sync" as incomplete (❌ red X) for tenants that had synced inventory but hadn't created products yet.

Example: "The Weather Company" tenant showed inventory sync as incomplete, but the Inventory Management page showed 36,750 items synced on Nov 04, 2025.

Root Cause

The setup checklist service was checking the Product table instead of the GAMInventory table to determine if inventory was synced. Since products are created AFTER inventory sync, this created a false negative.

Solution

Changed the inventory sync check in SetupChecklistService._check_critical_tasks() to query the GAMInventory table, which stores the actual synced ad units, placements, and targeting options from the ad server.

Changes

  • ✅ Updated src/services/setup_checklist_service.py to query GAMInventory instead of Product
  • ✅ Improved status details to show exact inventory count (e.g., "36,750 inventory items synced")
  • ✅ Updated test fixtures to include GAMInventory records
  • ✅ Added unit tests (tests/unit/test_inventory_sync_status.py)
  • ✅ Kept Products check as separate, independent task

Impact

  • Tenants with synced inventory will now correctly show inventory sync as complete ✅
  • Inventory sync status is now independent of whether products have been created
  • The Weather Company (and similar tenants) will see correct status immediately after deployment

Testing

  • ✅ Unit tests pass (2 new tests in test_inventory_sync_status.py)
  • ✅ Integration test fixtures updated with GAMInventory records
  • ✅ All pre-commit hooks pass
  • ✅ No data migration required

Before

Critical Tasks:
  ❌ Inventory Sync - (but actually 36,750 items synced)
  ❌ Products - No products created

After

Critical Tasks:
  ✅ Inventory Sync - 36,750 inventory items synced
  ❌ Products - No products created

🤖 Generated with Claude Code

bokelley and others added 6 commits November 6, 2025 05:31
…roducts

The dashboard was incorrectly showing "Inventory Sync" as incomplete for
tenants that had synced inventory but hadn't created products yet (e.g.,
"The Weather Company" with 36,750 synced items).

Root cause: Setup checklist was checking Product table instead of
GAMInventory table to determine sync status.

Changes:
- Update setup_checklist_service.py to query GAMInventory table
- Show detailed inventory count in status (e.g., "36,750 inventory items synced")
- Update test fixtures to include GAMInventory records
- Add unit tests verifying the fix

Impact: Tenants with synced inventory will now correctly show inventory
sync as complete, independent of whether products have been created.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
E2E tests were failing because the CI test data setup didn't create any
GAMInventory records, which caused the inventory sync status check to
fail after we fixed it to properly check GAMInventory instead of Products.

Changes:
- Add GAMInventory import to init_database_ci.py
- Create 4 sample inventory items (2 ad units, 1 placement, 1 targeting key)
- Check for existing inventory before creating to handle race conditions

This ensures E2E tests pass the inventory sync requirement in the setup
checklist.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Integration tests were failing because the sample_tenant fixture didn't
create any GAMInventory records, causing inventory sync check to fail.

Changes:
- Add GAMInventory import to conftest.py
- Create 2 sample inventory items (ad unit + placement) in sample_tenant
- Matches the fix already applied to E2E tests and CI database init

This ensures integration tests (like test_mcp_tool_roundtrip_minimal) pass
the inventory sync requirement in the setup checklist.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Integration V2 tests were failing (21 failures) because test fixtures
didn't create GAMInventory records, causing inventory sync check to fail.

Changes:
- Add GAMInventory import to integration_v2/conftest.py
- Update sample_tenant fixture to create 2 inventory items
- Update add_required_setup_data() helper to create GAMInventory
- Added to docstring (6th item in the setup requirements)

This fixes all 21 failing integration_v2 tests including:
- test_a2a_error_responses.py (3 tests)
- test_a2a_skill_invocation.py (1 test)
- test_create_media_buy_v24.py (5 tests)
- test_error_paths.py (4 tests)
- test_mcp_endpoints_comprehensive.py (1 test)
- test_minimum_spend_validation.py (7 tests)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
The test_minimum_spend_validation tests were still failing (7 failures)
because they use a custom setup_test_data fixture that creates its own
tenant without GAMInventory records.

Changes:
- Add GAMInventory import to test_minimum_spend_validation.py
- Create 2 inventory items in setup_test_data fixture
- Fixes all 7 remaining test failures

This is the final piece - all other test files were already fixed in
previous commits by updating the shared fixtures in conftest.py files.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
CRITICAL FIX: test_tenant_with_data is used by 144 tests across
integration and integration_v2 suites. This fixture was only creating
a bare Tenant without any required setup data.

Changes:
- tests/integration/conftest.py: Updated test_tenant_with_data to add
  all required setup data (CurrencyLimit, PropertyTag, AuthorizedProperty,
  Principal, GAMInventory)
- tests/integration_v2/conftest.py: Updated test_tenant_with_data to use
  add_required_setup_data() helper (includes GAMInventory)

Impact: This prevents 144 potential test failures that would have occurred
when tests using this fixture check inventory sync status.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@bokelley bokelley force-pushed the fix-inventory-sync-status branch from 54796e1 to fd85dc5 Compare November 6, 2025 13:55
bokelley and others added 4 commits November 6, 2025 08:56
Changed AuthorizedProperty name from 'Test Property' to 'Fixture Default
Property' in test fixtures to avoid conflicts with tests that create their
own properties and assert on unique names.

Fixes:
- test_authorized_properties_no_duplicates_with_tags: Was seeing 2 properties
  (1 from fixture + 1 from test) instead of 1
- test_analyze_ad_server: May have been affected by property name collision

Changes:
- tests/integration/conftest.py: Updated test_tenant_with_data fixture
- tests/integration_v2/conftest.py: Updated sample_tenant and
  add_required_setup_data() helper

Root cause: Our enhanced fixtures now create complete tenant data, but some
tests expect to be the only ones creating specific data with specific names.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Update cached AdCP schemas to match latest from adcontextprotocol.org:
- Updated schemas/v1/_schemas_v1_core_product_json.json
- Updated schemas/v1/_schemas_v1_core_format_json.json

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
The analyze_ad_server_inventory endpoint was trying to instantiate an
adapter but never used it - the function just returns mock data. Comment
out the adapter instantiation for now until we implement actual discovery.

This should fix the 500 error in test_analyze_ad_server.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@bokelley bokelley merged commit 193e87d into main Nov 6, 2025
10 checks passed
danf-newton pushed a commit to Newton-Research-Inc/salesagent that referenced this pull request Nov 24, 2025
…roducts (adcontextprotocol#708)

* fix: inventory sync status now checks GAMInventory table instead of Products

The dashboard was incorrectly showing "Inventory Sync" as incomplete for
tenants that had synced inventory but hadn't created products yet (e.g.,
"The Weather Company" with 36,750 synced items).

Root cause: Setup checklist was checking Product table instead of
GAMInventory table to determine sync status.

Changes:
- Update setup_checklist_service.py to query GAMInventory table
- Show detailed inventory count in status (e.g., "36,750 inventory items synced")
- Update test fixtures to include GAMInventory records
- Add unit tests verifying the fix

Impact: Tenants with synced inventory will now correctly show inventory
sync as complete, independent of whether products have been created.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add GAMInventory creation to CI database initialization

E2E tests were failing because the CI test data setup didn't create any
GAMInventory records, which caused the inventory sync status check to
fail after we fixed it to properly check GAMInventory instead of Products.

Changes:
- Add GAMInventory import to init_database_ci.py
- Create 4 sample inventory items (2 ad units, 1 placement, 1 targeting key)
- Check for existing inventory before creating to handle race conditions

This ensures E2E tests pass the inventory sync requirement in the setup
checklist.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add GAMInventory to integration test sample_tenant fixture

Integration tests were failing because the sample_tenant fixture didn't
create any GAMInventory records, causing inventory sync check to fail.

Changes:
- Add GAMInventory import to conftest.py
- Create 2 sample inventory items (ad unit + placement) in sample_tenant
- Matches the fix already applied to E2E tests and CI database init

This ensures integration tests (like test_mcp_tool_roundtrip_minimal) pass
the inventory sync requirement in the setup checklist.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add GAMInventory to integration_v2 test fixtures

Integration V2 tests were failing (21 failures) because test fixtures
didn't create GAMInventory records, causing inventory sync check to fail.

Changes:
- Add GAMInventory import to integration_v2/conftest.py
- Update sample_tenant fixture to create 2 inventory items
- Update add_required_setup_data() helper to create GAMInventory
- Added to docstring (6th item in the setup requirements)

This fixes all 21 failing integration_v2 tests including:
- test_a2a_error_responses.py (3 tests)
- test_a2a_skill_invocation.py (1 test)
- test_create_media_buy_v24.py (5 tests)
- test_error_paths.py (4 tests)
- test_mcp_endpoints_comprehensive.py (1 test)
- test_minimum_spend_validation.py (7 tests)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add GAMInventory to test_minimum_spend_validation fixture

The test_minimum_spend_validation tests were still failing (7 failures)
because they use a custom setup_test_data fixture that creates its own
tenant without GAMInventory records.

Changes:
- Add GAMInventory import to test_minimum_spend_validation.py
- Create 2 inventory items in setup_test_data fixture
- Fixes all 7 remaining test failures

This is the final piece - all other test files were already fixed in
previous commits by updating the shared fixtures in conftest.py files.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add GAMInventory to test_tenant_with_data fixtures (CRITICAL)

CRITICAL FIX: test_tenant_with_data is used by 144 tests across
integration and integration_v2 suites. This fixture was only creating
a bare Tenant without any required setup data.

Changes:
- tests/integration/conftest.py: Updated test_tenant_with_data to add
  all required setup data (CurrencyLimit, PropertyTag, AuthorizedProperty,
  Principal, GAMInventory)
- tests/integration_v2/conftest.py: Updated test_tenant_with_data to use
  add_required_setup_data() helper (includes GAMInventory)

Impact: This prevents 144 potential test failures that would have occurred
when tests using this fixture check inventory sync status.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: use unique names in test fixtures to avoid conflicts

Changed AuthorizedProperty name from 'Test Property' to 'Fixture Default
Property' in test fixtures to avoid conflicts with tests that create their
own properties and assert on unique names.

Fixes:
- test_authorized_properties_no_duplicates_with_tags: Was seeing 2 properties
  (1 from fixture + 1 from test) instead of 1
- test_analyze_ad_server: May have been affected by property name collision

Changes:
- tests/integration/conftest.py: Updated test_tenant_with_data fixture
- tests/integration_v2/conftest.py: Updated sample_tenant and
  add_required_setup_data() helper

Root cause: Our enhanced fixtures now create complete tenant data, but some
tests expect to be the only ones creating specific data with specific names.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* chore: update AdCP schemas to latest from registry

Update cached AdCP schemas to match latest from adcontextprotocol.org:
- Updated schemas/v1/_schemas_v1_core_product_json.json
- Updated schemas/v1/_schemas_v1_core_format_json.json

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: comment out unused adapter instantiation in analyze_ad_server

The analyze_ad_server_inventory endpoint was trying to instantiate an
adapter but never used it - the function just returns mock data. Comment
out the adapter instantiation for now until we implement actual discovery.

This should fix the 500 error in test_analyze_ad_server.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants