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

Appnexus Bid Adapter: update logic of native viewability script #8890

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

Since the introduction to the #8086 in 7.8.0, there was an issue accidentally introduced that prevented the appnexus native viewability script from properly executing. This PR rewrites the overall logic we had for the appnexus native viewability script to have it properly execute again.

Some historical context
There was custom logic implemented some years ago to initially suppress the native script, wait for the onBidWon trigger to execute, replace the native script's placeholders with values derived from the bid, and then activate the script on the page (see #4022 for details and the original logic).

One of the values that was needed to go into the script was the bid's adId. This value is normally created by prebid core (in the bidfactory.js) after the bid adapter creates the initial bid object (in the interpretResponse function) and passes it to the bidderFactory for further processing. So we previously waited for this adId value via the onBidWon trigger, when we knew it was made and available to be read.

Behavior of the issue
As part of the changes made with #8086, it shifted the location of the addWinningBid function call to occur separately (during the first Native Request postMessage call) instead of during the fireNativeTrackers postMessage call (https://github.com/prebid/Prebid.js/blame/master/src/secureCreatives.js#L118). This change was done to ensure that the native bid would always be marked as a winning bid from prebid.js' perspective (as there were certain use-cases with allAssetRequest in PUC that would not call Prebid.js to fire native trackers and have PUC take care of it).

Our native script was written to the page as part of the native javascriptTrackers process, which is triggered with the fireNativeTrackers function. The onBidWon trigger (that activated our script) is part of the addWinningBid function's process.

Prior to #8086, the fireNativeTracker was specifically called before the addWinningBid, which guaranteed the native script would be written (but not activated) onto the page and then the onBidWon would find the script, do the changes, and then activate it on the page. With the PR, this order of events basically got flipped and the onBidWon call never found the script on the page because it was written later in sequence (and remained inactive).

Fix
Thanks to the evolving nature of Prebid.js, now we're able to create the bid's adId in our adapter and have it be respected/used by prebid-core. This change enables use to remove the majority of the custom code and simply replace the script's placeholder values immediately when we receive the bid response (leaving the script in an active state that doesn't need further modifications during the onBidWon trigger).

I have ran some tests with the old native rendering (native-trk.js) and updated native rendering (native-render.js) and both seem to work with these changes.

PS: When I was updating the unit tests, I noticed that the big-richmedia adapter appears to be directly importing the spec object from the appnexusBidAdapter file. As a result, I needed to update their unit test that was impacted by this PR.

@jsnellbaker jsnellbaker requested review from dgirardi and Fawke August 23, 2022 17:58
@ChrisHuie ChrisHuie changed the title appnexus bid adapter - update logic of native viewability script Appnexus Bid Adapter: update logic of native viewability script Aug 24, 2022
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

Copy link
Collaborator

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

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

LGTM, with a caveat - this is not related to the viewability module is it? (otherwise I think it may be better to wait until we address #8928).

@jsnellbaker
Copy link
Collaborator Author

I don't know what the viewability module in prebid.js is tbh? Is that a newish feature?

@dgirardi
Copy link
Collaborator

dgirardi commented Aug 31, 2022

that answers my question :) yes, it's an attempt to support generic native viewability events (as defined in the native response eventtrackers; in the long term I suspect appnexus could use that without the need for special logic as in here, but it currently has some issues.

@patmmccann patmmccann merged commit 24637bc into master Aug 31, 2022
@patmmccann patmmccann deleted the appnexus_native_view_update branch August 31, 2022 20:34
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
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.

5 participants