Issue 4814 - Adds csv log to formmail2

sitescripts/formmail/test/test_formmail2.py

Index: sitescripts/formmail/test/test_formmail2.py
===================================================================
--- a/sitescripts/formmail/test/test_formmail2.py
+++ b/sitescripts/formmail/test/test_formmail2.py
@@ -7,104 +7,236 @@
 #
 # 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
 
 import pytest
 from wsgi_intercept import (urllib_intercept, add_wsgi_intercept,
                             remove_wsgi_intercept)
 
 from sitescripts.formmail.web import formmail2
 
 
-@pytest.fixture()
+@pytest.fixture
 def form_config():
     return formmail2.conf_parse(formmail2.get_config_items())['test']
 
 
-@pytest.fixture()
-def form_handler(form_config):
-    return formmail2.make_handler('test', form_config)[1]
+@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.

+
+
+@pytest.fixture
+def log_path(tmpdir):
+    return str(tmpdir.join('test.csv_log'))
 
 
 # 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'
+        'non_mandatory_email': 'test@test.com',
     }
 
 
-@pytest.fixture()
-def response_for(form_handler):
-    host, port = 'test.local', 80
-    urllib_intercept.install_opener()
-    add_wsgi_intercept(host, port, lambda: form_handler)
-    url = 'http://{}:{}'.format(host, port)
+@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(data):
-        if data is None:
-            response = urlopen(url)
-        else:
-            response = urlopen(url, urlencode(data))
-        return response.code, response.read()
-
-    yield response_for
+        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
     remove_wsgi_intercept()
 
 
 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(response_for, form_data, mocker):
-    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_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_non_mandatory_no_msg(response_for, form_data, mocker):
+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'] = ''
-    assert response_for(form_data) == (200, '')
+    for response_for in responses_for:
+        assert response_for(form_data) == (200, '')
 
 
-def test_invalid_email_cstm_msg(response_for, form_data, mocker, form_config):
+def test_invalid_email_cstm_msg(responses_for, form_data, mocker, form_config):
     mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
     form_data['email'] = 'bademail'
-    with pytest.raises(HTTPError) as error:
-        response_for(form_data)
-    assert error.value.read() == 'You failed the email validation'
+    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(response_for, form_data, mocker):
+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'
-    with pytest.raises(HTTPError) as error:
-        response_for(form_data)
-    assert error.value.read() == 'Invalid email'
+    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']
-    assert response_for(form_data) == (200, '')
+    for response_for in responses_for:
+        assert response_for(form_data) == (200, '')
 
 
-def test_mandatory_fail_dflt_msg(response_for, form_data, mocker):
+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.

-    with pytest.raises(HTTPError) as error:
-        response_for(form_data)
-    assert error.value.read() == 'No mandatory entered'
+    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
+
+
+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
+    """
+    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:
+        pass
+    try:
+        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:
+        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']