LiveYield Analytics Adapter #3443
Conversation
There was a problem hiding this comment.
Hi @tomek-jedro
I have some questions about some of the functions (see below); otherwise this overall looks good.
| * mapping. | ||
| */ | ||
| getAdUnitName: function(placementOrAdUnitCode) { | ||
| return placementOrAdUnitCode; |
There was a problem hiding this comment.
Is there more to be done here? The comment refers to a mapping of values, but this function just returns the argument variable.
If this setup is intended, could you clarify more about why this is needed and/or what this is adding?
There was a problem hiding this comment.
this function should be overridden by customer
There was a problem hiding this comment.
In that case - it may ideal to either surround the function calls in a try/catch block or validate the overridden functions set in the config.options to handle any accidental setups.
modules/liveyieldAnalyticsAdapter.js
Outdated
| * human friendly value. | ||
| */ | ||
| getPlacementOrAdUnitCode: function(bid, version) { | ||
| return version[1] === '0' ? bid.placementCode : bid.adUnitCode; |
There was a problem hiding this comment.
Could you expand upon the intended logic here?
Currently version comes in as a string like 1.37.0. So the version[1] would always equal a . character and cause the logic check to return the false value. Do you have a scenario/use-case where the true value of this expression would be returned?
There was a problem hiding this comment.
this will be fixed, that function should check for the first element
|
@jsnellbaker can you check it? everything should be fine, im happy to help |
There was a problem hiding this comment.
@tomek-jedro Thanks for making the recent requested updates. There is one more set of changes needed (due in part to something that was committed semi-recently).
Please take a look at the note below. We should be good to merge after this change is made.
modules/liveyieldAnalyticsAdapter.js
Outdated
| import adapter from 'src/AnalyticsAdapter'; | ||
| import adaptermanager from 'src/adaptermanager'; | ||
| import CONSTANTS from 'src/constants.json'; | ||
| import * as utils from 'src/utils'; |
There was a problem hiding this comment.
Due to a recent merge (see #3435), we now require any imports for a core or modules file (like this Analytics adapter file) to use a relative path style.
Additionally, the name of the file adaptermanager was changed to adapterManager in master (you may need to rebase with master for your branch to accept this type of change).
Can you please update these imports here?
|
@tomek-jedro Thanks for making these additional updates. Everything looks good now. |
* LiveYield Analytics Adapter * tests corrections * fixed getPlacementOrAdUnitCode function * corrections * paths fixed * adaptermanager import fixed * manager corrections
* LiveYield Analytics Adapter * tests corrections * fixed getPlacementOrAdUnitCode function * corrections * paths fixed * adaptermanager import fixed * manager corrections
* LiveYield Analytics Adapter * tests corrections * fixed getPlacementOrAdUnitCode function * corrections * paths fixed * adaptermanager import fixed * manager corrections
Type of change
Description of change
New PubOcean LiveYield Analytics adapter.