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

Add Relaido Adapter #5101

Merged
merged 3 commits into from
Apr 13, 2020
Merged

Add Relaido Adapter #5101

merged 3 commits into from
Apr 13, 2020

Conversation

relaido
Copy link
Contributor

@relaido relaido commented Apr 9, 2020

Type of change

  • New bidder adapter

Description of change

New adapter

  • test parameters for validating bids
{
  bidder: 'relaido',
  params: {
    placementId: '9900'
  }
}

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

@Fawke Fawke self-assigned this Apr 9, 2020
@Fawke Fawke self-requested a review April 9, 2020 11:33
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

@relaido,

Thanks for the PR. I have gone through your code, looks good for the most part. But am facing an issue while I'm trying to test your banner and outstream video ads, taken from the md file.

I'm getting this error:

Screenshot 2020-04-09 at 5 40 24 PM

I did a little bit of digging and thought this maybe because hasUuid() is returning false in isBidRequestValid function. And, this maybe because for the first time, I don't have the value of uuid set in my local storage.

I see you're setting the value of localStorage in this function:

function receiveMessage() {

Is it possible for you to remove the event listener (added in the function receiveMessage) once you've set the key? (We don't want any open handlers)

@relaido
Copy link
Contributor Author

relaido commented Apr 9, 2020

Hi @Fawke , thanks for the review.
event listener removed.

@Fawke
Copy link
Contributor

Fawke commented Apr 10, 2020

@relaido

Thanks for making the change. But, when I'm trying to load your banner ad and video ad following the example you shared in the md file, and plugged those values in the hello_world example, am getting this error.

Screenshot 2020-04-09 at 5 40 24 PM

I think your isBidRequestValid function is returning false because it doesn't find the uuid in localStorage. Can you have a look please?

@relaido
Copy link
Contributor Author

relaido commented Apr 10, 2020

@Fawke

The first access results in an error because there is no uuid.
Since uuid is registered after UserSync, it succeeds from the second time on.

@Fawke
Copy link
Contributor

Fawke commented Apr 10, 2020

@relaido

I put your adapter js file through a debugger and made some observations:

  1. In the function setUuid(e), this if condition ((e.data && e.data.relaido_uuid)) is always returning a false. It maybe because the value of e.data is a string.

  2. In your getUserSyncs function, you need to have a check in place like this: syncOtions.iframeEnabled. Please take a look at a similar implementation of the appnexus or the rubicon adapter.

  3. Also, in the same function getUserSyncs, the parameter serverResponses is coming empty. So, you might better wanna put a check in place, something like: severResponses.length > 0

(serverResponses is empty because no auction is taking place because the function, isBidRequestValid is returning a false)

Also, are there any particular reasons for setting the localStorage value in getUserSyncs function? What if the publisher doesn't allow userSync? What if the pub only allows image sync and not iframe sync?

Since uuid is registered after UserSync, it succeeds from the second time on.

What do you mean second time on? You mean if I refresh the page again? A bit confused, because your isBidRequestValid function is always returning a false, leading to no auction taking place for your bidder. So, how will it succeed the second time on?

@relaido
Copy link
Contributor Author

relaido commented Apr 10, 2020

@Fawke

this if condition ((e.data && e.data.relaido_uuid)) is always returning a false

Sometimes returns true. e.data can also send objects.

It maybe because the value of e.data is a string.

You're right. Check with isPlainObject.

In your getUserSyncs function, you need to have a check in place like this: syncOtions.iframeEnabled.

Fixed. However, some people have said this.
#5017 (comment)

So, you might better wanna put a check in place, something like: severResponses.length > 0

Fixed.


Also, are there any particular reasons for setting the localStorage value in getUserSyncs function?

I want to save uuid in localStorage at the first access.
When the page is updated again, the auction is performed using the uuid stored in localStorage.

What if the publisher doesn't allow userSync?

Unfortunately, no auction is possible.

What if the pub only allows image sync and not iframe sync?

Auction cannot be done here as well.


What do you mean second time on?

Is to visit the same page again.
It is the same as refreshing the page.

You mean if I refresh the page again?

The auction will be held because uuid is stored in localStorage.

A bit confused, because your isBidRequestValid function is always returning a false, leading to no auction taking place for your bidder.

Hmm. I can conduct an auction.

So, how will it succeed the second time on?

Do you allow userSync and iframe sync?
Is relaido_uuid stored in localStorage?

@Fawke
Copy link
Contributor

Fawke commented Apr 10, 2020

@relaido

Fixed. However, some people have said this.
#5017 (comment)

Unfortunately, that's not right. We still need the check in place. The deprecation was for the publisher facing API, but internally, the adapter is still using iframeEnabled.

Also, I noticed one thing, when sending a banner response, you are returning a vast and overwriting bidResponse.ad with your renderer code.

Is this done to overcome some of prebid's limitations?

Rest of things look okay to me.

@relaido
Copy link
Contributor Author

relaido commented Apr 11, 2020

@Fawke

The deprecation was for the publisher facing API, but internally, the adapter is still using iframeEnabled.

Thanks. understood.

Is this done to overcome some of prebid's limitations?

Banner could be a safeframe. to overcome that, I pass VASTXML as a parameter.
I referenced this program.
https://github.com/prebid/Prebid.js/blob/master/modules/adgenerationBidAdapter.js#L112-L120

Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

LGTM

@Fawke Fawke merged commit 0604558 into prebid:master Apr 13, 2020
Fawke added a commit that referenced this pull request Apr 13, 2020
@Fawke Fawke mentioned this pull request Apr 13, 2020
1 task
@relaido
Copy link
Contributor Author

relaido commented Apr 16, 2020

@Fawke

doc also please review.
prebid/prebid.github.io#1917

redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Apr 16, 2020
* 'master' of https://github.com/prebid/Prebid.js: (102 commits)
  Marsmedia - Add vastXml and fix id response (prebid#5067)
  PubMatic adapter to support image sync (prebid#5104)
  minor consentManagement fix (prebid#5050)
  fix circle ci failing tests (prebid#5113)
  Add Relaido Adapter (prebid#5101)
  Add new bid adapter for ConnectAd (prebid#4806)
  change payload (prebid#5105)
  Utils updates (prebid#5092)
  Read OpenRTB app objects if set in config + bug fix for when ad units are reloaded (prebid#5086)
  Criteo : added first party data mapping to bidder request (prebid#4954)
  updateAdGenerationManual (prebid#5032)
  New bid adapter: Wipes (prebid#5051)
  Prebid manager analytics utm tags (prebid#4998)
  CRITEO RTUS Integration with Yieldmo Prebid (prebid#5075)
  isSafariBrowser update  (prebid#5077)
  Support min &max duration for onevideo (prebid#5079)
  increment pre version
  Prebid 3.15.0 release
  prebid#5011 Fix to set Secure attribute on cookie when SameSite=none (prebid#5064)
  Prebid adapter for windtalker (prebid#5040)
  ...
@Fawke Fawke added LGTM and removed needs update labels Apr 23, 2020
iggyfisk pushed a commit to happypancake/Prebid.js that referenced this pull request Jun 22, 2020
* add relaido adapter

* remove event listener

* fixed UserSyncs and e.data

Co-authored-by: ishigami_shingo <s.ishigami@relaido.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants