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

VidoomyBidAdapter: refactor cookie sync #7601

Merged
merged 11 commits into from
Nov 2, 2021
Merged

Conversation

sasanfarokh
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

Just call cookie sync urls sequentially with a 500ms delay between them.

@sasanfarokh sasanfarokh changed the title fix: add delay between cookie sync calls VidoomyBidAdapter: add delay between cookie sync calls Oct 19, 2021
@lgtm-com
Copy link

lgtm-com bot commented Oct 19, 2021

This pull request introduces 1 alert when merging 5f816fe into a68796a - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

Copy link
Contributor

@FilipStamenkovic FilipStamenkovic left a comment

Choose a reason for hiding this comment

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

You need to change your cookiesync logic.
Your adapter needs to implement getUserSyncs function (you can take a look how other bid adapters do it). You cannot fire cookiesync in your adapter before auction is held.

modules/vidoomyBidAdapter.js Outdated Show resolved Hide resolved
modules/vidoomyBidAdapter.js Outdated Show resolved Hide resolved
@ChrisHuie
Copy link
Collaborator

ChrisHuie commented Oct 25, 2021

@sasanfarokh this was my bad when reviewing previously. Currently, this adapter is breaking a few guidelines. Can you please make the above changes requested by @FilipStamenkovic 👍

@ChrisHuie ChrisHuie self-requested a review October 25, 2021 13:45
@FilipStamenkovic
Copy link
Contributor

@sasanfarokh The main issue here is not that you are not using triggerPixel from utils.js, it's that you are doing cookie sync in a way that is not allowed.

Take a look at the comment made by @patmmccann: #7601 (comment)

@sasanfarokh
Copy link
Contributor Author

@sasanfarokh The main issue here is not that you are not using triggerPixel from utils.js, it's that you are doing cookie sync in a way that is not allowed.

Take a look at the comment made by @patmmccann: #7601 (comment)

Yes, I understood that, I was trying to check the getUserSyncs method,
It is a sync method, and as you see, Cookie sync urls are fetched from a json url, could you help me to find how can I do that?

@FilipStamenkovic
Copy link
Contributor

@sasanfarokh I hope this comment will help:

Steps:

  • remove cookiesync that is executed before auction is held
  • when requesting bids, from your backend in response return pixels you want to sync (the ones you are hosting here)
  • then implement getUserSyncs method, second argument is responses
  • you could utilize argument responses and retrieve pixels your backend has sent in step 2, something like:
const getUserSyncs = (syncOptions, responses, gdprConsent, uspConsent) => {
  if (syncOptions.iframeEnabled || syncOptions.pixelEnabled) {
    let pixelType = syncOptions.iframeEnabled ? 'iframe' : 'image';
    let pixels = deepAccess(responses, 'pixels') || [];

    let userSyncs = [];
    pixels.forEach(pixel => {
      userSyncs.push({
        type: pixelType,
        url: pixel + gdprConsent + uspConsent // here you need to construct url for each pixel
      });
    })

    return userSyncs;
  }
}

You don't have to do it exactly how I proposed, but something similar. If you are confused how this is working, you can use openx bidder on your local test page to see how they are syncing their cookies (it's similar to what I've proposed).

@FilipStamenkovic
Copy link
Contributor

@sasanfarokh Also, when your bidder returns empty response your adapter will log error message: ERROR: vidoomy: error parsing server response to Prebid format

You should consider doing early exit in interpretResponse function so that you don't log errors when there are no errors (you simply didn't have any bid, that's not an error). Something like:

if (!serverResponse.body) {
  return;
}

Copy link
Contributor

@FilipStamenkovic FilipStamenkovic left a comment

Choose a reason for hiding this comment

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

You test coverage is bellow 80%. You need more tests.

modules/vidoomyBidAdapter.js Show resolved Hide resolved
modules/vidoomyBidAdapter.js Outdated Show resolved Hide resolved
@ChrisHuie ChrisHuie removed their request for review October 28, 2021 14:01
Copy link
Contributor

@FilipStamenkovic FilipStamenkovic left a comment

Choose a reason for hiding this comment

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

While testing your bidder I found some small errors. You can see them in separate comments. From Prebid perspective, you don't have to fix those issues. But, I strongly suggest that you fix them anyway, it will make your adapter run a bit faster.

Also, why are you passing ua (userAgent string) in query params, you already have that information in HTTP header?

modules/vidoomyBidAdapter.js Show resolved Hide resolved
modules/vidoomyBidAdapter.js Outdated Show resolved Hide resolved
modules/vidoomyBidAdapter.js Show resolved Hide resolved
@sasanfarokh
Copy link
Contributor Author

While testing your bidder I found some small errors. You can see them in separate comments. From Prebid perspective, you don't have to fix those issues. But, I strongly suggest that you fix them anyway, it will make your adapter run a bit faster.

Also, why are you passing ua (userAgent string) in query params, you already have that information in HTTP header?

Hey, About the user-agent you're right, It's already exists in HTTP headers, but currently our backend apps may read it from querystring, I want to leave it as before.

Copy link
Contributor

@FilipStamenkovic FilipStamenkovic left a comment

Choose a reason for hiding this comment

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

Your tests are failing, please take a look at CircleCI output.

@FilipStamenkovic FilipStamenkovic changed the title VidoomyBidAdapter: add delay between cookie sync calls VidoomyBidAdapter: refactor cookie sync Nov 2, 2021
@FilipStamenkovic FilipStamenkovic merged commit e14b009 into prebid:master Nov 2, 2021
cpabst pushed a commit to sovrn/Prebid.js that referenced this pull request Jan 10, 2022
* fix: add delay between cookie sync calls

* fix(VidoomyBidAdapter): use default triggerPixel

* fix: use getUserSyncs hook to sync cookies

* fix: handle 204 no response without error log

* chore: refactor getUserSyncs

* test: update vidoomyBidAdapter tests

* fix: remove Macro

* fix: query params

* fix: gdpr consent

* fix: vidoomyBidAdapter tests

* fix: use USP_CONSENT in cookie sync urls
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.

4 participants