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 smartxBidAdapter #5275

Merged
merged 32 commits into from
Aug 25, 2020
Merged

Add smartxBidAdapter #5275

merged 32 commits into from
Aug 25, 2020

Conversation

smartclip-adtech
Copy link
Contributor

@smartclip-adtech smartclip-adtech commented May 20, 2020

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

  • new smartclipBidAdapter
  • test parameters for validating bids
    var adUnits = [{
        code: 'video1',
        mediaTypes: {
            video: {
                context: 'outstream',
                playerSize: [640, 360]
            }
        },
        bids: [{
            bidder: 'smartx',
            params: {
                tagId: 'Nu68JuOWAvrbzoyrOR9a7A',
                publisherId: '11986',
                siteId: '22860',
                bidfloor: 0.3,
                bidfloorcur: "EUR",
                at: 2,
                cur: ["EUR"],
                outstream_options: {
                    slot: 'video1'
                },
                user: {
                    data: [{
                        id: 'emq',
                        name: 'emq',
                        segment: [{
                            id: 'emq',
                            name: 'emq',
                            value: 'e0:k14:e24'
                        }]
                    }, {
                        id: 'gs',
                        name: 'gs',
                        segment: [{
                            id: 'gs',
                            name: 'gs',
                            value: 'tone_of_voice_dislike:tone_of_voice_negative:gs_health'
                        }]
                    }]
                }
            }
        }]
    }];

Be sure to test the integration with your adserver using the Hello World sample page.

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

Other information

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2020

This pull request introduces 4 alerts when merging ee2546c into 707a103 - view on LGTM.com

new alerts:

  • 4 for Invocation of non-function

@stale
Copy link

stale bot commented Jun 12, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 12, 2020
@stale stale bot removed the stale label Jun 15, 2020
@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2020

This pull request introduces 4 alerts when merging 7f09b06 into 8083239 - view on LGTM.com

new alerts:

  • 4 for Invocation of non-function

@stale
Copy link

stale bot commented Jun 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 29, 2020
@smartclip-adtech
Copy link
Contributor Author

The adapter is still "in review" - i don't know why.
Is there anything we have to do now?

@stale stale bot removed the stale label Jun 29, 2020
@lgtm-com
Copy link

lgtm-com bot commented Jun 29, 2020

This pull request introduces 4 alerts when merging 4a96e3a into 240e605 - view on LGTM.com

new alerts:

  • 4 for Invocation of non-function

@jsnellbaker
Copy link
Collaborator

@Skylinar

Thanks for making that update. The tests/files here look good.

Did you make the docs PR? I think that's the only outstanding item currently. If you did, please link it here.

Thanks!

},
video: {
context: 'outstream',
playerSize: [640, 360]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the player size itself to this fixed size or does it scale itself in a responsive way like the direct integration at the moment does?

Copy link
Contributor

Choose a reason for hiding this comment

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

The player sizes in a responsive way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay. So these sizes are just a hint? Or are they used in anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need these values for our system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But for what 😂

  • Some SSPs use those to calculate the ratio - then I need to make sure we insert widht x height with optimal ratio for our content
  • Some SSPs use those for the demand site - then we input the sizes with the highest demand
  • Some SSPs actually size the player with this - then I don't know what to do on a responsive site 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @muuki88,
we use the sizes to pass them on to the DSPs.

utils.logError(BIDDER_CODE + ': siteId is not present in bidder params');
return false;
}
if (!utils.getBidIdParameter('bidfloor', bid.params)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This parameter is actually optional, right? Below you check for empty string instead of undefined.

I would prefer leaving this parameter out instead of setting it to an empty string.

Also this seems to mix two types. Number and string , which I'm not sure is intended

Copy link
Contributor

Choose a reason for hiding this comment

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

No these are not optional. In the old approaches of the adapter these were optional, so these queries were written like this. Now these parameters are mandatory and so I threw out these queries and save the bidfloor and bidfloorcur in constants within the next commit.

utils.logError(BIDDER_CODE + ': bidfloorcur is not present in bidder params');
return false;
}
if (utils.deepAccess(bid, 'mediaTypes.video.context') == 'outstream') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Triple equals is usually the safer choice

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@lgtm-com
Copy link

