Issue 4814 - Adds csv log to formmail2

sitescripts/formmail/test/test_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/>.
 
-import datetime
-from csv import DictReader
 from urllib import urlencode
 from urllib2 import urlopen, HTTPError
+from csv import DictReader
 
 import pytest
 from wsgi_intercept import (urllib_intercept, add_wsgi_intercept,
                             remove_wsgi_intercept)
 
 from sitescripts.formmail.web import formmail2
 
-
-@pytest.fixture
-def form_config():
-    return formmail2.conf_parse(formmail2.get_config_items())['test']
-
-
-@pytest.fixture
-def form_handlers(form_config, log_path):
-    """ Returns two handlers, one with logging configured and one without """

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

It would be better to have all the docstrings follow the same style, compatible
with our style guide (i.e. with PEP-257). I would propose something like this:

def function():
    """One line docstring."""

def important_function():
    """Multiline docstring.

    Here's the continuation
    that goes over many lines
    and is very long.
    """

What do you think?

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

Done.

-    # override configured path to log file with tmpdir path
-    form_config['csv_log'].value = log_path
-    log_handler = formmail2.make_handler('log_test', form_config)[1]
-
-    # parse a new config because the make_handler function changes the provided
-    # config object which causes the config parsing to break when creating
-    # the handler which has no log
-    no_log_config = formmail2.conf_parse(formmail2.get_config_items())['test']
-    del no_log_config['csv_log']

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

it seems that this is actually not working as expected. For some reason even
though both form handlers have their own config object when the request is
actually made the first time there is no 'csv_log' key. Perhaps some duplication
would not be so bad here so that we are not bundling multiple responses into
each test and we can more explicitly test the functionality. What is you
opinion?

-
-    return log_handler, formmail2.make_handler('log_test', no_log_config)[1]

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

Maybe we also assign the result of `make_handler` to a variable, named something
like `nolog_handler`, for example, and then return a tuple of those two
variables? Just to make it a bit more symmetric... it's mostly a question of
aesthetics, I know, so if you don't think it helps, feel free to ignore.

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

I was just thinking about what someone else on the review might think about
having a purely aesthetic declaration, although I agree that it would be
slightly more readable but also I feel that it is clear what is happening here
from the earlier variable assignment if one read the whole function. So I think
I will leave it.

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

Acknowledged.

+HOST = 'test.local'
+LOG_PORT = 80
+NO_LOG_PORT = 81
 
 
 @pytest.fixture
 def log_path(tmpdir):
     return str(tmpdir.join('test.csv_log'))
+
+
+@pytest.fixture
+def log_form_config():
+    return formmail2.conf_parse(formmail2.get_config_items())['test']
+
+
+@pytest.fixture
+def form_config():
+    config = formmail2.conf_parse(formmail2.get_config_items())['test']
+    del config['csv_log']
+    return config
+
+
+@pytest.fixture
+def form_handler(log_path, form_config, log_form_config):
+    """ Create two handlers, one that logs and another that doesn't """
+    log_form_config['csv_log'].value = log_path
+    return (formmail2.make_handler('test', log_form_config)[1],
+            formmail2.make_handler('test', form_config)[1])
 
 
 # We make this a fixture instead of a constant so we can modify it in each
 # test as needed without affecting other tests.
 @pytest.fixture
 def form_data():
     return {
         'email': 'john_doe@gmail.com',
         'mandatory': 'john_doe@gmail.com',
         'non_mandatory_message': 'Once upon a time\nthere lived a king.',
         'non_mandatory_email': 'test@test.com',
     }
 
 
 @pytest.fixture
-def responses_for(form_handlers, log_path):
-    """
-    Returns a list of response functions for handlers configured both with and
-    without logging configured
-    """
-    responses = []
-    for form_handler in form_handlers:

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

Nifty!

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

Thanks :)

-        host, port = 'test.local', 80
-        urllib_intercept.install_opener()
-        add_wsgi_intercept(host, port, lambda: form_handler)
-        url = 'http://{}:{}'.format(host, port)
+def response_for(form_handler):
+    """ Registers two intercepts, returns responses for them based on bool """
+    urllib_intercept.install_opener()
+    add_wsgi_intercept(HOST, LOG_PORT, lambda: form_handler[0])
+    add_wsgi_intercept(HOST, NO_LOG_PORT, lambda: form_handler[1])
 
-        def response_for(data):
-            if data is None:
-                response = urlopen(url)
-            else:
-                response = urlopen(url, urlencode(data))
-            return response.code, response.read()
-        responses.append(response_for)
-    yield responses
+    def response_for(data, log=False):
+        if log:
+            url = 'http://{}:{}'.format(HOST, LOG_PORT)
+        else:
+            url = 'http://{}:{}'.format(HOST, NO_LOG_PORT)
+        if data is None:
+            response = urlopen(url)
+        else:
+            response = urlopen(url, urlencode(data))
+        return response.code, response.read()
+
+    yield response_for
     remove_wsgi_intercept()
+
+
+@pytest.fixture
+def sm_mock(mocker):
+    return mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
+
+
+@pytest.mark.parametrize('key,message', [
+    ('url', 'No URL configured for form handler: test'),
+    ('fields', 'No fields configured for form handler: test'),
+    ('template', 'No template configured for form handler: test'),
+])
+def test_config_errors(key, message, form_config):
+    del form_config[key]
+    with pytest.raises(Exception) as error:
+        formmail2.make_handler('test', form_config)[1]
+    assert error.value.message == message
+
+
+@pytest.mark.parametrize('field,message', [
+    (('new_field', 'foo'), 'Unexpected field/fields: new_field'),
+    (('mandatory', ''), 'No mandatory entered'),
+    (('non_mandatory_email', 'asfaf'), 'Invalid email'),
+    (('email', 'asfaf'), 'You failed the email validation'),
+    (('email', ''), 'You failed the email test'),
+])
+def test_http_errs(field, message, response_for, form_data, sm_mock):
+    key, value = field
+    form_data[key] = value
+    with pytest.raises(HTTPError) as error:
+        response_for(form_data)
+    assert error.value.read() == message
+
+
+@pytest.mark.parametrize('field,expected', [
+    (('non_mandatory_message', '\xc3\xb6'), (200, '')),
+    (('non_mandatory_message', ''), (200, '')),
+])
+def test_success(field, expected, log_path, response_for, form_data, sm_mock):
+    key, value = field
+    form_data[key] = value
+    assert response_for(form_data, log=False) == expected
+    assert sm_mock.call_count == 1
+
+    params = sm_mock.call_args[0][1]['fields']
+    assert set(params.keys()) == set(form_data.keys())
+    for key, value in form_data.items():
+        assert params[key] == value.decode('utf8')
+
+    assert response_for(form_data, log=True) == expected
+    assert sm_mock.call_count == 2
+
+    assert response_for(form_data, log=True) == expected
+    assert sm_mock.call_count == 3
+
+    with open(log_path) as log_file:
+        reader = DictReader(log_file)
+        row = reader.next()
+        # rows should not be equal because the time field
+        # is added by the logging function.
+        assert row != reader.next()
+
+
+def test_config_field_errors(form_config):
+    form_config['fields'] = {}
+    with pytest.raises(Exception) as error:
+        formmail2.make_handler('test', form_config)[1]
+    assert error.value.message == 'No fields configured for form handler: test'
+
+
+def test_config_template_errors(form_config):
+    form_config['template'].value = 'no'
+    with pytest.raises(Exception) as error:
+        formmail2.make_handler('test', form_config)[1]
+    assert error.value.message == 'Template not found at: no'
 
 
 def test_config_parse(form_config):
     assert form_config['url'].value == 'test/apply/submit'
     assert form_config['fields']['email'].value == 'mandatory, email'
 
 
-def test_success(responses_for, form_data, mocker):

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

Should this test check for the existence of the log file, or would it be better
to add a separate test for this check? My thinking is that perhaps this should
be broken into 'test_log_success' and 'test_no_log_success' each with their own
response_for and handler. I am open to discussion on this though. What do you
think?

-    for response_for in responses_for:
-        sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-        assert response_for(form_data) == (200, '')
-        assert sm_mock.call_count == 1
-        params = sm_mock.call_args[0][1]['fields']
-        assert set(params.keys()) == set(form_data.keys())
-        for key, value in form_data.items():
-            assert params[key] == value
+def test_sendmail_fail(log_path, response_for, form_data, sm_mock):
+    sm_mock.side_effect = Exception('Sendmail Fail')
+    with pytest.raises(HTTPError):
+        response_for(form_data, log=True)
 
-
-def test_utf8_success(responses_for, form_data, mocker):
-    form_data['non_mandatory_message'] = '\xc3\xb6'
-    for response_for in responses_for:
-        sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-        assert response_for(form_data) == (200, '')
-        assert sm_mock.call_count == 1
-        params = sm_mock.call_args[0][1]['fields']
-        assert set(params.keys()) == set(form_data.keys())
-        for key, value in form_data.items():
-            assert params[key] == value
-
-
-def test_non_mandatory_no_msg(responses_for, form_data, mocker):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    form_data['non_mandatory'] = ''
-    for response_for in responses_for:
-        assert response_for(form_data) == (200, '')
-
-
-def test_invalid_email_cstm_msg(responses_for, form_data, mocker, form_config):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    form_data['email'] = 'bademail'
-    for response_for in responses_for:
-        with pytest.raises(HTTPError) as error:
-            response_for(form_data)
-        assert error.value.read() == 'You failed the email validation'
-
-
-def test_valid_nan_mandatory_email(responses_for, form_data, mocker):

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

