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,19 +7,20 @@
 #
 # 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
 
 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())
@@ -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('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 log_formdata(params, path):
+    if os.path.isfile(path):
+        with open(path, 'ab+') as formlog:
+            formlog.seek(0)
+            reader = DictReader(formlog)
+            if reader.fieldnames != params.keys():
+                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():
+                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:
         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)
+        params = {field: params.get(field, '').encode('utf8')
+                  for field in fields}
+        time = datetime.datetime.now()
 
-        template_args = {
-            'time': datetime.datetime.now(),
-            'fields': {field: params.get(field, '') for field in fields}
-        }
-        sendMail(template, template_args)
+        if template is not None:

[Vasily Kuznetsov, 2017/03/09 19:58:09]

I like how you did it with 'csv_log' in config below, it's a lot more compact
than all this exception handling that we had before. Perhaps we do the same for
the template?

[Jon Sonesen, 2017/03/10 09:28:51]

I agree

+            template_args = {'time': time, 'fields': {field:

[Vasily Kuznetsov, 2017/03/09 19:58:09]

Yeah, like this it's hard to read indeed. What I was thinking about is just
making the value of 'fields' one line, like this:

            template_args = {
                'time': time,
                'fields': {field: params.get(field, '') for field in fields}
            }

[Jon Sonesen, 2017/03/10 09:28:51]

Done.

+                             params.get(field, '') for field in fields}}
+            sendMail(template, template_args)
+
+        params['time'] = time

[Vasily Kuznetsov, 2017/03/09 19:58:10]

This could also be inside of the if on the following line. Not a big deal, but I
think the logic is clearer this way, since it's only used inside of the if.

[Jon Sonesen, 2017/03/10 09:28:51]

Actually I thought that we always wanted a timestamp on the logs in which case
we would want it outside the if for the template and mail sending and inside of
the if statement for the log. Is that okay?

Also I am realizing that perhaps the sendMail call should be inside of a try
except in case the sendmail raises an exception the log will still be
written..does that make sense?

[Vasily Kuznetsov, 2017/03/10 09:54:28]

Yeah, indeed, we can just put the time into the params before sending and
logging. Sounds good.
Yeah, this defintely makes sense. Good catch.

[Jon Sonesen, 2017/03/14 19:41:26]

Done.

+        if 'csv_log' in config:
+            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():