lgtm-com bot commented Aug 24, 2020

This pull request introduces 4 alerts when merging a0a22f9 into 32066aa - view on LGTM.com

new alerts:

  • 4 for Invocation of non-function

@Skylinar
Copy link
Contributor

Hi @jsnellbaker,
here is the PR for docs: #2272

Thanks

@smartclip-adtech smartclip-adtech changed the title Add smartclipBidAdapter Add smartxBidAdapter Aug 25, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2020

This pull request introduces 4 alerts when merging aa4dbc1 into 01f4b28 - view on LGTM.com

new alerts:

  • 4 for Invocation of non-function

@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2020

This pull request introduces 4 alerts when merging 8b346a3 into 01f4b28 - view on LGTM.com

new alerts:

  • 4 for Invocation of non-function

};
const context1 = utils.deepAccess(currentBidRequest, 'mediaTypes.video.context');
const context2 = utils.deepAccess(currentBidRequest, 'params.ad_unit');
if (context1 == 'outstream' || context2 == 'outstream') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I overlooked those. Triple equals again is safer.

Why have two options configuring the context ? What happens when the context2 is oustream but the others are instream ? Then a player is loaded, but everything is configured for instream ?

Maybe this parameter can be removed all together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed context2, as we are not using params.ad_unit for context identifier.

var contextcustom = utils.deepAccess(bid, 'mediaTypes.video.context');
var placement = 1;

if (contextcustom == 'outstream') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Triple equals.

The context in the bid.params is not taken into account. See my comment below. Maybe the context in the params is redundant and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

… in params.ad_units. Adjusted query.

smartxBidAdapter_spec.js - fixed typo
@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2020

This pull request introduces 4 alerts when merging 3f2ceec into 01f4b28 - view on LGTM.com

new alerts:

  • 4 for Invocation of non-function

@jsnellbaker
Copy link
Collaborator

@smartclip-adtech @Skylinar

Just to confirm - are there any further changes planned (in lieu of the recent conversations)? Or are you all set from your end at this point? Thanks.

@Skylinar
Copy link
Contributor

We are ready at this time, but there will be updates in the future, for sure. Thanks a lot!

