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

[ads] Implement support for multiple conversion events #16091

Open
iambrianfung opened this issue May 26, 2021 · 6 comments · May be fixed by brave/brave-core#23615
Open

[ads] Implement support for multiple conversion events #16091

iambrianfung opened this issue May 26, 2021 · 6 comments · May be fixed by brave/brave-core#23615
Assignees
Projects

Comments

@iambrianfung
Copy link

The ads catalog may have multiple conversions set in the array.

When an conversion eligible user lands on the conversion URL, queue a conversion confirmation event with the 'name' value from the ads catalog.

Spec: https://docs.google.com/document/d/1-i2gBDQZ1bXnFIFHH3ZNz_kQH6Hr9hmzraFOOm_Fa_g/edit#

@iambrianfung iambrianfung added needs-discussion Although the issue is clear, we haven't yet reached a decision about the right solution. feature/ads OS/Android Fixes related to Android browser functionality OS/Desktop labels May 26, 2021
@iambrianfung iambrianfung added this to New issues in Ads via automation Jul 21, 2021
@tmancey tmancey moved this from New issues to Blocked in Ads Jul 26, 2021
@tmancey tmancey added blocked enhancement and removed OS/Android Fixes related to Android browser functionality labels Jul 26, 2021
@iambrianfung
Copy link
Author

iambrianfung commented Jul 28, 2021

Based on #17200; we have decided that if the local db and catalog have a conversion object with the same creative_set_id && same name but different url_pattern, update with the url_pattern from the catalog.
We do not want to write a new row bc updating the url_pattern via the catalog is often done to correct set up mistakes.

With VAC, it is crucial to update the row rather than creating a new one because we necessarily need the advertiser_public_key value, else we cannot encrypt for the conversionEnvelope

cc: @jsecretan @tmancey

@tmancey tmancey removed suggestion needs-discussion Although the issue is clear, we haven't yet reached a decision about the right solution. blocked labels Oct 24, 2023
@tmancey tmancey moved this from Blocked to Backlog in Ads Oct 24, 2023
@tmancey tmancey added priority/P3 The next thing for us to work on. It'll ride the trains. QA/Yes release-notes/exclude labels Oct 24, 2023
@tmancey tmancey changed the title Multiple Conversion Events [ads] Implement support for multiple conversion events Oct 25, 2023
@tmancey tmancey removed priority/P3 The next thing for us to work on. It'll ride the trains. QA/Yes release-notes/exclude labels Jan 31, 2024
@tmancey tmancey moved this from Backlog to Blocked in Ads Feb 21, 2024
@tmancey
Copy link
Contributor

tmancey commented Feb 21, 2024

Based on #17200; we have decided that if the local db and catalog have a conversion object with the same creative_set_id && same name but different url_pattern, update with the url_pattern from the catalog. We do not want to write a new row bc updating the url_pattern via the catalog is often done to correct set up mistakes.

With VAC, it is crucial to update the row rather than creating a new one because we necessarily need the advertiser_public_key value, else we cannot encrypt for the conversionEnvelope

cc: @jsecretan @tmancey

This was resolved in a previous browser release.

@tmancey
Copy link
Contributor

tmancey commented Apr 6, 2024

Most of this has been completed as part of other work. The only client-side tasks remaining are to change the primary key from creative set id to creative_set_id/name in the creative set conversion database table and to parse and add a name for each conversion which should be added to the confirmation token redemption payload for conversions. If the name node does not exist use an empty string, i.e., no-name. name will be added to a future catalog.

@tmancey
Copy link
Contributor

tmancey commented May 12, 2024

Based on #17200; we have decided that if the local db and catalog have a conversion object with the same creative_set_id && same name but different url_pattern, update with the url_pattern from the catalog. We do not want to write a new row bc updating the url_pattern via the catalog is often done to correct set up mistakes.

With VAC, it is crucial to update the row rather than creating a new one because we necessarily need the advertiser_public_key value, else we cannot encrypt for the conversionEnvelope

cc: @jsecretan @tmancey

Confirming this was fixed separate to this issue in the distant past.

@tmancey tmancey moved this from Blocked to Review in Ads May 13, 2024
@tmancey tmancey removed the blocked label May 13, 2024
@tmancey tmancey added priority/P3 The next thing for us to work on. It'll ride the trains. QA/Yes release-notes/exclude labels May 13, 2024
@tmancey tmancey self-assigned this May 13, 2024
@tmancey
Copy link
Contributor

tmancey commented May 13, 2024

@btlechowski this will require server-side changes, not-planned as of yet, however for QA we can override the catalog and add an optional urlPatternName for the conversion, if this optional field is not provided urlPatternName will use urlPattern as its name.

@tmancey
Copy link
Contributor

tmancey commented May 13, 2024

@ShivanKaul I will raise an privacy review once the PR is complete. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Ads
  
Review
Development

Successfully merging a pull request may close this issue.

2 participants