Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
api updates as of 2/3 #48
api updates as of 2/3 #48
Changes from 2 commits
c3996af
1b4093d
22c1834
b800879
31279d4
4ff43b7
878e97a
76d428f
89cb26f
933a942
b30ded8
4c284e8
ee6f2b6
7c6eca8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-sheringuyen so that you are aware - the
click_uri
field will contain data for both text and spotlight ad types. No other ad types contain data for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Csgoodman do you have an example of the structure of this updated field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
circling back for documentation - it seems that a creative can only be of a single type -- dynamic ad, text or reference to InMail Content. So this field will follow essentially the same formatting and will only hold data for one of the previously mentioned ads (no need to worry about comma separated lists or anything!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, this was the
version_tag
column that seems to be deprecated - with no alternative solutions (for now). This should be just thelast_modified_at
/last_modified_timestamp
column, however, there are currently duplicate records.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming back to this comment, if I recall you mentioned there was a rollout on the connector side that should have deleted these duplicate records. These tests should catch and raise duplicates, so aren't we intentionally skipping those with the addition of the _fivetran_synced field?
My proposal would be to either remove _fivetran_synced and catch the true duplicates so customers may be aware and address them, or remove the test altogether. What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I took some time to look into this a bit more today - in our data, it looks like there could still be duplicates if the creative object was deleted from the source and therefore no longer synced into the destination.
I don't think engineering plans on removing duplicates in the above instances and therefore even if the test captured duplicates, I'm not sure if there would be an action item. 🤔 While it doesn't 100% feel right for me to just remove the test all together, I don't know what other options we truly have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed and added messaging within the CHANGELOG!