search

maccesch / cmsplugin-contact

Extendable contact plugin for django-cms with spam protection and i18n
BSD 2-Clause "Simplified" License
71 stars 62 forks source link

Removed fallback "From:" email addess from getattr(). #51

Closed m000 closed 10 years ago

m000 commented 10 years ago

Also updated docs to make explicit that DEFAULT_FROM_ADDRESS is the one always used to send the emails.

Currently it seems we are ok with always using DEFAULT_FROM_ADDRESS. The old behaviour (before fixing #27), was to use the address supplied by the user to set the "From:" header.

We will need to add a new setting in case we want to be able to choose between the current and the old behaviour. Anyone who needs this should open a new issue.

m000 commented 10 years ago

Added info about the Reply-To: header as well. The info seem out of place in their current location (under DEFAULT_FROM_EMAIL) so I included a comment/reminder to move them in a better place when the docs are expanded/rearranged.

maccesch commented 10 years ago

Thanks a lot. Great work!

mitar commented 10 years ago

What? Why removed this field? OK, public servers maybe reject from field. But not everybody is using public servers. We should not fall into this everything in-cloud frenzy and forget that some people (including me) are running our own servers where we can have our own mail delivery. So I don't understand why removing a feature which works perfectly fine if you configure things?

For example, I am running this version of code and I don't have any issues with setting reply-to field. Now I understand why from is not set (because of default overriding it). I didn't have time to debug that yet, but now I understand.

So, I think that breaking backwards compatibility just because something does not work on some servers is a bad idea. When Django become something you run only with public mail servers?

maccesch commented 10 years ago

Why do you think we removed the Reply-To: header? We only removed code that wasn't used anyway.

mitar commented 10 years ago

OK, maybe I didn't explain well. What I wanted to say is that user can specify from address and that on some servers you can configure that as "From" of the e-mail. So previous code didn't really set that, it should, if the server supports that. What I believed that it is a bug before (not setting properly "From" because of "wrong" order of assignment) was now made as correct and only behavior. And I believe this is wrong. So the correct way to fix that code is not to remove part of it, but to make it so that you can use user-specified "From" if you want.

maccesch commented 10 years ago

Alright. But as @m000 correctly pointed out, checking for servers that don't support custom From: headers is ugly and unreliable. I really think it is not important to have that feature. A lot of people have used our plugin including you and it has never been a problem.

But if you want you can add a custom setting called USE_USER_EMAIL_AS_FROM_HEADER or something like that that defaults to False. This way users could specify that they want this behaviour. If you do so please update the docs as well and point out the risks.

m000 commented 10 years ago

@mitar continuing the discussion in this thread is sub-optimal. Please open a new issue with a feature request to discuss the issue there and let anyone interested to weigh in. In general I agree with @maccesch that this is a minor enhancement we could as well live without. I could have easily implemented it in my patch but I preferred not to. IMHO unless someone (you?) really needs this functionality adding it "because we can" is just bloating of the codebase.