Bonfire icon indicating copy to clipboard operation
Bonfire copied to clipboard

Difference between Site Email and System Email

Open mwhitneysdsu opened this issue 9 years ago • 8 comments

As mentioned by @JbalTero on the forums, the difference between Site Email and System Email is not clear.

There may be some instances of the users module using the Site Email when it should be using the System Email (as the 'from' address in emails sent by the system), and the description of the Site Email should be changed to reflect that the Site Email is intended for display to end users (while the System Email is the email address used in the 'from' address for emails sent by the system).

While the two can be the same, and probably were at one point, we want people to be able to use one email for display on the site (a contact address), and a different email for the from address on emails (for example, a noreply@ address that doesn't exist on the mail server).

mwhitneysdsu avatar Oct 26 '15 16:10 mwhitneysdsu

I encountered an error when I try to "Send New Email" on a freshly installed Bonfire. Failure creating emails: One or more required fields are missing.

The error persist until I set the Sytem Email on Email settings.

That's why I got that question on the forum.

jbalatero avatar Oct 26 '15 23:10 jbalatero

I think the best solution for that, in addition to the items above, would be to change the Emailer library's send() method to fall back to settings_item('site.system_email') if $data['from'] is empty and settings_item('sender_email') == false, which would require changing the following lines: https://github.com/ci-bonfire/Bonfire/blob/0.8-dev/bonfire/modules/emailer/libraries/Emailer.php#L120 https://github.com/ci-bonfire/Bonfire/blob/0.8-dev/bonfire/modules/emailer/libraries/Emailer.php#L130

For example, it might look something like this:

        if (empty($data['to'])
            || ! isset($data['message'])
            || ! isset($data['subject'])
            || (empty($data['from']) 
                && settings_item('sender_email') == false
                && settings_item('site.system_email') == false
            )
        ) {
            $this->error = lang('emailer_missing_data');
            return false;
        }

        // ... $to, $subject, etc.

        $from = empty($data['from']) ? (settings_item('sender_email') ?: settings_item('site.system_email')) : $data['from'];

or, to remove the nesting in the if() statement and perform one last check on the $from value:

    public function send($data = array(), $queueOverride = null)
    {
        // Ensure the required information is supplied.
        $from = empty($data['from']) ? (settings_item('sender_email') ?: settings_item('site.system_email')) : $data['from'];

        if (empty($data['to'])
            || ! isset($data['message'])
            || ! isset($data['subject'])
            || empty($from)
        ) {
            $this->error = lang('emailer_missing_data');
            return false;
        }

        // ... $to, $subject, $message, etc. ...

mwhitneysdsu avatar Oct 27 '15 13:10 mwhitneysdsu

sender_email and site.system_email is still confusing. I think sender_email should be something like email.system_email and site.system_email should be site.site_email though I think this refactoring needs to check every referenced string.

Your solution above is good enough whenever sender_email is not provided. And we should change the description for sender_email and site.system_email to avoid confusion and also with documentation.

jbalatero avatar Oct 30 '15 01:10 jbalatero

oh nevermind my refactoring suggestion. That would confuse more users especially upgrading users.

jbalatero avatar Oct 30 '15 01:10 jbalatero

It's a good idea, but even if we were to go down that road, it would be a compatibility break and would require some time between the decision to make the change and the change itself (marking it deprecated in the documentation, maintaining the new and old names in the system side-by-side for a while, etc.).

mwhitneysdsu avatar Oct 30 '15 12:10 mwhitneysdsu

I have the same problem @JbalTero but in my case is the limitation of password length, I open a case #1181.

andrekutianski avatar Nov 09 '15 13:11 andrekutianski

@andrebellafronte That's a completely different problem, but I've given you a potential fix over there. If that works, we can make the change in the module, as I don't see any additional problems with that.

mwhitneysdsu avatar Nov 09 '15 17:11 mwhitneysdsu

I've pushed the fallback to Site Email and clarified the wording on the settings view (though we'll need some updates to the translations). I haven't taken a look at the documentation for these settings, yet. I'm still considering whether we should work on the names of the settings.

mwhitneysdsu avatar Nov 10 '15 16:11 mwhitneysdsu