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

Remove smtp config from admin config page #5235

Merged
merged 2 commits into from Apr 29, 2020

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Apr 15, 2020

What? Why?

Closes #1534

This is a quick fix to 1534. We remove the fields because they need dev intervention: start/stop delayedjob.
Before:
image
After:
image

These values are updated on every deploy through the seeds task. The values are set in the secrets file used by ofn-install. This needs to be tested in staging as part of this PR.

What should we test?

We are still able to use the existing fields in this form and they still work.
As part of this PR we need to test that ofn-install will update the smtp password in the preferences table during a normal deploy.

Release notes

Changelog Category: Changed
Remvoed mail settings config from the UI as they can only be changed by developers.

@kristinalim
Copy link
Member

This PR needs both dev-test and user test. Tagging this dev-test first.

@kristinalim kristinalim added the dev-test A dev need to test this one label Apr 22, 2020
@sauloperez sauloperez self-assigned this Apr 29, 2020
@sauloperez
Copy link
Contributor

sauloperez commented Apr 29, 2020

UI-wise things work but I'm wondering about

One thing I can add in #5235 is to remove mail methods from spree_preference in a migration.

Do you plan to add this?

Re smtp password in the preferences table, in my docker dev setup db/seeds.rb successfully picks up the values from the available env vars set in config/application.yml. I'll check now in staging.

@luisramos0
Copy link
Contributor Author

thanks for mentioning that. I forgot about that.
I rebased to resolve conflicts in db/schema and added the migration.

drop_table :spree_mail_methods

# delete mail_method preferences associated with the old MailMethod model
execute "DELETE FROM spree_preferences WHERE key LIKE 'spree/mail_method%'"
Copy link
Contributor

Choose a reason for hiding this comment

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

this will skip smtp_username and smtp_password though. Is that what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think smtp_username and smtp_password are the values set in MailConfiguration and used in MailSettings.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, got it. These are old Spree preference and not ours. I wasn't sure what you were aiming at.

@sauloperez
Copy link
Contributor

sauloperez commented Apr 29, 2020

Confirmed it works.

openfoodnetwork=> select * from spree_preferences where key ilike '%mail_host%';
-[ RECORD 1 ]---------------------------------
id         | 2
value      | smtp.mandrillapp.com
created_at | 2019-07-23 10:01:26.371498
updated_at | 2020-04-29 11:43:10.463866
key        | spree/app_configuration/mail_host
value_type | string

openfoodnetwork=> delete from spree_preferences where key ilike '%mail_host%';
DELETE 1
openfoodnetwork=> select * from spree_preferences where key ilike '%mail_host%';
 id | value | created_at | updated_at | key | value_type 
----+-------+------------+------------+-----+------------
(0 rows)

*I deployed right here*

openfoodnetwork=> select * from spree_preferences where key ilike '%mail_host%';
-[ RECORD 1 ]---------------------------------
id         | 502
value      | smtp.mandrillapp.com
created_at | 2020-04-29 13:58:04.582884
updated_at | 2020-04-29 13:58:04.582884
key        | spree/app_configuration/mail_host
value_type | string

This is ready for regular testing.

@sauloperez sauloperez removed the dev-test A dev need to test this one label Apr 29, 2020
@luisramos0
Copy link
Contributor Author

great, thanks Pau!

@filipefurtad0 filipefurtad0 self-assigned this Apr 29, 2020
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Apr 29, 2020

Hi @sauloperez ,

Tested the remaining fields:

  • send email
  • send copy
  • intercept email

Used the Send Test Email and tested whether all expected emails arrive on a signing-in process.

All work fine - ready to go.

@luisramos0
Copy link
Contributor Author

oh, this was quick!!!

@luisramos0 luisramos0 merged commit 8a107be into openfoodfoundation:master Apr 29, 2020
@luisramos0 luisramos0 deleted the mail_methods branch April 29, 2020 18:10
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.

Email sending fails after changing mail method
5 participants