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-18288: Add after param for activities stream #24

Merged
merged 7 commits into from
Jun 15, 2022

Conversation

hpatel41
Copy link
Contributor

@hpatel41 hpatel41 commented Mar 24, 2022

Description of change

TDL-18288: Add after param for activities stream

  • Added after param for activities stream
  • Added orders param to sort the response in ascending order of tstamp

Manual QA steps

Risks

Rollback steps

  • revert this branch

@hpatel41 hpatel41 marked this pull request as ready for review March 28, 2022 07:09
Copy link
Contributor

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Looks good to me!


@mock.patch('requests.Session.request')
@mock.patch('tap_activecampaign.streams.Activities.process_records')
def test_activities_stream_params(self, mocked_process_records, mocked_request):

Choose a reason for hiding this comment

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

Could this test be executed at a higher level in the tap logic? What sets the after date that is passed to the sync function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a condition in the sync function which checks, if bookmark_query_field is set or not. If it is set, bookmark_query_field's value (here after) will be added to the query params and the date will be assigned to it.

@KrisPersonal KrisPersonal merged commit 894d6a9 into master Jun 15, 2022
@KrisPersonal KrisPersonal mentioned this pull request Jun 15, 2022
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.

6 participants