Do not update SMTP or omniauth attributes if no values are specified

[microstudi]

🎩 What? Why?

This PR is a quick fix for a very annoying situation that happens on updating organization on the /system admin.

If you configure your Decidim to use ENV variables (or any other method using the secrets.yml file), everytime you update in the /system admin some setting, but not the omniauth or smtp settings, these properties are filled with an empty hash. Then this is being used for confguring the SMTP or the omniauth configuration. Then you got your Decidim broken not being able to sent emails.

This is very annoying when using a multitenant system with shared smtp for instance (a very common situation).

I'd port this fix to the 0.27 series as well.

📌 Related Issues

Link your PR to an issue

• Related to #?
• Fixes #?

Testing

1. configure your Decidim with a omniauth or a smtp value using ENV vars
2. go to /system, edit something there (like the authoritzations enabled)
3. SMTP or Omniauth is not longer working,, you need to go to the console and nilify the smtp_attributes to fix it.

📷 Screenshots

Please add screenshots of the changes you are proposing

♥️ Thank you!

[request-info]

It seems like you did not give us much information about what you are trying to do here. We would appreciate it if you could provide us with more info about this issue/PR!

[github-actions]

This pull request does not contain a valid label. Please add one of the following labels: ['type: feature', 'type: change', 'type: fix', 'type: removal', 'target: developer-experience', 'type: internal']

[alecslupu]

@microstudi could you fix the conflict ?

[microstudi]

@alecslupu solved!

[andreslucena]

@microstudi I agreed with not saving the wrong contents in this form, but how about current installations that already have this problem?

I think that we should add a check like the all?(&:blank?) where we consume these values. For instance for the mailers I was playing with this approach:

diff --git a/decidim-core/app/mailers/decidim/application_mailer.rb b/decidim-core/app/mailers/decidim/application_mailer.rb
index 3a6c5cc79a..edaa1fb9dd 100644
--- a/decidim-core/app/mailers/decidim/application_mailer.rb
+++ b/decidim-core/app/mailers/decidim/application_mailer.rb
@@ -21,7 +21,7 @@ module Decidim
     attr_reader :organization

     def set_smtp
-      return if organization.nil? || organization.smtp_settings.blank?
+      return if organization.nil? || organization.smtp_settings.blank? || organization.smtp_settings.all? { |_key, value| value.blank? }

       mail.reply_to = mail.reply_to || Decidim.config.mailer_reply
       mail.delivery_method.settings.merge!(

I didn't get a good spec yet, but I think this would fix the problem. The two solutions can be compatible (one is preventing to save wrong values, the other one is preventing using these values if its already saved). What do you think? Does it makes sense? If not we should add a command to fix the current installations that have this bug.

Let me know if you don't have the bandwidth to tackle this one as we can assume it if not.

[microstudi]

@microstudi I agreed with not saving the wrong contents in this form, but how about current installations that already have this problem?

I think that we should add a check like the all?(&:blank?) where we consume these values. For instance for the mailers I was playing with this approach:

diff --git a/decidim-core/app/mailers/decidim/application_mailer.rb b/decidim-core/app/mailers/decidim/application_mailer.rb
index 3a6c5cc79a..edaa1fb9dd 100644
--- a/decidim-core/app/mailers/decidim/application_mailer.rb
+++ b/decidim-core/app/mailers/decidim/application_mailer.rb
@@ -21,7 +21,7 @@ module Decidim
     attr_reader :organization

     def set_smtp
-      return if organization.nil? || organization.smtp_settings.blank?
+      return if organization.nil? || organization.smtp_settings.blank? || organization.smtp_settings.all? { |_key, value| value.blank? }

       mail.reply_to = mail.reply_to || Decidim.config.mailer_reply
       mail.delivery_method.settings.merge!(

I didn't get a good spec yet, but I think this would fix the problem. The two solutions can be compatible (one is preventing to save wrong values, the other one is preventing using these values if its already saved). What do you think? Does it makes sense? If not we should add a command to fix the current installations that have this bug.

Let me know if you don't have the bandwidth to tackle this one as we can assume it if not.

I think the ideal would be to have a checkbox in the system saying "override default values", but this I don't have time to do.

For your proposed solutions, both work for me. I'll implement your suggestion as is the simplest one

[microstudi]

@andreslucena I've applied the proposed solution with the change of ignoring the from* attributes from the smtp_settings, this way it also solve the possible user case that you want to customized without changing the server settings

[andreslucena]

Great, thanks for the PR!