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

Prebid core: fix bug with some native assets being lost from ortb native responses #8785

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

dgirardi
Copy link
Collaborator

@dgirardi dgirardi commented Aug 4, 2022

Type of change

  • Bugfix

Description of change

For reasons that I don't understand, there are two ways native assets can be retrieved for rendering - getAssetMessage and getAllAssetsMessage. #8086 only updated one of them, this PR brings the other in line.

It's unclear to me what the impact of this is, except that it currently can only affect bids coming from the Prebid Server adapter (because it's the only one using ortb in native bid responses). I am not sure how many creatives use one way over the other.

@muuki88
Copy link
Collaborator

muuki88 commented Aug 8, 2022

@dgirardi we stumbled over those two methods as well and I also do not know why those exist.
From a publisher perspective I can say that the getAssetMessage is way better from an API usage perspective.
The PUC requests all keys that are found in the template, which gives an easy to debug solution. And prebid.js can provide defaults for requested keys that the bidder has not provided.

Relates to #8773

@ChrisHuie ChrisHuie requested a review from ncolletti August 9, 2022 16:48
@ChrisHuie ChrisHuie self-requested a review August 9, 2022 16:48
@ChrisHuie ChrisHuie self-assigned this Aug 9, 2022
@anitaschiller
Copy link

@dgirardi I just used the debugging module to test our native ads and realized the following:

When visiting e.g. https://www.gutefrage.net/frage/bist-du-mit-dem-9-ticket-nach-sylt-gefahren-um-da-party-zu-machen?utm_source=home&utm_medium=question_carousel&utm_campaign=Blickwechsel-Sylt

and pasting

pbjs.setConfig({
  debugging: {
    enabled: true,
    intercept: [{
      when: {
        adUnitCode: 'gf_content_4'
      },
      then: {
        cpm: 10,
        bidderCode: 'yieldlab',
        mediaType: 'native',
        native: {
          clickUrl: 'https://example.com',
          title: 'Click this incredible test ad for you and the world',
          body: 'Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea tak',
          image: 'https://native-designs.h5v.eu/assets/stars-1200x627.jpg',
          privacyLink: 'https://yieldlab.com/privacy-platform/'
        }
      }
    }, ]
  }
});

into the console, everything works as expected and the ad is shown in gf_content_4:

Screenshot 2022-08-10 at 16 31 30


However, when adding a cta and/or sponsoredBy nothing happens anymore and slot-level targeting in the Google console is completely empty:

pbjs.setConfig({
  debugging: {
    enabled: true,
    intercept: [{
      when: {
        adUnitCode: 'gf_content_4'
      },
      then: {
        cpm: 10,
        bidderCode: 'smartadserver',
        mediaType: 'native',
        native: {
          clickUrl: 'https://example.com',
          title: 'Click this incredible test ad for you and the world',
          body: 'Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea tak',
          image: 'https://native-designs.h5v.eu/assets/stars-1200x627.jpg',
          privacyLink: 'https://yieldlab.com/privacy-platform/', 
          cta: 'click me', 
          sponsoredBy: 'Highfivve'
        }
      }
    }, ]
  }
});

Screenshot 2022-08-10 at 16 36 48

Does your PR fix this issue?

@dgirardi
Copy link
Collaborator Author

It does now @anitaschiller . However I don't know if that's a real problem - it was throwing an error because the response contained assets that were not requested, which should hopefully not happen in practice.

@dgirardi dgirardi force-pushed the fix-native-asset-message branch from 37af498 to 17b213b Compare August 10, 2022 15:56
@dgirardi dgirardi force-pushed the fix-native-asset-message branch from 17b213b to 7cf6b86 Compare August 10, 2022 17:29
@muuki88
Copy link
Collaborator

muuki88 commented Aug 10, 2022

Thanks for the quick debugging @dgirardi . Indeed cta was missing in the native field type.
This seems all so very fragile 😢

Copy link
Contributor

@ncolletti ncolletti left a comment

Choose a reason for hiding this comment

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

lgtm! only a minor comment that isn't necessary as a blocker to merging this PR

const ortbResponse = {
...legacyPropertiesToOrtbNative(legacyResponse),
assets: []
};

function useRequestAsset(predicate, fn) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor, the function name could be clearer to note it applies a request asset to a response.

@ChrisHuie ChrisHuie merged commit 984b5cf into prebid:master Aug 17, 2022
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
…ive responses (prebid#8785)

* Prebid core: fix bug with some native assets being lost from ortb native responses

* Do not throw if native response contains assets that were not requested
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
…ive responses (prebid#8785)

* Prebid core: fix bug with some native assets being lost from ortb native responses

* Do not throw if native response contains assets that were not requested
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