Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Created:
April 2, 2015, 1:57 p.m. by Sebastian Noack
Modified:
Jan. 18, 2019, 11:33 p.m.
Reviewers:
kzar, Wladimir Palant
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
April 2, 2015, 1:58 p.m. (2015-04-02 13:58:56 UTC) #1
Sebastian Noack
April 5, 2015, 1:10 p.m. (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 ...
April 5, 2015, 3:45 p.m. (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 ...
April 5, 2015, 5:07 p.m. (2015-04-05 17:07:01 UTC) #4
kzar
LGTM
April 5, 2015, 7:29 p.m. (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 ...
April 5, 2015, 9:15 p.m. (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 ...
April 6, 2015, 9:53 a.m. (2015-04-06 09:53:17 UTC) #7
Sebastian Noack
April 6, 2015, 11:07 a.m. (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 ...
April 6, 2015, 6:07 p.m. (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: ...
April 6, 2015, 6:17 p.m. (2015-04-06 18:17:26 UTC) #10
Sebastian Noack
I have uploaded a new patchset featuring double opt-in.
April 16, 2015, 2:01 p.m. (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 ...
April 21, 2015, 3:17 p.m. (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, ...
April 21, 2015, 5:14 p.m. (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 ...
April 21, 2015, 6:10 p.m. (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 ...
April 22, 2015, 9:17 a.m. (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 ...
April 22, 2015, 10:06 a.m. (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 ...
April 22, 2015, 10:13 a.m. (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, ...
April 22, 2015, 12:19 p.m. (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 ...
April 22, 2015, 12:21 p.m. (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: > ...
April 22, 2015, 12:28 p.m. (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 ...
April 22, 2015, 12:48 p.m. (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 ...
April 22, 2015, 1:09 p.m. (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 ...
April 22, 2015, 1:31 p.m. (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: > ...
April 22, 2015, 1:42 p.m. (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 ...
April 22, 2015, 2:46 p.m. (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 ...
April 23, 2015, 2:29 p.m. (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 ...
April 23, 2015, 2:39 p.m. (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 ...
April 23, 2015, 2:48 p.m. (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', ...
April 23, 2015, 4:04 p.m. (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 ...
April 23, 2015, 4:29 p.m. (2015-04-23 16:29:41 UTC) #30
Wladimir Palant
LGTM
April 23, 2015, 6:46 p.m. (2015-04-23 18:46:02 UTC) #31
Sebastian Noack
The patchset has been updated, according to the changes documented in the issue.
April 24, 2015, 11:41 a.m. (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 ...
April 24, 2015, 12:57 p.m. (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.') ...
April 24, 2015, 10:50 p.m. (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" ...
April 27, 2015, 10:07 a.m. (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 ...
April 27, 2015, 1:39 p.m. (2015-04-27 13:39:10 UTC) #36
kzar
LGTM
April 27, 2015, 2:49 p.m. (2015-04-27 14:49:31 UTC) #37
Sebastian Noack
April 28, 2015, 10:50 a.m. (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 ...
April 28, 2015, 11:13 a.m. (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, ...
April 28, 2015, 11:31 a.m. (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 ...
April 28, 2015, 11:33 a.m. (2015-04-28 11:33:41 UTC) #41
Sebastian Noack
April 28, 2015, 11:36 a.m. (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.

Powered by Google App Engine
This is Rietveld