Skip to content

Fix SMTP settings update#6664

Merged
tramuntanal merged 22 commits intodecidim:developfrom
Dynnammo:backport/smtp_settings_fix_fields
Nov 10, 2020
Merged

Fix SMTP settings update#6664
tramuntanal merged 22 commits intodecidim:developfrom
Dynnammo:backport/smtp_settings_fix_fields

Conversation

@Dynnammo
Copy link
Copy Markdown
Contributor

@Dynnammo Dynnammo commented Oct 13, 2020 •
edited
Loading

🎩 What? Why?

When changing fields in System > Organization > <your-organization> > SMTP Settings (from_email and from_label), it does not update.

What's the matter ?
There is three fields concerned by such issue : from_email, from_label and from
from_email stores the bare bone email of the system ie. noreply@example.org
from_label stores the name under which we should send mails. As instance : 'My organization'
The from attribute is there to make a concatenation of both. It is defined in set_from in update_organization_form.rb

What does this pull request is :

  • correct the hash definition of smtp_settings from a string-based-key hash to a symbol-based-key hash
  • correct tests that comes along

📌 Related Issues

Link your PR to an issue

  • Related to #?
  • Fixes #?

Testing

  • Go to System > Organizations, edit your organization and change field from email and from label
  • Open a letter_opener : http://localhost:3000/letter_opener
  • Simulate a lost password / a procedure that sends email
  • check on letter_opener tab that the email hasn't changed

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@Dynnammo Dynnammo marked this pull request as ready for review October 14, 2020 06:44
tramuntanal
tramuntanal suggested changes Oct 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @Dynnammo 👍

I've found some confusing points in the PR, can you check please?
Also, it will be good to have some tests for the update_organization_form and the application_mailer.

@tramuntanal tramuntanal self-assigned this Oct 15, 2020
@Dynnammo Dynnammo marked this pull request as draft October 15, 2020 15:03
@Dynnammo
Copy link
Copy Markdown
Contributor Author

Dynnammo commented Oct 20, 2020 •
edited
Loading

I made the corrections I could.
I however do not get where is tested the mail configuration defined in application_mailer.rb. I may need some help in this point

The CI seems to fail randomly (Meetings, then Proposals (system public) and Proposals (unit tests) successively) but not locally. It seems like a flaky to me, isn't it ?

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Nov 3, 2020

Hi @Dynnammo , application_mailer.rb is an abstract mailer as it does not define any mailer action, but all mailers inherit from it.
You can create a spec and define a dummy mailer there, or create one in decidim-dev.

Regarding flaky tests, you're right we have again some flakies. You can try to solve or re-run failing workflows (we'll do if you don't have permissions).

@Dynnammo Dynnammo marked this pull request as ready for review November 5, 2020 08:40
@Dynnammo
Copy link
Copy Markdown
Contributor Author

Dynnammo commented Nov 5, 2020 •
edited
Loading

Hi @tramuntanal , thanks a lot ! The dummy mailer is in decidim-dev, as it seemed more logical to me. I've tested what I've corrected : values of mail.from and mail.reply_to depending on smtp_settings["from"] which is supposed to be set/unset depending on the different contexts.

tramuntanal
tramuntanal approved these changes Nov 10, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Ok @Dynnammo !

@tramuntanal tramuntanal merged commit 080e4a5 into decidim:develop Nov 10, 2020
edgarlatorre pushed a commit that referenced this pull request Nov 11, 2020
@Dynnammo @Quentinchampenois @edgarlatorre
Backport smtp settings correction (#6664)
f6ca24e 
* Backport smtp settings correction

* Remove byebug

* Correct Label in locales

* Correct Label in locales

* Remove unused key in en.yml

* Normalize locales

* Backport migration

* Lint migration

* Shift "from" to "from_email" in application_mailer.rb

* Reverse to previous help text in _smtp_settings.html.erb

* Swap values form mail.reply_to definition

* Correct hash definitions in update_organization_form.rb and related tests

* Update tests file to comply with from field

* light refactor register_organization_spec

* add tests for 'set_from' method in organization form

* WIP for application_mailer_spec.rb

* Finish application_mailer tests

* Move dummy mailer class in decidim-dev

Co-authored-by: quentinchampenois <26109239+Quentinchampenois@users.noreply.github.com>
@virgile-dev
Copy link
Copy Markdown
Contributor

virgile-dev commented Nov 13, 2020

Congrats @Dynnammo 👏

This was referenced Nov 17, 2020
Merged
Merged
@Dynnammo Dynnammo mentioned this pull request Nov 20, 2020
Closed
This was referenced Nov 20, 2020
Merged
Merged
This was referenced Nov 20, 2020
Closed
Merged
sauloperez added a commit to CoopCat-Confederacio-de-Cooperatives/decidim-coopcat that referenced this pull request Nov 20, 2020
@sauloperez
Upgrade to our fork's branch last commit
01c4c04 
There's a fix in decidim/decidim#6908 for the
regression introduced in decidim/decidim#6664,
but its revert done in decidim/decidim#6905 is
not merged either. We can't wait any longer so I'm doing the revert
myself.
@mrcasals mrcasals changed the title Backport smtp settings correction Backport SMTP settings correction Feb 26, 2021
@mrcasals mrcasals changed the title Backport SMTP settings correction Fix SMTP settings update Feb 26, 2021
@mrcasals mrcasals added module: system type: fix PRs that implement a fix for a bug labels Feb 26, 2021
@lahdeero lahdeero mentioned this pull request Mar 18, 2021
Merged
12 tasks
davidbeig pushed a commit to CoopCat-Confederacio-de-Cooperatives/decidim-coopcat that referenced this pull request Dec 2, 2025
@sauloperez
Upgrade to our fork's branch last commit
2195b4e 
There's a fix in decidim/decidim#6908 for the
regression introduced in decidim/decidim#6664,
but its revert done in decidim/decidim#6905 is
not merged either. We can't wait any longer so I'm doing the revert
myself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen left review comments

+1 more reviewer

@tramuntanal tramuntanal tramuntanal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tramuntanal tramuntanal

Labels

in-review module: system type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Dynnammo @tramuntanal @virgile-dev @ahukkanen @mrcasals @Quentinchampenois