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

[REF] l10n_pe: Making work properly the PCGE group related #32892

Open
wants to merge 1 commit into
base: 12.0
from

Conversation

Projects
None yet
3 participants
@nhomar
Copy link
Collaborator

commented Apr 24, 2019

Objective.

The current COA for Peru is not usable, at least not out of the box, bringing confusing behavior and a ton of configuration on the customer side, the objective of this changes are to make it usable Out of the Box and prepare the implementations since v12.0 and allow start clean installations with less consultancy from now on (at least at the very beginning).

With this CoA the minimal necessary accounts to follow all the accounting memento on the documentation are available, and some other common ones.

CoA (a.k.a. PCGE):

  • Moved parent accounts as groups.
  • Set minimal set of accounts to work with the most basic
    set of features in Odoo accounting related.
  • Prepare coding relatable with the official PCGE.
  • Cleanup the taxes in order to use the correct accounts.
  • Clean the author (this module is nothing like the original one).
  • Removed account types, those should be into the correct module.
    they are not necessary yet to create the reports due to the
    fact that they do not depend on the account types but in the
    accounts themselves on the PCGE and they will be annoying for
    multi-localization environments.

Other Fix:

Calling the onchange when an account is auto created in order to reuse such code instead do it each time we create an l10n_** module (necessary to have the reports related to codes here).

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

@robodoo robodoo added the seen 🙂 label Apr 24, 2019

@nhomar nhomar requested review from nim-odoo and jco-odoo Apr 24, 2019

@robodoo robodoo added the CI 🤖 label Apr 24, 2019

@nim-odoo nim-odoo requested review from jpp-odoo and removed request for nim-odoo Apr 24, 2019

@jco-odoo
Copy link
Contributor

left a comment

Of course, there are issues with the existing CoA, but just need to check that we don't put existing customers in a defunct state when they update the module.

Also, the onchange_code is something to debate outside of this pr. There is no way to update the account groups through xml where it is necessary?

@@ -769,6 +769,7 @@ def create(self, vals):
company = self.env['res.company'].browse(company_id)
account_vals = self._prepare_liquidity_account(vals.get('name'), company, vals.get('currency_id'), vals.get('type'))
default_account = self.env['account.account'].create(account_vals)
default_account.onchange_code()

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

What is the idea behind this? Creating account groups automatically?

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

With the CoA based on groups the onchange helps with this user story line:

Given that I create a new account then trigger the correct group instead make the user/accountant to look for it.

In Perú it is legally required the CoA, then almost all accountants simply Know the code and the more common behavior is create the account (by any mean) and call the onchange (that set the proper group).

But yes this can be discussed in a different PR just if we do this one then the the group_id:id entry on the csv can be removed for example (making consistent the behavior as if we create manually the accounts).

This comment has been minimized.

Copy link
@nhomar

nhomar May 5, 2019

Author Collaborator

This is being done in a different PR, without this change then all accounts autocreated are not having the proper group once create (even if it is automated).

image

@@ -1,4 +0,0 @@
# -*- coding: utf-8 -*-
# Part of Odoo. See LICENSE file for full copyright and licensing details.

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

It is still part of Odoo.

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

good point... todo: FIXME

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

fixed

</record>
<record id="tax_group_2" model="account.tax.group">
<field name="name">IGV 2%</field>
</record>

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

This tax_group_2 does not exist in the new one, which makes the system defunct when an existing user tries to update the module. (but if you call tax_group_02 tax_group_2 ...)

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

I do not get the point here, can you explain better what are you expecting here?

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

This one is important. Can you change tax_group_02 in your pr by tax_group_2? (avoids defunct state on upgrade on existing db)

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

fixed

"275230","Activos no corrientes ...- Depreciación acumulada.../ edificaciones, revaluación","275230","account.data_account_type_current_assets","l10n_pe.pe_chart_template","False"
"276210","Activos no corrientes ...- Depreciación acumulada.../ edificaciones, costo de adquisición o construcción ","276210","account.data_account_type_current_assets","l10n_pe.pe_chart_template","False"
"276220","Activos no corrientes ...- Depreciación acumulada.../ edificaciones, revaluación ","276220","account.data_account_type_current_assets","l10n_pe.pe_chart_template","False"
"276230","Activos no corrientes ...- Depreciación acumulada.../ edificaciones, costo

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

Only 48 lines?

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

I just added the minimal ones as we did in the past, to comply with a minimal company, I did not want to add them all (almost 200) but I can if you wish.

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

I was trying to add just the minimal accounts to achieve what is demonstrated here:

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

On the other hand, it is also that an SMB should not need to start creating accounts. (An accountant should have the impression he can start right away) Maybe the 200 accounts is more appropriate for that?

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

Ok, this will take me a few minuts but that make sense, I will. (TODO: FIxme)

note: Take care that some of those account are for operations that we do not actually support, then even if the account will exists, we will not have administrative features that will use them left them as simply waiting to be set manually.

This comment has been minimized.

Copy link
@nhomar

nhomar May 5, 2019

Author Collaborator

Hello,

I added all possible accounts, then nobody need to add new ones.

