Issue 29374647: Issue 4814 - Adds csv log to formmail2 (Closed)
|
Created:
Feb. 6, 2017, 6:44 p.m. by Jon Sonesen Modified:
March 23, 2017, 8:01 p.m. CC:
Wladimir Palant, mathias Base URL:
https://hg.adblockplus.org/sitescripts Visibility:
Public. |
DescriptionIssue 4814 - Adds csv log to formmail2
Patch Set 1 : #
Total comments: 16
Patch Set 2 : adds error logging and tests #
Total comments: 2
Patch Set 3 : #
Total comments: 25
Patch Set 4 : address comments, now encodes user input to utf8 #
Total comments: 12
Patch Set 5 : #
Total comments: 19
Patch Set 6 : #
Total comments: 24
Patch Set 7 : refactor tests to be parametrized, reducing duplication. Addressed some redundancies #
Total comments: 10
Patch Set 8 : #
Total comments: 10
Patch Set 9 : #
MessagesTotal messages: 39
Hi Jon, This looks pretty good. I've added a few comments below and we've discussed some ideas over TS. Cheers, Vasily https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.exampl... .sitescripts.example:166: test.csv_log = test.csv_log It would be more useful to make this look more like a path. Maybe something like this: test.csv_log = /var/log/www/test/form-log.csv This way it would be more immediately obvious, to someone who looks at this example, what's going on here. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:30: return formmail2.conf_parse(formmail2.get_config_items())['test'] As discussed over TS, let's override the csv_log with a temporary path here. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:60: monkeypatch.chdir(os.path.dirname(log_path)) As discussed over TS, we won't need this if override the `log_path` in `form_config`. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:152: We should also have a test for the situation when the configuration changes and the field list is not the same as in the log file. Then it should give an error and write to a temp file next to the log. It could be tricky to set this up, but I think if you just modify `form_config` to change the list of fields and then give it to `formmail2.make_handler` to make another handler, it will work. Since we don't need to reuse this modified config and handler they don't need to be fixtures, we just do it inside this one test. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:56: def collect_formdata(option, params, path): `option` is not used? https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:88: raise Exception('No log configured for form handler: ' + name) The log should be also optional and the exception should be raised when neither log nor template are configured. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:96: template = '' It would probably be cleaner to use None here and then check for 'is not None' on line 114.
Comments addressed, forgot to add new test but will do so and upload the patch. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:30: return formmail2.conf_parse(formmail2.get_config_items())['test'] On 2017/02/08 18:16:17, Vasily Kuznetsov wrote: > As discussed over TS, let's override the csv_log with a temporary path here. Done. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:60: monkeypatch.chdir(os.path.dirname(log_path)) On 2017/02/08 18:16:17, Vasily Kuznetsov wrote: > As discussed over TS, we won't need this if override the `log_path` in > `form_config`. Done. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:152: On 2017/02/08 18:16:16, Vasily Kuznetsov wrote: > We should also have a test for the situation when the configuration changes and > the field list is not the same as in the log file. Then it should give an error > and write to a temp file next to the log. > > It could be tricky to set this up, but I think if you just modify `form_config` > to change the list of fields and then give it to `formmail2.make_handler` to > make another handler, it will work. Since we don't need to reuse this modified > config and handler they don't need to be fixtures, we just do it inside this one > test. Great
comments for formmail2.py https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:56: def collect_formdata(option, params, path): On 2017/02/08 18:16:17, Vasily Kuznetsov wrote: > `option` is not used? Acknowledged. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:88: raise Exception('No log configured for form handler: ' + name) On 2017/02/08 18:16:17, Vasily Kuznetsov wrote: > The log should be also optional and the exception should be raised when neither > log nor template are configured. Done. https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:96: template = '' On 2017/02/08 18:16:17, Vasily Kuznetsov wrote: > It would probably be cleaner to use None here and then check for 'is not None' > on line 114. Done.
https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.exampl... .sitescripts.example:166: test.csv_log = test.csv_log On 2017/02/08 18:16:16, Vasily Kuznetsov wrote: > It would be more useful to make this look more like a path. Maybe something like > this: > > test.csv_log = /var/log/www/test/form-log.csv > > This way it would be more immediately obvious, to someone who looks at this > example, what's going on here. Acknowledged.
Hey Jon, Looks pretty good. There's just a couple of small nits about log paths in the config example and in the test. Also test_append_log is failing for me (it says "Field names have changed...."). I haven't investigated it since we need to get going but let me know if you can't reproduce it, I will try to figure out what's going on. Cheers, Vasily https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.exampl... .sitescripts.example:166: test.csv_log = test.csv_log On 2017/02/10 14:04:59, Jon Sonesen wrote: > On 2017/02/08 18:16:16, Vasily Kuznetsov wrote: > > It would be more useful to make this look more like a path. Maybe something > like > > this: > > > > test.csv_log = /var/log/www/test/form-log.csv > > > > This way it would be more immediately obvious, to someone who looks at this > > example, what's going on here. > > Acknowledged. I think /etc/ is normally for configs. Would you mind changing this to /var/log -- I think it gives a better idea of what this is about. https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:43: log_file = os.path.basename(form_config['csv_log'].value) That's clever, but on the other hand perhaps we don't really care for the tests. We could just do: return tmpdir.join('log.csv').strpath and it would be good enough. Or am I missing something?
On 2017/02/13 11:11:52, Vasily Kuznetsov wrote: > Hey Jon, > > Looks pretty good. There's just a couple of small nits about log paths in the > config example and in the test. > > Also test_append_log is failing for me (it says "Field names have changed...."). > I haven't investigated it since we need to get going but let me know if you > can't reproduce it, I will try to figure out what's going on. > > Cheers, > Vasily > > https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example > File .sitescripts.example (right): > > https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.exampl... > .sitescripts.example:166: test.csv_log = test.csv_log > On 2017/02/10 14:04:59, Jon Sonesen wrote: > > On 2017/02/08 18:16:16, Vasily Kuznetsov wrote: > > > It would be more useful to make this look more like a path. Maybe something > > like > > > this: > > > > > > test.csv_log = /var/log/www/test/form-log.csv > > > > > > This way it would be more immediately obvious, to someone who looks at this > > > example, what's going on here. > > > > Acknowledged. > > I think /etc/ is normally for configs. Would you mind changing this to /var/log > -- I think it gives a better idea of what this is about. > > https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmai... > File sitescripts/formmail/test/test_formmail2.py (right): > > https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmai... > sitescripts/formmail/test/test_formmail2.py:43: log_file = > os.path.basename(form_config['csv_log'].value) > That's clever, but on the other hand perhaps we don't really care for the tests. > We could just do: > > return tmpdir.join('log.csv').strpath > > and it would be good enough. Or am I missing something? You are right about the failure, it looks like on OSX when opening a file for appending and reading, it will require that you seek to the beginning of the file for the reader to get the headers and map the field names, because of this anytime a log is appended it thought the fieldnames werent matching. This has been addressed and now I added a seek to the end of the file on write as well.
https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:43: log_file = os.path.basename(form_config['csv_log'].value) On 2017/02/13 11:11:52, Vasily Kuznetsov wrote: > That's clever, but on the other hand perhaps we don't really care for the tests. > We could just do: > > return tmpdir.join('log.csv').strpath > > and it would be good enough. Or am I missing something? Done.
here is a patch that uses tmpdir instead of tmpdir factory and addresses errors with osx log append and read
https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.exampl... .sitescripts.example:166: test.csv_log = test.csv_log On 2017/02/13 11:11:52, Vasily Kuznetsov wrote: > On 2017/02/10 14:04:59, Jon Sonesen wrote: > > On 2017/02/08 18:16:16, Vasily Kuznetsov wrote: > > > It would be more useful to make this look more like a path. Maybe something > > like > > > this: > > > > > > test.csv_log = /var/log/www/test/form-log.csv > > > > > > This way it would be more immediately obvious, to someone who looks at this > > > example, what's going on here. > > > > Acknowledged. > > I think /etc/ is normally for configs. Would you mind changing this to /var/log > -- I think it gives a better idea of what this is about. What is about this comment? It hasn't been addressed yet. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:23: from csv import DictReader 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) https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:41: log = tmpdir.mkdir('logs').join('test.csv_log') The temporary variable seems unnecessary. Why not just combining those two lines? https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:127: reader = DictReader(csvfile) Any reason you didn't just inline those variables? assert reader.next() == DictReader(csvfile) https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:148: assert field in tuple(form_data.keys()) It seems unnecessary to coerce it to a tuple. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:184: except Exception as e: Since you ignore the exception, there is no point to save it into a variable. Hence the as-clause can be omitted. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:189: assert e.message == \ 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) https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py.orig (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py.orig:1: # This file is part of the Adblock Plus web scripts, It seems this file has been added by accident. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:19: from csv import DictWriter, DictReader The imports are incorrectly grouped here. See my comment in the tests. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:63: raise Exception( We usually don't use hanging indentation. I'd prefer following indentation style: raise Exception('Field names have changed, ' 'error log appended to ' + err_path) What do you think? https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:76: formlog.seek(0) This seek seems redundant. When I open a file with 'ab+' (or any other mode), the position is initially 0. If you have doubts, feel free to test it: f = open('/tmp/'fooo', 'ab+') f.tell() https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:80: formlog.seek(1024, os.SEEK_END) So you seek 1024 bytes after the end of the file. What is this good for?
https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (left): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:42: 'non_mandatory_message': 'Once upon a time\nthere lived a king.', 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. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:41: log = tmpdir.mkdir('logs').join('test.csv_log') On 2017/02/14 09:54:14, Sebastian Noack wrote: > The temporary variable seems unnecessary. Why not just combining those two > lines? Also mkdir() seems unnecessary. tmpdir.join('test.csv_log') would do it. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:76: formlog.seek(0) On 2017/02/14 09:54:14, Sebastian Noack wrote: > This seek seems redundant. When I open a file with 'ab+' (or any other mode), > the position is initially 0. If you have doubts, feel free to test it: > > f = open('/tmp/'fooo', 'ab+') > f.tell() This is not true on MacOS: $ python Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec 5 2015, 12:54:16) [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> f = open('multiplexer.py', 'ab+') >>> f.tell() 1147 It seems that the standard doesn't really specify where the file pointer should be in this situation. It only says that wherever you seek, the writing should be at the end of file (although it also says "the application shall ensure that ... input is not directly followed by output without an intervening call to a file positioning function ..." so the second seek is also necessary then).
Patch Set #3 Thanks for looking at this Sebastian and Vasily, I will address the comments I have replied to and get a patch set uploaded! https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:41: log = tmpdir.mkdir('logs').join('test.csv_log') On 2017/02/14 09:54:14, Sebastian Noack wrote: > The temporary variable seems unnecessary. Why not just combining those two > lines? Done. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:41: log = tmpdir.mkdir('logs').join('test.csv_log') On 2017/02/15 10:22:26, Vasily Kuznetsov wrote: > On 2017/02/14 09:54:14, Sebastian Noack wrote: > > The temporary variable seems unnecessary. Why not just combining those two > > lines? > > Also mkdir() seems unnecessary. tmpdir.join('test.csv_log') would do it. Done. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:127: reader = DictReader(csvfile) On 2017/02/14 09:54:14, Sebastian Noack wrote: > Any reason you didn't just inline those variables? > > assert reader.next() == DictReader(csvfile) Will do. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:148: assert field in tuple(form_data.keys()) On 2017/02/14 09:54:14, Sebastian Noack wrote: > It seems unnecessary to coerce it to a tuple. Yeah, you are right. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:184: except Exception as e: On 2017/02/14 09:54:13, Sebastian Noack wrote: > Since you ignore the exception, there is no point to save it into a variable. > Hence the as-clause can be omitted. I was playing around with asserting the message here as well and overlooked removing the statement. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:189: assert e.message == \ On 2017/02/14 09:54:14, Sebastian Noack wrote: > 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) I agree, it is better how you suggest, thank you https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py.orig (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py.orig:1: # This file is part of the Adblock Plus web scripts, On 2017/02/14 09:54:14, Sebastian Noack wrote: > It seems this file has been added by accident. Acknowledged. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:19: from csv import DictWriter, DictReader On 2017/02/14 09:54:14, Sebastian Noack wrote: > The imports are incorrectly grouped here. See my comment in the tests. Done. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:63: raise Exception( On 2017/02/14 09:54:14, Sebastian Noack wrote: > We usually don't use hanging indentation. I'd prefer following indentation > style: > > raise Exception('Field names have changed, ' > 'error log appended to ' + err_path) > > What do you think? Yeah, it seems easier to follow as well. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:80: formlog.seek(1024, os.SEEK_END) On 2017/02/14 09:54:14, Sebastian Noack wrote: > So you seek 1024 bytes after the end of the file. What is this good for? This was a mistake, I was reading the last line (so it should have been -1024 bytes from the end of the file and asserting it wasnt a header. But I realized this was unnecessary, but forgot to correct the line...will fix
Just an update, regarding the encoding problem it actually seems to be an issue with the wsgi app. I verified this by adding the utf string to test formmail and I recieve the same error. https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:23: from csv import DictReader On 2017/02/14 09:54:14, Sebastian Noack wrote: > 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) Done.
Addressing previous comments
Hi Jon, Thanks for addressing the comments, the non-ascii test looks pretty good. There seems to be a typo in formmail2.py that breaks the tests (they pass ok after I fixed it, but be more careful) and also I dug out a couple of old issues that seem to have slipped past us. See the comments below. Cheers, Vasly https://codereview.adblockplus.org/29374647/diff/29377670/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29377670/.sitescripts.exampl... .sitescripts.example:176: test.csv_log = /etc/server/test.csv_log Remember the comment about changing this to /var/log/formmail2/something.csv. The path starting with '/etc/...' just seems too config-like, don't you think? https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:62: 'non_mandatory_message': '\xc3\xb6', Perhaps we could make this by just taking `form_data` and changing 'non_mandatory_message' in it. Would be less repetition. https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:73: def collect_formdata(params, path): This function is basically adding the form data to csv log. Maybe we just call it `log_form_data` instead for more clarity? https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:110: raise Exception('No log configured for form handler: ' + name) It seems that the log is still mandatory here. Also on like 147 you're always calling the logging function and it should only be called if csv_log is configured. I think we should add a test without logging and make sure it passes. https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:141: 'fields': {field: params.get(field, '') This and the following lines could fit in one line, although, I guess it's a matter of taste so feel free to leave it as is. https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:147: ollect_formdata(params, log_path) The first 'c' seems to have disappeared somehow ;)
Patch Set #3 I will upload a solution for logging being optional, it required a slightly less obvious solution but hopefully the docstrings can convey what is happening well. Basically I end up adapting the form_handler and response_for fixtures to create two handlers and two intercepts one for logging configured and without logging configured and adapted all the integration tests to handle both. https://codereview.adblockplus.org/29374647/diff/29377670/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29377670/.sitescripts.exampl... .sitescripts.example:176: test.csv_log = /etc/server/test.csv_log On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > Remember the comment about changing this to /var/log/formmail2/something.csv. > The path starting with '/etc/...' just seems too config-like, don't you think? Will make it into that https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:62: 'non_mandatory_message': '\xc3\xb6', On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > Perhaps we could make this by just taking `form_data` and changing > 'non_mandatory_message' in it. Would be less repetition. Yes, I agree https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:73: def collect_formdata(params, path): On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > This function is basically adding the form data to csv log. Maybe we just call > it `log_form_data` instead for more clarity? I agree https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:110: raise Exception('No log configured for form handler: ' + name) On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > It seems that the log is still mandatory here. Also on like 147 you're always > calling the logging function and it should only be called if csv_log is > configured. > > I think we should add a test without logging and make sure it passes. Ok, yeah I think I meant to change that to an attribute error. But also I think both are unnecessary. I will add the test :) https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:141: 'fields': {field: params.get(field, '') On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > This and the following lines could fit in one line, although, I guess it's a > matter of taste so feel free to leave it as is. I usually feel it is easier on the eyes if the key/values are on their own lines, but since the whole dict fits I think I will put it on one line. https://codereview.adblockplus.org/29374647/diff/29377670/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:147: ollect_formdata(params, log_path) On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > The first 'c' seems to have disappeared somehow ;) geeez XD sorry
Hey Jon, I still have a few stylistic comments and... nah, actually just stylistic comments :D Cheers, Vasily https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:35: """ Returns two handlers, one with logging configured and one without """ 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? https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:46: return log_handler, formmail2.make_handler('log_test', no_log_config)[1] 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. https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:73: for form_handler in form_handlers: Nifty! https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:135: if template is not None: I like how you did it with 'csv_log' in config below, it's a lot more compact than all this exception handling that we had before. Perhaps we do the same for the template? https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:136: template_args = {'time': time, 'fields': {field: On 2017/03/07 12:11:25, Jon Sonesen wrote: > On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > > This and the following lines could fit in one line, although, I guess it's a > > matter of taste so feel free to leave it as is. > > I usually feel it is easier on the eyes if the key/values are on their own > lines, but since the whole dict fits I think I will put it on one line. Yeah, like this it's hard to read indeed. What I was thinking about is just making the value of 'fields' one line, like this: template_args = { 'time': time, 'fields': {field: params.get(field, '') for field in fields} } https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:140: params['time'] = time This could also be inside of the if on the following line. Not a big deal, but I think the logic is clearer this way, since it's only used inside of the if.
https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:46: return log_handler, formmail2.make_handler('log_test', no_log_config)[1] On 2017/03/09 19:58:09, Vasily Kuznetsov wrote: > 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. 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. https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:73: for form_handler in form_handlers: On 2017/03/09 19:58:09, Vasily Kuznetsov wrote: > Nifty! Thanks :) https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:135: if template is not None: On 2017/03/09 19:58:09, Vasily Kuznetsov wrote: > I like how you did it with 'csv_log' in config below, it's a lot more compact > than all this exception handling that we had before. Perhaps we do the same for > the template? I agree https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:136: template_args = {'time': time, 'fields': {field: On 2017/03/09 19:58:09, Vasily Kuznetsov wrote: > On 2017/03/07 12:11:25, Jon Sonesen wrote: > > On 2017/02/28 18:38:49, Vasily Kuznetsov wrote: > > > This and the following lines could fit in one line, although, I guess it's a > > > matter of taste so feel free to leave it as is. > > > > I usually feel it is easier on the eyes if the key/values are on their own > > lines, but since the whole dict fits I think I will put it on one line. > > Yeah, like this it's hard to read indeed. What I was thinking about is just > making the value of 'fields' one line, like this: > > template_args = { > 'time': time, > 'fields': {field: params.get(field, '') for field in fields} > } Done. https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:140: params['time'] = time On 2017/03/09 19:58:10, Vasily Kuznetsov wrote: > This could also be inside of the if on the following line. Not a big deal, but I > think the logic is clearer this way, since it's only used inside of the if. Actually I thought that we always wanted a timestamp on the logs in which case we would want it outside the if for the template and mail sending and inside of the if statement for the log. Is that okay? Also I am realizing that perhaps the sendMail call should be inside of a try except in case the sendmail raises an exception the log will still be written..does that make sense?
https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:46: return log_handler, formmail2.make_handler('log_test', no_log_config)[1] On 2017/03/10 09:28:51, Jon Sonesen wrote: > On 2017/03/09 19:58:09, Vasily Kuznetsov wrote: > > 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. > > 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. Acknowledged. https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:140: params['time'] = time On 2017/03/10 09:28:51, Jon Sonesen wrote: > On 2017/03/09 19:58:10, Vasily Kuznetsov wrote: > > This could also be inside of the if on the following line. Not a big deal, but > I > > think the logic is clearer this way, since it's only used inside of the if. > > Actually I thought that we always wanted a timestamp on the logs in which case > we would want it outside the if for the template and mail sending and inside of > the if statement for the log. Is that okay? Yeah, indeed, we can just put the time into the params before sending and logging. Sounds good. > Also I am realizing that perhaps the sendMail call should be inside of a try > except in case the sendmail raises an exception the log will still be > written..does that make sense? Yeah, this defintely makes sense. Good catch.
https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:35: """ Returns two handlers, one with logging configured and one without """ On 2017/03/09 19:58:09, Vasily Kuznetsov wrote: > 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? Done. https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:44: del no_log_config['csv_log'] 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? https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:95: def test_success(responses_for, form_data, mocker): 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? https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:134: def test_valid_nan_mandatory_email(responses_for, form_data, mocker): Yikes! this is actually meant to test 'invalid_non_mandatory_email' will change https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:149: del form_data['mandatory'] 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. https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:140: params['time'] = time On 2017/03/10 09:54:28, Vasily Kuznetsov wrote: > On 2017/03/10 09:28:51, Jon Sonesen wrote: > > On 2017/03/09 19:58:10, Vasily Kuznetsov wrote: > > > This could also be inside of the if on the following line. Not a big deal, > but > > I > > > think the logic is clearer this way, since it's only used inside of the if. > > > > Actually I thought that we always wanted a timestamp on the logs in which case > > we would want it outside the if for the template and mail sending and inside > of > > the if statement for the log. Is that okay? > > Yeah, indeed, we can just put the time into the params before sending and > logging. Sounds good. > > > Also I am realizing that perhaps the sendMail call should be inside of a try > > except in case the sendmail raises an exception the log will still be > > written..does that make sense? > > Yeah, this defintely makes sense. Good catch. Done.
Hi Jon, See comments below. It would be good to fix the typos and other small things like that and figure out a solution for logging exceptions during 500-errors before we land this. The code duplication in tests could be handled as a separate commit. Cheers, Vasily https://codereview.adblockplus.org/29374647/diff/29387563/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29387563/.sitescripts.exampl... .sitescripts.example:175: test.csv_log = /var/log/somthing.csv_log Not a big deal but typo :) https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:64: 'non_mandatory_email': 'test@test.com' I usually put the comma at the end of last item in multiline lists and dicts like this because the changes that add items to it later will look cleaner in diffs. Not a serious issue here, but in general it seems like a good rule to follow. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:77: if log: I think this would be more readable as: if log: url = ... else: url = ... https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:90: tmp_config = form_config What is the purpose of this assignment? Why don't we just use `form_config` directly? Same for the following functions. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:149: """ DictWriter does not accpet utf-8, call log handler """ typo in "accpet". https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:161: sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail') This and some following lines seem to be duplicated quite a bit in the functions around. I wonder if we could move them out somehow into a function or a fixture or maybe just use parametrization for the whole thing. If it looks too complicated, don't bother, we'll just do it separately in another change later. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:213: def test_valid_nan_mandatory_email(response_for, form_data, mocker): ...non_mandatory...? :) https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:233: """ Submits a form that does not have the dame fields as previous submissions typo in "dame"? https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:234: Here you're continuing the same sentence in the lines following the first line of the docstring. In such cases it doesn't make sense to follow the title-empty_line-full_text structure of the docstring so we don't need an empty line. My suggestion regarding the docstrings was that where it's possible to change them to title-empty_line-full_text structure, we should do so, but if it's not possible, just simple multiline without a blank line would be ok. Sorry if I didn't express this clear enough. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:141: start_response('500 Server Error', response_headers) 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`.
I think I may have found some unexpected bahavior https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:79: if reader.fieldnames != params.keys(): 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?
On 2017/03/22 09:23:31, Jon Sonesen wrote: > I think I may have found some unexpected bahavior > > https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... > File sitescripts/formmail/web/formmail2.py (right): > > https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... > sitescripts/formmail/web/formmail2.py:79: if reader.fieldnames != params.keys(): > 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? Yes, you're right. If the request has extraneous fields it's more of a 400 than 500.
https://codereview.adblockplus.org/29374647/diff/29387563/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29387563/.sitescripts.exampl... .sitescripts.example:175: test.csv_log = /var/log/somthing.csv_log On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > Not a big deal but typo :) Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:64: 'non_mandatory_email': 'test@test.com' On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > I usually put the comma at the end of last item in multiline lists and dicts > like this because the changes that add items to it later will look cleaner in > diffs. Not a serious issue here, but in general it seems like a good rule to > follow. Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:77: if log: On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > I think this would be more readable as: > > if log: > url = ... > else: > url = ... Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:90: tmp_config = form_config On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > What is the purpose of this assignment? Why don't we just use `form_config` > directly? Same for the following functions. whoops. Done https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:149: """ DictWriter does not accpet utf-8, call log handler """ On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > typo in "accpet". Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:161: sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail') On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > This and some following lines seem to be duplicated quite a bit in the functions > around. I wonder if we could move them out somehow into a function or a fixture > or maybe just use parametrization for the whole thing. > > If it looks too complicated, don't bother, we'll just do it separately in > another change later. The thing is that some of the test's have different behavior depending on whether the log is there or not. I can look into it though and submit what i come up with. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:213: def test_valid_nan_mandatory_email(response_for, form_data, mocker): On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > ...non_mandatory...? :) Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:233: """ Submits a form that does not have the dame fields as previous submissions On 2017/03/21 14:00:35, Vasily Kuznetsov wrote: > typo in "dame"? Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:234: On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: > Here you're continuing the same sentence in the lines following the first line > of the docstring. In such cases it doesn't make sense to follow the > title-empty_line-full_text structure of the docstring so we don't need an empty > line. > > My suggestion regarding the docstrings was that where it's possible to change > them to title-empty_line-full_text structure, we should do so, but if it's not > possible, just simple multiline without a blank line would be ok. Sorry if I > didn't express this clear enough. Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:79: if reader.fieldnames != params.keys(): On 2017/03/22 09:23:30, Jon Sonesen wrote: > 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? Done. https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:141: start_response('500 Server Error', response_headers) On 2017/03/21 14:00:35, Vasily Kuznetsov wrote: > 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`. Done.
Just an update here, I have found that I can reduce a ton of duplication in the test code by parametrizing the more generic tests. I am doing this now and should have a patcch up tonight or early in the morning
Found a couple more little things https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (left): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:58: url = config['url'].value 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? https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:63: get_template(template, autoescape=False) 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?
refactor tests to be parametrized, reducing duplication. Addressed some redundancies
Hi Jon, This looks pretty good. I especially like the parametrization of tests -- many lines of code removed and the coverage is now even better! I think it's also easier to read the tests this way. Just a few minor points below. Cheers, Vasily https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:146: assert row != reader.next() Are they not equal because of time field? What do you think about adding a comment here to explain the purpose of this comparison? https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:172: assert error.typename == 'HTTPError' You probably don't need to assert this since you've already told pytest to expect that two lines above and it will complain if a wrong error is thrown. https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:189: except Exception as e: Perhaps it's better to use with pytest.raises(...) to make sure the exception is thrown and the assert is executed. https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:192: try: I suppose there's some reason for trying this twice and it probably deserves to be written down as a comment. https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:195: assert e.message == ('Field names have changed, error log' Do we need to assert the whole message here again or maybe a simpler check will do, like startswith('Field names have changed')? Or even no check.
I will remove these redundancies from the tests, also did you see anything to talk about in web/formmail2.py? https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:146: assert row != reader.next() On 2017/03/23 13:19:16, Vasily Kuznetsov wrote: > Are they not equal because of time field? What do you think about adding a > comment here to explain the purpose of this comparison? Done. https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:172: assert error.typename == 'HTTPError' On 2017/03/23 13:19:16, Vasily Kuznetsov wrote: > You probably don't need to assert this since you've already told pytest to > expect that two lines above and it will complain if a wrong error is thrown. Acknowledged. https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:189: except Exception as e: On 2017/03/23 13:19:16, Vasily Kuznetsov wrote: > Perhaps it's better to use with pytest.raises(...) to make sure the exception is > thrown and the assert is executed. Done. Odd that I chose to use try's in the first place https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:192: try: On 2017/03/23 13:19:16, Vasily Kuznetsov wrote: > I suppose there's some reason for trying this twice and it probably deserves to > be written down as a comment. Done. https://codereview.adblockplus.org/29374647/diff/29391800/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:195: assert e.message == ('Field names have changed, error log' On 2017/03/23 13:19:16, Vasily Kuznetsov wrote: > Do we need to assert the whole message here again or maybe a simpler check will > do, like startswith('Field names have changed')? Or even no check. Agreed, I will remove the assertions
addressed redundancies and exceptions, added comments
addressed redundancies and exceptions, added comments
LGTM
https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:108: (('new_field', 'foo'), 'Unexpected field/fields: new_field'), Nit: The indentation seems inconsistent here. I suppose it should be 4 instead of 8 spaces. And the closing bracket below should be on the same column where the line opening the list starts. https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:125: ]) Nit: Same here, the closing bracket should be on the same column where the line opening the list starts. https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:187: del(form_data['email']) Nit: del isn't a function, but a statement. So the parenthesis are redundant. https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:15: from __future__ import print_function Nit: There should be a blank line above. https://codereview.adblockplus.org/29374647/diff/29393555/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29374647/diff/29393555/tox.ini#newcode102 tox.ini:102: --cov-config tox.ini --cov-report html --cov-report term --cov sitescripts \ Unrelated change?
https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:108: (('new_field', 'foo'), 'Unexpected field/fields: new_field'), On 2017/03/23 15:27:58, Sebastian Noack wrote: > Nit: The indentation seems inconsistent here. I suppose it should be 4 instead > of 8 spaces. And the closing bracket below should be on the same column where > the line opening the list starts. Done. https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:125: ]) On 2017/03/23 15:27:58, Sebastian Noack wrote: > Nit: Same here, the closing bracket should be on the same column where the line > opening the list starts. Done. https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/test/test_formmail2.py:187: del(form_data['email']) On 2017/03/23 15:27:58, Sebastian Noack wrote: > Nit: del isn't a function, but a statement. So the parenthesis are redundant. Done. https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmai... sitescripts/formmail/web/formmail2.py:15: from __future__ import print_function On 2017/03/23 15:27:58, Sebastian Noack wrote: > Nit: There should be a blank line above. Done. https://codereview.adblockplus.org/29374647/diff/29393555/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29374647/diff/29393555/tox.ini#newcode102 tox.ini:102: --cov-config tox.ini --cov-report html --cov-report term --cov sitescripts \ On 2017/03/23 15:27:58, Sebastian Noack wrote: > Unrelated change? Yeah, just adding html output of the coverage report, this could be broken out to a noissue commit as well
Despite you having replied with "Done" to my comments, I don't see a new patchset.
On 2017/03/23 18:49:31, Sebastian Noack wrote: > Despite you having replied with "Done" to my comments, I don't see a new > patchset. Sorry, had it hanging on the prompt for entering a patch set title
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
