Issue 4814 - Adds csv log to formmail2

sitescripts/formmail/web/formmail2.py

Index: sitescripts/formmail/web/formmail2.py
===================================================================
--- a/sitescripts/formmail/web/formmail2.py
+++ b/sitescripts/formmail/web/formmail2.py
@@ -7,20 +7,21 @@
 #
 # 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 os
 import datetime
 import collections
 
+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
 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'):
@@ -47,57 +48,103 @@
 
 
 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):
+    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)
+    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)
+
+
+def collect_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

+            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():
+                errors.append(make_error(spec, 'mandatory',
+                                         'No {} entered'.format(field)))
+        if 'email' in spec.value and field in params.keys():
+            try:
+                params[field] = encode_email_address(params[field])
+            except ValueError:
+                errors.append(make_error(spec, 'email', 'Invalid email'))
+    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)
     try:
         template = config['template'].value
-    except (KeyError, AttributeError):
-        raise Exception('No template configured for form handler:' + name)
+    except KeyError:
+        template = None
     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)
     if len(fields) == 0:
         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 = []
-        for field, spec in fields.items():
-            if 'mandatory' in spec.value:
-                if field not in params.keys():
-                    errors.append(make_error(spec, 'mandatory',
-                                             'No {} entered'.format(field)))
-            if 'email' in spec.value and field in params.keys():
-                try:
-                    params[field] = encode_email_address(params[field])
-                except ValueError:
-                    errors.append(make_error(spec, 'email', 'Invalid email'))
+        errors = validate_fields(fields, params)
         if errors:
             start_response('400 Bad Request', response_headers)
             return '\n'.join(errors)
-
-        template_args = {
-            'time': datetime.datetime.now(),
-            'fields': {field: params.get(field, '') for field in fields}
-        }
-        sendMail(template, template_args)
+        params['time'] = datetime.datetime.now()
+        if template is not None:
+            template_args = {
+                'time': params['time'],
+                'fields': {field: params.get(field, '') for field in fields}
+            }
+            sendMail(template, template_args)
+        collect_formdata(params, log_path)
         start_response('200 OK', response_headers)
         return ''
 
     return url, handler
 
 
 conf_dict = conf_parse(get_config_items())
 for name, config in conf_dict.items():