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

Fix Visitors and visits to include all visitors and their visits #15

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

Conversation

jdarrell
Copy link
Contributor

Description of change

  • The Visitors call has an additional parameter called only_identified that defaults to true. When it is true it only pulls visitor_ids for "identified" visitors, which means visitors that eventually became prospects. So by default the API call pulls a subset of all site visitors.
  • The Visits endpoint required that the list of input visitor_ids not have spaces between them.

Manual QA steps

  • Ran the tap locally, confirmed that the total number of visitors matched what was seen in the Pardot UI for our account.

Risks

  • None that I can see.

Rollback steps

  • revert this branch

@cmerrick
Copy link
Contributor

Hi @jdarrell, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@cmerrick
Copy link
Contributor

You did it @jdarrell!

Thank you for signing the Singer Contribution License Agreement.

@leslievandemark
Copy link
Contributor

Hi @jdarrell,
We released your PR and unfortunately had to roll it back because it caused other connections to fail.

One issue was the change of parameters in the ChildStream class caused Child streams other than Visits to fail. We were able to fix this issue by only applying your change for the Visits stream, not all ChildStreams in these two PRs #34 #32.

The other larger issue was that the added parameters to the Visitors stream caused this error: Detected out of order data. Current bookmark value 2020-10-25 02:01:40 is less than last bookmark value 2020-10-25 02:36:02. It appears that the additional visitors are not returned in order. Additional testing would be needed to confirm this, with possible changes to the bookmarking logic as a fix.

If you would like to test and submit a new PR with either or both fixes, we can move forward and re-release. Thanks again for your contribution.

@AlexFridman
Copy link

AlexFridman commented Jan 20, 2023

@leslievandemark @cmerrick hi! I've faced that issue -- visits and visitors streams are not populated. Why this pull request wasn't reviewed and merged yet?

If this change

params = {self.parent_id_param: ",".join([str(x) for x in parent_ids]), **self.get_params()}

can't be made to the ChildStream class simply override method for Visits class. This issue is a few years old. Could you please finally fix it?

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