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

send rp_secure to frank for video bids #2476

Merged
merged 4 commits into from
May 1, 2018

Conversation

moonshells
Copy link
Contributor

Type of change

  • Bugfix

Description of change

In rubiconBidAdapter, send secure info to Frank for video bids

@snapwich snapwich self-requested a review May 1, 2018 21:03
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

Discussed with @moonshells , LGTM

Rich Snapp [2:46 PM]
how come it doesn't use the same logic present here? https://github.com/rubicon-project/Prebid.js/blob/65bdb86365ce325e3a676e01e287345047b9517d/modules/rubiconBidAdapter.js#L230

Zhaoyong Ma [2:48 PM]
What I know is publishers may send bid request from a "http" page but video player always expect creative url to start with "https" (edited)
To get creative url to start with "https", we have to send secure == 1 to Ad Engine

Rich Snapp [2:50 PM]
why not just send `rp_secure: 1` then?

Zhaoyong Ma [2:50 PM]
We want to provide the flexibility...
If the publisher still wants to send secure as false...

Rich Snapp [2:51 PM]
so if it's undefined it should default to true?

Zhaoyong Ma [2:51 PM]
Yes

Rich Snapp [2:52 PM]
is it fine that here `page_url = bidRequest.params.secure ? page_url.replace(/^http:/i, 'https:') : page_url;` undefined is treated as false?

Zhaoyong Ma [2:52 PM]
I think we need to remove this line

Rich Snapp [2:53 PM]
as part of this pull-request?

Zhaoyong Ma [2:53 PM]
this is a work around proposed by Antoine
right?
Yes
I'll make an update

Rich Snapp [2:53 PM]
okay.  if that line is out it'd probably be better, because at least then it's consistently using the same default for undefined :+1:

Zhaoyong Ma [3:02 PM]
Yes, I just removed that line

@snapwich snapwich merged commit 0654474 into prebid:master May 1, 2018
@moonshells moonshells deleted the send_secure_for_video branch May 1, 2018 21:23
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* send rp_secure to frank for video bids

* remove the workaround

* update unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants