Skip to content
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

TDL-6619: Enable leads stream #180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

savan-chovatiya
Copy link
Contributor

@savan-chovatiya savan-chovatiya commented Dec 2, 2021

Description of change

TDL-6619: Enable leads stream.

  • Enabled leads stream
  • Added one condition for returning record with maximum replication_key for writing bookmark as currently, it is breaking tap(parsing None as a date) when no data is available for leads.

NOTE: No data available currently for leads and for that we need to create/activate ads and do some interaction but it's chargeable activity. As per card comment, scope related to 'leads' is already added in the OAuth app and marked ready to use so we will comment integration tests to not consider 'leads' stream in sync-related tests for now as per discussion.

Manual QA steps

Risks

Rollback steps

  • revert this branch

Comment on lines +490 to +491
if latest_lead:
return str(pendulum.parse(latest_lead[self.replication_key]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added above condition because the tap was breaking(parsing None as a date error) when no data is available for leads.
The above function returns the maximum replication key's value out of all leads for writing bookmark.

@@ -26,7 +26,7 @@ def expected_check_streams():
'ads_insights_region',
'ads_insights_dma',
"ads_insights_hourly_advertiser",
#'leads',
'leads',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is enabled as it's used in discover mode.

Copy link
Contributor

@kspeer825 kspeer825 left a comment

Choose a reason for hiding this comment

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

I believe there is test data for this stream, see my comment on the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants