Add product details, measurement providers, and UI enhancements #720
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ements - Add product detail fields (delivery measurement, product image) per AdCP spec - Implement tenant-level measurement provider configuration - Add markdown rendering support for product descriptions - Rename add/edit product templates for adapter clarity (GAM vs Mock) - Remove Description column from product list view to improve layout - Add database migrations for measurement_providers and product details - Improve form handling with better validation and UI organization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add database schema for dynamic products (is_dynamic, signals_agent_ids, etc.) - Implement variant generation service with signal querying - Add admin UI for configuring dynamic product templates - Integrate variant generation into get_products workflow - Add visual indicators for templates and variants in product list - Support per-product expiration configuration (variant_ttl_days) - Fix mypy errors in xandr.py and signals.py for new Product fields Dynamic products allow templates to generate variants based on signals agents. Variants are single-use, inherit all template settings, and include signal-based targeting that is transparent to buyers.
- Replace placeholder with actual signals agent query using SignalsAgentRegistry - Convert database SignalsAgent model to dataclass for registry compatibility - Parse auth credentials (type + credentials) from database model - Use _get_signals_from_agent to query specific configured agents - Handle async/sync bridge with new event loop - Proper error handling and logging for signal queries Dynamic products now fully integrate with the signals agent infrastructure, querying signals via MCP using the existing registry and AdCP library.
…ti-agent queries Address code review comments: 1. Use singleton SignalsAgentRegistry (no per-call DB queries or registry creation) 2. Multi-agent query in single call (registry already handles this) 3. Specify deployment per AdCP spec (deliver_to with our agent_url) 4. Extract activation key for our specific deployment only 5. Remove redundant query_signals_agent function Changes: - Use get_signals_agent_registry() singleton (not new SignalsAgentRegistry()) - Call registry.get_signals() once for all tenant agents (multi-agent built-in) - Pass our_agent_url (from tenant.virtual_host) for deployment matching - extract_activation_key() now filters by our deployment destination - Removed per-agent database queries and registry instantiation - Added deployment specification in context (deliver_to.destinations) Performance improvements: - One registry instance (singleton pattern) - One multi-agent query (not per-agent loops) - No per-call database queries for agent configuration - Registry handles auth, MCP client pooling, and multi-agent orchestration
Countries filtering will be added later when needed. For now, just pass the deployment destination (our agent_url) to the signals agent.
Resolved conflicts in schema metadata files: - schemas/v1/_schemas_v1_core_format_json.json.meta - schemas/v1/_schemas_v1_core_product_json.json.meta - schemas/v1/index.json.meta All conflicts were timestamp/etag updates from schema downloads. Used main's versions (more recent timestamps). Merged changes from main: - fix: require authentication for sync_creatives and update_media_buy (#721) - fix: signals agent test endpoint async handling (#718) - fix: remove inventory sync requirement for mock adapter (#719) - fix: remove /a2a suffix from A2A endpoint URLs and add name field to configs - Migrate agent registries to adcp v1.0.1 library (#712)
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Core Features: - Products can be marked as dynamic with configurable variant templates - Support for 'all agents' or specific signals agents selection - Dynamic variant generation from signals agent responses - Proper async context manager and event loop management Database Changes: - Add is_dynamic, signals_agent_ids columns to products - Add variant_name_template, variant_description_template columns - Migration: 149ad85edb6f Implementation: - DynamicProductsService generates variants with activation keys - ADCPMultiAgentClient properly managed with async context manager - Accept brand_manifest.url in get_products validation - UI with multi-select for signals agents and template configuration Fixes: - Fix async event loop and connection management - Fix test_connection to support async context manager - Handle signals_agent_ids=None (all agents) correctly - Improve dynamic products UI warnings and validation
bokelley
force-pushed
the
product-detail-fields
branch
from
736ac34 to
a667180
Compare
…manager - Replace Mock() with AsyncMock() in three test locations - Add __aenter__ and __aexit__ support for async with protocol - Fixes 'Mock object does not support asynchronous context manager protocol' error
bokelley
added a commit
that referenced
this pull request
Nov 9, 2025
The 'Select All Matching' button was not appearing after selecting ad units because applyInventorySelection() was calling extractSizesFromInventory() without awaiting it. Since extractSizesFromInventory() is async, it was returning immediately before completing, so updateFormatMatchIndicators() never ran to show the button. Fix: Make applyInventorySelection() async and await the call to extractSizesFromInventory(), ensuring size extraction and format matching complete before the function returns. Fixes regression introduced in #720 where dynamic products feature was added.
bokelley
added a commit
that referenced
this pull request
Nov 9, 2025
…ount auth (#723) * fix: await extractSizesFromInventory in applyInventorySelection The 'Select All Matching' button was not appearing after selecting ad units because applyInventorySelection() was calling extractSizesFromInventory() without awaiting it. Since extractSizesFromInventory() is async, it was returning immediately before completing, so updateFormatMatchIndicators() never ran to show the button. Fix: Make applyInventorySelection() async and await the call to extractSizesFromInventory(), ensuring size extraction and format matching complete before the function returns. Fixes regression introduced in #720 where dynamic products feature was added. * fix: handle null/undefined values in targeting selector When the API returns an error (e.g., 400 status), data.values can be null/undefined, causing 'TypeError: Spread syntax requires ...iterable not be null or undefined'. Changes: - Check response.ok before processing - Validate data.values exists and is an array before spreading - Show clear error messages to user instead of cryptic console errors - Add temporary error notification that auto-dismisses after 5 seconds Fixes the error seen in production when loading custom targeting values. * fix: improve error messages for targeting values API endpoint Split generic 'GAM not configured' 400 error into specific messages: - No adapter configured - GAM network code not configured - GAM authentication not configured (with helpful hint) This will help users understand exactly what's missing when they see the 400 error in production, making it easier to diagnose and fix the root cause instead of just handling the error gracefully. * debug: add logging to diagnose targeting values 400 error Add detailed logging to help identify which specific configuration check is failing when the endpoint returns 400. Logs: - Request details (tenant_id, key_id) - Adapter config existence - Network code and refresh token status This will help diagnose the production issue where GAM works for other operations but fails for targeting values endpoint. * fix: support service account authentication in targeting values endpoint The endpoint only checked for OAuth refresh tokens, causing 400 errors for tenants using service account authentication (like 'weather' tenant). Changes: - Check for EITHER refresh token OR service account JSON - Create appropriate GAM client based on auth method: - Service account: Write JSON to temp file, create credentials, cleanup - OAuth: Use existing refresh token flow - Add logging to show which auth method is being used This matches the pattern used in background_sync_service.py which already supports both auth methods. Fixes the 400 error at https://sales-agent.scope3.com/admin/tenant/weather/products/add * audit: ensure all GAM endpoints support both OAuth and service account auth Completed comprehensive audit of all files creating GAM clients to ensure they support BOTH OAuth refresh tokens AND service account authentication. ## Audit Results - ✅ All production endpoints support both auth methods - ✅ OAuth-only endpoints are intentional (setup/testing in api.py, gam.py) - ✅ 1 issue found and fixed: inventory.py:get_targeting_values() (commit 8cc6c7e) ## Prevention: New Pre-commit Hook Created `.pre-commit-hooks/check-gam-auth-support.py` to catch this pattern: - Detects new code creating GAM clients - Ensures they check for BOTH auth methods - Allows exceptions for setup/testing endpoints - Provides helpful error messages with examples ## Files Audited - src/services/background_sync_service.py ✅ (supports both) - src/adapters/gam/auth.py ✅ (supports both via GAMAuthManager) - src/adapters/gam/client.py ✅ (uses GAMAuthManager) - src/admin/blueprints/gam.py ✅ (supports both) - src/admin/blueprints/inventory.py ✅ (fixed in this PR) - src/admin/blueprints/api.py⚠️ (OAuth-only by design) - src/adapters/gam/utils/health_check.py ✅ (service account only) See AUDIT_RESULTS.md for full details. * test: update targeting values test to match new error message The test was expecting 'GAM not configured' but we improved the error message to be more specific: 'No adapter configured for this tenant'. This fixes the failing CI test. * chore: remove audit results file This was a temporary analysis document and doesn't need to be in the repo.
danf-newton
pushed a commit
to Newton-Research-Inc/salesagent
that referenced
this pull request
Nov 24, 2025
…ntextprotocol#720) * feat: add product details, measurement providers, and admin UI enhancements - Add product detail fields (delivery measurement, product image) per AdCP spec - Implement tenant-level measurement provider configuration - Add markdown rendering support for product descriptions - Rename add/edit product templates for adapter clarity (GAM vs Mock) - Remove Description column from product list view to improve layout - Add database migrations for measurement_providers and product details - Improve form handling with better validation and UI organization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: implement dynamic products with signals agent integration - Add database schema for dynamic products (is_dynamic, signals_agent_ids, etc.) - Implement variant generation service with signal querying - Add admin UI for configuring dynamic product templates - Integrate variant generation into get_products workflow - Add visual indicators for templates and variants in product list - Support per-product expiration configuration (variant_ttl_days) - Fix mypy errors in xandr.py and signals.py for new Product fields Dynamic products allow templates to generate variants based on signals agents. Variants are single-use, inherit all template settings, and include signal-based targeting that is transparent to buyers. * feat: integrate signals registry for dynamic product variant generation - Replace placeholder with actual signals agent query using SignalsAgentRegistry - Convert database SignalsAgent model to dataclass for registry compatibility - Parse auth credentials (type + credentials) from database model - Use _get_signals_from_agent to query specific configured agents - Handle async/sync bridge with new event loop - Proper error handling and logging for signal queries Dynamic products now fully integrate with the signals agent infrastructure, querying signals via MCP using the existing registry and AdCP library. * refactor: optimize dynamic products to use singleton registry and multi-agent queries Address code review comments: 1. Use singleton SignalsAgentRegistry (no per-call DB queries or registry creation) 2. Multi-agent query in single call (registry already handles this) 3. Specify deployment per AdCP spec (deliver_to with our agent_url) 4. Extract activation key for our specific deployment only 5. Remove redundant query_signals_agent function Changes: - Use get_signals_agent_registry() singleton (not new SignalsAgentRegistry()) - Call registry.get_signals() once for all tenant agents (multi-agent built-in) - Pass our_agent_url (from tenant.virtual_host) for deployment matching - extract_activation_key() now filters by our deployment destination - Removed per-agent database queries and registry instantiation - Added deployment specification in context (deliver_to.destinations) Performance improvements: - One registry instance (singleton pattern) - One multi-agent query (not per-agent loops) - No per-call database queries for agent configuration - Registry handles auth, MCP client pooling, and multi-agent orchestration * fix: remove countries parameter from signals agent context Countries filtering will be added later when needed. For now, just pass the deployment destination (our agent_url) to the signals agent. * feat: implement dynamic products with signals agent integration Core Features: - Products can be marked as dynamic with configurable variant templates - Support for 'all agents' or specific signals agents selection - Dynamic variant generation from signals agent responses - Proper async context manager and event loop management Database Changes: - Add is_dynamic, signals_agent_ids columns to products - Add variant_name_template, variant_description_template columns - Migration: 149ad85edb6f Implementation: - DynamicProductsService generates variants with activation keys - ADCPMultiAgentClient properly managed with async context manager - Accept brand_manifest.url in get_products validation - UI with multi-select for signals agents and template configuration Fixes: - Fix async event loop and connection management - Fix test_connection to support async context manager - Handle signals_agent_ids=None (all agents) correctly - Improve dynamic products UI warnings and validation * fix: use AsyncMock for ADCPMultiAgentClient to support async context manager - Replace Mock() with AsyncMock() in three test locations - Add __aenter__ and __aexit__ support for async with protocol - Fixes 'Mock object does not support asynchronous context manager protocol' error --------- Co-authored-by: Claude <noreply@anthropic.com>
danf-newton
pushed a commit
to Newton-Research-Inc/salesagent
that referenced
this pull request
Nov 24, 2025
…ount auth (adcontextprotocol#723) * fix: await extractSizesFromInventory in applyInventorySelection The 'Select All Matching' button was not appearing after selecting ad units because applyInventorySelection() was calling extractSizesFromInventory() without awaiting it. Since extractSizesFromInventory() is async, it was returning immediately before completing, so updateFormatMatchIndicators() never ran to show the button. Fix: Make applyInventorySelection() async and await the call to extractSizesFromInventory(), ensuring size extraction and format matching complete before the function returns. Fixes regression introduced in adcontextprotocol#720 where dynamic products feature was added. * fix: handle null/undefined values in targeting selector When the API returns an error (e.g., 400 status), data.values can be null/undefined, causing 'TypeError: Spread syntax requires ...iterable not be null or undefined'. Changes: - Check response.ok before processing - Validate data.values exists and is an array before spreading - Show clear error messages to user instead of cryptic console errors - Add temporary error notification that auto-dismisses after 5 seconds Fixes the error seen in production when loading custom targeting values. * fix: improve error messages for targeting values API endpoint Split generic 'GAM not configured' 400 error into specific messages: - No adapter configured - GAM network code not configured - GAM authentication not configured (with helpful hint) This will help users understand exactly what's missing when they see the 400 error in production, making it easier to diagnose and fix the root cause instead of just handling the error gracefully. * debug: add logging to diagnose targeting values 400 error Add detailed logging to help identify which specific configuration check is failing when the endpoint returns 400. Logs: - Request details (tenant_id, key_id) - Adapter config existence - Network code and refresh token status This will help diagnose the production issue where GAM works for other operations but fails for targeting values endpoint. * fix: support service account authentication in targeting values endpoint The endpoint only checked for OAuth refresh tokens, causing 400 errors for tenants using service account authentication (like 'weather' tenant). Changes: - Check for EITHER refresh token OR service account JSON - Create appropriate GAM client based on auth method: - Service account: Write JSON to temp file, create credentials, cleanup - OAuth: Use existing refresh token flow - Add logging to show which auth method is being used This matches the pattern used in background_sync_service.py which already supports both auth methods. Fixes the 400 error at https://sales-agent.scope3.com/admin/tenant/weather/products/add * audit: ensure all GAM endpoints support both OAuth and service account auth Completed comprehensive audit of all files creating GAM clients to ensure they support BOTH OAuth refresh tokens AND service account authentication. ## Audit Results - ✅ All production endpoints support both auth methods - ✅ OAuth-only endpoints are intentional (setup/testing in api.py, gam.py) - ✅ 1 issue found and fixed: inventory.py:get_targeting_values() (commit 8cc6c7e) ## Prevention: New Pre-commit Hook Created `.pre-commit-hooks/check-gam-auth-support.py` to catch this pattern: - Detects new code creating GAM clients - Ensures they check for BOTH auth methods - Allows exceptions for setup/testing endpoints - Provides helpful error messages with examples ## Files Audited - src/services/background_sync_service.py ✅ (supports both) - src/adapters/gam/auth.py ✅ (supports both via GAMAuthManager) - src/adapters/gam/client.py ✅ (uses GAMAuthManager) - src/admin/blueprints/gam.py ✅ (supports both) - src/admin/blueprints/inventory.py ✅ (fixed in this PR) - src/admin/blueprints/api.py⚠️ (OAuth-only by design) - src/adapters/gam/utils/health_check.py ✅ (service account only) See AUDIT_RESULTS.md for full details. * test: update targeting values test to match new error message The test was expecting 'GAM not configured' but we improved the error message to be more specific: 'No adapter configured for this tenant'. This fixes the failing CI test. * chore: remove audit results file This was a temporary analysis document and doesn't need to be in the repo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Changes
🤖 Generated with Claude Code