Mail signature

[scabux]

Added mail signature functionality.

[codecov]

Codecov Report

Merging #485 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #485      +/-   ##
==========================================
+ Coverage   81.96%   81.97%   +0.01%     
==========================================
  Files         165      165              
  Lines       11614    11623       +9     
==========================================
+ Hits         9519     9528       +9     
  Misses       2095     2095

Impacted Files	Coverage Δ
src/pretix/base/settings.py	100% <ø> (ø)	⬆️
src/pretix/base/services/mail.py	94.18% <100%> (+0.59%)	⬆️
src/pretix/control/forms/event.py	93.65% <100%> (+0.03%)	⬆️

[raphaelm]

Hey,

first of all, congrats to your first PR and thank you very much 🎉

This looks very good, I only have two very minor remarks:

• The default value apparently doesn't work. When sending an email from a new event, a signature of the literal word None is appended to the email. One could argue that this is a problem of either hierarkey or i18nfield and not your code and that is probably also correct. But for now, setting a default value of "" instead should work ;)
• To bring this to perfection, a unit test would be perfect. There already is the file tests/base/test_mail.py that you could just expand with a new function. You can start by copying test_send_mail_with_event_sender and then first set a value for the signature and then later check that the signature is actually contained in the email. (Should you want to read more about testing email in django, see here). You can use py.test tests/base/test_mail.py to just run the tests from this file locally (running all tests can take quite some time).

Cheers
Raphael

[scabux]

Hey,

thank you for your warm welcome and positive feedback.
I fixed the default value issue and tested it creating a new event. I also added a small unit test.

Can't wait to work on the next feature. ;)

Cheers
Marvin

EDIT: I will try to combine the small commits into few larger ones next time. :P

[raphaelm]

I fixed the default value issue and tested it creating a new event. I also added a small unit test.

Great! 💯 Merged!

Can't wait to work on the next feature. ;)

I've got an idea here that we should probably talk about shortly :)

EDIT: I will try to combine the small commits into few larger ones next time. :P

No worries here. GitHub recently got a very useful "Squash and merge" button so I can easily meld them together in one single commit as soon as I merge it. Previously, I'd have asked you to do this yourself. This is not required any more, but might still be an interesting read and give some insights into git. Actually, keeping the small commits between the different iterations can actually be helpful to review, so I don't have to re-review all of it again and can see what has changed between iterations.