Repository navigation
[19.0][IMP] partner_email_check: allow bypassing the checks per operation - #2416
cliffkujala wants to merge 2 commits into
Conversation
The checks run on every res.partner create and write. That is right for an address a user typed, and wrong for one the code did not collect from a user. Odoo's incoming mail gateway creates a partner for the sender of every message it accepts, and bulk senders commonly send from a subdomain that publishes no MX record. The deliverability check then raises, mail.thread.message_route treats any exception out of message_new as a misconfigured alias, and the result is a discarded message, an alias flagged invalid and a bounce aimed at an address that by definition cannot receive it. Nobody's data was improved by refusing it. Add the partner_email_check_skip_deliverability and partner_email_check_skip_syntax context keys so such a caller can bypass a check for a single operation. Skipping deliverability keeps validation and normalization, so only the DNS lookup goes. The company settings are untouched and nothing changes for callers that do not opt out.
| def test_syntax_still_checked_when_deliverability_skipped(self): | ||
| """Bypassing one check must not quietly bypass the other.""" | ||
| self.check_deliverability() | ||
| with self.assertRaises(ValidationError): |
There was a problem hiding this comment.
better to use assertRaisesRegex to ensure the exception is actually what you expect
There was a problem hiding this comment.
Good call, done in 6bd49a9. Both checks raise ValidationError, so without the message either test could pass on the other check's error.
| if self.env.context.get("partner_email_check_skip_deliverability"): | ||
| return False |
There was a problem hiding this comment.
Maybe it's a personal take, but IMO it's better to use positive flags
| if self.env.context.get("partner_email_check_skip_deliverability"): | |
| return False | |
| if not self.env.context.get("partner_email_check_deliverability", True): | |
| return False |
Meaning: by default it's True, but you can set it to False explicitly
There was a problem hiding this comment.
Thanks, I did weigh the positive form. I kept skip_ because a company setting is also in play here. With partner_email_check_deliverability, True wouldn't mean "check deliverability". It would mean "defer to the company setting", so a caller who writes with_context(partner_email_check_deliverability=True) to force the DNS check on a company that has it off would quietly get no check. A skip_ key only promises what it does: it can turn a check off for one operation, never on. It also matches how core names one-operation bypasses (skip_account_move_synchronization, skip_sms, skip_activity). The default-True flags like active_test have no setting competing with them.
If you'd still prefer a positive form, I think it should be a real override (unset means the company setting, True forces the check on, False turns it off), so that True means what it says. Happy to switch to that if you'd rather.
Syntax and deliverability failures both raise ValidationError, so assertRaises alone would let either test pass on the other check's error. Match the message instead.
What
Two context keys that turn a check off for a single operation:
partner_email_check_skip_deliverability— skips the DNS lookup, and keeps validation and normalization.partner_email_check_skip_syntax— skips both, exactly as disabling the company setting does.The company settings are untouched and nothing changes for callers that do not opt out.
Why
The checks run on every
res.partnercreate and write. That is right for an address a user typed, and wrong for one the code did not collect from a user.Odoo's incoming mail gateway creates a partner for the sender of every message it accepts, and bulk senders — HubSpot, Mailchimp, SendGrid — routinely send from a subdomain that publishes no MX record. With Check email deliverability enabled, storing that sender raises
ValidationError.mail.thread.message_routereads any exception out ofmessage_newas a misconfigured alias, so one such message produces:We hit that in production: a HubSpot campaign sent from
39698511m.em.knapheide.comtook a working inbound alias out of service until someone noticed and cleared the flag. Nobody's data was improved by refusing the address.The only workaround available today is disabling the company setting, which gives up the check for exactly the addresses it was written for — the ones staff type in. A caller that knows it is storing a sender rather than user input can say so instead.
Usage
Tests
Four added, covering that the deliverability bypass still normalizes (mixed case in, lowercase out), that it does not quietly take the syntax check with it, that it does not persist onto the record's own environment, and that the syntax key works while the company setting is on.
Documented in
readme/USAGE.md.