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
13.0 mig mail server relay disallowed #1762
13.0 mig mail server relay disallowed #1762
Conversation
'license': 'AGPL-3', | ||
'category': 'Social Network', | ||
'depends': ['base'], | ||
'data': [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this empty key ?
_test_logger = logging.getLogger('odoo.tests') | ||
|
||
|
||
class IrMailServer(osv.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
models.Model ?
) | ||
_logger.error(msg) | ||
raise MailDeliveryException(_("Mail Delivery Failed"), msg) | ||
return message_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here it should adapt changes done in original v13 code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
c858a71
to
02180cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review 👍
/ocabot merge |
On my way to merge this fine PR! |
@JordiBForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 13.0-ocabot-merge-pr-1762-by-JordiBForgeFlow-bump-no. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
) | ||
assert ( | ||
smtp_from | ||
), "The Return-Path or From header is required for any outbound email" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your module.
It seems than assert expression shouldn't not be in production code like here
https://stackoverflow.com/a/5143044 because could be skipped according to the execution mode of your app instance. Then assert should be reserve for unit tests where there is no production mode in execution.
But yes Odoo SA itself use this statement in its code not only for tests. As usual, OCA promotes the best practices before Odoo SA
https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement
There is a lot of documentation on this subject in python ecosystem
Not blocking for me, just for information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but here this part of code is just copied from odoo (it's an override) and we want to change the minor possible the code, in order to be able to check easy what change has been done comparing to original code.
02180cd
to
92d3db6
Compare
/ocabot merge |
Hi @JordiBForgeFlow. Your command failed:
Ocabot commands
More information
|
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at e025860. Thanks a lot for contributing to OCA. ❤️ |
No description provided.