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

[FIX] base: Allow default email_from by database #36837

Closed

Conversation

@Yajo
Copy link
Contributor

commented Sep 13, 2019

Affected versions: 10-12

It is possible to get to a situation where Odoo would try to send an email without a From: header address.

Open to see one example use case that would trigger that situation
  1. Install sale and account.
  2. Go to one invoice.
  3. Click send by email.
  4. Click in the "open" button for the template field.
  5. Click in the button to add the template to the model.
  6. Refresh browser.
  7. In the same invoice, hit Action > Send Mail (Invoicing: Invoice email).
  8. Send.

Since the template doesn't define a default "from" address, and it doesn't take the one from the user because it's in mass sending mode, and you don't have access to the server to add the --email-from CLI parameter, there are 2 possible outcomes:

  1. An AssertionError happens and goes unnoticed, if you had PYTHONOPTIMIZE="".
  2. An email is sent with an empty From: header, being rejected by almost any SMTP provider out there, if you had PYTHONOPTIMIZE="1".
In such case, you're unlucky if you don't have access to the underlying deployment, or if you use multiple databases in a single Odoo instance and each of them uses a different mail configuration.

To make this configuration easier to use and cover those use cases, here I add support for a new ICP: mail.default.from. It will be used when present, so it shouldn't affect existing deployments. When present, it will allow a admin to configure the default sending address just with Odoo itself.

This patch is half fix half feature, so I publish for v10 hoping it's considered a fix. I can re-target to a higher version if you consider it necessary.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
@Tecnativa TT19448

@robodoo robodoo added the seen 🙂 label Sep 13, 2019
Yajo added a commit to Tecnativa/doodba-scaffolding that referenced this pull request Sep 13, 2019
Just like odoo/odoo#36837 introduces this feature per database, here we're going to add support for it in Doodba directly, via config file, as it is currently supported in Odoo.

You should fill this new env variable with the address that should be used as a default sender address in case the mail template doesn't supply one, which is a not-so-uncommon case.

Usually it should match your `mail.catchall.alias@mail.catchall.domain` combination, or the same value as for `$SMTP_REAL_RELAY_USER`, but it could be any other.
@tde-banana-odoo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Would really love to have a test.

Yajo added a commit to Tecnativa/doodba-scaffolding that referenced this pull request Sep 13, 2019
Just like odoo/odoo#36837 introduces this feature per database, here we're going to add support for it in Doodba directly, via config file, as it is currently supported in Odoo.

You should fill this new env variable with the address that should be used as a default sender address in case the mail template doesn't supply one, which is a not-so-uncommon case.

Usually it should match your `mail.catchall.alias@mail.catchall.domain` combination, or the same value as for `$SMTP_REAL_RELAY_USER`, but it could be any other.
@robodoo robodoo added the CI 🤖 label Oct 2, 2019
@rafaelbn

This comment has been minimized.

Copy link

commented Oct 2, 2019

Hi @Yajo please add a test as @tde-banana-odoo said thanks!

This is labeled as Services, is this for @jem-odoo ? 🙏

Odoo Merge Sprint Salle Hocaille

@tde-banana-odoo tde-banana-odoo self-assigned this Oct 2, 2019
@Yajo Yajo force-pushed the Tecnativa:10.0-base-default_from_address_by_db branch from 0c1a0d1 to 2ee0a0f Oct 3, 2019
@Yajo Yajo changed the base branch from 10.0 to 11.0 Oct 3, 2019
@robodoo robodoo removed the CI 🤖 label Oct 3, 2019
@Yajo Yajo force-pushed the Tecnativa:10.0-base-default_from_address_by_db branch from 2ee0a0f to 8248ecb Oct 3, 2019
@Yajo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

Done, and rebased to 11.0

@rafaelbn

This comment has been minimized.

Copy link

commented Oct 4, 2019

Hello @tde-banana-odoo ! 🎉

Would really love to have a test.

It's done! 😄

@Feyensv

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

@Yajo Test doesn't work according to the runbot ;).

It is possible to get to a situation where Odoo would try to send an email without a `From:` header address.

In such case, you're unlucky if you don't have access to the underlying deployment, or if you use multiple databases in a single Odoo instance and each of them uses a different mail configuration.

To make this configuration easier to use and cover those use cases, here I add support for a new ICP: `mail.default.from`. It will be used when present, so it shouldn't affect existing deployments. When present, it will allow a admin to configure the default sending address just with Odoo itself.
@Yajo Yajo force-pushed the Tecnativa:10.0-base-default_from_address_by_db branch from 8248ecb to e41c38a Oct 7, 2019
@Yajo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

Yikes true, it should go ✔️ now.

@tde-banana-odoo

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

Hello,

Thanks for the update ! It looks good to me, thanks for adding tests :) .

@robodoo r+

@robodoo robodoo added the r+ 👌 label Oct 7, 2019
robodoo pushed a commit that referenced this pull request Oct 7, 2019
It is possible to get to a situation where Odoo would try to send an email without a `From:` header address.

In such case, you're unlucky if you don't have access to the underlying deployment, or if you use multiple databases in a single Odoo instance and each of them uses a different mail configuration.

To make this configuration easier to use and cover those use cases, here I add support for a new ICP: `mail.default.from`. It will be used when present, so it shouldn't affect existing deployments. When present, it will allow a admin to configure the default sending address just with Odoo itself.

closes #36837

Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

Merged at d6f4b2c, thanks!

@robodoo robodoo closed this Oct 7, 2019
@Yajo Yajo deleted the Tecnativa:10.0-base-default_from_address_by_db branch Oct 8, 2019
@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

17 similar comments
@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator

commented Oct 14, 2019

Whelp sorry for the spamming, I kinda screwed up updating the mergebot, should hopefully be resolved.

@fw-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2019

This pull request has forward-port PRs awaiting action (not merged or closed): #38096

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.