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

🐛 Source Linkedin Ads: Fix issue where Accounts stream did not correctly handle provided account IDs #38013

Merged
merged 4 commits into from
May 7, 2024

Conversation

tolik0
Copy link
Contributor

@tolik0 tolik0 commented May 7, 2024

What

Fixed an issue where the Accounts stream did not correctly handle provided account IDs. Resolves: https://github.com/airbytehq/oncall/issues/5218

How

The format of IDs used for searches has changed from just an ID to a URN. Additionally, unlike all other search strings, URNs should be URL-encoded.
Added a new config for CAT to test multiple account IDs.

User Impact

Fixed an issue with the Accounts stream that occurred when account IDs were provided.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@tolik0 tolik0 self-assigned this May 7, 2024
Copy link

vercel bot commented May 7, 2024

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 May 7, 2024 10:11am

@tolik0 tolik0 marked this pull request as ready for review May 7, 2024 09:36
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label May 7, 2024
@@ -11,7 +11,7 @@ data:
connectorSubtype: api
connectorType: source
definitionId: 137ece28-5434-455c-8f34-69dc3782f451
dockerImageTag: 2.1.0
dockerImageTag: 2.1.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove dockerImageTag: 2.0.0 as issue is fixed now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@@ -175,7 +175,7 @@ def test_next_page_token(self, requests_mock, response_json, expected):
assert expected == result

def test_request_params(self):
expected = "pageSize=500&q=search&search=(id:(values:List(1,2)))"
expected = "pageSize=500&q=search&search=(id:(values:List(urn%3Ali%3AsponsoredAccount%3A1,urn%3Ali%3AsponsoredAccount%3A2)))"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add test config with account ids for check in cat to prevent similar issue in future?

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 the new config

@tolik0 tolik0 merged commit 787352c into master May 7, 2024
32 checks passed
@tolik0 tolik0 deleted the tolik0/source-linkedin-ads/adaccount-search-fix branch May 7, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/linkedin-ads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants