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

Modify Corporate Standard Char Of Account and 2017 Small And Medium-s… #32075

Open
wants to merge 2 commits into
base: 12.0
from

Conversation

@Colinliz
Copy link
Contributor

Colinliz commented Mar 25, 2019

…ized Enterprises Accounting Chart By PSCloud

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 robodoo added the seen 🙂 label Mar 25, 2019

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Mar 25, 2019

Please communicate here in English, as this has global audience.

@Colinliz Colinliz force-pushed the Colinliz:12.0 branch from faa17b4 to 1c20d87 Mar 26, 2019

@jeffery9

This comment has been minimized.

Copy link
Contributor

jeffery9 commented Mar 27, 2019

@mart-e please help to merge this pr.

@jeffery9
Copy link
Contributor

jeffery9 left a comment

switch name and descripiton for all tax setting.

@Colinliz Colinliz force-pushed the Colinliz:12.0 branch from 1c20d87 to d1b4eaa Mar 27, 2019

@joshuajan
Copy link
Contributor

joshuajan left a comment

typo

@leangjia

This comment has been minimized.

Copy link

leangjia commented Mar 27, 2019

I need it.

@mart-e mart-e force-pushed the Colinliz:12.0 branch from 199845f to 94489b2 Mar 27, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 27, 2019

@GoodERPJeff

This comment has been minimized.

Copy link
Contributor

GoodERPJeff commented Mar 29, 2019

@mart-e it's ready for merge

@mart-e

This comment has been minimized.

Copy link
Contributor

mart-e commented Mar 29, 2019

Hi,

