[FIX] account_journal: avoid duplicate mail alias #159037

UemuS · 2024-03-25T07:08:15Z

Steps to reproduce:

Create an account journal.
Rename the created journal (let's say from A to B).
Create a new account journal named A.

Changing the name of a journal doesn't change the name of the alias, An error caused by duplicate mail alias names will occur. Additionally the error can happen if the user has manually added aliases with the same name before upgrading to 17.0.

Adding the code and company id of the journal to the alias to ensure uniqueness when an alias with the same name is already in the DB (as the combo company_id,code is unique).

Other possible fixes: (I'm not sure what's better here)

Automatically rename an alias when its journal is renamed (only when they are identical after being sanitized)
Add numbers as suffix to alias names

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

robodoo · 2024-03-25T07:09:17Z

Pull request status dashboard.

tde-banana-odoo

Hello, added some preliminary remarks / questions :)

Would say that functional side should be checked with accounting team to be sure this is the behavior they want for journal aliases, which is not standard compared to other models. There is quite a lot of automatic computation that is not present in other modules and we should be sure it works as intended.

Please also note that you should provide some unit tests in order to highlight the issue, otherwise it is a bit hard to see the final result :) .

tde-banana-odoo · 2024-03-25T13:48:44Z

addons/account/models/account_journal.py

+        sanitized_alias_name = self.env['mail.alias']._sanitize_alias_name(base_alias_name)
+        existing_alias = self.env['mail.alias'].search([('alias_name', '=', sanitized_alias_name)])
+        if len(existing_alias) > 0 and existing_alias.id != self.alias_id.id:
+            alias_name = f'{sanitized_alias_name}-{code}-{company.id}'


But code is already taken into account in base_alias_name ?

In the failing upgrades (around 17 since last week), when I tested in a fresh DB, and from reading the code (AFAICT) the mail alias generated from the journal doesn't have a code, journal 'Customer Invoices' would generate 'customer-invoices' as the alias name for example, and the full alias in most of the failing cases is -------@localhost

Please also note that you should provide some unit tests in order to highlight the issue, otherwise it is a bit hard to see the final result :) .

Ok! Indeed. :)

"-------@localhost" ? Encoding issue ?

Oh, you tagged the user localhost xD
I used "----" to ignore the less important part, i meant to say that the domain is most of the time localhost, and when it's not, it's still not unique, this is in the context of same company, the error is easy to reproduce..

But I didn't know about the uniqueness in a given alias domain, I will fix things and add a unit test!

But you talk about upgrade. However alias names should be unique already in 16, did something change ?

But you talk about upgrade. However alias names should be unique already in 16, did something change ?

They are, but the way they are created isn't always unique.

As said in the commit message, the issue is mainly when the name of a journal is changed, because that doesn't change the name of the alias that was created with it:

