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/>.
 
 from urllib import urlencode
 from urllib2 import urlopen, HTTPError
+from csv import DictReader
 
 import pytest
-import datetime
 from wsgi_intercept import (urllib_intercept, add_wsgi_intercept,
                             remove_wsgi_intercept)
-from csv import DictReader

[Sebastian Noack, 2017/02/14 09:54:14]

The imports are grouped incorrectly here. According to PEP-8 it should be:

  corelib modules (i.e. urllib, urllib2, datetime, csv)
  <blank line>
  third party modules (i.e. pytest, wsgi_intercept)
  <blank line>
  modules that are part of this project (i.e. sitescripts)

[Jon Sonesen, 2017/02/21 14:19:15]

Done.

+
 from sitescripts.formmail.web import formmail2
 
+HOST = 'test.local'
+LOG_PORT = 80
+NO_LOG_PORT = 81
 
-@pytest.fixture()
-def form_config():
+
+@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_handler(form_config, log_path):
-    # override configured path to log file with tmpdir path
-    form_config['csv_log'].value = log_path
-    return formmail2.make_handler('test', form_config)[1]
+@pytest.fixture
+def form_config():
+    config = formmail2.conf_parse(formmail2.get_config_items())['test']
+    del config['csv_log']
+    return config
 
 
-@pytest.fixture()
-def log_path(form_config, tmpdir):
-    log = tmpdir.mkdir('logs').join('test.csv_log')

[Sebastian Noack, 2017/02/14 09:54:14]

The temporary variable seems unnecessary. Why not just combining those two
lines?

[Vasily Kuznetsov, 2017/02/15 10:22:26]

Also mkdir() seems unnecessary. tmpdir.join('test.csv_log') would do it.

[Jon Sonesen, 2017/02/20 09:55:15]

Done.

[Jon Sonesen, 2017/02/20 09:55:16]

Done.

-    return str(log)
+@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 response_for(form_handler, log_path):
-    host, port = 'test.local', 80
+@pytest.fixture
+def response_for(form_handler):
+    """ Registers two intercepts, returns responses for them based on bool """
     urllib_intercept.install_opener()
-    add_wsgi_intercept(host, port, lambda: form_handler)
-    url = 'http://{}:{}'.format(host, port)
+    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):
+    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(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_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_non_mandatory_no_msg(response_for, form_data, mocker):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    form_data['non_mandatory'] = ''
-    assert response_for(form_data) == (200, '')
-
-
-def test_invalid_email_cstm_msg(response_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'
-
-
-def test_valid_nan_mandatory_email(response_for, form_data, mocker):
-    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'
-
-    del form_data['non_mandatory_email']
-    assert response_for(form_data) == (200, '')
-
-
-def test_mandatory_fail_dflt_msg(response_for, form_data, mocker):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    del form_data['mandatory']
-    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.collect_formdata(form_data, log_path)
-    with open(log_path) as csvfile:
-        reader = DictReader(csvfile)

[Sebastian Noack, 2017/02/14 09:54:14]

Any reason you didn't just inline those variables?

  assert reader.next() == DictReader(csvfile)

[Jon Sonesen, 2017/02/20 09:55:15]

Will do.

-        row = reader.next()
-        assert row == form_data
-
-
-def test_collect_no_tmpl(log_path, form_data, form_config):
-    del(form_config['template'])
-    form_data['time'] = 'test'
-    formmail2.collect_formdata(form_data, log_path)
-    with open(log_path) as csvfile:
-        reader = DictReader(csvfile)
-        row = reader.next()
-        assert row == form_data
-
-
-def test_fieldnames(log_path, form_data):
-    form_data['time'] = str(datetime.datetime.now())
-    formmail2.collect_formdata(form_data, log_path)
-    with open(log_path) as csvfile:
-        reader = DictReader(csvfile)
-        for field in reader.fieldnames:
-            assert field in tuple(form_data.keys())

[Sebastian Noack, 2017/02/14 09:54:14]

It seems unnecessary to coerce it to a tuple.

[Jon Sonesen, 2017/02/20 09:55:15]

Yeah, you are right.

-
-
-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.collect_formdata(form_data, log_path)
-    del(form_config['fields']['email'])
-    del(form_data['email'])
-    try:
-        formmail2.collect_formdata(form_data, log_path)
-    except Exception as e:
-        assert e.message == \
-                'Field names have changed, error log written to {}'\
-                .format(log_path + '_error')
-
-    with open(log_path+'_error') as error_log:
-        reader = DictReader(error_log)
-        assert reader.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):
+    """ 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
     """
-    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.collect_formdata(form_data, log_path)
-    del(form_config['fields']['email'])
-    del(form_data['email'])
-    try:
-        formmail2.collect_formdata(form_data, log_path)
-    except Exception as e:

[Sebastian Noack, 2017/02/14 09:54:13]

Since you ignore the exception, there is no point to save it into a variable.
Hence the as-clause can be omitted.

[Jon Sonesen, 2017/02/20 09:55:15]

I was playing around with asserting the message here as well and overlooked
removing the statement.

-        pass
-    try:
-        formmail2.collect_formdata(form_data, log_path)
-    except Exception as e:
-        assert e.message == \

[Sebastian Noack, 2017/02/14 09:54:14]

The way the code if wrapped here seems hard to follow. How about this:

  assert e.message == ('Field names have changed, error log '
                       'appended to {}_error').format(log_path)

[Jon Sonesen, 2017/02/20 09:55:15]

I agree, it is better how you suggest, thank you

-                'Field names have changed, error log appended to {}'\
-                .format(log_path + '_error')
+    formmail2.log_formdata(form_data, log_path)
+    del form_data['email']
 
-    with open(log_path+'_error') as error_log:
+    # 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)
+    with pytest.raises(Exception):
+        formmail2.log_formdata(form_data, log_path)
+
+    with open(log_path + '_error') as error_log:
         reader = DictReader(error_log)
-        # two identical rows should be in the rror 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.collect_formdata(form_data, log_path)
-    form_data['email'] = 'test@foo.com'
-    formmail2.collect_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']