Sorry but it is not. The branch has conflicts and is making changes that I don't think are suitable for a stable branch (removing records,...).
Also, please explain what changes you are doing to specify in the commit message. See our guidelines about it (don't explain just what, also explain why it is important to change that).

Also @jco-odoo is responsible for the localisations so should review the changes before merging.

@Colinliz Colinliz force-pushed the Colinliz:12.0 branch from 4a871ff to 05288ed Mar 29, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 29, 2019

@Colinliz

This comment has been minimized.

Copy link
Contributor Author

Colinliz commented Apr 2, 2019

@jco-odoo
Hi,
Can you help to review the changes before merging?
I have modified small and medium-sized enterprises accounting chart, and deleted included and excluded tax: 17% 11% 6% 3%, added included and excluded tax: 13% 16%.

@jco-odoo

This comment has been minimized.

Copy link
Contributor

jco-odoo commented Apr 2, 2019

@Colinliz Nice to meet you on Github. The changes are indeed quite extensive. You just changed the Chart of Accounts with yours. Did you check if there are compatibilities with the old one? (if the differences are really that huge?)

For the taxes: when did the taxes of 17, 11, 6, 3% change? Because if it is not too long ago, it might be interesting to keep them for the moment. (check also what should be the tax by default, should be 16% I suppose) If I check online, the 6% vat is still very much active.

As there is no connection with any tags, there might be a way to do this in a stable version, but then we have to be sure it is a real improvement. Maybe it is better to do something solid with tax reports, ... in master.

@GoodERPJeff

This comment has been minimized.

Copy link
Contributor

GoodERPJeff commented Apr 2, 2019

@Colinliz Nice to meet you on Github. The changes are indeed quite extensive. You just changed the Chart of Accounts with yours. Did you check if there are compatibilities with the old one? (if the differences are really that huge?)

For the taxes: when did the taxes of 17, 11, 6, 3% change? Because if it is not too long ago, it might be interesting to keep them for the moment. (check also what should be the tax by default, should be 16% I suppose) If I check online, the 6% vat is still very much active.

As there is no connection with any tags, there might be a way to do this in a stable version, but then we have to be sure it is a real improvement. Maybe it is better to do something solid with tax reports, ... in master.

China VAT just change to :13%,9%,6% 。
For this module just add template to database, not account or tax, so it's safe to remove other tax or account tamplates.
please change the xml file @Colinliz
other change is ok to me

@Colinliz Colinliz force-pushed the Colinliz:12.0 branch from 05288ed to 83cbb0d Apr 2, 2019

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

@Colinliz

This comment has been minimized.

Copy link
Contributor Author

Colinliz commented Apr 2, 2019

@jco-odoo Thank you very much for your suggestion.I have checked that my revised account chart is fully compatible with the old one, and it also applies to small businesses today.

For the taxes:I keep 13% 9% and 6% account tax, and remove
17% 11% and 3%. After discussion I think 13% account tax should be default, because China has already changed 16% tax to 13% tax.

It's hard for you to review my code again and give your opinion. I'll revise it in time.

@Colinliz Colinliz force-pushed the Colinliz:12.0 branch 2 times, most recently from 31bd442 to 0a965d7 Apr 6, 2019

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

@Colinliz

This comment has been minimized.

Copy link
Contributor Author

Colinliz commented Apr 7, 2019

@jco-odoo Sorry to bother you, but do you have time to review the code for me?

"id","code","name","reconcile","user_type_id/id","chart_template_id/id"
"small_business2017_chart1001","1001","库存现金","False","account.data_account_type_liquidity","l10n_cn_small_business.l10n_chart_china_small_business"
"small_business2017_chart1002","1002","银行存款","False","account.data_account_type_liquidity","l10n_cn_small_business.l10n_chart_china_small_business"
"small_business2017_chart1012","1012","其他货币资金","False","account.data_account_type_current_assets","l10n_cn_small_business.l10n_chart_china_small_business"

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 8, 2019

Contributor

Why change xml id? It is breaking stuff for breaking stuff. If you kept l10n_cn_ and the same structure, we could really see the difference between the two.

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 8, 2019

Contributor

And sometimes you have a code of length 7, while you put code digits at 4

This comment has been minimized.

Copy link
@GoodERPJeff

GoodERPJeff Apr 8, 2019

Contributor

@jco-odoo code digit define the "minimal" length of account code, it keep adding trialling 00 to first level account code when it is "6". change it to "4" is the must we have from 10 years ago in openerp.

Yes, xml id should be changed back @Colinliz

<field name="tax_group_id" ref="l10n_cn.l10n_cn_tax_group_vat_17"/>
<!-- Account Tax Tags-->
<record id="tax_tag1" model="account.account.tag">
<field name="name">6%销项税</field>

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 8, 2019

Contributor

What do you need the tags for?

@jco-odoo

This comment has been minimized.

Copy link
Contributor

jco-odoo commented Apr 8, 2019

@Colinliz I would say as the VAT rates (16% -> 13%) came into force on the first of April, it can be a good idea to keep them if people still need to create an invoice for March e.g., but we don't have the 16% in the chart, only the 17%.

Is not 13% automatically already the default tax? I agree with @mart-e that some context can help us. We can always do a call if you want.

Colinliz added some commits Apr 8, 2019

[FIX] l10n_cn_small_business: Modify Small And Medium-sized Enterpris…
…es Accounting Chart

Steps to reproduce the bug:
- Add some child account and modify some parent account according to 'Accounting Standards for 2017 Small and Medium-sized Enterprises'
- Delete included and excluded tax: 17% 11% 3%
Signed-off-by: Colinliz <lizheng02@inspur.com>
[FIX] l10n_cn_standard: Modify Corporate Standard Char Of Account
Steps to reproduce the bug:
- Delete included and excluded tax: 17% 11% 3%
- Add included and excluded tax: 13% 9% 6%

Signed-off-by: Colinliz <lizheng02@inspur.com>

@Colinliz Colinliz force-pushed the Colinliz:12.0 branch from 0a965d7 to e9be25a Apr 8, 2019

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

@GoodERPJeff

This comment has been minimized.

Copy link
Contributor

GoodERPJeff commented Apr 8, 2019

@Colinliz I would say as the VAT rates (16% -> 13%) came into force on the first of April, it can be a good idea to keep them if people still need to create an invoice for March e.g., but we don't have the 16% in the chart, only the 17%.

Is not 13% automatically already the default tax? I agree with @mart-e that some context can help us. We can always do a call if you want.

16% will be only hardly used in Apil 2019, no reason keep it in our chart.

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

@jco-odoo
Copy link
Contributor

jco-odoo left a comment

Do you agree we do this in master and maybe you can make another pr with only the change of taxes to be done in 12? @Colinliz

@@ -2,14 +2,23 @@
<odoo>

<data noupdate="1">
<record id="account_1003" model="account.account.template">

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 15, 2019

Contributor

can go in the csv

@@ -1,76 +1,206 @@
"id","name","code","user_type_id/id","chart_template_id/id","reconcile"
"l10n_cn_1012","Other Monetary Funds","1012","account.data_account_type_current_assets","l10n_cn_small_business.l10n_chart_china_small_business","False"

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 15, 2019

Contributor

It is of course logical that the CoA is in the language of the country itself, but it is quite a heavy change for stable. On the other hand, the l10n_cn module is multilang, so you could add the translation to English. (I suppose there is a reason it was originally in English)

<record id="l10n_chart_china_small_business" model="account.chart.template">
<field name="name">小企业会计科目表(财会[2011]17号《小企业会计准则》)</field>
<field name="code_digits" eval="6"/>
<field name="code_digits" eval="4"/>
<field name="currency_id" ref="base.CNY"/>
<field name="cash_account_code_prefix">1001</field>
<field name="bank_account_code_prefix">1002</field>
<field name="transfer_account_code_prefix">1012</field>
<field name="spoken_languages" eval="'en_US'"/>

This comment has been minimized.

Copy link
@jco-odoo

jco-odoo Apr 15, 2019

Contributor

By changing the CoA, you change to Chinese here, but as said before, you can add both.

@jco-odoo

This comment has been minimized.

Copy link
Contributor

jco-odoo commented Apr 15, 2019

If you do export translations with "new language" and "po file", that will give you a .pot file that you can put in 10n_cn_small_business/i18n_extra/l10n_cn_small_business.pot Next to that you could add a file for English as well e.g. (or another language) (but I don't know e.g. in which languages you can report)

@jco-odoo

This comment has been minimized.

Copy link
Contributor

jco-odoo commented Apr 15, 2019

@Colinliz Accounts in Chinese, maybe we can still do it in stable as it is inconsistent with the taxes being in Chinese, but the accounts not.

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.