Issue 2267 - Unify form handling by reusing form_handler()

sitescripts/reports/web/updateReport.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-present 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 re
 import random
-from sitescripts.utils import get_config, get_template, encode_email_address
-from sitescripts.web import url_handler, form_handler, send_simple_response
+from sitescripts.utils import get_config, get_template
+from sitescripts.web import url_handler, form_handler
 from sitescripts.reports.utils import (calculateReportSecret,
                                        calculateReportSecret_compat, getReport,
                                        saveReport, sendUpdateNotification,
                                        getUserId, updateUserUsefulness)
 
+GUID_REGEX = r'^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$'
+
 
 @url_handler('/updateReport')
 @form_handler
-def handleRequest(environ, start_response, data):
-    params = {name: data.get(name, '').strip() for name in ['name', 'email',

[Vasily Kuznetsov, 2019/02/07 20:19:16]

These checks were not there before, right? If this is so, this would be new and
not completely related functionality. Maybe it's better to put these changes
into a separate change just to make things more clear. What do you think?

[rhowell, 2019/02/08 01:34:51]

Actually, it seems that most of these params are not mandatory anyway. (I think
only `guid` and `secret` are mandatory.) I was looking at formmail.py and
thinking it was updateReport.py. Oops, that's where my confusion came from.
Sorry!

https://codereview.adblockplus.org/5717434384252928/diff/5747976207073280/sit...

-              'subject', 'message', 'guid', 'secret', 'status', 'usefulness',

[Vasily Kuznetsov, 2019/02/07 20:19:16]

What do you think about putting this list of fields into a constant at the start
of the module? Would be a bit more organized and this line wouldn't have to be
broken so much.

[rhowell, 2019/02/08 01:34:52]

Turns out, we don't need this list for anything!

[Vasily Kuznetsov, 2019/02/08 19:06:07]

Acknowledged.

-                                                            'notify']}
-    missing = [k for k, v in params.iteritems() if not v]
-    if missing:
-        text = 'Missing fields: ' + ', '.join(missing)
-        return send_simple_response(start_response, 400, text)
-
-    try:
-        params['email'] = encode_email_address(params['email'])
-    except ValueError:
-        return send_simple_response(start_response, 400,
-                                    'Invalid email address')
-
+def handleRequest(environ, start_response, params):
     guid = params.get('guid', '').lower()
-    guid_regex = r'^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$'

[Vasily Kuznetsov, 2019/02/07 20:19:16]

This could also be a constant at the top of the module.

[rhowell, 2019/02/08 01:34:52]

Done.

-    if not re.match(guid_regex, guid):
+    if not re.match(GUID_REGEX, guid):
         return showError('Invalid or missing report GUID', start_response)
 
     reportData = getReport(guid)
 
     if reportData is None:
         return showError('Report does not exist', start_response)
 
     secret = calculateReportSecret(guid)
     if (params.get('secret', '') != secret and
        params.get('secret', '') != calculateReportSecret_compat(guid)):
@@ skipping 28 matching lines @@
     newURL += '#secret=' + secret
     start_response('302 Found', [('Location', newURL.encode('utf-8'))])
     return []
 
 
 def showError(message, start_response):
     template = get_template(get_config().get('reports', 'errorTemplate'))
     start_response('400 Processing Error',
                    [('Content-Type', 'application/xhtml+xml; charset=utf-8')])
     return [template.render({'message': message}).encode('utf-8')]