Source FB marketing: include_deleted isn't being set to true

[sherifnada]

Expected Behavior

I expect setting the "include_deleted" option to true to cause the connector to pull data for deleted/archived/defunct campaigns

Current Behavior

A user @MaxKrog reported he is setting the flag to true but it in the logs, is seeing it being set to false.

Logs

https://files.slack.com/files-pri/T01AB4DDR2N-F01U0GY9NV7/download/logs-9-0.txt

Steps to Reproduce

TBD

Severity of the bug for you

High -- user is not able to correctly use the connector

Airbyte Version

See logs

Connector Version (if applicable)

See logs

[Zirochkaa]

The error is with ads_insights_country stream (and also in general with AdsInsightAPI class) - we can't pass include_deleted argument to it's __init__() function. And because of this in StreamAPI.__init__() function on line 59 we have:

self._include_deleted = include_deleted if self.enable_deleted else False

I think that AdsInsightAPI .__init__() function should take *args, **kwargs arguments also and pass them to super().__init__() (right now we only pass api=api). This should fix the problem.

This should take about 4-5 hours.

Note

I'm not sure what to do with other streams like ads_insights_age_and_gender, ads_insights_region, etc (6 in general) which is also a AdsInsightAPI instances.

[sherifnada]

There is also an open question about whether we're passing the parameter correctly to the API in the campagins and adsets and Ads streams case. @Zirochkaa will verify the following:

1. Are we passing the parameters correctly to the API? If yes, no work to do here
2. if not, fix the issue and pass it correctly to the API

[sherifnada]

@Zirochkaa can you confirm that ads insights accepts the include_deleted parameter?

[sherifnada]

also @Zirochkaa can you add a custom integration test that hardcodes the ID of a deleted campaign and verifies that we pull it when the include_deleted flag is set to true?

[sherifnada]

also we should understand why the log statement is displaying deleted as false -- is there an actual bug or is the log statement faulty? if the latter then we should fix the log statement

[Zirochkaa]

There is also an open question about whether we're passing the parameter correctly to the API in the campagins and adsets and Ads streams case. @Zirochkaa will verify the following:

1. Are we passing the parameters correctly to the API? If yes, no work to do here
2. if not, fix the issue and pass it correctly to the API

About first question - right now I can't confirm that we are passing parameters correctly or not. I tested campaigns stream with both "include_deleted": "false" and "include_deleted": "true" and got the same output - 32 records. Same was for adsets - 14 records and for ads - 4 records. This could be because one of two reasons (or both):

1. we don't pass parameters correctly;
2. we don't have deleted campaigns, adsets and ads.

I don't have an access to facebook where I could check this in UI. I tried login/password from LastPass but they don't work for me (see screenshot).

So right now I'm not sure how I could check whether we pass parameters correctly or not.

About second question - it depends on first question.

[Zirochkaa]

About above comment - that was my mistake, I misread information on how to use include_deleted field in config.json and that was the reason why I was getting the same results for all 3 streams. I changed the way how I define include_deleted field in config.json and everything is working properly:

1. if include_deleted is true then we get all records.
2. if include_deleted is false then all records with status ARCHIVED are excluded from output.

So I can confirm that we are passing include_deleted parameter correctly to the API for campaigns, adsets and ads streams.

[Zirochkaa]

@Zirochkaa can you confirm that ads insights accepts the include_deleted parameter?

I never find information that it accepts include_deleted parameter. But this doesn't exclude the fact that it accepts this parameter and I just didn't find it (facebook documentation is a maze 😅). For now I can say that I checked and I didn't find information that ads insights accepts the include_deleted parameter.

[Zirochkaa]

also we should understand why the log statement is displaying deleted as false -- is there an actual bug or is the log statement faulty? if the latter then we should fix the log statement

It's expected behavior because even if include_deleted option is set to true in config file, in AdsInsightAPI class this option is turned off. So no matter what you put into config file you'll always get include_deleted=false.

[sherifnada]

Looks to be working as expected. @Zirochkaa will add a test and call it good!