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

QuantcastId : Add support for firing pixel in quantcastIdSystem submodule #7107

Merged
merged 17 commits into from Jul 14, 2021

Conversation

sachinsurfs
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

This introduces a callback function to send a pixel request to Quantcast servers once the document is loaded when the right configurations are provided and we have received the necessary consent. Please contact asig@quantcast.com for any issues.

@ncolletti
Copy link
Contributor

ncolletti commented Jul 7, 2021

@sachinsurfs Could you provide a markdown file?

Add a markdown file for the Quantcast Id System.
@sachinsurfs
Copy link
Contributor Author

Hey @ncolletti - Done, I added a markdown file for the Quantcast Id System.

Comment on lines 211 to 217
// Callbacks on Event Listeners won't trigger if the event is already complete so this check is required
if (document.readyState === 'complete') {
firePixel(clientId, cookieExpDays);
}
window.addEventListener('load', function () {
firePixel(clientId, cookieExpDays);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the firePixel callback within the load event be within the else block of the document.readyState? May help to prevent rare cases where both can fire as readyState is marked 'complete' right before the window 'load' event fires.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like you mentioned, it is pretty rare since they ideally must happen at the same time. Regardless, I added an else block to be safe.

modules/quantcastIdSystem.md Outdated Show resolved Hide resolved
@ncolletti
Copy link
Contributor

@sachinsurfs Please add your userId config to userId.md

@ncolletti
Copy link
Contributor

ncolletti commented Jul 9, 2021

There are tests failing on this PR, the first test within 'QuantcastId module' and four within the 'fire pixel' -- "quantcastIdSubmodule.findRootDomain is not a function".

@sachinsurfs
Copy link
Contributor Author

@ncolletti - Regarding failed tests, can you tell me the steps you took to reproduce those errors? I found that gulp and browserstack tests on my machine and circleci are passing.

Testing on individual files like this -
gulp test --file "test/spec/modules/quantcastIdSystem_spec.js".
must be failing due to submodules effectively inheriting findRootDomain from the userId module (userId/index.js) and not being available during test time. There are similar errors on lotamePanoramaIdSystem_spec. I could fix the unit test by adding

import {findRootDomain} from 'modules/userId/index.js'

quantcastIdSubmodule["findRootDomain"] = findRootDomain;
sinon.stub(quantcastIdSubmodule, 'findRootDomain');

This, however, wouldn't catch situations where the submodule genuinely couldn't inherit that function. Is there any fix you would suggest?

Also, I see the CircleCI tests failing in another adaptor unrelated to this change. Do you know how I can resolve that?

Copy link
Contributor

@ncolletti ncolletti left a comment

Choose a reason for hiding this comment

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

LGTM

@ncolletti ncolletti merged commit 1a24e4d into prebid:master Jul 14, 2021
prebidtappx pushed a commit to prebidtappx/Prebid.js that referenced this pull request Nov 15, 2021
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.

None yet

3 participants