@jsnellbaker jsnellbaker merged commit 71ff934 into prebid:master Aug 25, 2020
var rs = this.readyState;
if (rs && rs != 'complete' && rs != 'loaded') return;
try {
SmartPlay(elementId, smartPlayObj);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jsnellbaker,
the adapter is released, thanks a lot.

We are facing now problems due to minified code that gets delivered when downloading the adapter from the official page. At this line we create our Outstream-Script Player and we are calling several functions (sc_smartIntxtStart(n), sc_smartIntxtNoad(n), sc_smartIntxtEnd(n), SmartPlay(elementId, smartPlayObj)) related to our out-stream player but in the minified version they get replaced with (void 0)(r) and console error Prebid ERROR: error caught : TypeError: (void 0) is not a function so the outstream part doesn't work, what can we do so that the function createOutstreamScript(bid) is not changed in the later download version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if you try to call the script function from the window instead of calling it on its own? Since it doesn't seem the SmartPlay variable gets defined within the scope of the bidAdapter code (which is likely why it converted it to the void(0) to check if it was undefined), calling from the window sould give it a definite reliable source even in the minified state.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are loading our out-stream playerlibary with the "https://dco.smartclip.net/?plc=7777777" call, there the mentioned functions are available. I have implemented the prebid.js on a test page and replaced the voids with the originally defined functions, everything works fine there.So in our opinion it is only necessary to prevent that the 4 function calls are not changed. Is this possible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue with the minified build is that it is likely seeing these variables never get assigned an actual value. While this isn't the case in a real page (because you expect the script to execute and overwrite the placeholders), it doesn't change from what build stack sees in the code.

In regards to the 3 'callback' functions, why are they defined here in the adapter file and not included as part of the script code? These functions don't read any values from inside the bidAdapter file to pass along into the script file. It's just my guess, but the callback functions appear to be internal functions that are within the script file. Could these 3 functions (onStartCallback, onCappedCallback, onEndCallback) just be taken out of the config object and moved inside the script itself?

For the SmartPlay call at line 376, I tried to change the reference to be window.SmartPlay(... and it appeared to work with the build file was minified (see example below).
Screen Shot 2020-08-28 at 10 09 44 AM

Can you give this a try on your end?

Copy link
Contributor

Choose a reason for hiding this comment

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

We use callback functions to give customers the flexibility to use callbacks should certain events occur. For example, customers can define the function sc_smartIntxtStart() on their webpage, which will be called by our player when the promotional video starts and the event gets fired.

The issue with the minified build is that it is likely seeing these variables never get assigned an actual value.

Do you mean this variables?

  // eslint-disable-next-line camelcase
  var sc_smartIntxtStart;
  // eslint-disable-next-line camelcase
  var sc_smartIntxtNoad;
  // eslint-disable-next-line camelcase
  var sc_smartIntxtEnd;
  var SmartPlay;

If yes: We added them back then on your hint due to lint errors, actually we don't need them and can leave them out. Or is it possible that exactly these variables confuse the minifier?
If not: Do you mean the functions?

Here is the test site only with added 4 functions, I've tested it with window. before and it seems to work as well but I would prefer to let them be as they are and use this solution only if no other is available:
https://gt-adtech.de/var/www/testsite/prod/prebid_test.html
Could you please check if there are options for the minifier to ignore these 4 functions or could be the solution to remove the above not needed variables?

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jsnellbaker,
did you already have time to have a look at it? We want to fix the problem as soon as possible, because the current version does not work like this.

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @Skylinar

I meant callback functions altogether. We can't have improperly defined functions within the code since it could lead to the chance that someone doesn't define the overridden function properly on their site and it would cause a hard undefined exception error in prebid - interfering with the rest of the auction.

If they're values that can be overwritten by the page so the publisher can customize what happens, then maybe this part of the logic could be handled inside the script itself instead of passing it in as part of a tentative config.

By removing them from being in an 'undefined'/uninitialized state in the adapter, it would avoid the lint errors and confusing part of the logic (since on its own, it's not clear where these functions are coming from or what they're doing).

I understand if there may be other reasons why this may not be an option (or at least something quick to implement).
In this case, I think you'll have to read these functions from the window to preserve their proper function status. I don't know of anything else we can do in the minifying step to just skip over this part of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jsnellbaker,
I've created a new PR where we call it with window. Could you please check?

#5689

Thank you!

},
onStartCallback: function (m, n) {
try {
sc_smartIntxtStart(n);
Copy link
Contributor

@Skylinar Skylinar Aug 28, 2020

Choose a reason for hiding this comment

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

See comment above.

},
onCappedCallback: function (m, n) {
try {
sc_smartIntxtNoad(n);
Copy link
Contributor

@Skylinar Skylinar Aug 28, 2020

Choose a reason for hiding this comment

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

See comment above.

},
onEndCallback: function (m, n) {
try {
sc_smartIntxtEnd(n);
Copy link
Contributor

@Skylinar Skylinar Aug 28, 2020

Choose a reason for hiding this comment

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

See comment above.

// const slot = utils.getBidIdParameter('slot', bid.renderer.config.outstream_options);
utils.logMessage('[SMARTX][renderer] Handle SmartX outstream renderer');
const elementId = bid.adUnitCode;
// eslint-disable-next-line camelcase
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that lines 335-340 are causing the problem? As far as i can remember it was pointed out in the course of the PR that we should define the callback functions as empty variables before to pass the lint test.

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.

7 participants