✨ PromoteIQ : add #20694
✨ PromoteIQ : add #20694
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
|
thanks! |
|
Hello! 😄 Adding a new extension in AMP needs run through a design/implementation process, please check here for details. In general, we'd like to see an I2I (Intent-to-implement) discussing the requirements and high level tech design, and get them reviewed and approved before jumping into a PR. See an example of an I2I here: #20653 Thank you for opening this PR, and looking forward to your contribution 👍 |
|
@torch2424 I think they're just adding an ad network support |
|
@lannka Oops! Thanks for noticing that, I assumed it was a new extension from: @aisleypay Please disregard my previous comment, @lannka and myself can do the review for this 😄 Thank you for your contribution! |
|
@torch2424 Sorry for the wrong wording! I was a little confused myself when I classified this PR. |
There was a problem hiding this comment.
@aisleypay No worries at all! 😄
So I also went ahead and tested everything, and I couldn't get the ad to render. Also, what's odd, is that I get the renderStart_() function called on our end, even though you are not implementing it?
Let me know if this is all expected. cc @zhouyx for the renderStart comment I made above.
Thanks! 😄
examples/ads.amp.html
Outdated
| type="promoteiq" | ||
| data-src="https://cdn.tagdelivery.com/ad/client/standard.js" | ||
| data-params='{"slot": 1160, "targets": { "category": ["6222", "16158", "274", "17912"]}, "count": 2}' | ||
| data-sfcallback='function (response) { return response;}'> |
There was a problem hiding this comment.
Oh also, @aisleypay do you think this is possibly a security issue by allowing the user to embed / run their own JS through an attribute, that is then just run?
There was a problem hiding this comment.
Each of our clients have their own CDN. To enable our clients who want to use PromoteIQ on AMP-enabled pages, they have to pass that specific CDN, which includes client specific PromoteIQ logic.
The sfcallback is for client specific rendering. Each client can render an ad in any way they please. Our clients take our responses and render the responses in their desired format. In order for PromoteIQ to work on AMP enabled pages, the ability to allow clients to still use their custom rendering logic is key.
There was a problem hiding this comment.
@aisleypay Ah, thank you for the reason! That makes sense, but it definitely opens up a lot of security / Developer experience on the AMP side of things. Thus, I think it would be good to get a response from @lannka and/or @cramforce
There was a problem hiding this comment.
interesting, so you don't mind if the client is manipulating the ads or even fake clicks?
anyway, I don't see a security issue to AMP pages as we are rendering it in an iframe sandbox. It's more ad network's choice.
There was a problem hiding this comment.
@lannka Can you clarify what you mean by 'manipulating the ads'?
As far as fake clicks go, we work closely with our clients on inventory implementation - QA and constant monitoring. Impression/click capture and monitoring is part of the PromoteIQ integration process and for the lifetime of our relationship with the client. When we see suspicious behavior we work with our clients to resolve it.
There was a problem hiding this comment.
Got you. I guess you are probably doing more reservation ads, so you don't need to worry about pub modifying the ad contents to have click fraud.
There was a problem hiding this comment.
@lannka @torch2424 Indeed! Do either of you have any further questions or concerns per this thread?
* PromoteIQ - add * PromoteIQ - add * promoteiq - added global reference to TagDeliveryContent object * promoteiq - added newline to end of file and removed trailing spaces * removed spaces after curly brackets * added promoteiq properties to ads.extern.js for compiling purposes * replaces JSON.parse with parseJson wrapper function * updated variable referencing * updated promoteiq example and sfcallback function * removed promoteiq from file * updated dot notation to bracket notation
* PromoteIQ - add * PromoteIQ - add * promoteiq - added global reference to TagDeliveryContent object * promoteiq - added newline to end of file and removed trailing spaces * removed spaces after curly brackets * added promoteiq properties to ads.extern.js for compiling purposes * replaces JSON.parse with parseJson wrapper function * updated variable referencing * updated promoteiq example and sfcallback function * removed promoteiq from file * updated dot notation to bracket notation
Hi AMP community,
I'm Paisley Singh, Client Solutions Engineer at PromoteIQ.
Existing PromoteIQ customers want to use PromoteIQ on AMP-enabled pages. The purpose of this pull request is to create an AMP extension for PromoteIQ to enable our clients who use AMP technologies to continue to leverage PromoteIQ’s ad sciences.