I will propose a little improvement on the display_name in order to allow the combination with the group, because the account by themselves in some cases have non descriptive and repetitive name in different levels.

This comment has been minimized.

Copy link
@nhomar

nhomar May 6, 2019

Author Collaborator

the change proposed on groups will be managed here cc @jco-odoo

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo May 6, 2019

Contributor

200 and 1200 is quite a difference... I see also a lot of accounts with the same name. (because it is supposed to be part of the hierarchy) Does not that complicate matters for the user to select the correct account? @nhomar

This comment has been minimized.

Copy link
@nhomar

nhomar May 6, 2019

Author Collaborator

@jco-odoo indeed, that's why I did this other PR:

#33171

To solve exactly such problem.

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo May 7, 2019

Contributor

@nhomar Maybe I should have said the 48 were just fine. (the pr #33171 seems problematic for stable) It is not the first time I see this case where you have the same subaccounts, but I wonder if it really makes sense and if everyone does it that way. They can not just be grouped to one account?

@@ -1,1540 +1,48 @@
"id","name","code","user_type_id/id","chart_template_id/id","reconcile"

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

No way to keep the chart template xml_id the same and try to reuse the same xml_id pattern? Also, for migration, we still need to know which account type becomes which standard.

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

I do not get the question.

AFAIK the accounts are untouched once they are autocreated, this are the xmli_id's just for the template layer.

For migration I strongly would recommend base them on code (which bTW were incorrect mostly) I hardly believe somebody is using the original CoA as it is coming from the old version.

Then that's why I change even the xml_id (to avoid any technical confusion here).

But I can rethink this if you think it adds any value for the future.

This comment has been minimized.

Copy link
@nhomar

nhomar May 6, 2019

Author Collaborator

I reverted the xml_id change cc @jco-odoo

<field name="tag_ids" eval="[(6,0,[ref('tax_tag_sale_18')])]"/>
<field name="tax_group_id" ref="tax_group_18"/>
</record>

<record id="OTAX_18" model="account.tax.template">
<field name="chart_template_id" ref="pe_chart_template"/>
<record id="vendor_tax_02" model="account.tax.template">

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

I was thinking about keeping the same xml_id here, but I don't think it will improve anything, because the tags itself are kept. (and the new ones are for new taxes) Btw, do we have an idea what to do with these tags afterwards?

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

This tags will be used in l10n_pe_reports (once done) to set the proper accounting reports, for now on, they are just basic data.

@@ -2,7 +2,7 @@
<odoo>
<data noupdate="1">
<function model="account.chart.template" name="try_loading_for_current_company">
<value eval="[ref('l10n_pe.pe_chart_template')]"/>

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 24, 2019

Contributor

Is it necessary to change xmlid?

This comment has been minimized.

Copy link
@nhomar

nhomar Apr 24, 2019

Author Collaborator

I would like to simplify (as short as possible) the CoA code and call it as the peruvian standard, if you think this can bring problems, I am ok lefting it, but as this is a competely new approach, I think it is better change even the soul of the CoA ;-).

(I can bring back the old one but I think it is better if we have a new one instead).

This comment has been minimized.

Copy link
@nhomar

nhomar May 6, 2019

Author Collaborator

reverted the change of the xml_id cc @jco-odoo

@nhomar

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 24, 2019

About:

but just need to check that we don't put existing customers in a defunct state when they update the module.

I will check a before-after with and without -u on the server, That would be enough? or do you want I check another thing?

@nhomar nhomar force-pushed the vauxoo-dev:12.0-new-l10n_pe branch from 3dcd642 to 2491e88 Apr 24, 2019

@robodoo robodoo removed the CI 🤖 label Apr 24, 2019

@nhomar nhomar force-pushed the vauxoo-dev:12.0-new-l10n_pe branch from 2491e88 to 0465a91 Apr 24, 2019

@robodoo robodoo added the CI 🤖 label Apr 24, 2019

@nhomar nhomar force-pushed the vauxoo-dev:12.0-new-l10n_pe branch from 0465a91 to 4390096 Apr 25, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 25, 2019

@nhomar nhomar force-pushed the vauxoo-dev:12.0-new-l10n_pe branch from 4390096 to 96368d3 May 1, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 1, 2019

@nhomar nhomar force-pushed the vauxoo-dev:12.0-new-l10n_pe branch from c04abad to cf192c0 May 5, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 5, 2019

[REF] l10n_pe: Current CoA for Perú is unusable.
- Moved parent accounts as groups.
- Set minimal set of accounts to work with the most basic
  set of features in odoo accounting related.
- Prepare coding relatable with the official PCGE.
- Cleanup the taxes in order to use the correct accounts.
- Clean the author (this module is nothing like the original one).
- Removed account types, those should be into the correct module.
  they are not necessary **yet** to create the reports due to the
  fact that they do not depend on the account types but in the
  accounts themselves on the PCGE and they will be anoying for
  multi-localization environments.

@nhomar nhomar force-pushed the vauxoo-dev:12.0-new-l10n_pe branch from 37de46c to 5246452 May 6, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 6, 2019

@jco-odoo

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@nhomar Indeed, with and without -u.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.