Issue 4814 - Adds csv log to formmail2

sitescripts/formmail/web/formmail2.py

 # This file is part of the Adblock Plus web scripts,
-# Copyright (C) 2006-2016 Eyeo GmbH
+# Copyright (C) 2006-2017 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/>.
+
+from __future__ import print_function
+
 import os
+import sys
 import datetime
+import traceback
 import collections
+from csv import DictWriter, DictReader
 
-from csv import DictWriter, DictReader

[Sebastian Noack, 2017/02/14 09:54:14]

The imports are incorrectly grouped here. See my comment in the tests.

[Jon Sonesen, 2017/02/20 09:55:16]

Done.

-from sitescripts.utils import get_config, sendMail, encode_email_address
+import jinja2
+
+from sitescripts.utils import (get_config, sendMail, encode_email_address,
+                               get_template)
 from sitescripts.web import registerUrlHandler, form_handler
 
 
 def get_config_items():
     config = get_config()
     default_keys = set(config.defaults())
     for name, value in config.items('formmail2'):
         if name not in default_keys:
             yield name, value
 
@@ skipping 15 matching lines @@
         store_value(conf_dict, path, value)
     return conf_dict
 
 
 def make_error(spec, check_type, default_message):
     if check_type in spec:
         return spec[check_type].value
     return default_message
 
 
-def formfield_error(parameters, log_path):
+def log_formfield_error(parameters, log_path):
     err_file = os.path.basename(log_path) + '_error'
     err_path = os.path.join(os.path.dirname(log_path), err_file)
     if os.path.isfile(err_path):
         with open(err_path, 'a') as error_log:
             writer = DictWriter(error_log, fieldnames=parameters.keys())
             writer.writerow(parameters)
-        raise Exception(

[Sebastian Noack, 2017/02/14 09:54:14]

We usually don't use hanging indentation. I'd prefer following indentation
style:

  raise Exception('Field names have changed, '
                  'error log appended to ' + err_path)

What do you think?

[Jon Sonesen, 2017/02/20 09:55:16]

Yeah, it seems easier to follow as well.

-                'Field names have changed, error log appended to ' + err_path)
+        raise Exception('Field names have changed, error log '
+                        'appended to ' + err_path)
     with open(err_path, 'w') as error_log:
         writer = DictWriter(error_log, fieldnames=parameters.keys())
         writer.writeheader()
         writer.writerow(parameters)
-    raise Exception(
-            'Field names have changed, error log written to ' + err_path)
+    raise Exception('Field names have changed, error log '
+                    'written to ' + err_path)
 
 
-def collect_formdata(params, path):
+def log_formdata(params, path):
     if os.path.isfile(path):
         with open(path, 'ab+') as formlog:
             formlog.seek(0)

[Sebastian Noack, 2017/02/14 09:54:14]

This seek seems redundant. When I open a file with 'ab+' (or any other mode),
the position is initially 0. If you have doubts, feel free to test it:

   f = open('/tmp/'fooo', 'ab+')
   f.tell()

[Vasily Kuznetsov, 2017/02/15 10:22:26]

This is not true on MacOS:

    $ python
    Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec  5 2015, 12:54:16)
    [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> f = open('multiplexer.py', 'ab+')
    >>> f.tell()
    1147

It seems that the standard doesn't really specify where the file pointer should
be in this situation. It only says that wherever you seek, the writing should be
at the end of file (although it also says "the application shall ensure that ...
input is not directly followed by output without an intervening call to a file
positioning function ..." so the second seek is also necessary then).

             reader = DictReader(formlog)
             if reader.fieldnames != params.keys():
-                formfield_error(params, path)
-            formlog.seek(1024, os.SEEK_END)

[Sebastian Noack, 2017/02/14 09:54:14]

So you seek 1024 bytes after the end of the file. What is this good for?

[Jon Sonesen, 2017/02/20 09:55:16]

This was a mistake, I was reading the last line (so it should have been -1024
bytes from the end of the file and asserting it wasnt a header. But I realized 
this was unnecessary, but forgot to correct the line...will fix

+                log_formfield_error(params, path)
+            formlog.seek(os.SEEK_END)
             writer = DictWriter(formlog, fieldnames=params.keys())
             writer.writerow(params)
         return
     with open(path, 'w') as new_formlog:
         writer = DictWriter(new_formlog, fieldnames=params.keys())
         writer.writeheader()
         writer.writerow(params)
     return
 
 
 def validate_fields(fields, params):
     errors = []
     for field, spec in fields.items():
-        if 'mandatory' in spec.value:
-            if field not in params.keys():
+        if 'mandatory' in spec.value and field not in params:
                 errors.append(make_error(spec, 'mandatory',
                                          'No {} entered'.format(field)))
-        if 'email' in spec.value and field in params.keys():
+        if 'email' in spec.value and field in params:
             try:
                 params[field] = encode_email_address(params[field])
             except ValueError:
                 errors.append(make_error(spec, 'email', 'Invalid email'))
+
+    unexpected_fields = ' '.join(set(params.keys()) - set(fields.keys()))
+    if unexpected_fields:
+        errors.append('Unexpected field/fields: ' + str(unexpected_fields))
     return errors
 
 
 def make_handler(name, config):
     try:
-        log_path = config['csv_log'].value
-    except KeyError:
-        raise Exception('No log configured for form handler: ' + name)
-    try:
         url = config['url'].value
     except (KeyError, AttributeError):
-        raise Exception('No URL configured for form handler:' + name)
+        raise Exception('No URL configured for form handler: ' + name)
     try:
         template = config['template'].value
-    except KeyError:
-        template = None
+        get_template(template, autoescape=False)
+    except (KeyError, AttributeError):
+        raise Exception('No template configured for form handler: ' + name)
+    except (jinja2.TemplateNotFound):
+        raise Exception('Template not found at: ' + template)
     try:
         fields = config['fields']
         for field, spec in fields.items():
             spec.value = {s.strip() for s in spec.value.split(',')}
     except KeyError:
-        raise Exception('No fields configured for form handler:' + name)
+        raise Exception('No fields configured for form handler: ' + name)
     if len(fields) == 0:
-        raise Exception('No fields configured for form handler:' + name)
+        raise Exception('No fields configured for form handler: ' + name)
 
     @form_handler
     def handler(environ, start_response, params):
         response_headers = [('Content-Type', 'text/plain; charset=utf-8')]
         errors = validate_fields(fields, params)
         if errors:
             start_response('400 Bad Request', response_headers)
             return '\n'.join(errors)
-        params['time'] = datetime.datetime.now()
-        if template is not None:
-            template_args = {
-                'time': params['time'],
-                'fields': {field: params.get(field, '') for field in fields}
-            }
+        time = datetime.datetime.now()
+        template_args = {
+            'time': time,
+            'fields': {field: params.get(field, '') for field in fields}
+        }
+        try:
             sendMail(template, template_args)
-        collect_formdata(params, log_path)
+        except:
+            print(traceback.print_exc(), file=sys.stderr)
+            start_response('500 Server Error', response_headers)
+            return ''
+        finally:
+            if 'csv_log' in config:
+                params = {field: params.get(field, '').encode('utf8')
+                          for field in fields}
+                params['time'] = time
+                log_formdata(params, config['csv_log'].value)
         start_response('200 OK', response_headers)
         return ''
 
     return url, handler
 
 
 conf_dict = conf_parse(get_config_items())
 for name, config in conf_dict.items():
     url, handler = make_handler(name, config)
     registerUrlHandler(url, handler)