I create a journal named A, automatically an alias named A will be created. Afterwards if the journal name is changed, the alias will remain A. (changing the journal name doesn't affect the alias name)
Now a new journal named A can be created because that name is not taken. This will fail because the creation of that journal will trigger the creation of an alias A which is already exists.
This also happens when users changed or manually translated the name of journals that where created by odoo because it doesn't suit their needs. As during the upgrade, Odoo will try to recreate the journals since the name is not the same anymore.

I see two different issues then

create a journal, rename it, create a new journal with old name -> that's effectively a standard issue; would say we probably don't care in standard addons (there is few automatic alias name generation), but we can effectively improve accounting (but still not sure for the heuristic here, to check how accounting see it; shouldn't we add a number as suffix ?)

upgrade trying to create journals again -> isn't it an upgrade issue ? Should it do it ?

shouldn't we add a number as suffix ?

That was the first version of the code, but the combination of code and company_id is unique, so we can have a unique alias without having numbering like 1,2,3...

upgrade trying to create journals again -> isn't it an upgrade issue ? Should it do it ?

During the upgrade, as in the creation of a new odoo DB, data is added from data/demo directories to the DB, that's normal

The issue with the account journals made by the chart template is that they don't have an xmlid, see here how they are created:
https://github.com/odoo/odoo/blob/17.0/addons/account/models/chart_template.py#L913
(Currently looking into why they are made this way)

So almost any change to these template journals will make them unrecognisable, when the chart is loaded again or during an upgrade, it will seem like data created by the user.

tde-banana-odoo · 2024-03-25T13:48:56Z

addons/account/models/account_journal.py

+        # Check for duplicate alias, if exists append a suffix to ensure uniqueness
+        # It's not a duplicate if it's the same journal
+        sanitized_alias_name = self.env['mail.alias']._sanitize_alias_name(base_alias_name)
+        existing_alias = self.env['mail.alias'].search([('alias_name', '=', sanitized_alias_name)])


That's not true anymore with introduction of alias domains, alias names are not unique anymore, but unique in a given alias domain.

hupo-odoo · 2024-03-25T15:48:24Z

Hello I discussed the issue with a PO and he prefers the solution with adding a suffix on the new alias. We don't want to change the alias when changing the name of a journal as the domain might have already been transmitted to external sources (suppliers, ...).

hupo-odoo

Good job for the PR. I've made a few comments. Also, when testing on the web interface, I see that setting the name, then the journal type, then the code makes the alias name without the code, it is not recomputed when writing the code, and it therefore write something like vendor-bills-false@mycompany.com. Is it something expected ?

hupo-odoo · 2024-04-08T11:57:37Z

addons/account/tests/test_account_journal.py

+
+    def test_alias_journal_after_rename(self):
+        """ Test alias when journal is created with name matching existing alias """
+        company_name = 'company_1_data'


Suggested change

company_name = 'company_1_data'

company_name = self.company_data['company'].name

Thank you for the thorough review, it seems I misunderstood some things while reading and testing how the aliases are generated and I will start working on a better version.

However I'm unable to recreate the case where the code is False since the journal cannot be created without a code, in my case the alias is successfully created with the code. Could you tell me what you tried?

Here is an example video where the code is False: https://app.screencastify.com/v3/watch/bymZIwT2wVgmPntjqBxV
maybe I misunderstood something, don't hesitate to tell me if so

hupo-odoo · 2024-04-08T12:19:00Z

addons/account/models/account_journal.py

+                base_alias_name = f'{base_alias_name}-{company_identifier}'
+
+        # Check for duplicate alias, if exists append a suffix to ensure uniqueness
+        # It's not a duplicate if it's the same journal


Suggested change

# It's not a duplicate if it's the same journal

No need to specify this in my opinion

hupo-odoo · 2024-04-08T12:44:03Z

addons/account/models/account_journal.py

+        existing_alias = self.env['mail.alias'].search([('alias_name', '=', sanitized_alias_name)])
+        if len(existing_alias) > 0 and existing_alias.id != self.alias_id.id:
+            alias_name = f'{sanitized_alias_name}-{code}'
+        else:
+            alias_name = sanitized_alias_name


Suggested change

existing_alias = self.env['mail.alias'].search([('alias_name', '=', sanitized_alias_name)])

if len(existing_alias) > 0 and existing_alias.id != self.alias_id.id:

alias_name = f'{sanitized_alias_name}-{code}'

else:

alias_name = sanitized_alias_name

existing_alias = self.env['mail.alias'].search([('alias_name', '=', sanitized_alias_name), ('id', '!=', self.alias_id.id)])

alias_name = f'{sanitized_alias_name}-{code}' if existing_alias else sanitized_alias_name

return self.env['mail.alias']._sanitize_alias_name(alias_name)

Something like this is a bit cleaner IMO, but you seem to have forgotten about TDE's comment on the uniqueness of alias name inside an alias domain.

Also, I see the function you try to improve is defined with a api.model, meaning that the content of the self should not be relevant. However, you now use self.alias_id.id.

hupo-odoo · 2024-04-08T12:44:45Z

addons/account/tests/test_account_journal.py

+        })
+        # check that the alias name is correctly set, name + company name + journal code
+        self.assertEqual(journal_2.alias_name,
+                        self.env['mail.alias']._sanitize_alias_name(f'test-journal-{company_name}-{journal_2.code}'))


Suggested change

self.env['mail.alias']._sanitize_alias_name(f'test-journal-{company_name}-{journal_2.code}'))

self.env['mail.alias']._sanitize_alias_name(f'test-journal-{company_name}-{journal_2.code}'))

Suggestion to align with the comparison line above

hupo-odoo · 2024-04-08T12:44:55Z

addons/account/tests/test_account_journal.py

+                        self.env['mail.alias']._sanitize_alias_name(f'test-journal-{company_name}-{journal_2.code}'))
+        self.assertTrue(journal_2.alias_id)
+        self.assertEqual(journal_2.alias_id.alias_parent_thread_id, journal_2.id,
+                        'Journal alias owned by journal itself')


