Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify email configuration #683

Open
mkllnk opened this issue Dec 16, 2020 · 9 comments
Open

Simplify email configuration #683

mkllnk opened this issue Dec 16, 2020 · 9 comments

Comments

@mkllnk
Copy link
Member

mkllnk commented Dec 16, 2020

Description

We store our email configuration in the ofn-secrets repository. When provisioning a server, we create /etc/defaults/openfoodnetwork to store the email configuration in environment variables for the user. Those are only loaded in a login session though and not available in the unicorn process.

During deployment, we run rake db:seed which reads the environment variables and writes them to the database via Spree::Config. The application (unicorn and delayed_job) reads these values through Spree::Config. These values are cached though so that the retrieval depends on memcached to be running, otherwise defaults are provided. A stale cache can also lead to different processes seeing different configurations and not looking at the database.

This process is complex and brittle and led to #6529.

Expected Behavior

The application reads the email configuration from a file or the environment without depending on the database or a cache.

Actual Behaviour

Email fails if the cache is out of sync or down. We also have multiple places storing the config (on disk and in the database).

Steps to Reproduce

  1. As ofn-admin: sudo systemctl stop memcached.service
  2. bundle exec rails console
  3. Verify output of ActionMailer::Base.smtp_settings

Severity

tech-debt

Possible Fix

Since Rails 4.1 we can store configuration like this in config/secrets.yml and configure ActionMailer with that.

https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#config-secrets-yml
https://guides.rubyonrails.org/4_1_release_notes.html#config-secrets-yml

@sauloperez
Copy link
Contributor

Related to #696

@sauloperez
Copy link
Contributor

sauloperez commented Feb 19, 2021

@andrewpbrett hit this again while fixing Canada's Gmail credentials issue today. What we didn't like was the fact that we had to redeploy for the ENV var change to be applied. This slowed us down a lot.

Wouldn't it be as simple as copying the ENV vars values to ActionMailer::Base.smtp_settings and forget about anything else, including Spree::Config?

@mkllnk
Copy link
Member Author

mkllnk commented Feb 22, 2021

Yes, since we configure mail settings via ofn-install, we shouldn't need Spree::Config at all for this.

@sauloperez
Copy link
Contributor

it sounds like we could get rid of quite some code and end up with a plain simple configuration 😍

@Matt-Yorkley
Copy link
Collaborator

Matt-Yorkley commented Feb 22, 2021

Great, this is long overdue!

We set the mail configs in environment variables here when provisioning: https://github.com/openfoodfoundation/ofn-install/blob/807e6fd12dd1960eae61303ba154bd86f4090d90/roles/unicorn_user/templates/defaults.j2

We apply those configs via db/seed.rb during deployment (which is a bit crazy): https://github.com/openfoodfoundation/openfoodnetwork/blob/55215bb59a5b2695b3d6838654a1f078d7ff23f6/db/seeds.rb#L5-L18

...which calls this service to persist them in the (problematic) Spree Configs here: https://github.com/openfoodfoundation/openfoodnetwork/blob/55215bb59a5b2695b3d6838654a1f078d7ff23f6/app/services/mail_configuration.rb#L6-L10

Lets clean it up 💪

I think we can leave the environment variables as they are, move the setup out of db/seed.rb, and update that service to avoid Spree::Config.

@sauloperez
Copy link
Contributor

sauloperez commented Feb 22, 2021

There's another piece here. It's https://github.com/openfoodfoundation/openfoodnetwork/blob/master/lib/spree/core/mail_settings.rb#L16 the line that ends up configuring Rails' SMTP settings. In a vanilla Rails app that's the only line you need to make it work.

Also, we can't forget about the couple of form fields we still have in /admin/mail_methods/edit. They may ruin the party...

@Matt-Yorkley
Copy link
Collaborator

🔥🔥🔥🔥🔥🔥

@romale
Copy link

romale commented May 13, 2021

Why not make something like this, as you know, this is also a RoR application? Mail settings don't change that often. We can set it up once and forget about it. Today, there are regularly some problems with the mail settings, especially after updates.

@mkllnk
Copy link
Member Author

mkllnk commented May 18, 2021

Yes, that's what we are aiming for. The problem was that Spree stores this configuration in the database and we have been moving away from that. At the moment, we are in an awkward in-between phase but we want a simple config file as you described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: All the things 💤
Development

No branches or pull requests

4 participants