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,
                                get_template)
 from sitescripts.web import registerUrlHandler, form_handler
 
 
 def get_config_items():
     config = get_config()
@@ -48,16 +49,66 @@
 
 
 def make_error(spec, check_type, default_message):
     if check_type in spec:
         return spec[check_type].value
     return default_message
 
 
+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('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():

[Jon Sonesen, 2017/03/22 09:23:30]

I think this is incorrect, after some local testing I found that if the fields
submitted by the user do not match what has been configured (ie the user is
crafting their own post requests) sendMail raises a 500, but the error log is
written with the additional field since only validates the user submited params.
So I think I need to also add logic to check that the fieldnames differ from the
configured fields. 

Does that make sense?

[Jon Sonesen, 2017/03/22 14:28:21]

Done.

+                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():
+                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

[Jon Sonesen, 2017/03/22 15:50:34]

while parametrizing the tests which cover config errors, i changed the test to
set the config['url'].value to an empty string and it does not raise an
exception. I think this should be changed to make sure that the value is no an
empty string and if so then it should raise an exception. This could occur of
someone added the url option but did not assign it a value. What do you think?

     except (KeyError, AttributeError):
         raise Exception('No URL configured for form handler: ' + name)
     try:
         template = config['template'].value
         get_template(template, autoescape=False)

[Jon Sonesen, 2017/03/22 15:50:34]

Currently we are letting this raise jinjas TemplateNotFound, which is
inconsistent with the rest of the logic in this function. I propose this instead
of allowing it to raise the TemplateNotFound exception we follow suit and catch
Template not found raising a generic exception with the custom message. This is
mostly so that we have consistency in the method. What do you think?

@@ -70,36 +121,36 @@
     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)
-
+        time = datetime.datetime.now()
         template_args = {
-            'time': datetime.datetime.now(),
+            'time': time,
             'fields': {field: params.get(field, '') for field in fields}
         }
-        sendMail(template, template_args)
+        try:
+            sendMail(template, template_args)
+        except:
+            start_response('500 Server Error', response_headers)

[Vasily Kuznetsov, 2017/03/21 14:00:35]

I wonder if we could log the stacktrace here somehow. Perhaps `stderr` would go
to the server log. Let's ask Matze what happens when multiplexer scripts print
to `stderr`.

[Jon Sonesen, 2017/03/22 14:28:21]

Done.

+            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():