Issue 4413 - Merge formmail.py and formmail2.py

Issue 4413 - Merge formmail.py and formmail2.py

[Vasily Kuznetsov, 2019-01-17 11:28:13]

Hi Rosie,

LGTM.

However, note that the path of the handler is now different and it reads its
settings from a different section of the config. Because of these differences,
this commit must be pushed in sync with an infrastructure change that would
modify the configs that the handler is using. Please coordinate with Ops
regarding this or let me know if I can be of assistance.

Cheers,
Vasily

[rhowell, 2019-01-18 00:46:07]

On 2019/01/17 11:28:13, Vasily Kuznetsov wrote:
> Please coordinate with Ops regarding this or let me know if I can be of
assistance.

Hi Vasily,

I talked to Paco and he'll coordinate the push with me. But, I'm having some
trouble writing the formmail README. So far, it's basically just a template that
may or may not have any accurate information in it. I'd appreciate any help,
either helping me learn how to properly set up the test environment and run the
script, or even just correcting any info that's on the README so far.

Thank you!

[Vasily Kuznetsov, 2019-01-18 16:52:13]

Hi Rosie!

Great that you're coordinating with Paco and good start on the README. I have
some comments about this one so I'm going to temporarily revoke my approval of
this review (let's see how this goes...).

Not LGTM

Cheers,
Vasily

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:3: This script generates a form and sends an
email.? It provides the following URLs:
The script doesn't generate the form, it only accepts the POST request that
comes when it's submitted, plugs the values from the request into an email
template and sends it to the configured address. I guess this introduction could
read something like:

    The web handler that extracts form data from a POST request, uses it to
    populate an email template and then sends the produced email to a
    configured list of addresses.

I think we probably shouldn't talk about provided URL(s) in this section because
it all depends on the configuration.

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:11: * _Anthing else?_
The handler also uses Jinja2 for rendering the template. I'm not sure if it
makes much sense to mention testing dependencies here -- there are many and they
are listed in tox.ini, it's probably better to just refer to it instead.

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:15: The script can be run as a module, using the
following command:
formmail.py is not really a standalong script, it's a web handler that is
normally run via the multiplexer (multiplexer.py or multiplexer.fcgi at the
root, they are described in the main README file). I'd say we mention this here,
refer to the multiplexer section in the main README and explain how to activate
the handler (can use the text from the section about mutliplexer config below
from lines 28-32).

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:31: [multiplexer]
This part should probably be in # Running the web handler, because this is about
configuring the multiplexer.

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:36: [formmail]
This part will need more explanation. I think we should start by saying that
formmail.py can handle multiple URLs and  forms and that each URL will
correspond to a group of config variables that all start with the same prefix,
for example: handler1.

Then we can go over each variable, starting from `handler1.url` (the URL where
the form should be submitted), `handler1.csv_log` (CSV file into which all
submissions will be saved, optional), `handler1.template` (Jinja2 template for
the email including the headers, such as From: and To:), and
`handler1.fields.xxx` (a subgroup that includes the descriptions of the form
fields).

I think you can draft the text even better then I would. Feel free to ping be by
IRC if you have questions about the config (but don't forget there are examples
and the source ;).

[rhowell, 2019-01-18 20:08:08]

Hi Vasily!

Thanks for all the great feedback, it's much appreciated. I just have a couple
more questions (below). A let me know if you see any other issues?

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:3: This script generates a form and sends an
email.? It provides the following URLs:
On 2019/01/18 16:52:13, Vasily Kuznetsov wrote:
> The script doesn't generate the form, it only accepts the POST request that
> comes when it's submitted, plugs the values from the request into an email
> template and sends it to the configured address. I guess this introduction
could
> read something like:
> 
>     The web handler that extracts form data from a POST request, uses it to
>     populate an email template and then sends the produced email to a
>     configured list of addresses.
> 
> I think we probably shouldn't talk about provided URL(s) in this section
because
> it all depends on the configuration.

Done.

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:11: * _Anthing else?_
On 2019/01/18 16:52:13, Vasily Kuznetsov wrote:
> The handler also uses Jinja2 for rendering the template. I'm not sure if it
> makes much sense to mention testing dependencies here -- there are many and
they
> are listed in tox.ini, it's probably better to just refer to it instead.

Done.

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:15: The script can be run as a module, using the
following command:
On 2019/01/18 16:52:13, Vasily Kuznetsov wrote:
> formmail.py is not really a standalong script, it's a web handler that is
> normally run via the multiplexer (multiplexer.py or multiplexer.fcgi at the
> root, they are described in the main README file). I'd say we mention this
here,
> refer to the multiplexer section in the main README and explain how to
activate
> the handler (can use the text from the section about mutliplexer config below
> from lines 28-32).

Done.

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:31: [multiplexer]
On 2019/01/18 16:52:13, Vasily Kuznetsov wrote:
> This part should probably be in # Running the web handler, because this is
about
> configuring the multiplexer.

Done.

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:36: [formmail]
On 2019/01/18 16:52:13, Vasily Kuznetsov wrote:

> `handler1.fields.xxx` (a subgroup that includes the descriptions of the form
> fields).


I'm still not exactly clear what the `.fields` section refers to, but maybe
whoever is configuring the server would understand it more clearly. Does it
change the behavior of the actual form? Or send an error response to the POST
request if something mandatory is missing?

https://codereview.adblockplus.org/29984580/diff/29985558/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29985558/sitescripts/formmai...
sitescripts/formmail/README.md:40: handler1.csv_log = /var/log/something.csv_log
Does the spacing around the `=` matter? This is the only one with spaces.

[Vasily Kuznetsov, 2019-01-21 13:54:18]

Hi Rosie!

All going in the right direction so far. See my replies below.

Cheers,
Vasily

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:36: [formmail]
On 2019/01/18 20:08:07, rhowell wrote:
> On 2019/01/18 16:52:13, Vasily Kuznetsov wrote:
> 
> > `handler1.fields.xxx` (a subgroup that includes the descriptions of the form
> > fields).
> 
> 
> I'm still not exactly clear what the `.fields` section refers to, but maybe
> whoever is configuring the server would understand it more clearly. Does it
> change the behavior of the actual form? Or send an error response to the POST
> request if something mandatory is missing?

The form itself is usually another page, so its behavior is unchanged by this
config but the two have to match. Also the handler can return error messages
that depend on the configuration here and can be further changed by it. I think
we should document all of it here, otherwise the people configuring the server
would have to ask.

The ".fields" group describes the fields that are expected in the POST request
and then become available to the template. Each variable in the group defines a
field and its value can contain "mandatory" (which makes the field mandatory)
and/or "email" (which makes the field an email) or it can be empty (just a
normal optional field).

For mandatory fields we can also define "group-name.fields.field-name.mandatory"
to override the error message that will be returned by the handler if the field
was empty. Likewise for email fields we can define
"group-name.fields.field-name.email" to set the error message that's returned if
the content of the field doesn't look like an email.

This is not the most elegant way to configure forms, but it's simple and
flexible enough. See validate_fields() at
https://hg.adblockplus.org/sitescripts/file/tip/sitescripts/formmail/web/form...
 for more insight on how this works.

https://codereview.adblockplus.org/29984580/diff/29985558/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29985558/sitescripts/formmai...
sitescripts/formmail/README.md:40: handler1.csv_log = /var/log/something.csv_log
On 2019/01/18 20:08:07, rhowell wrote:
> Does the spacing around the `=` matter? This is the only one with spaces.

AFAIK it doesn't. This is parsed by python ConfigParser and it seems to ignore
them. With this in mind, it probably makes sense to have spaces around "=" -- it
looks better this way and is easier to read.

[rhowell, 2019-01-22 22:53:54]

Hey Vasily!

Thanks for your help. Hopefully it's looking better now?

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmai...
sitescripts/formmail/README.md:36: [formmail]
On 2019/01/21 13:54:17, Vasily Kuznetsov wrote:
> On 2019/01/18 20:08:07, rhowell wrote:
> > On 2019/01/18 16:52:13, Vasily Kuznetsov wrote:
> > 
> > > `handler1.fields.xxx` (a subgroup that includes the descriptions of the
form
> > > fields).
> > 
> > 
> > I'm still not exactly clear what the `.fields` section refers to, but maybe
> > whoever is configuring the server would understand it more clearly. Does it
> > change the behavior of the actual form? Or send an error response to the
POST
> > request if something mandatory is missing?
> 
> The form itself is usually another page, so its behavior is unchanged by this
> config but the two have to match. Also the handler can return error messages
> that depend on the configuration here and can be further changed by it. I
think
> we should document all of it here, otherwise the people configuring the server
> would have to ask.
> 
> The ".fields" group describes the fields that are expected in the POST request
> and then become available to the template. Each variable in the group defines
a
> field and its value can contain "mandatory" (which makes the field mandatory)
> and/or "email" (which makes the field an email) or it can be empty (just a
> normal optional field).
> 
> For mandatory fields we can also define
"group-name.fields.field-name.mandatory"
> to override the error message that will be returned by the handler if the
field
> was empty. Likewise for email fields we can define
> "group-name.fields.field-name.email" to set the error message that's returned
if
> the content of the field doesn't look like an email.
> 
> This is not the most elegant way to configure forms, but it's simple and
> flexible enough. See validate_fields() at
>
https://hg.adblockplus.org/sitescripts/file/tip/sitescripts/formmail/web/form...
>  for more insight on how this works.

Ahh, gotcha. Makes sense. Thank you for the very clear explanation, I used most
of it.

https://codereview.adblockplus.org/29984580/diff/29985558/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29985558/sitescripts/formmai...
sitescripts/formmail/README.md:40: handler1.csv_log = /var/log/something.csv_log
On 2019/01/21 13:54:18, Vasily Kuznetsov wrote:
> On 2019/01/18 20:08:07, rhowell wrote:
> > Does the spacing around the `=` matter? This is the only one with spaces.
> 
> AFAIK it doesn't. This is parsed by python ConfigParser and it seems to ignore
> them. With this in mind, it probably makes sense to have spaces around "=" --
it
> looks better this way and is easier to read.

Done.

[Vasily Kuznetsov, 2019-01-23 12:23:13]

Hi Rosie!

Great. I think we have all the information we need in the README now. I just
have a couple of nits related to imaginary file naming (see below).

Also, could you please format it so the lines are below 80 characters in length
(so 80 is already too long) so we conform to the unspoken README formatting
standard (hm... maybe we should document this "standard" somewhere) and then
we're good to go.

Cheers,
Vasily

https://codereview.adblockplus.org/29984580/diff/29987575/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29987575/sitescripts/formmai...
sitescripts/formmail/README.md:40: handler1.csv_log = /var/log/something.csv_log
Nit: let's rename the file to `handler1-log.csv`, i think it makes it a bit more
clear that it's the log of handler1 and that the file format is CSV.

https://codereview.adblockplus.org/29984580/diff/29987575/sitescripts/formmai...
sitescripts/formmail/README.md:45: handler1.template =
formmail/test/template/test.mail
Nit: here we can also rename the template file to make its name a bit more
telling. Maybe something like formmail/handler1/mail-template.tmpl -- how does
this look?

[rhowell, 2019-01-23 21:40:26]

Hey Vasily,

Good idea renaming those imaginary files. The new names are much more useful,
thanks.

I edited all the lines down to 79 characters, but just wanted to confirm that
this is a hard rule, no exceptions? I usually check how a README is rendered on
https://dillinger.io/, and some of the lines are very uneven now. Especially
links, which (AFAIK) cannot be separated by a line break. Also, many of our
other READMEs go over by a few characters here and there. Just wanted to
double-check.

https://codereview.adblockplus.org/29984580/diff/29987575/sitescripts/formmai...
File sitescripts/formmail/README.md (right):

https://codereview.adblockplus.org/29984580/diff/29987575/sitescripts/formmai...
sitescripts/formmail/README.md:40: handler1.csv_log = /var/log/something.csv_log
On 2019/01/23 12:23:13, Vasily Kuznetsov wrote:
> Nit: let's rename the file to `handler1-log.csv`, i think it makes it a bit
more
> clear that it's the log of handler1 and that the file format is CSV.

Done.

https://codereview.adblockplus.org/29984580/diff/29987575/sitescripts/formmai...
sitescripts/formmail/README.md:45: handler1.template =
formmail/test/template/test.mail
On 2019/01/23 12:23:13, Vasily Kuznetsov wrote:
> Nit: here we can also rename the template file to make its name a bit more
> telling. Maybe something like formmail/handler1/mail-template.tmpl -- how does
> this look?

Done.

[Vasily Kuznetsov, 2019-01-24 13:16:28]

Hi Rosie!

LGTM!

On 2019/01/23 21:40:26, rhowell wrote:
> 
> I edited all the lines down to 79 characters, but just wanted to confirm that
> this is a hard rule, no exceptions? I usually check how a README is rendered
on
> https://dillinger.io/, and some of the lines are very uneven now. Especially
> links, which (AFAIK) cannot be separated by a line break. Also, many of our
> other READMEs go over by a few characters here and there. Just wanted to
> double-check.

It's not a completely hard rule, exceptions can me made, especially for URLs and
pre-formatted text. The current version looks good though: I checked it using
Grip (https://pypi.org/project/grip/) that essentially uses GitHub renderer and
the output looks just like it should.

Cheers,
Vasily