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,50 +7,60 @@
 #
 # 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
 
 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
 
 
 @pytest.fixture()
 def form_config():
     return formmail2.conf_parse(formmail2.get_config_items())['test']
 
 
 @pytest.fixture()
-def form_handler(form_config):
+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 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)
+
+
 # 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.',

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

I've just noticed that if we put non-ascii data here, the whole thing blows up.
To replicate replace the value in this line with 'There lived a k\xc3\xb6nig.'
(that's UTF8-encoded) and run the tests. Non-ascii characters could be expected
in this form so we'd need to fix this. Not sure if it worked before but now the
exception is thrown from csv module.

-        'non_mandatory_email': 'test@test.com'
+        'non_mandatory_email': 'test@test.com',
     }
 
 
 @pytest.fixture()
-def response_for(form_handler):
+def response_for(form_handler, log_path):
     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)
@@ -103,8 +113,105 @@
 
 
 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
+
+
+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.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')
+
+    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']