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
[IMP] account: install and load COA on company creation #34490
base: master
Are you sure you want to change the base?
[IMP] account: install and load COA on company creation #34490
Conversation
14cb041
to
67d73ab
Compare
67d73ab
to
2886a21
Compare
2886a21
to
cec4a58
Compare
cec4a58
to
b64ca0d
Compare
b64ca0d
to
c860e88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check this comments and also backport to 12.5?
addons/account/models/company.py
Outdated
module_ids.with_context({'allowed_company_ids': [company.id]}).button_immediate_install() | ||
#localizaton for the given country is already installed. | ||
else: | ||
#filter out the list of templates that belong to newly created company's country and load the first one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed as it is the localization itself that decides whether it already installs the CoA or not. Leave out the else part. And why button_install in the first and button_immediate_install in the second?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get you. This code is there to load appropriate templates for the new company. If we remove this, templates won't be loaded for new company even when they are installed.
kindly let me know If I misunderstood something. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hdh-odoo In most localizations, there is a script that loads it automatically for the current company. (try_loading...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jva-odoo The logic should be different: 1 check if the module has been installed or not: if not, install / if already there: create a method to install the chart of accounts that localizations can inherit. (with more or less in it what is below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jco-odoo here 1 we check module is installed or not using "module_ids": after if not installed then we install using button_immediate_install else installed then we call try_loading_for_current_company.
Logic is the same as you say....so I didn't get where this is wrong or need improvement...!!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_init or not, it should check if the module is installed or not. And actually installing the CoA could be the same thing. So, why do we need from_init in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jco-odoo from company create method if I call button_install then it's not installing a module so I need to call button_immediate_install instead of button_install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button_immediate_install is needed (put it in a comment) for blocking concurrent access with the crons that might be running.
def create(self, vals): | ||
company = super(ResCompany, self).create(vals) | ||
company._install_localization_packages() | ||
return company |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also do this on the write when the country_id was False before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make the appropriate changes. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this still be adapted?
addons/account/models/company.py
Outdated
if country_code in europe_country_codes: | ||
module_list.append('account_sepa') | ||
module_list.append('account_bank_statement_import_camt') | ||
module_ids = company.env['ir.module.module'].search([('name', 'in', module_list), ('state', '=', 'uninstalled')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be self.env
addons/account/__init__.py
Outdated
module_list.append('account_bank_statement_import_camt') | ||
module_ids = env['ir.module.module'].search([('name', 'in', module_list), ('state', '=', 'uninstalled')]) | ||
module_ids.sudo().button_install() | ||
env.company._install_localization_packages(from_init = True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_init=True
addons/account/models/company.py
Outdated
@@ -278,6 +280,73 @@ def _validate_fiscalyear_lock(self, values): | |||
if nb_draft_entries: | |||
raise ValidationError(_('There are still unposted entries in the period you want to lock. You should either post or delete them.')) | |||
|
|||
def _install_localization_packages(self, from_init=False): | |||
for company in self: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not do the for-loop. We are only going to install one company at a time. Better to use ensure_one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jco-odoo because we also call from write so I think it's good to do in for loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I put for loop in write method. 👍
addons/account/models/company.py
Outdated
#a new company is being created. | ||
#localization for the given country isn't installed. | ||
if module_ids: | ||
module_ids.with_context({'allowed_company_ids': [company.id]}).button_immediate_install() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use sudo() and you only need to set company_id (check how it is done elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
addons/account/models/company.py
Outdated
module_ids.with_context({'allowed_company_ids': [company.id]}).button_immediate_install() | ||
#localizaton for the given country is already installed. | ||
else: | ||
#filter out the list of templates that belong to newly created company's country and load the first one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button_immediate_install is needed (put it in a comment) for blocking concurrent access with the crons that might be running.
addons/account/models/company.py
Outdated
chart_template_xml_ids = self.env['ir.model.data'].search([('module', 'in', module_list), ('model', '=', 'account.chart.template')]) | ||
if chart_template_xml_ids: | ||
chart_template = self.env['account.chart.template'].browse(chart_template_xml_ids[0].res_id) | ||
chart_template.with_context({'allowed_company_ids': [company.id]}).try_loading_for_current_company() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment + try_loading_for_current_company is deprecated. (just try_loading)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
def create(self, vals): | ||
company = super(ResCompany, self).create(vals) | ||
company._install_localization_packages() | ||
return company |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still todo
c860e88
to
2db9894
Compare
@jco-odoo change done 👍 |
@jva-odoo Do you rebase with last master and check if it is green? |
ok 👍 |
2db9894
to
6f95916
Compare
addons/account/models/company.py
Outdated
def write(self, values): | ||
#restrict the closing of FY if there are still unposted entries | ||
self._validate_fiscalyear_lock(values) | ||
|
||
company_without_coa = self.filtered(lambda c: not c.chart_template_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use "existing accounting" method, but better to check it only when not self.country_id and values.get('country_id') I don't think you check not self.country_id here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be closed? |
@pedrobaeza Why? We are working on it. |
It appears to me in codetriage.com, and seeing the age, I thought it was stalled. |
@jva-odoo Is this the latest one? Because I made comments here, but I don't see any adaptations. |
6f95916
to
1208b80
Compare
61feb7e
to
074758c
Compare
074758c
to
dfe89de
Compare
dfe89de
to
48332ba
Compare
48332ba
to
430d5d5
Compare
430d5d5
to
a7cc611
Compare
Now, on creating company, appropriate localization module will be installed and loaded based on the country selected for that company. If, user did not set country while creating company then whenever user sets country for that company, appropriate CoA will be loaded. The functionality is disabled when company created from running test cases as module installation are forbidden from test cases. l10n_ar: Since there are multiple CoA for Argentina country and appropriate CoA for particular company can only be determined when user sets AFIP Responsibility field, this thing has been managed also. Task: 2027057 Closes: odoo#34490 Related: odoo/enterprise#8189
Now, on creating company, appropriate localization module will be installed and loaded based on the country selected for that company. If, user did not set country while creating company then whenever user sets country for that company, appropriate CoA will be loaded. The functionality is disabled when company created from running test cases as module installation are forbidden from test cases. l10n_ar: Since there are multiple CoA for Argentina country and appropriate CoA for particular company can only be determined when user sets AFIP Responsibility field, this thing has been managed also. Task: 2027057 Closes: odoo#34490 Related: odoo/enterprise#8189
Now, on creating company, appropriate localization module will be installed and loaded based on the country selected for that company. If, user did not set country while creating company then whenever user sets country for that company, appropriate CoA will be loaded. The functionality is disabled when company created from running test cases as module installation are forbidden from test cases. l10n_ar: Since there are multiple CoA for Argentina country and appropriate CoA for particular company can only be determined when user sets AFIP Responsibility field, this thing has been managed also. Task: 2027057 Closes: odoo#34490 Related: odoo/enterprise#8189
Company card should always have Time Off project and task configured. The same is managed through overriding BaseModel's init(). In scenario where localization module automatically installs itself at the time of company creation if country selected for it, application throws missing attribute exception. Below is the link describing the traceback: https://pastebin.com/cAzSEA1g Steps to reproduce traceback are as follows: 1. Install account module. 2. Install project_timesheet_holidays 3. Attempt to create a new company with country Singapore set. Similar tracebacks observed for German, and Mexican localizations.
0ce4011
to
a39c59a
Compare
Description of the issue/feature this PR addresses:
Task: https://www.odoo.com/web#id=2027057&action=327&model=project.task&view_type=form&menu_id=4720
Pad: https://pad.odoo.com/p/r.6da4e4477f06fcf5134a27af571775b0
Description
-previously COA were only being installed and loaded by themselves
when we were creating a new database.
-after this PR, this functionality will be available in
case of new company creation as well.
--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr