[FIX+IMP] mass_mailing_list_dynamic: tests, icons, filters... #211

pedrobaeza · 2017-11-10T19:20:14Z

Brand new icon
Added feature of loading an existing filter as criteria
Tests as SavepointCase for optimizing times
Tests in post-install for avoiding errors on res.partner not null constraints
when several modules added them.
Updated documentation.
Fix mock in test for not commiting test data.

yajo

I added a fix to another bug we were hitting: when the sync process had to create contacts in a fully synchronized list, there was an exception:

Besides that, I see a bit confusing the new button, UX POV. Now there are 2 ways of defining a domain instead of just one, so I think the UI should be a little bit more self-explanatory on when to use one or the other. Also, the new button should be aligned to the criteria field, the same way the sync now button is aligned to the sync method.

To finish, there are a couple of minor things that this PR is un-improving, commented below. Please keep those as they were 😉

Thanks! Good job 😊

yajo · 2017-11-13T08:33:03Z

mass_mailing_list_dynamic/tests/test_dynamic_lists.py

            self.mail.send_mail()
-            self.assertEqual(6, send_email.call_count)


Please keep this check, it was added on purpose because there was a bug in my initial implementation that sent many mails per recipient.

It doesn't make sense, as I have changed the mocked call (because yours was incorrect as was causing a commit to be performed). Any way, I let the assert with the count=1, that is the one needed.

yajo · 2017-11-13T08:40:36Z

mass_mailing_list_dynamic/models/mail_mass_mailing_contact.py

@@ -11,7 +11,7 @@ class MassMailingContact(models.Model):

    @api.constrains("partner_id", "list_id", "name", "email")
    def _check_no_manual_edits_on_fully_synced_lists(self):
-        if self.env.context.get("syncing"):
+        if self.env.context.get("syncing"):  # pragma: no cover


Don't do this, it's already being tested: https://github.com/OCA/social/pull/211/files#diff-3c3cb2de40288fdbce8b7e58014c6028L60

pedrobaeza · 2017-12-05T08:17:35Z

@cubells what are you doing with this PR?

* Brand new icon * Added feature of loading an existing filter as criteria * Tests as SavepointCase for optimizing times * Tests in post-install for avoiding errors on res.partner not null constraints when several modules added them. * Updated documentation. * Fix mock in test for not commiting test data.

…lly synced lists Syncing context was being set in the wrong object. Added to test too.

pedrobaeza · 2017-12-21T18:39:14Z

@yajo ready to merge

@cubells @rafaelbn please review

…rtner Module mass_mailing_partner writes back certain values from partner to mass_mailing_contact. Module should allow that write operation.

cubells

Minor changes ready to approve.

cubells · 2017-12-28T19:40:24Z

mass_mailing_list_dynamic/i18n/es.po

-" in fully synchronized lists."
-msgstr ""
+msgid "<i class=\"fa fa-info-circle\"/> You cannot make manual editions of contacts in fully synchronized lists."
+msgstr "<i class=\"fa fa-info-circle\"/> No puede editar manualmente los contactos en las listas que están completamente sincronizadas."

 #. module: mass_mailing_list_dynamic
 #: selection:mail.mass_mailing.list,sync_method:0
 msgid "Add and remove records as needed"
 msgstr ""


Are not translated, It isn't?

Spanish file is incomplete.

cubells · 2017-12-28T19:42:38Z

mass_mailing_list_dynamic/wizards/mail_mass_mailing_load_filter_views.xml

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- Copyright 2017 Tecnativa - Pedro M. Baea


Your last name is incorrect.

cubells

README file style is broken because a blank line is needed before second indentation. But is an error from previous versions.

yajo · 2018-01-05T09:53:14Z

mass_mailing_list_dynamic/tests/test_dynamic_lists.py

@@ -73,7 +80,7 @@ def test_list_sync(self):
            contact1.partner_id = self.partners[0]

    def test_sync_when_sending_mail(self):
-        """Dynamic list is synced before mailing to it."""
+        """Check that list in synced when sending a mass mailing."""


rafaelbn

👍 using it in production

rafaelbn · 2018-01-25T00:16:14Z

@cubells please review again and unblock 😄

rafaelbn · 2018-01-25T00:29:57Z

Hi @pedrobaeza ,

Have you deleted the option of filtering? The only way now is onle loading filter?

Thanks

rafaelbn

❔

pedrobaeza · 2018-01-25T02:01:01Z

Because web_advanced_search_x2x is not installed here.

rafaelbn · 2018-02-01T23:19:10Z

@cubells please review, thanks

cubells

👍

LGTM

