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

oneVideo Adapter: User sync pixel updates (VIDEOPUB-17981) #5583

Merged
merged 12 commits into from
Aug 18, 2020
Merged

oneVideo Adapter: User sync pixel updates (VIDEOPUB-17981) #5583

merged 12 commits into from
Aug 18, 2020

Conversation

adam-browning
Copy link
Contributor

Type of change

  • [ x] Bugfix

Description of change

Update to oneVideoBidAdapter cookie sync pixel urls.
Updated DBM sync pixel to ups/57304 + removed old yahoo sync pixel.
adapter version jump to 3.0.4

Other information

@DeepthiNeeladri please review thank you!

@adam-browning adam-browning deleted the VDEFECTS-5223 branch August 9, 2020 11:35
@adam-browning adam-browning restored the VDEFECTS-5223 branch August 9, 2020 11:36
@adam-browning
Copy link
Contributor Author

failed unit tests with GDPR

@adam-browning adam-browning reopened this Aug 9, 2020
@adam-browning adam-browning changed the title oneVideo Adapter: User sync pixel updates (VDEFECTS-5223) oneVideo Adapter: User sync pixel updates (VIDEOPUB-17981) Aug 10, 2020
Copy link
Collaborator

@harpere harpere left a comment

Choose a reason for hiding this comment

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

please remove package-lock.json. otherwise LGTM

@harpere harpere self-assigned this Aug 10, 2020
@adam-browning
Copy link
Contributor Author

Hi @harpere and thanks for reviewing!
I have removed the package-lock.json
FYI the failed circleci tests are not related to oneVideoBidAdapter AFAIK.

expect(pixel[2].type).to.equal('image');
expect(pixel[2].url).to.equal('https://sync-tm.everesttech.net/upi/pid/m7y5t93k?gdpr=1&gdpr_consent=' + GDPR_CONSENT_STRING + '&redir=https%3A%2F%2Fpixel.advertising.com%2Fups%2F55986%2Fsync%3Fuid%3D%24%7BUSER_ID%7D%26_origin%3D0&gdpr=1&gdpr_consent=' + encodeURI(GDPR_CONSENT_STRING));
expect(pixel[1].type).to.equal('image');
expect(pixel[1].url).to.equal('https://sync-tm.everesttech.net/upi/pid/m7y5t93k?gdpr=1&gdpr_consent=' + GDPR_CONSENT_STRING + '&redir=https%3A%2F%2Fpixel.advertising.com%2Fups%2F55986%2Fsync%3Fuid%3D%24%7BUSER_ID%7D%26_origin%3D0&gdpr=1&gdpr_consent=' + encodeURI(GDPR_CONSENT_STRING));
Copy link

@kprasadpvv kprasadpvv Aug 11, 2020

Choose a reason for hiding this comment

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

@adam-browning, Not Important , just a suggestion:
It would be good if we check for pixel[0].url / pixel[2].url to as you are actually changing those pixels.
like below:
expect(pixel[0].url).to.equal('https://pixel.advertising.com/ups/57304/sync?gdpr=&gdpr_consent=&_origin=0&redir=true')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood and thank you for the review @prasad457!
Updated tests accordingly.

@adam-browning adam-browning requested a review from harpere August 12, 2020 06:37
@adam-browning
Copy link
Contributor Author

Hi @harpere
Kindly requesting your review at your convenience.
Thanks in advance.
Adam

@harpere harpere merged commit b01fe09 into prebid:master Aug 18, 2020
@adam-browning adam-browning deleted the VDEFECTS-5223 branch August 24, 2020 12:16
BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
* updated DBM sync pixel to ups/57304 + removed yahoo sync pixel

* updated unit tests

* updated GDPR tests

* Update package-lock.json

* Update package-lock.json

* update local branch to origin

* added sync pixel unit tests

* Update package-lock.json
BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants