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

[MIG] mail_tracking: Migrated to 10.0 #108

Merged
merged 4 commits into from
Nov 29, 2016
Merged

[MIG] mail_tracking: Migrated to 10.0 #108

merged 4 commits into from
Nov 29, 2016

Conversation

bouvyd
Copy link
Contributor

@bouvyd bouvyd commented Oct 14, 2016

No description provided.

@api.depends('name', 'recipient')
def _compute_display_name(self):
def _comp_display_name(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this function since it triggered a warning that the field depended on itself.
The _compute_display_name function in models.py adds display_name as a dependency, so to avoid this warning I had to rename the function...

@bouvyd
Copy link
Contributor Author

bouvyd commented Oct 14, 2016

I have only done a basic form of testing (besides the python tests), which consists of sending e-mails to a mailcatcher and checking that the state "opened" and "read" are triggered. No idea how to actually test the spam states, etc.

BTW, have you ever thought about adding a mailcatcher on your runbot? It makes it possible to see outgoing emails and it's quite useful at times. I use this quite a lot on our runbots, and it would make this module a teensy bit more easily testable. Anyway, just a passing remark :-)

This module is quite nice & interesting btw!

@api.depends('name', 'recipient')
def _compute_display_name(self):
def _compute_tracking_display_name(self):
Copy link
Contributor Author

@bouvyd bouvyd Oct 14, 2016

Choose a reason for hiding this comment

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

I renamed this function since it triggered a warning that the field depended on itself.
The _compute_display_name function in models.py adds _rec_name as a dependency, so to avoid this warning I had to rename the function...

My initial renaming didn't start with _compute; travis-bot didn't like that! Quite useful, we should consider adding it one day ourselves; would avoid new code with api.one's in it.

@danimaribeiro
Copy link

👍
Did a local test, working
working tracking email

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

👍 code, no test. Thanks @dbo-odoo !

@lasley lasley added this to the 10.0 milestone Oct 14, 2016
@lasley
Copy link
Contributor

lasley commented Oct 14, 2016

@dbo-odoo - I like your idea of a mail catcher. Are you just using https://mailcatcher.me/ or similar?

@pedrobaeza pedrobaeza mentioned this pull request Oct 14, 2016
21 tasks
@pedrobaeza
Copy link
Member

Can you please cherry-pick recent commits 811bea1 and cfdc7d9?

@pedrobaeza
Copy link
Member

And cherry-pick also 60ea386?

@bouvyd
Copy link
Contributor Author

bouvyd commented Nov 25, 2016

@pedrobaeza I've cherry-picked blindly for the moment; will have to check tests and runbot again...

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Merging this now. Thanks for the work!

@pedrobaeza pedrobaeza merged commit d42d5a9 into OCA:10.0 Nov 29, 2016
@bouvyd
Copy link
Contributor Author

bouvyd commented Nov 29, 2016

@pedrobaeza you're quite welcome :-) I've got some other pet projects at the moment but I keep the OCA repo in mind if I ever get some free time to dedicate to it!

@pedrobaeza
Copy link
Member

If you want me to take a look, just point me to the repo 😄

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

5 participants