* [FIX+IMP] mass_mailing_list_dynamic: tests, icons, filters... * Brand new icon * Added feature of loading an existing filter as criteria * Tests as SavepointCase for optimizing times * Tests in post-install for avoiding errors on res.partner not null constraints when several modules added them. * Updated documentation. * Fix mock in test for not commiting test data. * [FIX] mass_mailing_list_dynamic: Wasn't able to create contacts in fully synced lists Syncing context was being set in the wrong object. Added to test too. * [FIX] mass_mailing_list_dynamic: Allow to write back vals from res.partner Module mass_mailing_partner writes back certain values from partner to mass_mailing_contact. Module should allow that write operation.

pedrobaeza added the needs review label Nov 10, 2017

pedrobaeza added this to the 10.0 milestone Nov 10, 2017

pedrobaeza force-pushed the 10.0-mass_mailing_list_dynamic-filter_imp branch from 1eed447 to b655190 Compare November 10, 2017 19:26

yajo requested changes Nov 13, 2017

View reviewed changes

pedrobaeza and others added 2 commits December 21, 2017 19:05

[FIX] mass_mailing_list_dynamic: Wasn't able to create contacts in fu…

36fa9c2

…lly synced lists Syncing context was being set in the wrong object. Added to test too.

pedrobaeza force-pushed the 10.0-mass_mailing_list_dynamic-filter_imp branch from e55be60 to 3f63739 Compare December 21, 2017 18:38

pedrobaeza force-pushed the 10.0-mass_mailing_list_dynamic-filter_imp branch from 3f63739 to 7da6223 Compare December 21, 2017 18:59

[FIX] mass_mailing_list_dynamic: Allow to write back vals from res.pa…

1689386

…rtner Module mass_mailing_partner writes back certain values from partner to mass_mailing_contact. Module should allow that write operation.

pedrobaeza force-pushed the 10.0-mass_mailing_list_dynamic-filter_imp branch from 7da6223 to 1689386 Compare December 21, 2017 20:38

cubells suggested changes Dec 28, 2017

View reviewed changes

yajo approved these changes Jan 5, 2018

View reviewed changes

Merge branch '10.0' into 10.0-mass_mailing_list_dynamic-filter_imp

e1f6c84

yajo force-pushed the 10.0-mass_mailing_list_dynamic-filter_imp branch from a561194 to e1f6c84 Compare January 23, 2018 10:45

rafaelbn approved these changes Jan 25, 2018

View reviewed changes

rafaelbn reviewed Jan 25, 2018

View reviewed changes

cubells approved these changes Feb 2, 2018

View reviewed changes

rafaelbn merged commit 956a606 into OCA:10.0 Feb 2, 2018

rafaelbn deleted the 10.0-mass_mailing_list_dynamic-filter_imp branch February 2, 2018 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX+IMP] mass_mailing_list_dynamic: tests, icons, filters... #211

[FIX+IMP] mass_mailing_list_dynamic: tests, icons, filters... #211

pedrobaeza commented Nov 10, 2017

yajo left a comment

yajo Nov 13, 2017

pedrobaeza Dec 21, 2017

yajo Nov 13, 2017

pedrobaeza commented Dec 5, 2017

pedrobaeza commented Dec 21, 2017

cubells left a comment

cubells Dec 28, 2017

cubells Dec 28, 2017

cubells left a comment

yajo Jan 5, 2018

rafaelbn left a comment

rafaelbn commented Jan 25, 2018

rafaelbn commented Jan 25, 2018

rafaelbn left a comment

pedrobaeza commented Jan 25, 2018

rafaelbn commented Feb 1, 2018

cubells left a comment

		self.mail.send_mail()
		self.assertEqual(6, send_email.call_count)

		@@ -0,0 +1,34 @@
		<?xml version="1.0" encoding="utf-8"?>
		<!-- Copyright 2017 Tecnativa - Pedro M. Baea

[FIX+IMP] mass_mailing_list_dynamic: tests, icons, filters... #211

[FIX+IMP] mass_mailing_list_dynamic: tests, icons, filters... #211

Conversation

pedrobaeza commented Nov 10, 2017

yajo left a comment

Choose a reason for hiding this comment

yajo Nov 13, 2017

Choose a reason for hiding this comment

pedrobaeza Dec 21, 2017

Choose a reason for hiding this comment

yajo Nov 13, 2017

Choose a reason for hiding this comment

pedrobaeza commented Dec 5, 2017

pedrobaeza commented Dec 21, 2017

cubells left a comment

Choose a reason for hiding this comment

cubells Dec 28, 2017

Choose a reason for hiding this comment

cubells Dec 28, 2017

Choose a reason for hiding this comment

cubells left a comment

Choose a reason for hiding this comment

yajo Jan 5, 2018

Choose a reason for hiding this comment

rafaelbn left a comment

Choose a reason for hiding this comment

rafaelbn commented Jan 25, 2018

rafaelbn commented Jan 25, 2018

rafaelbn left a comment

Choose a reason for hiding this comment

pedrobaeza commented Jan 25, 2018

rafaelbn commented Feb 1, 2018

cubells left a comment

Choose a reason for hiding this comment