Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

Description

Issue 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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+206 lines, -90 lines) Patch
M .gitignore View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M .hgignore View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M .sitescripts.example View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M sitescripts/formmail/test/test_formmail2.py View 1 2 3 4 5 6 7 8 1 chunk +124 lines, -75 lines 0 comments Download
M sitescripts/formmail/web/formmail2.py View 1 2 3 4 5 6 7 8 2 chunks +78 lines, -14 lines 0 comments Download
M tox.ini View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 39
Jon Sonesen
Feb. 6, 2017, 6:44 p.m. (2017-02-06 18:44:34 UTC) #1
Vasily Kuznetsov
Hi Jon, This looks pretty good. I've added a few comments below and we've discussed ...
Feb. 8, 2017, 6:16 p.m. (2017-02-08 18:16:17 UTC) #2
Jon Sonesen
Comments addressed, forgot to add new test but will do so and upload the patch. ...
Feb. 10, 2017, 1:57 p.m. (2017-02-10 13:57:00 UTC) #3
Jon Sonesen
comments for formmail2.py https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmail/web/formmail2.py File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29374720/sitescripts/formmail/web/formmail2.py#newcode56 sitescripts/formmail/web/formmail2.py:56: def collect_formdata(option, params, path): On 2017/02/08 ...
Feb. 10, 2017, 2:04 p.m. (2017-02-10 14:04:43 UTC) #4
Jon Sonesen
https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example#newcode166 .sitescripts.example:166: test.csv_log = test.csv_log On 2017/02/08 18:16:16, Vasily Kuznetsov wrote: ...
Feb. 10, 2017, 2:05 p.m. (2017-02-10 14:05:00 UTC) #5
Vasily Kuznetsov
Hey Jon, Looks pretty good. There's just a couple of small nits about log paths ...
Feb. 13, 2017, 11:11 a.m. (2017-02-13 11:11:52 UTC) #6
Jon Sonesen
On 2017/02/13 11:11:52, Vasily Kuznetsov wrote: > Hey Jon, > > Looks pretty good. There's ...
Feb. 13, 2017, 12:21 p.m. (2017-02-13 12:21:05 UTC) #7
Jon Sonesen
https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmail/test/test_formmail2.py File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29375563/sitescripts/formmail/test/test_formmail2.py#newcode43 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: ...
Feb. 13, 2017, 12:21 p.m. (2017-02-13 12:21:16 UTC) #8
Jon Sonesen
here is a patch that uses tmpdir instead of tmpdir factory and addresses errors with ...
Feb. 13, 2017, 12:56 p.m. (2017-02-13 12:56:17 UTC) #9
Jon Sonesen
Feb. 13, 2017, 2:28 p.m. (2017-02-13 14:28:49 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29374720/.sitescripts.example#newcode166 .sitescripts.example:166: test.csv_log = test.csv_log On 2017/02/13 11:11:52, Vasily Kuznetsov wrote: ...
Feb. 14, 2017, 9:54 a.m. (2017-02-14 09:54:14 UTC) #11
Vasily Kuznetsov
https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmail/test/test_formmail2.py File sitescripts/formmail/test/test_formmail2.py (left): https://codereview.adblockplus.org/29374647/diff/29375582/sitescripts/formmail/test/test_formmail2.py#oldcode42 sitescripts/formmail/test/test_formmail2.py:42: 'non_mandatory_message': 'Once upon a time\nthere lived a king.', I've ...
Feb. 15, 2017, 10:22 a.m. (2017-02-15 10:22:26 UTC) #12
Jon Sonesen
Patch Set #3 Thanks for looking at this Sebastian and Vasily, I will address the ...
Feb. 20, 2017, 9:55 a.m. (2017-02-20 09:55:16 UTC) #13
Jon Sonesen
Just an update, regarding the encoding problem it actually seems to be an issue with ...
Feb. 21, 2017, 2:19 p.m. (2017-02-21 14:19:15 UTC) #14
Jon Sonesen
Addressing previous comments
Feb. 28, 2017, 3:59 p.m. (2017-02-28 15:59:34 UTC) #15
Vasily Kuznetsov
Hi Jon, Thanks for addressing the comments, the non-ascii test looks pretty good. There seems ...
Feb. 28, 2017, 6:38 p.m. (2017-02-28 18:38:50 UTC) #16
Jon Sonesen
Patch Set #3 I will upload a solution for logging being optional, it required a ...
March 7, 2017, 12:11 p.m. (2017-03-07 12:11:26 UTC) #17
Vasily Kuznetsov
Hey Jon, I still have a few stylistic comments and... nah, actually just stylistic comments ...
March 9, 2017, 7:58 p.m. (2017-03-09 19:58:10 UTC) #18
Jon Sonesen
https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmail/test/test_formmail2.py File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmail/test/test_formmail2.py#newcode46 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 ...
March 10, 2017, 9:28 a.m. (2017-03-10 09:28:52 UTC) #19
Vasily Kuznetsov
https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmail/test/test_formmail2.py File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmail/test/test_formmail2.py#newcode46 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 ...
March 10, 2017, 9:54 a.m. (2017-03-10 09:54:28 UTC) #20
Jon Sonesen
https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmail/test/test_formmail2.py File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29378678/sitescripts/formmail/test/test_formmail2.py#newcode35 sitescripts/formmail/test/test_formmail2.py:35: """ Returns two handlers, one with logging configured and ...
March 14, 2017, 7:41 p.m. (2017-03-14 19:41:26 UTC) #21
Vasily Kuznetsov
Hi Jon, See comments below. It would be good to fix the typos and other ...
March 21, 2017, 2 p.m. (2017-03-21 14:00:35 UTC) #22
Jon Sonesen
I think I may have found some unexpected bahavior https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmail/web/formmail2.py File sitescripts/formmail/web/formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmail/web/formmail2.py#newcode79 sitescripts/formmail/web/formmail2.py:79: ...
March 22, 2017, 9:23 a.m. (2017-03-22 09:23:31 UTC) #23
Vasily Kuznetsov
On 2017/03/22 09:23:31, Jon Sonesen wrote: > I think I may have found some unexpected ...
March 22, 2017, 10:33 a.m. (2017-03-22 10:33:03 UTC) #24
Jon Sonesen
https://codereview.adblockplus.org/29374647/diff/29387563/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29374647/diff/29387563/.sitescripts.example#newcode175 .sitescripts.example:175: test.csv_log = /var/log/somthing.csv_log On 2017/03/21 14:00:34, Vasily Kuznetsov wrote: ...
March 22, 2017, 2:28 p.m. (2017-03-22 14:28:22 UTC) #25
Jon Sonesen
Just an update here, I have found that I can reduce a ton of duplication ...
March 22, 2017, 3:15 p.m. (2017-03-22 15:15:18 UTC) #26
Jon Sonesen
Found a couple more little things https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmail/web/formmail2.py File sitescripts/formmail/web/formmail2.py (left): https://codereview.adblockplus.org/29374647/diff/29387563/sitescripts/formmail/web/formmail2.py#oldcode58 sitescripts/formmail/web/formmail2.py:58: url = config['url'].value ...
March 22, 2017, 3:50 p.m. (2017-03-22 15:50:34 UTC) #27
Jon Sonesen
refactor tests to be parametrized, reducing duplication. Addressed some redundancies
March 22, 2017, 8:14 p.m. (2017-03-22 20:14:23 UTC) #28
Vasily Kuznetsov
Hi Jon, This looks pretty good. I especially like the parametrization of tests -- many ...
March 23, 2017, 1:19 p.m. (2017-03-23 13:19:17 UTC) #29
Jon Sonesen
I will remove these redundancies from the tests, also did you see anything to talk ...
March 23, 2017, 1:31 p.m. (2017-03-23 13:31:02 UTC) #30
Jon Sonesen
addressed redundancies and exceptions, added comments
March 23, 2017, 1:32 p.m. (2017-03-23 13:32:46 UTC) #31
Jon Sonesen
addressed redundancies and exceptions, added comments
March 23, 2017, 1:41 p.m. (2017-03-23 13:41:20 UTC) #32
Vasily Kuznetsov
LGTM
March 23, 2017, 2:47 p.m. (2017-03-23 14:47:03 UTC) #33
Sebastian Noack
https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmail/test/test_formmail2.py File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmail/test/test_formmail2.py#newcode108 sitescripts/formmail/test/test_formmail2.py:108: (('new_field', 'foo'), 'Unexpected field/fields: new_field'), Nit: The indentation seems ...
March 23, 2017, 3:27 p.m. (2017-03-23 15:27:59 UTC) #34
Jon Sonesen
https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmail/test/test_formmail2.py File sitescripts/formmail/test/test_formmail2.py (right): https://codereview.adblockplus.org/29374647/diff/29393555/sitescripts/formmail/test/test_formmail2.py#newcode108 sitescripts/formmail/test/test_formmail2.py:108: (('new_field', 'foo'), 'Unexpected field/fields: new_field'), On 2017/03/23 15:27:58, Sebastian ...
March 23, 2017, 6:42 p.m. (2017-03-23 18:42:17 UTC) #35
Jon Sonesen
March 23, 2017, 6:42 p.m. (2017-03-23 18:42:19 UTC) #36
Sebastian Noack
Despite you having replied with "Done" to my comments, I don't see a new patchset.
March 23, 2017, 6:49 p.m. (2017-03-23 18:49:31 UTC) #37
Jon Sonesen
On 2017/03/23 18:49:31, Sebastian Noack wrote: > Despite you having replied with "Done" to my ...
March 23, 2017, 6:51 p.m. (2017-03-23 18:51:09 UTC) #38
Sebastian Noack
March 23, 2017, 7:20 p.m. (2017-03-23 19:20:16 UTC) #39
LGTM

Powered by Google App Engine
This is Rietveld