Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(292)

Issue 5177883412660224: Issue 2234 - Add a WSGI controller to collect email addresses for the Adblock Browser iOS launch (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by Sebastian Noack
Modified:
1 year, 1 month ago
Reviewers:
Wladimir Palant, kzar
CC:
Felix Dahlke, saroyanm, Philip Hill, mathias
Visibility:
Public.

Description

Issue 2234 - Add a WSGI controller to collect email addresses for the Adblock Browser iOS launch

Patch Set 1 : #

Patch Set 2 : Use codecs.open() #

Total comments: 12

Patch Set 3 : Addressed comments #

Total comments: 2

Patch Set 4 : Implemented double opt-in #

Patch Set 5 : Forbid new lines in email address #

Patch Set 6 : Use print statement #

Patch Set 7 : Added verification email template #

Patch Set 8 : Use parse_qs() instead dict(parse_qsl()) #

Total comments: 24

Patch Set 9 : Added missing colon #

Patch Set 10 : Got rid of redundant variable #

Patch Set 11 : Simplified verification code validation #

Total comments: 16

Patch Set 12 : Addressed comment #

Total comments: 8

Patch Set 13 : Renamed files #

Total comments: 2

Patch Set 14 : Moved signing logic into submit_email module #

Patch Set 15 : Changed response texts for consistency with landing page #

Patch Set 16 : Send error response when verification request is invalid #

Patch Set 17 : Removed "beta" from email text #

Patch Set 18 : Refactored #

Total comments: 7

Patch Set 19 : Addressed comments #

Patch Set 20 : URL-encode language before inserting into URL #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+185 lines, --2 lines) Patch
M .sitescripts.example View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +6 lines, -0 lines 0 comments Download
A sitescripts/submit_email/__init__.py View 1 2 3 4 5 6 7 8 9 10 11 12 0 chunks +-1 lines, --1 lines 0 comments Download
A sitescripts/submit_email/template/verification.mail View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +31 lines, -0 lines 0 comments Download
A sitescripts/submit_email/web/__init__.py View 1 2 3 4 5 6 7 8 9 10 11 12 0 chunks +-1 lines, --1 lines 0 comments Download
A sitescripts/submit_email/web/submit_email.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +93 lines, -0 lines 4 comments Download
M sitescripts/utils.py View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +19 lines, -0 lines 0 comments Download
M sitescripts/web.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +38 lines, -0 lines 0 comments Download

Messages

Total messages: 42
Sebastian Noack
4 years, 10 months ago (2015-04-02 13:58:56 UTC) #1
Sebastian Noack
4 years, 10 months ago (2015-04-05 13:10:08 UTC) #2
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sitescripts.example#newcode18 .sitescripts.example:18: sitescripts.extensions.web.downloads = I think you might need to rebase ...
4 years, 10 months ago (2015-04-05 15:45:14 UTC) #3
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sitescripts.example#newcode18 .sitescripts.example:18: sitescripts.extensions.web.downloads = On 2015/04/05 15:45:14, kzar wrote: > I ...
4 years, 10 months ago (2015-04-05 17:07:01 UTC) #4
kzar
LGTM
4 years, 10 months ago (2015-04-05 19:29:43 UTC) #5
Wladimir Palant
Only nits below, I commented on the conceptual concerns in the issue. http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/sitescripts/formsubscribe/web/formsubscribe.py File sitescripts/formsubscribe/web/formsubscribe.py ...
4 years, 10 months ago (2015-04-05 21:15:17 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/sitescripts/formsubscribe/web/formsubscribe.py File sitescripts/formsubscribe/web/formsubscribe.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/sitescripts/formsubscribe/web/formsubscribe.py#newcode33 sitescripts/formsubscribe/web/formsubscribe.py:33: with codecs.open(filename, 'a', 'utf-8', buffering=0) as file: On 2015/04/05 ...
4 years, 10 months ago (2015-04-06 09:53:17 UTC) #7
Sebastian Noack
4 years, 10 months ago (2015-04-06 11:07:28 UTC) #8
Wladimir Palant
Implementation looks fine, there are some conceptual issues being discussed in Trac however. http://codereview.adblockplus.org/5177883412660224/diff/5747976207073280/sitescripts/submitEmail/web/submitEmail.py File ...
4 years, 10 months ago (2015-04-06 18:07:29 UTC) #9
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5747976207073280/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5747976207073280/sitescripts/submitEmail/web/submitEmail.py#newcode37 sitescripts/submitEmail/web/submitEmail.py:37: file.write(u'\n') On 2015/04/06 18:07:30, Wladimir Palant wrote: > Nit: ...
4 years, 10 months ago (2015-04-06 18:17:26 UTC) #10
Sebastian Noack
I have uploaded a new patchset featuring double opt-in.
4 years, 10 months ago (2015-04-16 14:01:28 UTC) #11
Sebastian Noack
I've added the template for the verification email. So now this patch is no longer ...
4 years, 10 months ago (2015-04-21 15:17:47 UTC) #12
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt Nit: These option names seem a bit long, ...
4 years, 10 months ago (2015-04-21 17:14:31 UTC) #13
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/21 17:14:31, kzar wrote: > Nit: These ...
4 years, 10 months ago (2015-04-21 18:10:30 UTC) #14
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/21 18:10:30, Sebastian Noack wrote: > On ...
4 years, 10 months ago (2015-04-22 09:17:46 UTC) #15
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/22 09:17:46, kzar wrote: > On 2015/04/21 ...
4 years, 10 months ago (2015-04-22 10:06:00 UTC) #16
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/22 10:06:00, Sebastian Noack wrote: > On ...
4 years, 10 months ago (2015-04-22 10:13:04 UTC) #17
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/sitescripts/submitEmail/web/submitEmail.py#newcode54 sitescripts/submitEmail/web/submitEmail.py:54: if verification_code in ('', os.path.curdir, os.path.pardir): On 2015/04/22 10:13:04, ...
4 years, 10 months ago (2015-04-22 12:19:06 UTC) #18
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py#newcode38 sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) If we're going to use a ...
4 years, 10 months ago (2015-04-22 12:21:53 UTC) #19
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py#newcode38 sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 12:21:54, kzar wrote: > ...
4 years, 10 months ago (2015-04-22 12:28:44 UTC) #20
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py#newcode35 sitescripts/submitEmail/web/submitEmail.py:35: verification_code_regexp = re.compile(r'^[%s]+$' % re.escape(VERIFICATION_CODE_CHARS)) Shouldn't this be called ...
4 years, 10 months ago (2015-04-22 12:48:13 UTC) #21
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py#newcode35 sitescripts/submitEmail/web/submitEmail.py:35: verification_code_regexp = re.compile(r'^[%s]+$' % re.escape(VERIFICATION_CODE_CHARS)) On 2015/04/22 12:48:13, kzar ...
4 years, 10 months ago (2015-04-22 13:09:49 UTC) #22
kzar
LGTM http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py#newcode38 sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 13:09:49, Sebastian Noack ...
4 years, 10 months ago (2015-04-22 13:31:34 UTC) #23
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/web/submitEmail.py#newcode38 sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 13:31:34, kzar wrote: > ...
4 years, 10 months ago (2015-04-22 13:42:28 UTC) #24
Wladimir Palant
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/template/verification.mail File sitescripts/submitEmail/template/verification.mail (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/sitescripts/submitEmail/template/verification.mail#newcode3 sitescripts/submitEmail/template/verification.mail:3: Subject: Please confirm your Adblock Browser notification Ok, now ...
4 years, 10 months ago (2015-04-22 14:46:58 UTC) #25
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/21 18:10:30, Sebastian Noack wrote: > On ...
4 years, 10 months ago (2015-04-23 14:29:34 UTC) #26
Wladimir Palant
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/23 14:29:34, Sebastian Noack wrote: > While ...
4 years, 10 months ago (2015-04-23 14:39:24 UTC) #27
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sitescripts.example#newcode166 .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/23 14:39:25, Wladimir Palant wrote: > On ...
4 years, 10 months ago (2015-04-23 14:48:56 UTC) #28
Wladimir Palant
http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/sitescripts/signing.py File sitescripts/signing.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/sitescripts/signing.py#newcode6 sitescripts/signing.py:6: _SECRET = get_config().get('DEFAULT', 'secret') No, it should be .get('submit_email', ...
4 years, 10 months ago (2015-04-23 16:04:39 UTC) #29
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/sitescripts/signing.py File sitescripts/signing.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/sitescripts/signing.py#newcode6 sitescripts/signing.py:6: _SECRET = get_config().get('DEFAULT', 'secret') On 2015/04/23 16:04:40, Wladimir Palant ...
4 years, 10 months ago (2015-04-23 16:29:41 UTC) #30
Wladimir Palant
LGTM
4 years, 10 months ago (2015-04-23 18:46:02 UTC) #31
Sebastian Noack
The patchset has been updated, according to the changes documented in the issue.
4 years, 10 months ago (2015-04-24 11:41:49 UTC) #32
Sebastian Noack
And there is one more change: Apparently there won't be a beta, but only a ...
4 years, 10 months ago (2015-04-24 12:57:21 UTC) #33
Wladimir Palant
http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/sitescripts/submit_email/web/submit_email.py File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/sitescripts/submit_email/web/submit_email.py#newcode41 sitescripts/submit_email/web/submit_email.py:41: return send_simple_response(start_response, 400, 'Please enter a valid email address.') ...
4 years, 10 months ago (2015-04-24 22:50:48 UTC) #34
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sitescripts.example#newcode168 .sitescripts.example:168: successful_verification_redirect_location=https://adblockplus.org/en/adblock-browser/verification-success It seems wrong to have the locale "en" ...
4 years, 9 months ago (2015-04-27 10:07:02 UTC) #35
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sitescripts.example File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sitescripts.example#newcode168 .sitescripts.example:168: successful_verification_redirect_location=https://adblockplus.org/en/adblock-browser/verification-success On 2015/04/27 10:07:02, kzar wrote: > It seems ...
4 years, 9 months ago (2015-04-27 13:39:10 UTC) #36
kzar
LGTM
4 years, 9 months ago (2015-04-27 14:49:31 UTC) #37
Sebastian Noack
4 years, 9 months ago (2015-04-28 10:50:51 UTC) #38
kzar
http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/sitescripts/submit_email/web/submit_email.py File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/sitescripts/submit_email/web/submit_email.py#newcode91 sitescripts/submit_email/web/submit_email.py:91: location = location.format(lang=quote(params.get('lang') or 'en', '')) Looks like a ...
4 years, 9 months ago (2015-04-28 11:13:42 UTC) #39
Sebastian Noack
http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/sitescripts/submit_email/web/submit_email.py File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/sitescripts/submit_email/web/submit_email.py#newcode91 sitescripts/submit_email/web/submit_email.py:91: location = location.format(lang=quote(params.get('lang') or 'en', '')) On 2015/04/28 11:13:42, ...
4 years, 9 months ago (2015-04-28 11:31:24 UTC) #40
kzar
LGTM http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/sitescripts/submit_email/web/submit_email.py File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/sitescripts/submit_email/web/submit_email.py#newcode91 sitescripts/submit_email/web/submit_email.py:91: location = location.format(lang=quote(params.get('lang') or 'en', '')) On 2015/04/28 ...
4 years, 9 months ago (2015-04-28 11:33:41 UTC) #41
Sebastian Noack
4 years, 9 months ago (2015-04-28 11:36:04 UTC) #42
http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site...
File sitescripts/submit_email/web/submit_email.py (right):

http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site...
sitescripts/submit_email/web/submit_email.py:91: location =
location.format(lang=quote(params.get('lang') or 'en', ''))
On 2015/04/28 11:31:25, Sebastian Noack wrote:
> On 2015/04/28 11:13:42, kzar wrote:
> > Looks like a typo at the end there? ", ''"
> 
> The empty string at the end is the second argument to quote() to make it
escape
> backslashes as well.

I meant (forward) slashes.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5