[PATCH 1 of 5] e-mail: describe and clarify e-mail settings in the ini file

[Mads Kiilerich]

On 07/13/2015 09:26 PM, Thomas De Schampheleire wrote:
> On Mon, Jul 13, 2015 at 8:19 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 07/13/2015 10:45 AM, Thomas De Schampheleire wrote:
>>> # HG changeset patch
>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>> # Date 1436644666 -7200
>>> #      Sat Jul 11 21:57:46 2015 +0200
>>> # Node ID c22a219636b830bac4b6125b306f6c8506f81d99
>>> # Parent  4a3708db60aef76020f4fefacf861ee830abc564
>>> e-mail: describe and clarify e-mail settings in the ini file
>>>
>>> Add some description of the e-mail related settings in the ini file.
>>> Use sensible (uncommented) defaults for app_email_from and email_prefix.
>>
>> I wonder, why are these new values more sensible?
> In configuration files for a piece of software, it is common practice
> that key/value lines that are commented out show the default value.
> However, for the Kallithea config files it is not at all true. For
> example: the subject prefix looks as defaulting to [Kallithea], but
> the code itself defaults to an empty prefix.
>
> So, I think the defaults in the ini file and the defaults in the code
> should line up.
>
> See below for details.

Ok, so it is not just "the template content (enabled or disabled) is the 
default" but "the value in the template is what would be used if nothing 
was configured". It is not your definition of 'sensible' but just 
consistent and explicit.

It would perhaps be nice to also show "examples of other good values".

(Also, in some places we have comments that are insufficiently 
self-contained and can become very misleading if the context changes 
(for example by commenting/uncommenting). Worst case is "THE LINE BELOW 
MUST BE UNCOMMENTED"...)

>>
>>> Clarify that the default value of smtp_use_ssl is false by updating the
>>> commented setting.
>>>
>>> This also deletes the commented setting of 'error_message' which does not
>>> seem to be used. It is referring to the error_message variable in Paste,
>>> which has as description (Paste:ErrorMiddleware):
>>>       When debug mode is off, the error message to show to users.
>>> However, setting this value apparently made no effect at all in Kallithea.
>>
>> It would be nice if such related but separate spin-off changes could be done
>> in separate changesets.
> Ok, will do.
>
>> It would also be nice if the .ini files could be updated with the templates
>> so the diff remains as small as possible.
> Ah, yes, I forgot that there were also real .ini files in the
> Mercurial tree. But, what is their purpose, really? Should we really
> keep them?

They can be convenient but I guess they perhaps can be removed if the 
rest of the code base is updated too (especially documentation). 
Especially 'development.ini' might however require 'something else' to 
replace it. test.ini is even more essential. That doesn't leave much to 
remove...

>
>>> diff --git a/kallithea/bin/template.ini.mako
>>> b/kallithea/bin/template.ini.mako
>>> --- a/kallithea/bin/template.ini.mako
>>> +++ b/kallithea/bin/template.ini.mako
>>> @@ -10,22 +10,29 @@ debug = true
>>>    pdebug = false
>>>    <%text>
>>>
>>> ################################################################################
>>> -## Uncomment and replace with the address which should receive
>>> ##
>>> -## any error reports after application crash
>>> ##
>>> -## Additionally those settings will be used by Kallithea mailing system
>>> ##
>>> +## E-mail settings
>>> ##
>>> +##
>>> ##
>>> +## email_to: The e-mail address to send error reports to.
>>> ##
>>
>> I think it would be better if the description could be rephrased to answer:
>>
>> Who will send which error reports ... and when?
>>
>> The texts in kallithea/templates/admin/settings/settings_email.html (and
>> perhaps some code comments) should perhaps be synchronized and clarified
>> while we are at it.
> I'll look into it.
>
>>> +## error_email_from: The sender of error e-mails.
>>> ##
>>> +## app_email_from: The sender of mails originating from Kallithea, for
>>> ##
>>> +##                 example to notify users about new comments, pull
>>> requests, ##
>>> +##                 etc.
>>> ##
>>> +## email_prefix: The subject prefix for mails originating from Kallithea.
>>> ##
>>> +##
>>> ##
>>> +## Note: your SMTP server may require app_email_from/error_email_from to
>>> be   ##
>>> +## valid, existing addresses.
>>> ##
>>>
>>> ################################################################################</%text>
>>>    #email_to = admin at localhost
>>> -#error_email_from = paste_error at localhost
>>> -#app_email_from = kallithea-noreply at localhost
>>> -#error_message =
>>> -#email_prefix = [Kallithea]
>>> +#error_email_from = kallithea-noreply at localhost
>>> +app_email_from = kallithea-noreply at localhost
>>
>> Why set app_email_from but not error_email_from?
> I think my reasoning was that it didn't make sense to set
> error_email_from when email_to is not set (and for which we cannot
> provide a reasonable default). But maybe that reasoning is bogus and
> error_email_from can be set just as well.
>
>> And is "@localhost" really a default value that is expected to be correct
>> and work in most cases? I don't think so. The System itself will probably be
>> better at coming up with a @hostname address.
> The default app_email_from is 'Kallithea', so not even an e-mail
> address.

Most(?) mail servers will add their default domain or hostname if they 
see an address without domain. That will often be a good approximation.

> Having an @localhost address seems already better to me, even
> though a real admin will probably want to change it. Still, for cases
> where the SMTP server doesn't really care about the from address, the
> @localhost address should work fine.

@localhost is _always_ wrong when exchanging mails between machines and 
it makes a lot of sense for mail servers to reject such mails - either 
in correctness checks or to avoid some spam.

>
>>> +email_prefix = [Kallithea]
>>
>> Is it really an improvement to enable this by default? And should it tag
>> everything with [Kallithea]? This is information that follows from the
>> sender ... and a user might be using many different code hosting sites where
>> it not really is important which of them use Kallithea.
> As mentioned, the default suggested by the ini file does not match
> with the actual default.
> The subject prefix '[Kallithea]' seemed sensible to me as default, but
> empty is fine for me too if you prefer. In fact, the latter is also
> what we have set in production.

I think that is an example of a setting where it would be nice if the 
template showed both the default (empty=disabled) and an example 
(suggesting how square brackets often is used).

/Mads