Skip to content

Set "from" email address using settings table instead of hard-coding#273

Merged
tom93 merged 7 commits intoNZOI:masterfrom
tom93:pr/remove-hard-coded-nztrain-email
Mar 20, 2025
Merged

Set "from" email address using settings table instead of hard-coding#273
tom93 merged 7 commits intoNZOI:masterfrom
tom93:pr/remove-hard-coded-nztrain-email

Conversation

@tom93
Copy link
Contributor

@tom93 tom93 commented Mar 19, 2025

Second version of #272.

This PR renames the setting system/mailer/username to system/mailer/email and uses that for both SMTP authentication and as "from" email address (previously it was used only for SMTP authentication, and the "from" address was hard-coded to "nztrain@gmail.com").

It also sets the sender's display name to "NZOI Training" and cleans up a couple of things.

Holmes98 and others added 7 commits March 19, 2025 21:39
Passwords should never be hard-coded in the source code for security.
The default user name was somewhat useful for the official instance,
but it's cleaner to avoid it.

(Note: Now that the default value is blank, it's not clear that the
setting "system/mailer/username" is supposed to be an email address.
This will be addressed in a later commit that renames that setting to
"system/mailer/email".)
Refactor the code to set the sender email address in one place instead
of multiple places.

This is in preparation for making the "from" address configurable.

Note that `config.action_mailer.default()` doesn't work[1], so we must
use `default_options=`.

Also drop `:charset => "utf-8"`, because the default is already UTF-8
(and that line wasn't even doing anything due to [1]).

[1] https://www.github.com/rails/rails/issues/54700
Previously the "from" address was hard-coded to nztrain@gmail.com and
could not be configured.

This commit uses the value of the "system/mailer/username" setting,
which is the user name for SMTP authentication. It will be the same as
the "from" email when using Gmail and in most other setups. (A later
commit will rename that setting to "system/mailer/email".)

* We provide a default email in the test environment, otherwise the
  specs fail.
* We don't provide a default email in the production environment,
  because the admin must configure email properly in order
  for mail delivery to work.
* We don't provide a default email in the development environment,
  because mail delivery will fail anyway (unless SMTP is configured
  properly). The admin will still be able to read the emails by
  looking in the development log.

In the future we may wish to add a setting for overriding the
"Reply-To" address; it could be useful for redirecting responses to an
mailbox that's actually monitored
(https://www.github.com/NZOI/nztrain/pull/272#issuecomment-2704959896).
But for now we don't bother.
It is now used for both SMTP authentication and for the "from" email
address, so the term "email" is clearer and more accurate.
It's better to use seeds for adding initial data to the database.
Ideally it should be configurable, but we hard-code it for now like in
other places for simplicity.

Suggested-by: Jonathan Khoo <jkhoo98@gmail.com>
@coveralls
Copy link

Coverage Status

coverage: 37.537% (+0.006%) from 37.531%
when pulling d3d216c on tom93:pr/remove-hard-coded-nztrain-email
into 197eaf5 on NZOI:master.

Copy link
Member

@Holmes98 Holmes98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@tom93 tom93 merged commit 64d4532 into NZOI:master Mar 20, 2025
3 checks passed
@tom93 tom93 deleted the pr/remove-hard-coded-nztrain-email branch March 20, 2025 04:08
@tom93
Copy link
Contributor Author

tom93 commented Mar 20, 2025

Deployed, thanks for your patience :)

screenshot-nztrain-email

(I forgot you already created #254 to use seeds.rb.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants