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

Improve verifiable conversion regex #16966

Closed
btlechowski opened this issue Jul 15, 2021 · 2 comments · Fixed by brave/brave-core#9453
Closed

Improve verifiable conversion regex #16966

btlechowski opened this issue Jul 15, 2021 · 2 comments · Fixed by brave/brave-core#9453

Comments

@btlechowski
Copy link

Correctly verifiable conversion regex works, but can be improved.
Regex was introduced in: https://github.com/brave/brave-core/pull/9089/files
Improvement suggested by @iambrianfung

Steps to Reproduce

  1. Check verifiable conversion regex

Actual result:

Regex is "^[a-zA-Z0-9/-]*$"

Expected result:

Regex is "^[-a-zA-Z0-9]*$"

Reproduces how often:

Easily reproduced

Brave version (brave://version info)

Brave 1.27.105 Chromium: 92.0.4515.93 (Official Build) (64-bit)
Revision 6eb43ff7850a1d710c3f827a0555737c74edab5c-refs/branch-heads/4515@{#1378}
OS Ubuntu 18.04 LTS

cc @jsecretan @iambrianfung @tmancey

@iambrianfung
Copy link
Collaborator

@tmancey
did a bit of searching around. it seems that the Golang regex checkers should be sufficiently good for our purposes. that being because re2 is developed by Google, as is Golang.
https://pkg.go.dev/regexp links to https://github.com/google/re2/wiki/Syntax
maybe the simplest way is to add a test against the strings foo-1234/, /foo-1234, foo/1234 and see that they should fail with ^[-a-zA-Z0-9]*$ but not ^[a-zA-Z0-9/-]*$
i also noticed that ^[a-zA-Z0-9-]*$ also works

@tmancey tmancey added QA/Yes priority/P3 The next thing for us to work on. It'll ride the trains. labels Jul 15, 2021
@tmancey tmancey self-assigned this Jul 15, 2021
@tmancey tmancey added this to the 1.29.x - Nightly milestone Jul 16, 2021
@btlechowski btlechowski added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Aug 18, 2021
@btlechowski
Copy link
Author

Verification passed on

Brave 1.29.74 Chromium: 93.0.4577.58 (Official Build) (64-bit)
Revision c4410ece044414ea42fa4ba328d08195e818a99c-refs/branch-heads/4577@{#1076}
OS Linux

Verified for conversion id qa123

Verified conversion was scheduled

[25430:25430:0829/062643.124868:VERBOSE1:conversions.cc(322)] Conversion for campaign id 8b30cb36-e5d4-460b-9e95-3199d80720bb, creative set id 46fde2a5-3cef-4960-bd77-36af52eb3192, creative instance id e793e5b5-c073-4ceb-8174-f5462d782d95 and advertiser id 9db2f01d-07d4-409b-a017-8415c80df432 on Sunday, August 29, 2021 at 6:26:43 AM
[25430:25430:0829/062643.158986:VERBOSE6:conversions.cc(375)] Successfully logged conversion event
[25430:25430:0829/062643.160440:VERBOSE3:conversions.cc(398)] Successfully appended conversion to queue
[25430:25430:0829/062643.167148:VERBOSE1:conversions.cc(503)] Convert campaign id 8b30cb36-e5d4-460b-9e95-3199d80720bb, creative set id 46fde2a5-3cef-4960-bd77-36af52eb3192, creative instance id e793e5b5-c073-4ceb-8174-f5462d782d95 and advertiser id 9db2f01d-07d4-409b-a017-8415c80df432 in 4 hours, 49 minutes, 47 seconds at 11:16 AM

Verified the conversion queue contains the conversion id
image

Verified the envelop has been attached to the confirmation
image

Verified for conversion id qa-123

Verified conversion was scheduled

[28748:28748:0829/224122.376338:VERBOSE1:conversions.cc(322)] Conversion for campaign id 8b30cb36-e5d4-460b-9e95-3199d80720bb, creative set id 46fde2a5-3cef-4960-bd77-36af52eb3192, creative instance id e793e5b5-c073-4ceb-8174-f5462d782d95 and advertiser id 9db2f01d-07d4-409b-a017-8415c80df432 on Sunday, August 29, 2021 at 10:41:22 PM
[28748:28748:0829/224122.394146:VERBOSE6:conversions.cc(375)] Successfully logged conversion event
[28748:28748:0829/224122.413191:VERBOSE3:conversions.cc(398)] Successfully appended conversion to queue
[28748:28748:0829/224122.423904:VERBOSE1:conversions.cc(503)] Convert campaign id 8b30cb36-e5d4-460b-9e95-3199d80720bb, creative set id 46fde2a5-3cef-4960-bd77-36af52eb3192, creative instance id e793e5b5-c073-4ceb-8174-f5462d782d95 and advertiser id 9db2f01d-07d4-409b-a017-8415c80df432 in 18 hours, 2 minutes, 37 seconds at 4:43 PM

Verified the conversion queue contains the conversion id
image

Verified the envelop has been attached to the confirmation
image

@btlechowski btlechowski added QA Pass-Linux and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Aug 29, 2021
@tmancey tmancey added this to Ads Jun 10, 2024
@tmancey tmancey moved this to Done in Ads Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants