Invibes Bid Adapter : change referrer formatting #10017
Conversation
Removed cutoff of url parameters from location param.
patmmccann
changed the title
add unit test for cut off location
|
@patmmccann I've added a unit test and all unit tests are now passing. The only error is triggered by failed test on gothamadsBidAdapter_spec on which I don't have control |
ChrisHuie
changed the title
|
Tests failed to run, circleci says 0 of 0 complete |
modules/invibesBidAdapter.js
Outdated
| @@ -372,7 +372,7 @@ function generateRandomId() { | |||
| } | |||
|
|
|||
| function getDocumentLocation(topWin) { | |||
| return topWin.location.href.substring(0, 300).split(/[?#]/)[0]; | |||
| return topWin.location.href.substring(0, 300); | |||
There was a problem hiding this comment.
Why not use site.page on the request object
There was a problem hiding this comment.
Greetings @patmmccann ! The purpose of this change request is only to stop truncating the domain url after the '?#' characters. We do not wish to change how we are getting the domain url. We have been using topWin.location.href since the adapter has been submitted years ago, we have had no issues with this and if we change it now, we would have to retest a lot of the integrations that are working fine now.
There was a problem hiding this comment.
prebid team and community have put a lot of work into making sure https://github.com/prebid/Prebid.js/blob/master/src/refererDetection.js gives the best possible results. Publishers and buyers often expect consistent determinations of site.page across integrations.
There was a problem hiding this comment.
Hello @patmmccann ! I understand the concern but you are requesting a change in the Invibes adapter that is not part of the scope of this pull request. From my knowledge this is not standard prebid policy.
This pull request however was for a bugfix on one of our integrations, the change was minor (removal of split function, NOT access to window object) and we needed to implement it as fast as possible (it has been dragging on for weeks at this point)
We will of course implement this if required as a SEPARATE issue, with a different pull request and we require the appropriate time for it.
Thank you for understanding
There was a problem hiding this comment.
This is not out of scope; you are accessing the page url incorrectly. Your pr revises how to gather the pageurl. It remains incorrect
There was a problem hiding this comment.
Hey @patmmccann, we have made the requested changes
There was a problem hiding this comment.
Hey @patmmccann , an update please?
| it('has location not cut off', function () { | ||
| const request = spec.buildRequests(bidRequests, {auctionStart: Date.now()}); | ||
| const parsedData = request.data; | ||
| expect(parsedData.location).to.exist; |
There was a problem hiding this comment.
Write out an actual input and output
updated unit tests to do a better check on location href
|
@patmmccann Can you please review? We made the requested changes more than 2 weeks ago. |
modules/invibesBidAdapter.js
Outdated
| var site = getConfig('site') || {}; | ||
|
|
||
| if (!site.page) { | ||
| site.page = bidderRequest.refererInfo.page; |
There was a problem hiding this comment.
this is all you need, you don't need to check config; however your circle ci tests failed
There was a problem hiding this comment.
hey @patmmccann , I've simplified the mode to get the page url and removed the config.
Regarding the unit tests, all of our unit tests are passsing, the errors are on gothamadsBidAdapter_spec.js and e_volutionBidAdapter_spec.js which are NOT our adapters so they are our of our control.
Please let me know if everything else is ok.
There was a problem hiding this comment.
Yes; looking into test failure, hopefully should have an answer quickly
simplified the fetching of the page url
remove unused config library
|
@mihaisandu07 can you pull in recent commits on master into this pr. Should fix the errors. |
|
@ChrisHuie thanks, I've synced the MASTER and everything looks ok now. |
* Update invibesBidAdapter.js Removed cutoff of url parameters from location param. * Update invibesBidAdapter_spec.js add unit test for cut off location * Update invibesBidAdapter_spec.js updated unit tests to do a better check on location href * changed the way InvibesBidAdapter gets the page location. * Update invibesBidAdapter.js simplified the fetching of the page url * Update invibesBidAdapter.js remove unused config library * updated invibes unit tests to accomodate the new page url fetching method
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
Removed cutoff of url parameters from location param sent to AdServer.
Other information