Skip to content

[ADD] mail - #790

Merged
pedrobaeza merged 3 commits into
OCA:10.0from
hbrunn:10.0-mail
Apr 15, 2017
Merged

[ADD] mail#790
pedrobaeza merged 3 commits into
OCA:10.0from
hbrunn:10.0-mail

Conversation

@hbrunn

@hbrunn hbrunn commented Apr 5, 2017

Copy link
Copy Markdown
Member

No description provided.

@hbrunn hbrunn added this to the 10.0 milestone Apr 5, 2017
@hbrunn
hbrunn force-pushed the 10.0-mail branch 2 times, most recently from 7e659b5 to c4d430b Compare April 5, 2017 18:57
@hbrunn hbrunn changed the title 10.0 mail [ADD] mail Apr 5, 2017
@hbrunn
hbrunn requested a review from JordiBForgeFlow April 5, 2017 20:33
"""update mail_message_res_partner_needaction_rel rel
set is_email=True, email_status=case
when m.state in ('sent', 'exception') then m.state
when m.state='outgoing' then 'ready'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like a redundant check, given the next line?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree, but don't have my ssh keys at hand

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please attend this comment and we are good to go with this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@StefanRijnhart

Copy link
Copy Markdown
Member

Loading noupdate data here: hbrunn#2

@StefanRijnhart

StefanRijnhart commented Apr 13, 2017

Copy link
Copy Markdown
Member

What, no env? hbrunn#3
But really, you might want to refactor to use env everywhere (or specify use_env=False) because we planned for the decorator to return an env by default in version 10.
-- edit: included in the PR --

@hbrunn

hbrunn commented Apr 13, 2017

Copy link
Copy Markdown
Member Author

I thought we only wanted to default to an env in 11? I'd like to stick to the cr version as soon as it's the default and I only use cursors anyways
If we want to change the default in openupgradelib, let's do that now

@StefanRijnhart

Copy link
Copy Markdown
Member

Ah, that explains. Docstring says 'Starting from version 10, this is the default', but the code says > 10.

@StefanRijnhart

Copy link
Copy Markdown
Member

@StefanRijnhart

Copy link
Copy Markdown
Member

While you are here, could you confirm that this line is redundant? #790 (diff)

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Curious mechanism that Odoo has used with _table = 'mail_message_res_partner_needaction_rel' for keeping the same many2many table, but it makes things simpler.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't change in this PR other modules doc

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this was my judgement of the dependencies, but your PR says otherwise

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, it can be barely consider as nothing to do, as this is only have side-effects sometimes, but I prefer to stay safe with the previous known issues. Anyway, my comment was for isolating this kind of changes from PR with the scripts of one module. We can make a PR for these changes, or directly a commit including this documentation (as it doesn't affect Travis status and thus doesn't require even optimistic merging).

@hbrunn
hbrunn force-pushed the 10.0-mail branch 2 times, most recently from 99e90b6 to 2440e07 Compare April 15, 2017 18:05
@pedrobaeza
pedrobaeza merged commit 3ac4300 into OCA:10.0 Apr 15, 2017
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.

3 participants