Adds Configurable property for action breakdowns#260
Adds Configurable property for action breakdowns#260vishalp-dev wants to merge 14 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds configurability for action_breakdowns in the Facebook Ads tap, allowing users to specify which action breakdowns to include in their data extraction. The feature maintains backward compatibility by defaulting to all action breakdowns (action_type, action_target_id, action_destination) when no configuration is provided.
Key Changes:
- Introduced
parse_action_breakdowns()function to validate and parse user-provided action breakdown configurations - Modified
AdsInsightsinitialization to accept configured action breakdowns - Updated version from 1.24.0 to 1.25.0
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tap_facebook/init.py | Added parse_action_breakdowns() parser function with validation, updated initialize_stream() to pass configured breakdowns to AdsInsights, and added configuration parsing in main_impl() |
| tests/unittests/test_action_breakdowns.py | Comprehensive unit test suite covering edge cases for the new parse_action_breakdowns() function including validation, whitespace handling, case insensitivity, and deduplication |
| setup.py | Version bump from 1.24.0 to 1.25.0 |
| CHANGELOG.md | Added entry for action_breakdowns configurability feature |
| tests/test_facebook_all_fields.py | Removed video_id from adcreative MISSING_FIELDS set (appears unrelated to this PR) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if name in INSIGHTS_BREAKDOWNS_OPTIONS: | ||
| return AdsInsights(name, account, stream_alias, catalog_entry, state=state, | ||
| options=INSIGHTS_BREAKDOWNS_OPTIONS[name]) | ||
| options=INSIGHTS_BREAKDOWNS_OPTIONS[name], action_breakdowns=CONFIG["action_breakdowns"]) |
There was a problem hiding this comment.
Using CONFIG["action_breakdowns"] with direct dict access will raise a KeyError when the key doesn't exist. This breaks existing tests (e.g., tests/unittests/test_tap_facebook.py) and any other code that calls initialize_stream before main_impl sets this config value.
Use .get() with a default value instead:
action_breakdowns=CONFIG.get("action_breakdowns", ALL_ACTION_BREAKDOWNS)This maintains backward compatibility and ensures the code works in all contexts.
| options=INSIGHTS_BREAKDOWNS_OPTIONS[name], action_breakdowns=CONFIG["action_breakdowns"]) | |
| options=INSIGHTS_BREAKDOWNS_OPTIONS[name], action_breakdowns=CONFIG.get("action_breakdowns", ALL_ACTION_BREAKDOWNS)) |
| def test_non_string_type_returns_all_breakdowns(self): | ||
| """Test that non-string types return all default breakdowns with warning""" | ||
| result = parse_action_breakdowns(123) | ||
| self.assertEqual(result, ALL_ACTION_BREAKDOWNS) | ||
|
|
||
| result = parse_action_breakdowns(['action_type']) | ||
| self.assertEqual(result, ALL_ACTION_BREAKDOWNS) | ||
|
|
||
| result = parse_action_breakdowns({'breakdown': 'action_type'}) | ||
| self.assertEqual(result, ALL_ACTION_BREAKDOWNS) |
There was a problem hiding this comment.
The test docstring mentions "with warning" but the test doesn't verify that a warning is actually logged. Consider using assertLogs to verify the warning is emitted:
def test_non_string_type_returns_all_breakdowns(self):
"""Test that non-string types return all default breakdowns with warning"""
with self.assertLogs('tap_facebook', level='WARNING') as cm:
result = parse_action_breakdowns(123)
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)
self.assertIn("action_breakdowns must be a string", cm.output[0])
# ... similar for other casesThis ensures the warning behavior is actually tested, not just documented.
| def test_mixed_valid_and_invalid(self): | ||
| """Test parsing mix of valid and invalid breakdowns""" | ||
| result = parse_action_breakdowns("action_type,invalid_breakdown,action_destination") | ||
| self.assertEqual(result, ['action_type', 'action_destination']) | ||
| self.assertEqual(len(result), 2) | ||
|
|
||
| result = parse_action_breakdowns("invalid1,action_type,invalid2") | ||
| self.assertEqual(result, ['action_type']) |
There was a problem hiding this comment.
This test should verify that warnings are logged for invalid breakdown values. Consider using assertLogs:
def test_mixed_valid_and_invalid(self):
"""Test parsing mix of valid and invalid breakdowns"""
with self.assertLogs('tap_facebook', level='WARNING') as cm:
result = parse_action_breakdowns("action_type,invalid_breakdown,action_destination")
self.assertEqual(result, ['action_type', 'action_destination'])
self.assertEqual(len(result), 2)
self.assertIn("Invalid action breakdown invalid_breakdown", cm.output[0])This ensures that invalid values are actually being logged as expected.
| self.assertEqual(result, ['action_destination', 'action_type']) | ||
|
|
||
| result = parse_action_breakdowns("action_target_id,action_destination,action_type") | ||
| self.assertEqual(result, ['action_target_id', 'action_destination', 'action_type']) No newline at end of file |
Description of change
Manual QA steps
Risks
Rollback steps
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code