Skip to content

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

Open
jdarrell wants to merge 8 commits into
singer-io:masterfrom
immuta:master
Open

Fix Visitors and visits to include all visitors and their visits#15
jdarrell wants to merge 8 commits into
singer-io:masterfrom
immuta:master

Conversation

@jdarrell

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

You did it @jdarrell!

Thank you for signing the Singer Contribution License Agreement.

@leslievandemark

Copy link
Copy Markdown
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

AlexFridman commented Jan 20, 2023

Copy link
Copy Markdown

@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