Suggested change

'Journal alias owned by journal itself')

'Journal alias owned by journal itself')

hupo-odoo · 2024-04-08T12:51:38Z

addons/account/tests/test_account_journal.py

+        self.assertTrue(journal_2.alias_id)
+        self.assertEqual(journal_2.alias_id.alias_parent_thread_id, journal_2.id,
+                        'Journal alias owned by journal itself')


What exactly does this test ? I don't really understand, as your code seems to only improve the naming of the alias names

tde-banana-odoo

Not sure how you ended up doing this, but this is the wrong approach...

addons/mail/data/ir_sequence_data.xml

addons/mail/models/mail_alias_mixin_optional.py

Steps to reproduce: Create an account journal. Rename the created journal (let's say from A to B). Create a new account journal named A. Changing the name of a journal doesn't change the name of the alias, An duplicate error will occur. Additionally the error can happen if the user has manually added aliases with the same name before upgrading to 17.0. Adding the code of the journal to the alias name on creation when needed.

hupo-odoo

I've made a few comments 😃

hupo-odoo · 2024-05-28T15:11:35Z

addons/account/models/account_journal.py

+                ('alias_name', '=', alias_name),
+            ])
+
+        if existing_alias > 0:


Suggested change

if existing_alias > 0:

if existing_alias:

hupo-odoo · 2024-05-28T15:15:53Z

addons/account/models/account_journal.py

+        existing_alias = None
+        if alias_domain_name:
+            existing_alias = self.env['mail.alias'].search_count([
+                ('alias_name', '=', alias_name),
+                ('alias_domain', '=', alias_domain_name),
+            ])
+        else:
+            existing_alias = self.env['mail.alias'].search_count([
+                ('alias_name', '=', alias_name),
+            ])


Suggested change

existing_alias = None

if alias_domain_name:

existing_alias = self.env['mail.alias'].search_count([

('alias_name', '=', alias_name),

('alias_domain', '=', alias_domain_name),

])

else:

existing_alias = self.env['mail.alias'].search_count([

('alias_name', '=', alias_name),

])

domain = [('alias_name', '=', alias_name)]

if alias_domain_name:

domain.append(('alias_domain', '=', alias_domain_name))

existing_alias = self.env['mail.alias'].search_count(domain, limit=1)

Can be simplified a bit, also adding limit=1 is better for performances

hupo-odoo · 2024-05-28T15:26:14Z

addons/account/models/account_journal.py

@@ -604,6 +604,30 @@ def _alias_prepare_alias_name(self, alias_name, name, code, jtype, company):
                alias_name = f"{alias_name}-{company_identifier}"
        return self.env['mail.alias']._sanitize_alias_name(alias_name)

+    def _check_unique_alias(self, vals, company):


Suggested change

def _check_unique_alias(self, vals, company):

api.model

def _check_unique_alias(self, vals, company):

self is irrelevant in your function so you can put api.model

hupo-odoo · 2024-05-28T17:51:20Z

addons/account/models/account_journal.py

+                vals['alias_name'] = self._alias_prepare_alias_name(
+                    False, vals.get('name'), vals.get('code'), journal_type, company
+                )
+            vals['alias_name'] = self._check_unique_alias(vals, company)


why not trying to put the logic of the function _check_unique_alias inside the function _alias_prepare_alias_name ? This way the function _alias_prepare_alias_name always returns a unique name. It's just an idea so you might have arguments against. Also, this way, if set properly, the function _sanitize_alias_name would be called after, making sure that no special character are used

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch from 48c881a to f81e395 Compare March 25, 2024 07:09

UemuS changed the title ~~[FIX][17.0] account_journal: avoid duplicate mail alias~~ [FIX] account_journal: avoid duplicate mail alias Mar 25, 2024

UemuS mentioned this pull request Mar 25, 2024

[FIX] account_journal: avoid duplicate mail alias #159036

Closed

UemuS requested a review from tde-banana-odoo March 25, 2024 07:10

C3POdoo requested review from a team and hupo-odoo and removed request for a team March 25, 2024 07:12

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch from f81e395 to 050b332 Compare March 25, 2024 07:24

C3POdoo added the RD research & development, internal work label Mar 25, 2024

