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

parrableIdSystem: Send current page location to back-end #5123

Merged
merged 2 commits into from
Apr 16, 2020

Conversation

icflournoy
Copy link
Contributor

Type of change

  • Bugfix

Description of change

Send the page location (window.location.href) as determined by the logic in the refererDetection module to Parrable's ID system.

The non-Prebid integration with Parrable sends the value of window.location.href when making a request to our system. This change makes Prebid more closely match our non-Prebid integration.

@icflournoy
Copy link
Contributor Author

Hey @jdwieland8282 @jaiminpanchal27 I see that the CircleCI build failed on the BrowserStack step for this specific test:

TOTAL: 1 FAILED, 48977 SUCCESS

1) should notify after timeout period
     ExpiringQueue
     Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

This seems like a timing issue and not related to my change. Could you point me in the right direction on re-running a build, or updating the failing test so it is more robust?

@harpere harpere self-assigned this Apr 15, 2020
@harpere harpere self-requested a review April 15, 2020 19:34
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.

I know it's a small change, but ideally you add a test for the new param.

@harpere
Copy link
Collaborator

harpere commented Apr 15, 2020

@icflournoy unfortunately these tests are flakey and fail sporadically. Usually the reviewer will rerun if necessary.

@icflournoy
Copy link
Contributor Author

Hey, thanks for the feedback @harpere

I spent a bit of time trying to figure out how to add a good test for this additional property, but could not find an easy way forward. The property is something we are sending off to our backend servers, via an Xhr request.

I see that Prebid has an Xhr mock that is used in a few places to create a fake http server:

let result = JSON.parse(server.requests[0].requestBody);

This part makes sense to me.

I'm having trouble getting the parrableIdSystem module to actually have its getId callback method invoked during my test so that the mocked xhr object catches the outgoing request.

My understanding is that when userSync.syncDelay is set to 0 then the userId submodule callbacks are immediately invoked:

// when syncDelay is zero, process callbacks now, otherwise delay process with a setTimeout
if (syncDelay > 0) {
setTimeout(function() {
processSubmoduleCallbacks(submodulesWithCallbacks);
}, syncDelay);
} else {
processSubmoduleCallbacks(submodulesWithCallbacks);

but that doesn't seem to be happening.

General development questions:

  • How can I run only my parrableIdSystem_spec.js test? There is a note in the gulpfile that says I can do gulp test --file path/to/my/test.js but this does not work (fails on using import syntax)

    // If --file "<path-to-test-file>" is given, the task will only run tests in the specified file.

  • How do I enable debug logging during the test run? I've tried settings DEBUG_MODE=TRUE in constants.json, among other things, to get the utils.log* methods to print out, perhaps they are, but I do not see them when running the karma tests in a headless browser.

@harpere
Copy link
Collaborator

harpere commented Apr 16, 2020

Hi @icflournoy - I don't think it's worth all that trouble to add the test. Sorry to put you through that, but looking on the bright side, maybe a good learning experience! Going to try to get you answers to your dev questions, but in the mean time will approve and merge this.

@harpere harpere merged commit 6d45035 into prebid:master Apr 16, 2020
@icflournoy icflournoy deleted the PBID-19 branch April 16, 2020 13:24
@idettman
Copy link
Contributor

idettman commented Apr 16, 2020

How can I run only my parrableIdSystem_spec.js test? There is a note in the gulpfile that says I can do gulp test --file path/to/my/test.js but this does not work (fails on using import syntax)

Unfortunately, the ability to run a single test file is not functional at the moment, I'm not sure if there are plans to fix or not yet. But you can add .skip or .only to the describe blocks to ignore or isolate.

To add debug logging you'll need to add eslint ignore comments before your console.log calls

redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Apr 20, 2020
* 'master' of https://github.com/prebid/Prebid.js:
  Revert "New LuponMedia Bid Adapter (prebid#5120)" (prebid#5145)
  New LuponMedia Bid Adapter (prebid#5120)
  Feature/send publisher domain (prebid#5121)
  update test adunit params (prebid#5135)
  add parameter to the conversant adapter to override the url (prebid#5133)
  Replace call to old url module with call to utils (prebid#5136)
  GumGum: uses encodeURIComponent inline (prebid#5124)
  User ID's (liveramp, britepool) and gdpr (prebid#5114)
  fix wipes adapter response (prebid#5134)
  Submitting Tercept Analytics Adapter (prebid#5099)
  hotfix - undefined consent (prebid#5127)
  parrableIdSystem: Send current page location to back-end (prebid#5123)
  Increment pre version
  Prebid 3.16.0 Release
  Use a deepmerge function to merge both globa level config & bidder specific config (prebid#5039)
  Update appnexusBidAdapter.js (prebid#4893)
  Quantcast: Block bids without purpose 1 consent (prebid#5046)
  ShowHeroes adapter v2 (prebid#5085)
  Qc/qc usersyncs (prebid#4923)
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Apr 20, 2020
* master:
  Revert "New LuponMedia Bid Adapter (prebid#5120)" (prebid#5145)
  New LuponMedia Bid Adapter (prebid#5120)
  Feature/send publisher domain (prebid#5121)
  update test adunit params (prebid#5135)
  add parameter to the conversant adapter to override the url (prebid#5133)
  Replace call to old url module with call to utils (prebid#5136)
  GumGum: uses encodeURIComponent inline (prebid#5124)
  User ID's (liveramp, britepool) and gdpr (prebid#5114)
  fix wipes adapter response (prebid#5134)
  Submitting Tercept Analytics Adapter (prebid#5099)
  hotfix - undefined consent (prebid#5127)
  parrableIdSystem: Send current page location to back-end (prebid#5123)
  Increment pre version
  Prebid 3.16.0 Release
  Use a deepmerge function to merge both globa level config & bidder specific config (prebid#5039)
  Update appnexusBidAdapter.js (prebid#4893)
  Quantcast: Block bids without purpose 1 consent (prebid#5046)
  ShowHeroes adapter v2 (prebid#5085)
  Qc/qc usersyncs (prebid#4923)
iggyfisk pushed a commit to happypancake/Prebid.js that referenced this pull request Jun 22, 2020
* PBID-19: Send url (location.href) to Parrable ID system

* PBID-19: Remove es6 destructuring syntax for ie compat
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