-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns #4864
Changes from 8 commits
1fa4461
4cf644e
ab6fc13
2601654
b9dfed8
2338a0f
892279f
528cd44
b47be69
8c0f243
8dcf971
20df1e0
dd00338
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ | |
# Copyright (c) 2021 Airbyte, Inc., all rights reserved. | ||
# | ||
|
||
import json | ||
from datetime import datetime | ||
from typing import Any, List, Mapping, Tuple, Type | ||
from typing import Any, List, Mapping, Optional, Tuple, Type | ||
|
||
from airbyte_cdk.entrypoint import logger | ||
from airbyte_cdk.logger import AirbyteLogger | ||
from airbyte_cdk.models import AuthSpecification, ConnectorSpecification, DestinationSyncMode, OAuth2Specification | ||
from airbyte_cdk.sources import AbstractSource | ||
from airbyte_cdk.sources.streams import Stream | ||
|
@@ -25,6 +28,17 @@ | |
) | ||
|
||
|
||
class InsightConfig(BaseModel): | ||
|
||
name: str = Field(description="The name value of insight") | ||
|
||
fields: Optional[List[str]] = Field(description="A list of chosen fields for fields parameter") | ||
|
||
breakdowns: Optional[List[str]] = Field(description="A list of chosen breakdowns for breakdowns") | ||
|
||
action_breakdowns: Optional[List[str]] = Field(description="A list of chosen action_breakdowns for action_breakdowns") | ||
|
||
|
||
class ConnectorConfig(BaseModel): | ||
class Config: | ||
title = "Source Facebook Marketing" | ||
|
@@ -57,6 +71,12 @@ class Config: | |
minimum=1, | ||
maximum=30, | ||
) | ||
insights: Optional[List[InsightConfig]] = Field( | ||
vladimir-remar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description="A defined list wich contains insights entries, each entry must have a name and can contain these entries(fields, breakdowns or action_breakdowns)", | ||
examples=[ | ||
'[{"name": "AdsInsights","fields": ["account_id","account_name","ad_id","ad_name","adset_id","adset_name","campaign_id","campaign_name","date_start","impressions","spend"],"breakdowns": [],"action_breakdowns": []}]' | ||
], | ||
) | ||
|
||
|
||
class SourceFacebookMarketing(AbstractSource): | ||
|
@@ -94,7 +114,7 @@ def streams(self, config: Mapping[str, Any]) -> List[Type[Stream]]: | |
days_per_job=config.insights_days_per_job, | ||
) | ||
|
||
return [ | ||
streams = [ | ||
Campaigns(api=api, start_date=config.start_date, include_deleted=config.include_deleted), | ||
AdSets(api=api, start_date=config.start_date, include_deleted=config.include_deleted), | ||
Ads(api=api, start_date=config.start_date, include_deleted=config.include_deleted), | ||
|
@@ -108,6 +128,8 @@ def streams(self, config: Mapping[str, Any]) -> List[Type[Stream]]: | |
AdsInsightsActionType(**insights_args), | ||
] | ||
|
||
return self._update_insights_streams(insights=config.insights, args=insights_args, streams=streams) | ||
|
||
def spec(self, *args, **kwargs) -> ConnectorSpecification: | ||
""" | ||
Returns the spec for this integration. The spec is a JSON-Schema object describing the required configurations (e.g: username and password) | ||
|
@@ -126,3 +148,28 @@ def spec(self, *args, **kwargs) -> ConnectorSpecification: | |
), | ||
), | ||
) | ||
|
||
def _update_insights_streams(self, insights, args, streams) -> List[Type[Stream]]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest the following approach for these custom streams:
This way it is very very obvious to the user what is happening. This is especially important as the connector's config is updated over time e.g: a user might call a stream There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the change in the lastest commit |
||
"""Update method, if insights have values returns streams replacing the | ||
default insights streams else returns streams | ||
|
||
""" | ||
if not insights: | ||
return streams | ||
|
||
insights_custom_streams = list() | ||
|
||
for insight in insights: | ||
args["name"] = insight.name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this mutating the same config over and over, so in the end all the streams have the same config? can we copy the args instead? e.g:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sherifnada the streams being created each time, so config never shared |
||
args["fields"] = insight.fields | ||
args["breakdowns"] = insight.breakdowns | ||
args["action_breakdowns"] = insight.action_breakdowns | ||
insight_stream = AdsInsights(**args) | ||
insights_custom_streams.append(insight_stream) | ||
|
||
new_streams = list() | ||
for stream in streams: | ||
if stream.name not in [e.name for e in insights_custom_streams]: | ||
new_streams.append(stream) | ||
|
||
return new_streams + insights_custom_streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vladimir-remar did you validate that the json output by
spec
works with the UI via the instructions here? Just checking because I don't remember if we support a list of objectsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sherifnada we do actually :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sherifnada thanks to @keu, I attached some images from UI
It will be filled like this
And it will look like this with two elements
and from destination side