Yikes! this is actually meant to test 'invalid_non_mandatory_email' will change

-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    form_data['non_mandatory_email'] = 'asfaf'
-    for response_for in responses_for:
-        with pytest.raises(HTTPError) as error:
-            response_for(form_data)
-        assert error.value.read() == 'Invalid email'
-
-    del form_data['non_mandatory_email']
-    for response_for in responses_for:
-        assert response_for(form_data) == (200, '')
-
-
-def test_mandatory_fail_dflt_msg(responses_for, form_data, mocker):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    del form_data['mandatory']

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

it is actually unlikely the request will be sent missing an actual field which
is what this does. So I will change it to set the form_data['mandatory'] to an
empty string.

-    for response_for in responses_for:
-        with pytest.raises(HTTPError) as error:
-            response_for(form_data)
-        assert error.value.read() == 'No mandatory entered'
-
-
-def test_collect_with_tmpl(log_path, form_data):
-    form_data['time'] = 'test'
-    formmail2.log_formdata(form_data, log_path)
-    with open(log_path) as csvfile:
-        assert DictReader(csvfile).next() == form_data
-
-
-def test_collect_no_tmpl(log_path, form_data, form_config):
-    del(form_config['template'])
-    form_data['time'] = 'test'
-    formmail2.log_formdata(form_data, log_path)
-    with open(log_path) as csvfile:
-        assert DictReader(csvfile).next() == form_data
-
-
-def test_fieldnames(log_path, form_data):
-    form_data['time'] = str(datetime.datetime.now())
-    formmail2.log_formdata(form_data, log_path)
-    with open(log_path) as csvfile:
-        for field in DictReader(csvfile).fieldnames:
-            assert field in tuple(form_data.keys())
-
-
-def test_field_err(form_config, form_data, log_path):
-    """
-    Submits a form that does not have the dame fields as previous submissions
-    that have the same form name, asserts that proper message is returned and
-    the row was properly written
-    """
-    formmail2.log_formdata(form_data, log_path)
-    del(form_config['fields']['email'])
-    del(form_data['email'])
-    try:
-        formmail2.log_formdata(form_data, log_path)
-    except Exception as e:
-        assert e.message == ('Field names have changed, error log '
-                             'written to {}_error').format(log_path)
-
-    with open(log_path+'_error') as error_log:
-        assert DictReader(error_log).next() == form_data
+    with open(log_path) as log_file:
+        row = DictReader(log_file).next()
+        assert row != form_data
 
 
 def test_append_field_err(form_config, form_data, log_path):
-    """
-    Submits two identical forms that do not match the previous fields
-    found in the log file, triggering two rows to be added to the error
-    log and asserting the proper message is returned and that the rows
-    were written as expected
+    """ Checks that error logs are correctly written and appended
+
+    Submits three forms, the second two have different fields to the first
+    and should be added to the same log file as each other, and be identical
     """
     formmail2.log_formdata(form_data, log_path)
-    del(form_config['fields']['email'])
-    del(form_data['email'])
-    try:
+    del form_data['email']
+
+    # submit two forms with fields that dont match the config
+    # this should append the second form to the error log file
+    with pytest.raises(Exception):
         formmail2.log_formdata(form_data, log_path)
-    except Exception:
-        pass
-    try:
+    with pytest.raises(Exception):
         formmail2.log_formdata(form_data, log_path)
-    except Exception as e:
-        assert e.message == ('Field names have changed, error log'
-                             ' appended to {}_error').format(log_path)
 
-    with open(log_path+'_error') as error_log:
+    with open(log_path + '_error') as error_log:
         reader = DictReader(error_log)
-        # two identical rows should be in the error log
         assert reader.next() == form_data
         assert reader.next() == form_data
-
-
-def test_append_log(form_data, log_path):
-    """
-    Collect data twice, altering a field in the second call
-    assert that the 2nd row is equal to the resulting form data
-    """
-    form_data['time'] = str(datetime.datetime.now())
-    formmail2.log_formdata(form_data, log_path)
-    form_data['email'] = 'test@foo.com'
-    formmail2.log_formdata(form_data, log_path)
-    with open(log_path) as csvfile:
-        reader = DictReader(csvfile, fieldnames=form_data.keys())
-        # header
-        reader.next()
-        row1 = reader.next()
-        row2 = reader.next()
-
-        assert row2['email'] == form_data['email']
-        assert row2['email'] != row1['email']