Refactor of Kargo Prebid adapter.

[njflynn]

Type of change

• Refactoring (no functional changes, no api changes)

Description of change

Refactoring of existing adapter to clean up code and send a smaller payload to the server

[jsadwith]

Hey Neil - thanks for working on this. I have some initial notes to get started. Keep in mind that the unit testing will need to be updated to support the new data formats.

@andyrusiecki you had mentioned (or was it Julian) that it'd be good to update some of the coding style in here to be more modern. Any ideas you can share?

[ssadman22]

looks nice, existing comments

[juliangan07]

mostly things that are already mentioned.

[ssadman22]

Looks good so far, minor things and comments above

[ssadman22]

+1 on the above

There's a lot of global variables here, some that are just used once. Consider using local variables instead
Ref: https://www.w3schools.com/js/js_best_practices.asp

[ssadman22]

Should eids be nested within user?

"user": {
        "kid": "9449b364-1e40-ff76-a2f1-e9a3caafe1c0", // Kargo ID 
        "cid": "de979bd7-73b5-4785-913e-baf079872c20", // Kargo client ID
        "crbids": {}, // Cerberus IDs
        "tdid": "ed1562d5-e52b-406f-8e65-e5ab3ed5583c", // Trade Desk ID
        "usp": "1---",
        "gdpr": {
            "consent": "",
            "applies": false
        },
        "eids": { // 3rd party user info from userIdAsEids
            [ 
                {

Ref: https://kargo1.atlassian.net/wiki/spaces/KRAK/pages/3171876871/Prebid.js+Adapter+-+Request+Format+Proposal

[ssadman22]

Nit: probably not necessary but should the entire route just be a const?

[njflynn]

In the latest changes I've added a "HOST" and "REQUEST_ENDPOINT" consts as part of a BIDDER object. That would allow for future reusability of both individually, should the need arise.

[ssadman22]

This looks like its duplicated

[njflynn]

Yep, nice spot

[juliangan07]

I think we can refactor this to use ES6's Object.entries. i.e:

  if (isEmpty(bids)) {
    return bidResponses;
  }

  if (typeof bids !== 'object') {
    return bidResponses;
  }

  Object.entries(bids).forEach((entry) => {
    const [bidID, adUnit] = entry
    let meta = {
      mediaType: adUnit.mediaType && BIDDER.SUPPORTED_MEDIA_TYPES.includes(adUnit.mediaType) ? adUnit.mediaType : BANNER,
    };

    if (adUnit.metadata && adUnit.metadata.landingPageDomain) {
      meta.clickUrl = adUnit.metadata.landingPageDomain[0];
      meta.advertiserDomains = adUnit.metadata.landingPageDomain;
    }

    const bidResponse = {
      requestId: bidID,
      cpm: Number(adUnit.cpm),
      width: adUnit.width,
      height: adUnit.height,
      ttl: 300,
      creativeId: adUnit.id,
      dealId: adUnit.targetingCustom,
      netRevenue: true,
      currency: adUnit.currency || bidRequest.currency,
      mediaType: meta.mediaType,
      meta: meta
    };

    if (meta.mediaType == VIDEO) {
      bidResponse.vastXml = adUnit.adm;
    } else {
      bidResponse.ad = adUnit.adm;
    }

    bidResponses.push(bidResponse);
  })

[juliangan07]

I think these can't reference spec anymore since it's now a function on the file itself and not part of the spec object

Suggested change

	const seed = spec._generateRandomUuid();
	const seed = _generateRandomUuid();

[njflynn]

I have removed this reference where I can. Some need to be maintain it to ensure unit test stubbing works correctly

[juliangan07]

Now that these are functions declared in the file, can probably omit the usages of _<functionName>.

Suggested change

	function _getCrbFromLocalStorage() {
	function getCrbFromLocalStorage() {

[njflynn]

I have done this for all non-exported functions. Functions that are exported in 'spec' object that are not expected still carry this prefix to distinguish them. Happy to remove from all if you think so

[juliangan07]

Looks good, just a suggestion.

[juliangan07]

Consider using Object.keys().forEach() or Object.entries() over a for in loop for some performance gain and modernizing this to use ES6.

[njflynn]

Yep, sorry I overlooked this in your previous review. Made that change now

[fionasequeira]

thanks for all the work here @njflynn was a good source of learning and understanding the adapter!

[fionasequeira]

just couple of questions i had when going through this PR

[ssadman22]

This looks great! Nice work taking care of such a large refactor. Just a few comments

[ssadman22]

Optional nit, but ES6 syntax:

SUA_ATTRIBUTES.forEach(suaKey => {
      const suaValue = uaClientHints[suaKey]
      if(!suaValue) {
        continue
      }

      // Do not pass any empty strings
      if(typeof suaValue == 'string' && suaValue.trim() === '') {
        continue
        
      switch (suaKey) {
        case SUA.MOBILE, SUA.SOURCE:
          if (suaValue < 1) {
            return;
          };
        default:
          suaValidAttributes.push(suaKey);
      }
});

[ssadman22]

Nit: extra line

Suggested change

	timeoutData.forEach((bid) => {
	sendTimeoutData(bid.auctionId, bid.timeout);
	});
	}
	timeoutData.forEach((bid) => {
	sendTimeoutData(bid.auctionId, bid.timeout);
	});
	}

[ssadman22]

Should this be generateRandomUUID? Same with _getCrb and _getSessionId

[ssadman22]

Just read your comment

I have done this for all non-exported functions. Functions that are exported in 'spec' object that are not expected still carry this prefix to distinguish them. Happy to remove from all if you think so

Feel free to disregard :)

[jsadwith]

Thanks for all the work on this @njflynn. I'm running some local tests on this today to compare that we're getting the same data we were previously getting. Will keep you updated.

[jsadwith]

@njflynn I tested both banner and video and things are looking good. I made some minor changes in the code, but all data is being passed as expected! Great work on this! Once the server-side is deployed, we can do some actual test requests with this and if all looks good, we can get it tested on some Fabrik sites (like Distractify) where we control the Prebid.js integrations.