[IMP] sql_export_mail, add partner as export email recipient #979
Conversation
|
Hi @legalsylvain, |
joepsanders
force-pushed
the
oca-158-sqlexportmail-add-respartner-recipient
branch
from
dc2119d to
4301da4
Compare
joepsanders
force-pushed
the
oca-158-sqlexportmail-add-respartner-recipient
branch
from
b4fd008 to
678e37b
Compare
|
squashed PR feedback @legalsylvain, could you review this addition? |
There was a problem hiding this comment.
hi @joepsanders Thanks for this addition.
Some minor remarks inline. Otherwise LGTM.
sql_export_mail/__manifest__.py
Outdated
| @@ -1,7 +1,7 @@ | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html). | |||
| { | |||
| "name": "SQL Export Mail", | |||
| "version": "16.0.2.0.2", | |||
| "version": "16.0.2.1.0", | |||
There was a problem hiding this comment.
no need to bump the version manually, as there is no migration script.
There was a problem hiding this comment.
isn't this version update following the OCA guidelines? https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#12version-numbers
"y (Minor): Increments when new features are added that do not break backward compatibility. A module upgrade will likely be necessary."
There was a problem hiding this comment.
That's not my point.
The point is : Do not propose a change of version. When merging, maintainers will call "ocabot merge patch /minor / major". And then it will bump the version at this step.
Otherwise, if another PR bump also the version in the meantime, you'll face a conflict.
sql_export_mail/models/sql_export.py
Outdated
| @@ -138,13 +146,38 @@ def check_mail_user(self): | |||
| if not user.email: | |||
| raise UserError(_("The user does not have any e-mail address.")) | |||
|
|
|||
| @api.constrains("mail_partner_ids") | |||
There was a problem hiding this comment.
I guess the constrains should be raised, when query is changed. Or did I missed something ?
| else: | ||
| mail_users = self.mail_user_ids | ||
| return ",".join([x.email for x in mail_users if x.email]) |
There was a problem hiding this comment.
there was a test if user email was defined (or not). Why is the test removed ?
There was a problem hiding this comment.
The mapped function skips the missing emails by design: https://github.com/odoo/odoo/blob/3443fc707e9794f5fd329b1ca0cb1f95a47cda0b/odoo/models.py#L6401
There was a problem hiding this comment.
I don't think so.
Just tested in a shell :
(Console)
>>> env["res.partner"].create({"name": "bob"}).mapped("email")
[False]
>>> ",".join([False])
Traceback (most recent call last):
File "", line 1, in
TypeError: sequence item 0: expected str instance, bool found
There was a problem hiding this comment.
I stand corrected, thanks ;). See 2nd fixup commit
Hi @legalsylvain, thanks for your swift review! See my answers / questions to your remarks and a fixup commit (to be squashed). |
joepsanders
force-pushed
the
oca-158-sqlexportmail-add-respartner-recipient
branch
from
9ba142d to
deb77d3
Compare
joepsanders
force-pushed
the
oca-158-sqlexportmail-add-respartner-recipient
branch
from
90d70f7 to
3ba4d18
Compare
Adds partner_ids additional to user_ids to send the result of an sql_export by email.