:bug: Source Facebook Marketing: Add statuses filters by iSINUS · Pull Request #32449 · airbytehq/airbyte

Siarhei Ivanou (iSINUS) · 2023-11-11T07:33:31Z

What

Add status filters to source connector Facebook Marketing for streams Campaigns, AdSets and Ads (#32421).
Remove option "include_deleted" as it from older versions of Marketing API and operates with not actual fields and values.

New config UI looks like:

How

Updated the specification and stream filtering. State migration was implemented to ensure no disruptions for users. The include_deleted option is now mapped to all possible statuses and is still retained in the stream status for backward compatibility.

🚨 User Impact 🚨

If user are interested in getting "deleted" stream data it is required to update configuration and set desired statuses ("ARCHIVED", "DELETED") per stream. To match the behaviour with include_deleted from the previous version, all possible statuses should be selected.
Previous version of code worked not really correctly (there is no field delivery_info by which it was filtered and statuses used in code were in lower case and not correct for current Marketing API version).
Incorrect current version call:
/adsets?filtering=[ { "field": "adset.delivery_info", "operator": "IN", "value": [ "paused" ] }]
New version call:
/adsets?filtering=[ { "field": "adset.effective_status", "operator": "IN", "value": [ "PAUSED" ] }]

vercel · 2023-11-11T07:33:35Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Feb 20, 2024 6:23pm

CLAassistant · 2023-11-11T07:33:36Z

All committers have signed the CLA.

github-actions · 2023-11-11T07:33:50Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

M. Marx (marcosmarxm)

Hello Siarhei Ivanou (@iSINUS) thanks for the contribution and patience. I'll run tests and return with any update during the week.

Siarhei Ivanou (iSINUS) · 2023-11-27T10:28:24Z

Hi, M. Marx (@marcosmarxm)! Is there any update?

Siarhei Ivanou (iSINUS) · 2023-12-14T12:32:49Z

Hi, M. Marx (@marcosmarxm)! Is there any update?

M. Marx (marcosmarxm) · 2023-12-18T13:17:58Z

I asked to the team make the final review. I'll ask again, sorry the delay Siarhei Ivanou (@iSINUS) !

Serhii Lazebnyi (lazebnyi)

Need change fields to enum and follow to braking change procedure (M. Marx (@marcosmarxm))

Serhii Lazebnyi (lazebnyi) · 2024-02-08T16:13:40Z

        if self._cursor_values:
            result_state = {account_id: {self.cursor_field: cursor_value} for account_id, cursor_value in self._cursor_values.items()}
-            result_state["include_deleted"] = self._include_deleted
+            result_state["filter_statuses"] = self._filter_statuses


We need avoid braking change and if user has included_deleted parameter use it.

The main idea of the change is to avoid single option to do not process deleted and have specific statuses for streams.
So there will be no changes from our side.

I understand the main idea, but for users who have already set up synchronization, this will be a breaking change, requiring them to reconfigure their connections.

We aim to avoid this inconvenience. Therefore, let's revise the logic so that if the include_deleted parameter exists, we will utilize the old logic. However, if the filter_statuses list is set up, we will exclusively use this list.

It is breaking change - who do not need new functionality should stay on older version. I will not over-engineer for backward compatibility. The issue will be if both parameters are set, but filter_statuses exclude "DELETED" - it will require additional explanation to user (should we include deleted in such cases?) - not straight forward behaviour.

Serhii Lazebnyi (lazebnyi) · 2024-02-08T19:08:11Z

        if self._cursor_values:
            result_state = {account_id: {self.cursor_field: cursor_value} for account_id, cursor_value in self._cursor_values.items()}
-            result_state["include_deleted"] = self._include_deleted
+            result_state["filter_statuses"] = self._filter_statuses


I understand the main idea, but for users who have already set up synchronization, this will be a breaking change, requiring them to reconfigure their connections.

We aim to avoid this inconvenience. Therefore, let's revise the logic so that if the include_deleted parameter exists, we will utilize the old logic. However, if the filter_statuses list is set up, we will exclusively use this list.

Anatolii Yatsuk (tolik0) · 2024-02-19T13:31:42Z

Siarhei Ivanou (@iSINUS) Hi, could you grant me permission to push changes to your branch? I'd like to add my fixes and then merge your PR. Thanks!

Siarhei Ivanou (iSINUS) · 2024-02-19T13:41:22Z

Siarhei Ivanou (@iSINUS) Hi, could you grant me permission to push changes to your branch? I'd like to add my fixes and then merge your PR. Thanks!

Anatolii Yatsuk (@tolik0) Hi! I have no rights to repo with branch, person who has is on vacation today. Will ask tomorrow. Thank you!

Siarhei Ivanou (iSINUS) · 2024-02-20T12:12:29Z

Siarhei Ivanou (@iSINUS) Hi, could you grant me permission to push changes to your branch? I'd like to add my fixes and then merge your PR. Thanks!

Anatolii Yatsuk (@tolik0) Done. Now you should have access.

Please, take into account that you have removed AdCreative status, but it is required - https://developers.facebook.com/docs/marketing-api/reference/ad-creative/
"status enum {ACTIVE, IN_PROCESS, WITH_ISSUES, DELETED} | The status of the creative. WITH_ISSUES and IN_PROCESS are available for 4.0 or higher"

…book-marketing-statuses

Anatolii Yatsuk (tolik0) · 2024-02-20T13:19:11Z

Siarhei Ivanou (@iSINUS) Hello. The AdCreative endpoint doesn't support status filtering. Please check the docs: the Parameters block shows which fields can be used for filtering.

Status:  400\n  Response:\n    {\n      \"error\": {\n        \"message\": \"(#100) Filtering field 'adcreative.status' with operation 'in' is not supported\"

Siarhei Ivanou (iSINUS) · 2024-02-20T14:36:28Z

Siarhei Ivanou (@iSINUS) Hello. The AdCreative endpoint doesn't support status filtering. Please check the docs: the Parameters block shows which fields can be used for filtering.
Status:  400\n  Response:\n    {\n      \"error\": {\n        \"message\": \"(#100) Filtering field 'adcreative.status' with operation 'in' is not supported\"

Anatolii Yatsuk (@tolik0) OK. Thank you.

Co-authored-by: Anatolii Yatsuk <tolikyatsuk@gmail.com>

Octavia Squidington III (octavia-squidington-iii) added area/connectors Connector related issues area/documentation Improvements or additions to documentation community connectors/source/facebook-marketing labels Nov 11, 2023

Siarhei Ivanou (iSINUS) changed the title ~~🚨🚨 Add statuses filters to source connector Facebook Marketing~~ 🚨🚨 Source Facebook Marketing: Add statuses filters Nov 11, 2023

Siarhei Ivanou (iSINUS) marked this pull request as ready for review November 11, 2023 07:50

Siarhei Ivanou (iSINUS) added 2 commits November 11, 2023 10:52

all except integration tests

e67f0cd

fix tests

4fc52c2

Siarhei Ivanou (iSINUS) force-pushed the sivanou/source-facebook-marketing-statuses branch from e9daa94 to 4fc52c2 Compare November 11, 2023 07:52

update docs and unit test

6f2bb68

vercel Bot deployed to Preview November 11, 2023 08:15

View deployment

M. Marx (marcosmarxm) reviewed Nov 20, 2023

View reviewed changes

M. Marx (marcosmarxm) added the gl-review label Nov 29, 2023

Oleksandra Ustynova (oustynova) added the team/connectors-python label Nov 29, 2023

Merge branch 'master' into sivanou/source-facebook-marketing-statuses

62371e7

vercel Bot deployed to Preview December 8, 2023 07:16

View deployment

Serhii Lazebnyi (lazebnyi) requested changes Dec 22, 2023

View reviewed changes

Comment thread airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/spec.py

Siarhei Ivanou (iSINUS) requested review from Serhii Lazebnyi (lazebnyi) and M. Marx (marcosmarxm) December 22, 2023 14:55

Serhii Lazebnyi (lazebnyi) requested changes Dec 28, 2023

View reviewed changes

Comment thread airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/spec.py

Siarhei Ivanou (iSINUS) added 2 commits January 8, 2024 11:01

add default ALL status

34b1078

Merge branch 'master' into sivanou/source-facebook-marketing-statuses

80e76c0

Siarhei Ivanou (iSINUS) requested a review from Serhii Lazebnyi (lazebnyi) January 8, 2024 08:05

vercel Bot deployed to Preview January 8, 2024 08:05

View deployment

Serhii Lazebnyi (lazebnyi) removed gl gl-review labels Feb 7, 2024

Merge branch 'master' into sivanou/source-facebook-marketing-statuses

baafe0f

vercel Bot deployed to Preview February 8, 2024 13:43

View deployment

Serhii Lazebnyi (lazebnyi) requested changes Feb 8, 2024

View reviewed changes

correct test spec

a4f5e93

Siarhei Ivanou (iSINUS) requested a review from Serhii Lazebnyi (lazebnyi) February 8, 2024 18:29

Serhii Lazebnyi (lazebnyi) requested changes Feb 8, 2024

View reviewed changes

Serhii Lazebnyi (lazebnyi) requested review from Roman Yermilov [GL] (roman-yermilov-gl) and Anatolii Yatsuk (tolik0) and removed request for Roman Yermilov [GL] (roman-yermilov-gl) February 8, 2024 19:16

Siarhei Ivanou (iSINUS) requested a review from Serhii Lazebnyi (lazebnyi) February 8, 2024 19:25

Anatolii Yatsuk (tolik0) mentioned this pull request Feb 14, 2024

[run CI] Source Facebook Marketing: Add status filters #35276

Closed

Add state migration and fix errors

b3057a8

vercel Bot deployed to Preview February 20, 2024 13:03

View deployment

Merge remote-tracking branch 'origin/master' into sivanou/source-face…

13ea3fc

…book-marketing-statuses

vercel Bot deployed to Preview February 20, 2024 13:13

View deployment

Anatolii Yatsuk (tolik0) added 2 commits February 20, 2024 18:07

Delete wrong file from merge conflict

0030acc

Update pyproject.toml

6921ec0

Anatolii Yatsuk (tolik0) approved these changes Feb 20, 2024

View reviewed changes

Anatolii Yatsuk (tolik0) merged commit dd01ba3 into airbytehq:master Feb 20, 2024

Siarhei Ivanou (iSINUS) deleted the sivanou/source-facebook-marketing-statuses branch February 21, 2024 05:30

Anatolii Yatsuk (tolik0) changed the title ~~🚨🚨 Source Facebook Marketing: Add statuses filters~~ 🐛 Source Facebook Marketing: Add statuses filters Feb 21, 2024

Xiaohan Song (xiaohansong) pushed a commit that referenced this pull request Feb 27, 2024

🚨🚨 Source Facebook Marketing: Add statuses filters (#32449)

db2d63b

Co-authored-by: Anatolii Yatsuk <tolikyatsuk@gmail.com>

Conversation

Siarhei Ivanou (iSINUS) commented Nov 11, 2023 • edited by tolik0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Recommended reading order

🚨 User Impact 🚨

Uh oh!

vercel Bot commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 11, 2023

Before Merging a Connector Pull Request

Uh oh!

M. Marx (marcosmarxm) left a comment

Choose a reason for hiding this comment

Uh oh!

Siarhei Ivanou (iSINUS) commented Nov 27, 2023

Uh oh!

Siarhei Ivanou (iSINUS) commented Dec 14, 2023

Uh oh!

M. Marx (marcosmarxm) commented Dec 18, 2023

Uh oh!

Serhii Lazebnyi (lazebnyi) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Serhii Lazebnyi (lazebnyi) Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Siarhei Ivanou (iSINUS) Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Serhii Lazebnyi (lazebnyi) Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Siarhei Ivanou (iSINUS) Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Serhii Lazebnyi (lazebnyi) Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Anatolii Yatsuk (tolik0) commented Feb 19, 2024

Uh oh!

Siarhei Ivanou (iSINUS) commented Feb 19, 2024

Uh oh!

Siarhei Ivanou (iSINUS) commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anatolii Yatsuk (tolik0) commented Feb 20, 2024

Uh oh!

Siarhei Ivanou (iSINUS) commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Siarhei Ivanou (iSINUS) commented Nov 11, 2023 •

edited by tolik0

Loading

vercel Bot commented Nov 11, 2023 •

edited

Loading

CLAassistant commented Nov 11, 2023 •

edited

Loading

Siarhei Ivanou (iSINUS) Feb 8, 2024 •

edited

Loading

Siarhei Ivanou (iSINUS) commented Feb 20, 2024 •

edited

Loading