tde-banana-odoo reviewed Mar 25, 2024

View reviewed changes

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch 4 times, most recently from df643e6 to 7ecbcea Compare April 2, 2024 09:32

UemuS requested a review from tde-banana-odoo April 2, 2024 11:17

hupo-odoo reviewed Apr 8, 2024

View reviewed changes

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch from 7ecbcea to 88b8522 Compare April 10, 2024 08:44

C3POdoo requested review from a team April 10, 2024 08:46

UemuS removed request for a team April 10, 2024 08:55

UemuS marked this pull request as draft April 10, 2024 08:56

tde-banana-odoo requested changes Apr 10, 2024

View reviewed changes

addons/mail/data/ir_sequence_data.xml Outdated Show resolved Hide resolved

addons/mail/models/mail_alias_mixin_optional.py Outdated Show resolved Hide resolved

addons/mail/models/mail_alias_mixin_optional.py Outdated Show resolved Hide resolved

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch 5 times, most recently from b8da169 to 1239ae2 Compare April 11, 2024 18:43

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch from 1239ae2 to baf3651 Compare April 30, 2024 08:25

UemuS marked this pull request as ready for review April 30, 2024 08:27

C3POdoo requested review from a team and rugo-odoo and removed request for a team April 30, 2024 08:28

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch from baf3651 to f2628e8 Compare April 30, 2024 08:34

UemuS removed the request for review from rugo-odoo April 30, 2024 08:34

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch 6 times, most recently from 10be5b8 to 5de9d4a Compare April 30, 2024 11:33

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch from 5de9d4a to 96afa34 Compare May 8, 2024 08:16

UemuS force-pushed the 17.0-avoid_duplicate_auto_mail_alias-yaih branch from 96afa34 to 1bc1365 Compare May 21, 2024 10:00

hupo-odoo reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] account_journal: avoid duplicate mail alias #159037

[FIX] account_journal: avoid duplicate mail alias #159037

UemuS commented Mar 25, 2024 •

edited

robodoo commented Mar 25, 2024

tde-banana-odoo left a comment

tde-banana-odoo Mar 25, 2024

UemuS Mar 25, 2024

tde-banana-odoo Mar 25, 2024

UemuS Mar 25, 2024

tde-banana-odoo Mar 25, 2024

UemuS Mar 25, 2024

tde-banana-odoo Mar 25, 2024

UemuS Mar 25, 2024 •

edited

tde-banana-odoo Mar 25, 2024

hupo-odoo commented Mar 25, 2024

hupo-odoo left a comment

hupo-odoo Apr 8, 2024

UemuS Apr 8, 2024 •

edited

hupo-odoo Apr 8, 2024 •

edited

hupo-odoo Apr 8, 2024

hupo-odoo Apr 8, 2024

hupo-odoo Apr 8, 2024

hupo-odoo Apr 8, 2024

hupo-odoo Apr 8, 2024

tde-banana-odoo left a comment

hupo-odoo left a comment

hupo-odoo May 28, 2024

hupo-odoo May 28, 2024

hupo-odoo May 28, 2024

hupo-odoo May 28, 2024

	company_name = 'company_1_data'
	company_name = self.company_data['company'].name

	self.env['mail.alias']._sanitize_alias_name(f'test-journal-{company_name}-{journal_2.code}'))
	self.env['mail.alias']._sanitize_alias_name(f'test-journal-{company_name}-{journal_2.code}'))

	'Journal alias owned by journal itself')
	'Journal alias owned by journal itself')

	def _check_unique_alias(self, vals, company):
	api.model
	def _check_unique_alias(self, vals, company):

[FIX] account_journal: avoid duplicate mail alias #159037

Are you sure you want to change the base?

[FIX] account_journal: avoid duplicate mail alias #159037

Conversation

UemuS commented Mar 25, 2024 • edited

robodoo commented Mar 25, 2024

tde-banana-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UemuS Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hupo-odoo commented Mar 25, 2024

hupo-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UemuS Apr 8, 2024 • edited

Choose a reason for hiding this comment

hupo-odoo Apr 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tde-banana-odoo left a comment

Choose a reason for hiding this comment

hupo-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UemuS commented Mar 25, 2024 •

edited

UemuS Mar 25, 2024 •

edited

UemuS Apr 8, 2024 •

edited

hupo-odoo Apr 8, 2024 •

edited