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

[IMP] override_mail_recipients. Allow whitelisted domain. #104

Merged
merged 1 commit into from Jun 6, 2019

Conversation

NL66278
Copy link
Contributor

@NL66278 NL66278 commented Mar 7, 2019

This was done for a customer that still wanted to test emails send to their own addresses, but still making certain no other of their partners receive emails.

Apart from the new domain whitelist functionality, I moved the send_email to the top of the code, according to the principle that main functions should be above helper functions.

(Editted after following up on the suggestions of @hbrunn ).

@hbrunn
Copy link
Contributor

hbrunn commented Mar 7, 2019

if I understand correctly, filling in @therp.nl will fail all mails going to non-therp.nl addresses, but will allow a mail to someone@therp.nl unchanged? So it's basically a filter for outgoing mails? That would overstretch the purpose of this module in my opinion and merit its own module.

But with a slight change I think this will be a great addition here: Can't we introduce another config parameter for the whitelisting which is a comma separated list of domains, and those won't be touched by the override? This way, they can test internal mails just fine, and external mails go to odoomailtest@customer or whatever is filled into the override parameter? Seems even like added value to me.

@hbrunn hbrunn added this to the 11.0 milestone Mar 7, 2019
@hbrunn hbrunn added the question label Mar 7, 2019
@hbrunn
Copy link
Contributor

hbrunn commented Mar 7, 2019

PS: No idea what's going on with travis

@NL66278
Copy link
Contributor Author

NL66278 commented Mar 7, 2019

You understood correctly. I will go the extra mile and do it this way.

@hbrunn
Copy link
Contributor

hbrunn commented Mar 7, 2019

for what it's worth, I updated our fork of MTQ and now we're happily green

@NL66278 NL66278 force-pushed the 11.0-override_email_recipients-selective branch from 2449689 to 6d7986c Compare March 7, 2019 16:32
@codecov-io
Copy link

codecov-io commented Mar 7, 2019

Codecov Report

Merging #104 into 11.0 will increase coverage by 10.87%.
The diff coverage is 95.65%.

Impacted file tree graph

@@             Coverage Diff             @@
##             11.0     #104       +/-   ##
===========================================
+ Coverage   80.55%   91.42%   +10.87%     
===========================================
  Files           2        2               
  Lines          36       70       +34     
===========================================
+ Hits           29       64       +35     
+ Misses          7        6        -1
Impacted Files Coverage Δ
override_mail_recipients/models/ir_mail_server.py 96.42% <95.65%> (+10.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a662450...8b20aad. Read the comment docs.

@NL66278 NL66278 force-pushed the 11.0-override_email_recipients-selective branch 2 times, most recently from 216fbae to 3b4d606 Compare March 8, 2019 09:02
@NL66278 NL66278 force-pushed the 11.0-override_email_recipients-selective branch from 3b4d606 to 8b20aad Compare March 8, 2019 09:30
extract_rfc2822_addresses(override) \
if override != 'disable' else []
assert override_recipients or whitelisted_domains, \
'No valid override_email_to'
Copy link
Contributor

Choose a reason for hiding this comment

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

please add bf6b463 or something similar to make this not fail on new installations

@hbrunn hbrunn merged commit b04f0c5 into 11.0 Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants