Issue 2846 - Service for processing Microsoft Edge announcement subscription

sitescripts/submit_email/web/submit_email.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import fcntl
 import hmac
 import hashlib
 import wsgiref.util
 from urlparse import parse_qsl, urljoin
 from urllib import urlencode, quote
 
 from sitescripts.utils import get_config, sendMail, encode_email_address
 from sitescripts.web import url_handler, form_handler, send_simple_response
 
 VERIFICATION_PATH = '/verifyEmail'
+DEFAULT_PRODUCT = 'adblockbrowser'
 
 def sign(config, data):
   secret = config.get('submit_email', 'secret')
   return hmac.new(secret, data, hashlib.sha1).hexdigest()
 
 @url_handler('/submitEmail')
 @form_handler
 def submit_email(environ, start_response, data):
   email = data.get('email', '').strip()
   try:
     email = encode_email_address(email)
   except ValueError:
     return send_simple_response(
       start_response, 400,
       'Please enter a valid email address.'
     )
 
   config = get_config()
   params = [('email', email), ('signature', sign(config, email))]
   lang = data.get('lang')
   if lang:
     params.append(('lang', lang))
 
-  if data.get('product', '') == '':

[Oleksandr, 2015/08/13 04:12:07]

This "if" and the one below are here so that we would produce a non-ugly file
and template names. We might have stuff like edge_verification_template and
ios_verification_template, but I wasn't sure if we should be touching/renaming
the ios part.

[kzar, 2015/08/18 09:28:56]

Nit: `if data.get('product') is None`? (Or `if not data.get('product')`?)

[Sebastian Noack, 2015/08/18 16:35:03]

I think we should simply fallback to "adblockbrowser", adjusting the
configuration on the server, rather than having conditional logic here.

[Oleksandr, 2015/08/19 09:46:20]

Done.

-    email_template = 'verification_email_template'
-  else:
-    email_template = data.get('product', '') + '_verification_email_template'

[kzar, 2015/08/18 09:28:56]

We should probably assign data.get('product') to a variable above, and then
re-use it here.

[kzar, 2015/08/18 09:28:57]

Use string formatting like `"%s_verification_email_template" % product`?

[Sebastian Noack, 2015/08/18 16:35:03]

I think using the + operator when concatenating only two strings is fine. Format
strings neither make the code more readable nor faster in that case. Actually it
does the opposite. While it is the other way around when you concatenate more
strings.

[Oleksandr, 2015/08/19 09:46:20]

Done.

-    params.append(('product', data.get('product', '')))

[kzar, 2015/08/18 09:28:56]

Again, we should re-use the variable instead of performing `data.get('product',
'')`.

[Oleksandr, 2015/08/19 09:46:20]

Done.

+  product = data.get('product', DEFAULT_PRODUCT)
+  email_template = product + '_verification_email_template'
+  params.append(('product', product))
 
   sendMail(
     config.get('submit_email', email_template),
     {
       'recipient': email,
       'verification_url': '%s?%s' % (
         urljoin(wsgiref.util.application_uri(environ), VERIFICATION_PATH),
         urlencode(params)
       )
     }
@@ skipping 11 matching lines @@
   params = dict(parse_qsl(environ.get('QUERY_STRING', '')))
 
   email = params.get('email', '')
   signature = params.get('signature', '')
   if sign(config, email) != signature:
     return send_simple_response(
       start_response, 403,
       'Invalid signature in verification request.'
     )
 
-  if params.get('product', '') == '':

[kzar, 2015/08/18 09:28:57]

Same issues as above in this code.

-    filename = config.get('submit_email', 'filename')
-  else:
-    filename = config.get('submit_email', params.get('product', '') + '_filename')
+  product = params.get('product', DEFAULT_PRODUCT)
+  filename = config.get('submit_email', product + '_filename')
 
   with open(filename, 'ab', 0) as file:
     fcntl.lockf(file, fcntl.LOCK_EX)
     try:
       print >>file, email
     finally:
       fcntl.lockf(file, fcntl.LOCK_UN)
 
   location = config.get('submit_email', 'successful_verification_redirect_location')
   location = location.format(lang=quote(params.get('lang') or 'en', ''))
   start_response('303 See Other', [('Location', location)])
   return []