-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
bidWatch Analytics Adapter : add creative endpoint #8710
Conversation
@matthieularere-msq can you please add unit tests for this pr? |
@ChrisHuie not that much. I've just add one line which should make sure that the lines related to gdpr vendor data is covered. However I don't really see how to add specific test for what was added as I don't see how to get access to my module's specific functions ? |
Ok, I found the way to add test for what my changes. |
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.
I have some suggestions below but this LGTM as it is.
let increment = args['cpm']; | ||
if (typeof saveEvents['auctionEnd'] == 'object') { | ||
for (let i = 0; i < saveEvents['auctionEnd'].length; i++) { | ||
let tmpAuction = saveEvents['auctionEnd'][i]; |
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.
these types of loops would look a bit cleaner as
for (let tmpAuction of saveEvents['auctionEnd']) { ... }`
or
saveEvents['auctionEnd'].forEach((tmpAuction) => {...})
Although I think auction
, bid
would be better names than tmpAuction
, tmpBid
etc.
if (removead && typeof arg['ad'] != 'undefined') { | ||
arg['ad'] = 'emptied'; | ||
} | ||
if (typeof arg['gdprConsent'] != 'undefined' && typeof arg['gdprConsent']['vendorData'] != 'undefined') { |
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.
IMO it'd be better to explicitly list what to keep rather than what to remove. As more features are added your payload will probably continue to grow, and I can't imagine new, unrecognized fields being too useful on your end. It would also make it less likely that we'd accidentally rename or remove some field that is important to you, because a simple code search will show us what you are depending on.
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.
Thanks for the suggestion @dgirardi
I am no javascript specialist so I often go to the easiest way I know without searching for subtilities. As I have another PR to send very shortly I'll make the suggested changes with this next one.
* New Analytics Adapter bidwatch * test for bidwatch Analytics Adapter * change maintainer address * Update bidwatchAnalyticsAdapter.js * Update bidwatchAnalyticsAdapter.js * Update bidwatchAnalyticsAdapter.md * Update bidwatchAnalyticsAdapter.md * add features to bidwatchAnalyticsAdapter * update tests * add test and made improvements
* New Analytics Adapter bidwatch * test for bidwatch Analytics Adapter * change maintainer address * Update bidwatchAnalyticsAdapter.js * Update bidwatchAnalyticsAdapter.js * Update bidwatchAnalyticsAdapter.md * Update bidwatchAnalyticsAdapter.md * add features to bidwatchAnalyticsAdapter * update tests * add test and made improvements
Type of change
Description of change