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 urlparse import parse_qsl
 from sitescripts.utils import get_config, get_template
-from sitescripts.web import url_handler
+from sitescripts.web import url_handler, form_handler
 from sitescripts.reports.utils import (calculateReportSecret,
                                        calculateReportSecret_compat, getReport,
                                        saveReport, sendUpdateNotification,
                                        getUserId, updateUserUsefulness)
 
-
-def _getReport(guid, test_mode):
-    if not test_mode:
-        return getReport(guid)
-    return {'usefulness': 1}

[rhowell, 2019/02/02 05:45:22]

If I also return an email address here, we can test more of the code in
handleRequest(), but then we'd also have to intercept updateUserUsefulness() and
sendUpdateNotification(). Wanted to check with you about how it looks so far
before potentially going too far in the wrong direction.

[Vasily Kuznetsov, 2019/02/04 17:48:50]

I think it would make more sense to mock getReport and saveReport (in the
testing code) instead of introducing all this testing-related logic here in
production code. Something like this:

def test_xxx(mocker):
    mocker.patch('sitescripts.reports.web.updateReport.getReport',
                 new=lambda *args: {'usefulness': 1})
    mocker.patch('sitescripts.reports.web.updateReport.saveReport',
                 new=test_saveReport)
    # do our thing and the patching will be undone at the end of the test.

See the documentation of pytest-mock (https://pypi.org/project/pytest-mock/) for
more info on how it works.

[rhowell, 2019/02/07 03:54:32]

Done.

-
-
-def _saveReport(guid, report_data, test_mode):
-    if not test_mode:
-        return saveReport(guid, report_data)
+GUID_REGEX = r'^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$'
 
 
 @url_handler('/updateReport')
-def handleRequest(environ, start_response, test_mode=False):
-    if environ['REQUEST_METHOD'].upper() != 'POST' or not environ.get('CONTENT_TYPE', '').startswith('application/x-www-form-urlencoded'):
-        return showError('Unsupported request method', start_response)
-
-    try:
-        request_body_length = int(environ['CONTENT_LENGTH'])
-    except:
-        return showError('Invalid or missing Content-Length header', start_response)
-
-    request_body = environ['wsgi.input'].read(request_body_length)
-    params = {}
-    for key, value in parse_qsl(request_body):
-        params[key] = value.decode('utf-8')
-
+@form_handler
+def handleRequest(environ, start_response, params):
     guid = params.get('guid', '').lower()
-    if not re.match(r'^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$', guid):
+    if not re.match(GUID_REGEX, guid):
         return showError('Invalid or missing report GUID', start_response)
 
-    reportData = _getReport(guid, params['test_mode'])
+    reportData = getReport(guid)
 
-    if reportData == None:
+    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):
+    if (params.get('secret', '') != secret and
+       params.get('secret', '') != calculateReportSecret_compat(guid)):
         return showError('Wrong secret value', start_response)
 
     reportData['status'] = params.get('status', '')
     if len(reportData['status']) > 1024:
         reportData['status'] = reportData['status'][:1024]
 
     oldusefulness = reportData.get('usefulness', '0')
     reportData['usefulness'] = params.get('usefulness', '0')
 
     if 'email' in reportData:
-        updateUserUsefulness(getUserId(reportData['email']), reportData['usefulness'], oldusefulness)
+        updateUserUsefulness(getUserId(reportData['email']),
+                             reportData['usefulness'], oldusefulness)
 
-    _saveReport(guid, reportData, params['test_mode'])
+    saveReport(guid, reportData)
 
     if params.get('notify', '') and 'email' in reportData:
         email = reportData['email']
         email = re.sub(r' at ', r'@', email)
         email = re.sub(r' dot ', r'.', email)
         if re.match(r'^[\w.%+-]+@[\w.%+-]+(\.[\w.%+-]+)+', email):
             sendUpdateNotification({
                 'email': email,
                 'url': get_config().get('reports', 'urlRoot') + guid,
                 'status': reportData['status'],
             })
 
     newURL = get_config().get('reports', 'urlRoot') + guid
     newURL += '?updated=' + str(int(random.uniform(0, 10000)))
     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')])
+    start_response('400 Processing Error',
+                   [('Content-Type', 'application/xhtml+xml; charset=utf-8')])
     return [template.render({'message': message}).encode('utf-8')]