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] mass_mailing_partner: Migration to 10.0 #171

Merged
merged 8 commits into from
Aug 30, 2017

Conversation

chienandalu
Copy link
Member

@yajo yajo added this to the 10.0 milestone Jun 15, 2017
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Please clean up commit history

@chienandalu chienandalu force-pushed the 10.0-mig-mass_mailing_partner branch 2 times, most recently from b691054 to 0c16dac Compare June 15, 2017 10:43
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.

Add your copyright in the files you touch and check Travis status. It should be green.

At first install, all existing mass mailing contacts are matched against
partners. And also mass mailing statistics are matched using model and res_id.

NOTE: When upgrading from version 1.0.0, no mass mailing statistics matching
Copy link
Member

Choose a reason for hiding this comment

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

This note can be removed and it doesn't apply anymore.

Bugs are tracked on `GitHub Issues <https://github.com/OCA/crm/issues>`_.
In case of trouble, please check there if your issue has already been reported.
If you spotted it first, help us smashing it by providing a detailed and welcomed feedback
`here <https://github.com/OCA/crm/issues/new?body=module:%20mass_mailing_partner%0Aversion:%208.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.
Copy link
Member

Choose a reason for hiding this comment

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

Remove this part according new README template

`here <https://github.com/OCA/crm/issues/new?body=module:%20mass_mailing_partner%0Aversion:%208.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.


License
Copy link
Member

Choose a reason for hiding this comment

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

Remove this unneeded section

"name": "Link partners with mass-mailing",
"version": "10.0.1.0.0",
"author": "Tecnativa, "
"Antiun Ingeniería S.L., "
Copy link
Member

Choose a reason for hiding this comment

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

Remove Antiun and Serv. Tecnol. ...

_('Partner already exists in this mailing list.'))
]

@api.one
Copy link
Member

Choose a reason for hiding this comment

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

Don't put @api.one here.

vals = self._set_name_email(vals)
return super(MailMassMailingContact, self).create(vals)

@api.one
Copy link
Member

Choose a reason for hiding this comment

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

Rewrite the method to be @api.multi or this will hit performance.

_("This partner '%s' is subscribed to one or more "
"mailing lists. Email must be assigned." % self.name))

@api.one
Copy link
Member

Choose a reason for hiding this comment

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

Rewrite it for @api.multi

def _compute_mass_mailing_contacts_count(self):
self.mass_mailing_contacts_count = len(self.mass_mailing_contact_ids)

@api.one
Copy link
Member

Choose a reason for hiding this comment

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

Make it @api.multi

@api.multi
@api.depends('mass_mailing_stats')
def _compute_mass_mailing_stats_count(self):
self.mass_mailing_stats_count = len(self.mass_mailing_stats)
Copy link
Member

Choose a reason for hiding this comment

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

This needs a for loop

@api.depends('mass_mailing_contact_ids',
'mass_mailing_contact_ids.opt_out')
def _compute_mass_mailing_contacts_count(self):
self.mass_mailing_contacts_count = len(self.mass_mailing_contact_ids)
Copy link
Member

Choose a reason for hiding this comment

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

This needs a forloop

@chienandalu chienandalu force-pushed the 10.0-mig-mass_mailing_partner branch from fa6fa72 to 17da5df Compare June 21, 2017 14:41
@chienandalu
Copy link
Member Author

@pedrobaeza @yajo Changes done. Travis is failing on mail_tracking and mail_tracking_mailgun

@@ -0,0 +1,140 @@
# Translation of Odoo Server.
Copy link
Member

Choose a reason for hiding this comment

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

This file should not exist

if self.mass_mailing_contact_ids and not self.email:
raise ValidationError(
_("This partner '%s' is subscribed to one or more "
"mailing lists. Email must be assigned." % self.name))
Copy link
Member

Choose a reason for hiding this comment

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

The string modulo must be after the translation, not before.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yajo I'm not sure about what you mean here 😕

<footer>
<button string="Add contacts to mailing list" name="add_to_mail_list"
type="object" class="oe_highlight"/>
or
Copy link
Member

Choose a reason for hiding this comment

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

Remove the or

@chienandalu
Copy link
Member Author

@yajo @pedrobaeza Changes done

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Code review

@pedrobaeza pedrobaeza mentioned this pull request Jul 17, 2017
21 tasks
@pedrobaeza pedrobaeza force-pushed the 10.0-mig-mass_mailing_partner branch from e1bdf9b to 66a1ff2 Compare August 3, 2017 11:22
@pedrobaeza
Copy link
Member

Please check errors

@chienandalu chienandalu force-pushed the 10.0-mig-mass_mailing_partner branch from 66a1ff2 to 5c095f6 Compare August 3, 2017 13:16
@chienandalu
Copy link
Member Author

@pedrobaeza Errors seem to be unrelated. Rebasing to rebuild tests and see.

@chienandalu
Copy link
Member Author

@pedrobaeza Well, this is runbot's log now 😕

Unable to find image 'oca-social:5c095f6_3' locally
docker: Error response from daemon: repository oca-social not found: does not exist or no pull access.
See 'docker run --help'.

@pedrobaeza
Copy link
Member

Uhm, right, please try to fix main branch in other PR.

Javier Iniesta and others added 7 commits August 30, 2017 12:54
[IMP] mass_mailing_partner: Link mail statistics to partner
[FIX][mass_mailing_partner] Exclude opt_out.

Now opted-out records will not be counted in the "Mailing lists" smart button
in the partner form.
By indicating the exact `partner_id` and ensuring no contacts associated to it are found, you avoid possible duplication errors when several partners share the same name or email.
@chienandalu chienandalu force-pushed the 10.0-mig-mass_mailing_partner branch 2 times, most recently from f364baf to 6a88a09 Compare August 30, 2017 13:55
@chienandalu
Copy link
Member Author

@pedrobaeza It's all green now

@pedrobaeza pedrobaeza force-pushed the 10.0-mig-mass_mailing_partner branch from fe638e9 to 2837986 Compare August 30, 2017 17:02
@pedrobaeza pedrobaeza merged commit f80ac7a into OCA:10.0 Aug 30, 2017
@pedrobaeza pedrobaeza deleted the 10.0-mig-mass_mailing_partner branch August 30, 2017 17:02
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