[14.0] mass_mailing_partner : issues with company_name and incompatibility with mass_mailing_unique

[ivantodorovich]

As mentioned in this comment: #773 (comment)

With mass_mailing_unique, we have email = fields.Char(copy=False), to avoid the IntegrityError if the user tries to copy it.
That makes mass_mailing_partner NOT to copy the email on those 2 lines, which is later backfiring in other module's tests. I haven't dug too much on why mass_mailing_partner is copying data there, but it doesn't seem right. Seems it's not accounting for fields with copy=False.

Apart from that, there are other issues in mass_mailing_partner tests, like trying to create mailing lists with duplicate names:

• social/mass_mailing_partner/tests/base.py

  Lines 31 to 38 in bebbfbd

  	self.mailing_list = self.create_mailing_list({"name": "List test"})
  	self.mailing_list2 = self.create_mailing_list(
  	{
  	"name": "List test 2",
  	"partner_mandatory": True,
  	"partner_category": self.category_3.id,
  	}
  	)

• social/mass_mailing_partner/tests/test_mail_mass_mailing_contact.py

  Line 116 in bebbfbd

  list_2 = self.create_mailing_list({"name": "List test 2"})

Traceback (most recent call last):
  File "/odoo/external-src/social/mass_mailing_partner/tests/test_mail_mass_mailing_contact.py", line 116, in test_partners_merge
    list_2 = self.create_mailing_list({"name": "List test 2"})
  File "/odoo/external-src/social/mass_mailing_partner/tests/base.py", line 50, in create_mailing_list
    return m_mailing_list.create(vals)
  File "<decorator-gen-69>", line 2, in create
  File "/odoo/src/odoo/api.py", line 344, in _model_create_multi
    return create(self, [arg])
  File "/odoo/src/odoo/addons/base/models/ir_fields.py", line 534, in create
    recs = super().create(vals_list)
  File "<decorator-gen-13>", line 2, in create
  File "/odoo/src/odoo/api.py", line 345, in _model_create_multi
    return create(self, arg)
  File "/odoo/src/odoo/models.py", line 3864, in create
    records = self._create(data_list)
  File "/odoo/src/odoo/models.py", line 3970, in _create
    cr.execute(query, params)
  File "<decorator-gen-3>", line 2, in execute
  File "/odoo/src/odoo/sql_db.py", line 101, in check
    return f(self, *args, **kwargs)
  File "/odoo/src/odoo/sql_db.py", line 298, in execute
    res = self._obj.execute(query, params)
psycopg2.IntegrityError: duplicate key value violates unique constraint "mailing_list_unique_name"
DETAIL:  Key (name)=(List test 2) already exists.

btw there's more weird stuff that I don't get, like this:

social/mass_mailing_partner/models/mailing_contact.py

Lines 76 to 86 in bebbfbd

	def _get_company(self):
	company_id = False
	if self.company_name:
	company_id = (
	self.env["res.company"].search([("name", "=", self.company_name)]).id
	)
	if not company_id:
	company_id = (
	self.env["res.company"].create({"name": self.company_name}).id
	)
	return company_id

Creating a company from the mailing.contact's company_name ? I think that's misconceived btw. company_name is supposed to be the contact's employeer/company name AFAICS (same concept than company_name in res.partner odoo/odoo@bf88456)

It's proposed to test this module separately in #773.

This issue is created to track these issues and eventually fix them and revert the tests isolation

[ivantodorovich]

Also maybe consider using mass_mailing_contact_partner instead #767.

It's a completely different module, as the relation between mailing.contact and res.partner is in the opposite direction, but IMO makes more sense given the data model (multiple partners can share an email address). I don't know the functional requirement for mass_mailing_partner but maybe mass_mailing_contact_partner achieves the same, in a way?

[chienandalu]

With mass_mailing_unique, we have email = fields.Char(copy=False), to avoid the IntegrityError if the user tries to copy it.
That makes mass_mailing_partner NOT to copy the email on those 2 lines, which is later backfiring in other module's tests

Would it be possible to have a context check in your side that we could use to keep the compatibility avoding the expection for that case?

Apart from that, there are other issues in mass_mailing_partner tests, like trying to create mailing lists with duplicate name

Ok, we can fix that easily

Creating a company from the mailing.contact's company_name

Wow, it's terrible, indeed and it should go away.

I don't know the functional requirement for mass_mailing_partner but maybe mass_mailing_contact_partner achieves the same, in a way?

There's one important thing we'd miss that is creating the partner on the fly when the contact doesn't have one to match with.

[chienandalu]

ping @ernestotejeda

[chienandalu]

@ivantodorovich I just made #777 It's for 12.0 but once meged we'll forward port it up to 14.0.

[ivantodorovich]

Would it be possible to have a context check in your side that we could use to keep the compatibility avoding the expection for that case?

Hmmm.. like how?

mass_mailing_unique implementation is super simple and straightforward. It'd be a shame to making it more complex just because of mass_mailing_partner:

https://github.com/camptocamp/social/blob/14.0-mig-mass_mailing_unique/mass_mailing_unique/models/mailing_contact.py

I would re-check the need for copy_data during write, because it seems if some fields have copy=False unexpected things happen. With mass_mailing_unique it's the email field.. but what about other modules in the future also adding fields with copy=False?

There's one important thing we'd miss that is creating the partner on the fly when the contact doesn't have one to match with.

Well that wouldn't be so difficult to add, and it's an optional feature. If the data model makes more sense I'd integrate that in mass_mailing_contact_partner and deprecate mass_mailing_partner (or simply refactor/merge both into mass_mailing_partner for historic reasons)

More improtantly I think the conception is wrong, at least in 14.0. You can have mupltiple partners sharing the same email address. It's quite common actually. This is why mass_mailing_contact_partner's relation is in the opposite direction.
In a way similar to the mail.blacklist and res.partner relation in core, through the mail.thread.blacklist mixin: https://github.com/odoo/odoo/blob/4f6c3ee35e9c54a251db45d81289d7d46fea204d/addons/mail/models/mail_thread_blacklist.py#L39

---

There are other redflags that I get from mass_mailing_partner simply by quickly looking at the source. I think a full code review + ref is needed because this module is old coming from several migrations.

For instance, check this out:

social/mass_mailing_partner/models/mailing_contact.py

Line 109 in bebbfbd

partner = m_partner.search([("email", "=ilike", email)], limit=1)

I'd use email_normalized instead of =ilike. You might be missing matches with emails like John Doe <[email protected]> and Doe, John <[email protected]>, thus creating duplicate partners.

[pedrobaeza]

The weird thing is that an SQL constraint shouldn't act on a new record that is only on the ORM memory.

[ivantodorovich]

The weird thing is that an SQL constraint shouldn't act on a new record that is only on the ORM memory.

But it doesn't.
The issue is the new record doesn't copy the email AFAICS

[pedrobaeza]

Ah, then the code should be pass such email in the values of the copy. Probably on the initial created version (v8), the field was copy=True, and there wasn't such problem, but tests are not very well done for detecting these things on migration.