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.

Sign up for GitHub

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

Jump to bottom

Upgrade Linkedin API version from 202304 to 202403 #69

Merged
merged 12 commits into from
Apr 15, 2024
Merged

Upgrade Linkedin API version from 202304 to 202403 #69

merged 12 commits into from
Apr 15, 2024

Conversation

sgandhi1311
Copy link
Member

@sgandhi1311 sgandhi1311 commented Apr 13, 2024 •
edited
Loading

Description of change

  • API Updates

    • Bumped to API version 202304 (#69)
    • Updated API endpoints for the following streams: campaign_groups, campaigns, creatives

  • Pagination Enhancements

    • Implemented cursor-based pagination for the following streams: accounts, campaign_groups, campaigns, creatives

  • API Query Param Adjustments

    • Removed unsupported pivot and pivotValue from fields query parameters in Analytics API requests
    • Incorporated these values within the tap to ensure consistency with the previous tap version

  • Video Ads Stream

    • Added new fields (#71)
    • Requires scope - r_organization_social to sync the records.
    • data not available for change_audit_stamps in new endpoint

Manual QA Steps

  • Stream Verification

    • Synced all streams and verified the updated API endpoints for correctness.

  • Pagination Testing

    • Conducted tests to ensure the pagination logic functions correctly.

  • Data Consistency Check

    • Verified the record count aligns with the previous tap version 2.2.0.

  • Test Suite Maintenance

    • Updated and fixed unit tests and integration tests to reflect the recent changes.

Risks

  • Risk Level: Medium
    • Despite thorough testing, there is a possibility of encountering corner case scenarios.
    • The changes were made quickly to meet the API's deadline before it expired.

Rollback steps

  • Fix the error (if any) and deploy the change, as rollback won't work due to the API expiry.

@sgandhi1311 sgandhi1311 marked this pull request as ready for review April 14, 2024 16:35
somethingmorerelevant and others added 3 commits April 15, 2024 00:50
@somethingmorerelevant
Upgrade api version 202403 video ads (#71)
1ec1015 
* added video-ads implementation
* fixed pylint issues
* added handler for missing permission, removed test for skip condition
* updated message
* fstring -> format for 3.6 compatibility
@sgandhi1311
unit test fix
bcf7a97
@sgandhi1311
update setup.py and changelog.md
9885719
@sgandhi1311 sgandhi1311 changed the title Upgrade api version 202403 Upgrade Linkedin api version from 202304 to 202403 Apr 15, 2024
@sgandhi1311 sgandhi1311 changed the title Upgrade Linkedin api version from 202304 to 202403 Upgrade Linkedin API version from 202304 to 202403 Apr 15, 2024
rdeshmukh15
rdeshmukh15 reviewed Apr 15, 2024
View reviewed changes
CHANGELOG.md Outdated

- **Video Ads Stream**
- Added new fields ([#71](https://github.com/singer-io/tap-linkedin-ads/pull/71))
- Requires scope - `r_organization_social` to sync the records.
Copy link

@rdeshmukh15 rdeshmukh15 Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Scope requirements should be mentioned in README.md and not in CHANGELOG.md unless you have made changes on the tap side; specific to the scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

rdeshmukh15
rdeshmukh15 reviewed Apr 15, 2024
View reviewed changes
tap_linkedin_ads/streams.py Outdated
return super().sync_endpoint(*args, **kwargs)
except Exception as error:
if "Not enough permissions to access: partnerApiPostsExternal" in str(error):
LOGGER.info("Access to the video-ads API is denied due to insufficient permissions. Please reauthenticate or verify the required permissions.")
Copy link

Choose a reason for hiding this comment

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

Using LOGGER.warning seems more suitable at the place of LOGGER.info

Copy link
Member Author

Choose a reason for hiding this comment

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

done

rdeshmukh15
rdeshmukh15 reviewed Apr 15, 2024
View reviewed changes
tap_linkedin_ads/streams.py
"q": "search",
"sort.field": "ID",
"sort.order": "ASCENDING"
"q": "search"
Copy link

Choose a reason for hiding this comment

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

Is the data coming in sorted order, by default, in the new API version?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, as mentioned in docs

rdeshmukh15
rdeshmukh15 reviewed Apr 15, 2024
View reviewed changes
tap_linkedin_ads/streams.py Outdated
Comment on lines 151 to 153
# adding pivot and pivot_value to make it compatible with the previous tap version 2.2.0
element['pivot'] = pivot
element["pivot_value"] = temp_pivotValue
Copy link

@rdeshmukh15 rdeshmukh15 Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is making tap compatible with the previous version 2.2.0 really needed? Because the previous tap version is already deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed 2.2.0 from comments

rdeshmukh15
rdeshmukh15 requested changes Apr 15, 2024
View reviewed changes
Copy link

@rdeshmukh15 rdeshmukh15 left a comment

Choose a reason for hiding this comment

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

Not a blocker to merge this PR but should be considered removing the pivot and pivotValue occurrences if it is not supported anymore.

@sgandhi1311
Copy link
Member Author

Not a blocker to merge this PR but should be considered removing the pivot and pivotValue occurrences if it is not supported anymore.

We can think of it in the future, currently, the fields - pivot and pivotValue are set to automatic. This PR targets to update the API version on priority.

@somethingmorerelevant
Copy link
Member

somethingmorerelevant commented Apr 15, 2024 •
edited
Loading

@rdeshmukh15 there are few more deprecations with video_ads stream too, but since the api deprecation deadline is very near, we don't want to remove any schema objects thus avoiding a major version bump.
i am going to document these compatibility changes to be removed in the next major release.

Can you please approve this PR?

@sgandhi1311 sgandhi1311 merged commit bbab5c7 into master Apr 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdeshmukh15 rdeshmukh15 rdeshmukh15 approved these changes

@somethingmorerelevant somethingmorerelevant somethingmorerelevant approved these changes

@RushiT0122 RushiT0122 Awaiting requested review from RushiT0122

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sgandhi1311 @somethingmorerelevant @rdeshmukh15