Adtelligent support both userSync types #3444
Conversation
…elligentAddSyncs
…elligentAddSyncs
There was a problem hiding this comment.
The point of the userSync type is that you should use iframe if the publisher supports it, else image. You shouldn't need BOTH iframe and image types in the same sync.
Please update this code so it has more selective behavior, or help me understand why you need both formats.
if the pub allows iframes, use the iframe version of the sync
else if the pub allows images, use the image version of the sync
Also, the way this code is structured, it's assumed that values in cookieURLs can be either iframes or images. Is that true in your case? If so, how? Your usersync server will need to respond with the appropriate thing: either a block of html or an image. How can the server know which type of request this is without something in the URL?
|
@bretg Yes, your comment makes perfect sense. I think this change was requested to avoid pre-integration work with publishers.
This could be achieved by checking content-type of request, either it's an image or document. |
| }) | ||
|
|
||
| describe('user syncs as iframe', function () { | ||
| it('should be returned if pixel enabled', function () { |
There was a problem hiding this comment.
I think you mean 'should be returned if iframe enabled'
|
|
||
| expect(syncs.map(s => s.url)).to.deep.equal(SERVER_DISPLAY_RESPONSE.cookieURLs); | ||
| expect(syncs.map(s => s.url)).to.deep.equal(SERVER_DISPLAY_RESPONSE_WITH_MIXED_SYNCS.cookieURLs); |
There was a problem hiding this comment.
So again - the original idea behind usersync was that iframes would be used if they were allowed by the pub and that image syncs were a fallback. We wouldn't expect both types of syncs if both are allowed. Just iframe. Trying to minimize the number of syncs here.
There was a problem hiding this comment.
Sorry. Did not get your point.
There was a problem hiding this comment.
if two syncs are required, and everything is allowed, then 2 syncs are called as expected. Our lineItems may require triggering multiple syncs for single bid, and their types do not matter.
|
Yieldbot test failing CI here. |
|
Note that this is only in the master (2.x) branch right now. If you need it in the 1.40.x-legacy branch, will need to be copied there. |
|
@bretg yeaaaah, I also need this change in 1.40 |
* Add user sync pixel logic in Adtelligent adapter * Add gdpr support * Add gdpr support * Update tests * Support both types of user syncs * More logical code with syncs * Remove "only" from test * Update test messages
Type of change
Description of change
Adapter response may provide sync type.
PS: Tests updated