Skip to content
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog

## 1.25.0
* Make action_breakdowns configurable [#260](https://github.com/singer-io/tap-facebook/pull/260)

## 1.24.0
* Bump facebook_business SDK to v23.0.1 [#255](https://github.com/singer-io/tap-facebook/pull/255)
* Remove Deprecated Fields from adcreative [#255](https://github.com/singer-io/tap-facebook/pull/255)
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from setuptools import setup

setup(name='tap-facebook',
version='1.24.0',
version='1.25.0',
description='Singer.io tap for extracting data from the Facebook Ads API',
author='Stitch',
url='https://singer.io',
Expand Down
24 changes: 23 additions & 1 deletion tap_facebook/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,23 @@ def __iter__(self):
"primary-keys": ['hourly_stats_aggregated_by_advertiser_time_zone']},
}

def parse_action_breakdowns(breakdown_str):
if not breakdown_str:
return ALL_ACTION_BREAKDOWNS
if not isinstance(breakdown_str, str):
LOGGER.warning("action_breakdowns must be a string, got %s", type(breakdown_str))
return ALL_ACTION_BREAKDOWNS

selected_breakdowns = []
act_breakdowns = [b.strip().lower() for b in breakdown_str.split(',')]
for breakdown in act_breakdowns:
if not breakdown: # Skip empty strings
continue
if breakdown in ALL_ACTION_BREAKDOWNS and breakdown not in selected_breakdowns:
selected_breakdowns.append(breakdown)
else:
LOGGER.warning("Invalid action breakdown %s", breakdown)
return selected_breakdowns if selected_breakdowns else ALL_ACTION_BREAKDOWNS

def initialize_stream(account, catalog_entry, state): # pylint: disable=too-many-return-statements

Expand All @@ -821,7 +838,7 @@ def initialize_stream(account, catalog_entry, state): # pylint: disable=too-many

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"])
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
options=INSIGHTS_BREAKDOWNS_OPTIONS[name], action_breakdowns=CONFIG["action_breakdowns"])
options=INSIGHTS_BREAKDOWNS_OPTIONS[name], action_breakdowns=CONFIG.get("action_breakdowns", ALL_ACTION_BREAKDOWNS))

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be updated

elif name == 'campaigns':
return Campaigns(name, account, stream_alias, catalog_entry, state=state)
elif name == 'adsets':
Expand Down Expand Up @@ -959,6 +976,11 @@ def main_impl():
request_timeout = float(config_request_timeout)
else:
request_timeout = REQUEST_TIMEOUT # If value is 0,"0","" or not passed then set default to 300 seconds.

ab_params = CONFIG.get("action_breakdowns")
CONFIG["action_breakdowns"] = parse_action_breakdowns(ab_params)

LOGGER.info("Using %d action breakdown(s): %s", len(CONFIG["action_breakdowns"]), CONFIG["action_breakdowns"])

global API
API = FacebookAdsApi.init(access_token=access_token, timeout=request_timeout)
Expand Down
1 change: 0 additions & 1 deletion tests/test_facebook_all_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class FacebookAllFieldsTest(AllFieldsTest, FacebookBaseTest):
'link_url',
'adlabels',
'source_instagram_media_id',
'video_id'
},
"ads_insights_country": {
'video_p75_watched_actions',
Expand Down
126 changes: 126 additions & 0 deletions tests/unittests/test_action_breakdowns.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import unittest
from tap_facebook import parse_action_breakdowns, ALL_ACTION_BREAKDOWNS


class TestParseActionBreakdowns(unittest.TestCase):
"""Test suite for parse_action_breakdowns function"""

def test_none_returns_all_breakdowns(self):
"""Test that None input returns all default breakdowns"""
result = parse_action_breakdowns(None)
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)
self.assertEqual(len(result), 3)

def test_empty_string_returns_all_breakdowns(self):
"""Test that empty string returns all default breakdowns"""
result = parse_action_breakdowns("")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)
self.assertEqual(len(result), 3)

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)
Comment on lines +20 to +29
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cases

This ensures the warning behavior is actually tested, not just documented.

Copilot uses AI. Check for mistakes.

def test_single_valid_breakdown(self):
"""Test parsing a single valid breakdown"""
result = parse_action_breakdowns("action_type")
self.assertEqual(result, ['action_type'])
self.assertEqual(len(result), 1)

def test_multiple_valid_breakdowns(self):
"""Test parsing multiple valid breakdowns"""
result = parse_action_breakdowns("action_type,action_destination")
self.assertEqual(result, ['action_type', 'action_destination'])
self.assertEqual(len(result), 2)

def test_all_valid_breakdowns(self):
"""Test parsing all three valid breakdowns"""
result = parse_action_breakdowns("action_type,action_target_id,action_destination")
self.assertEqual(result, ['action_type', 'action_target_id', 'action_destination'])
self.assertEqual(len(result), 3)

def test_whitespace_handling(self):
"""Test that whitespace is properly stripped"""
result = parse_action_breakdowns(" action_type , action_destination ")
self.assertEqual(result, ['action_type', 'action_destination'])

result = parse_action_breakdowns(" action_type , action_target_id ")
self.assertEqual(result, ['action_type', 'action_target_id'])

def test_case_insensitivity(self):
"""Test that input is case-insensitive"""
result = parse_action_breakdowns("ACTION_TYPE,Action_Destination")
self.assertEqual(result, ['action_type', 'action_destination'])

result = parse_action_breakdowns("ACTION_TARGET_ID")
self.assertEqual(result, ['action_target_id'])

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'])
Comment on lines +65 to +72
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

def test_all_invalid_returns_all_breakdowns(self):
"""Test that all invalid values returns default breakdowns"""
result = parse_action_breakdowns("invalid1,invalid2,invalid3")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)

result = parse_action_breakdowns("foo,bar,baz")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)

def test_empty_values_skipped(self):
"""Test that empty values from comma-separation are skipped"""
result = parse_action_breakdowns("action_type,,action_destination")
self.assertEqual(result, ['action_type', 'action_destination'])

result = parse_action_breakdowns(",action_type,")
self.assertEqual(result, ['action_type'])

result = parse_action_breakdowns(",,action_type,,")
self.assertEqual(result, ['action_type'])

def test_only_commas_returns_all_breakdowns(self):
"""Test that only commas returns default breakdowns"""
result = parse_action_breakdowns(",,,")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)

def test_duplicate_breakdowns(self):
"""Test handling of duplicate breakdown values"""
result = parse_action_breakdowns("action_type,action_type,action_destination")
# Should not include duplicates since deduplication is performed
self.assertEqual(result, ['action_type', 'action_destination'])

def test_whitespace_only_string(self):
"""Test whitespace-only string is treated as empty"""
result = parse_action_breakdowns(" ")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)

result = parse_action_breakdowns("\t\n")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)

def test_partial_match_not_accepted(self):
"""Test that partial matches are not accepted"""
result = parse_action_breakdowns("action_typ")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)

result = parse_action_breakdowns("action_type_extra")
self.assertEqual(result, ALL_ACTION_BREAKDOWNS)

def test_order_preserved(self):
"""Test that order of valid breakdowns is preserved"""
result = parse_action_breakdowns("action_destination,action_type")
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'])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing empty line

Loading