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

[ADD] l10n_za: add chart of accounts for South Africa #22873 #26475

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@h4818
Copy link
Contributor

h4818 commented Aug 20, 2018

Description of the issue/feature this PR addresses:
Potential localization for South Africa issue #22873

Current behavior before PR:
No localization available

Desired behavior after PR is merged:

Localization works and is compatible with odoo enterprise

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

@h4818 h4818 changed the title [IMP] add chart of accounts for South Africa closes #22873 [ADD] l10n_za: add chart of accounts for South Africa #22873 Sep 8, 2018

@h4818

This comment has been minimized.

Copy link
Contributor Author

h4818 commented Sep 26, 2018

@lap-odoo Any feedback?

@lap-odoo

This comment has been minimized.

Copy link
Contributor

lap-odoo commented Sep 27, 2018

@avw-odoo That's for you ;-)

@h4818

This comment has been minimized.

Copy link
Contributor Author

h4818 commented Oct 11, 2018

@avw-odoo is this in the pipeline? Should I do a new PR on 12.0? Along with Enterprise 12.0 for reports

@avw-odoo

This comment has been minimized.

Copy link

avw-odoo commented Oct 12, 2018

@h4818 Hello, yes, our technical team will check and get back to you if needed.

Thank you for your help and four patience.

@adekock11

This comment has been minimized.

Copy link
Contributor

adekock11 commented Oct 24, 2018

👍

@oco-odoo
Copy link
Contributor

oco-odoo left a comment

Some small changes to make/questions ;)

# Copyright (C) 2017 Paradigm Digital (<http://www.paradigmdigital.co.za>).

{
'name': 'South Africa ZA - Accounting',

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

Please name it 'South Africa - Accounting' instead, for consistency with other modules ;)

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

Done

'data/account_chart_template.yml',

],
'demo' : [

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

Please remove that, it's not used.

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

Done

================================================================================
- a generic chart of accounts
- SARS VAT Ready Structure
- other adaptations""",

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

Could you be a little more precise than "other adaptations" ? :)

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

Removed

<record id="210010" model="account.account.template">
<field name="code">210010</field>
<field name="name">Company Credit Card </field>
<field name="reconcile" eval='True'/>

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

Credit card accounts should not be reconcilable, just like bank accounts

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

Done, should Stock input and output accounts be reconcilable by default?

This comment has been minimized.

@oco-odoo

oco-odoo Mar 18, 2019

Contributor

I know that they have to be in anglosaxon accounting; but for South Africa, I unfortunately have no idea :/ There is no technical limitation on that, at least.

@@ -0,0 +1,108 @@
"external_id","code","name","user_type_id:id","reconcile","chart_template_id:id"

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

This file seems to be duplicated in addons/l10n_za/data/account_template.xml , right ? You can use csv or xml to define these data, but you don't need to have both. Please remove the csv version, we will keep the xml one.

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

@h4818 forget what I just said! I just learned that we were now busy replacing all xml declarations of account templates by csv for performance reasons, so rather keep the csv ;)

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

I have kept the csv, but not used in module.

This comment has been minimized.

@oco-odoo

oco-odoo Mar 18, 2019

Contributor

We should only use the csv; it makes the module installation faster ;)

</record>


<record id="l10n_za" model="account.chart.template">

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

The complete id of this chart template is thus l10n_za.l10n_za. Could you make its id a little more explicit ? (something like default_chart_template, for example)

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

Done, changed all references

@@ -0,0 +1,3 @@
"id","name","property_account_receivable_id:id","property_account_payable_id:id","property_account_expense_categ_id:id","property_account_income_categ_id:id","income_currency_exchange_account_id:id","expense_currency_exchange_account_id:id","use_anglo_saxon"

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

Same remark: either csv or xml ; please keep xml

This comment has been minimized.

@h4818

h4818 Feb 15, 2019

Author Contributor

This can be removed as it is not used.

<?xml version="1.0" encoding="utf-8"?>
<odoo>
<menuitem id="account_reports_za_statements_menu" name="South Africa" parent="account.menu_finance_reports" sequence="0" groups="account.group_account_user"/>

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

Why declaring those accounts here, and not with the other ones?

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

Important Accounts that are referenced in the chart template below. While less important accounts/taxes are in separate files.

This comment has been minimized.

@oco-odoo

oco-odoo Mar 18, 2019

Contributor

We should declare everything at the same place; important/not important is irrelevant from a technical point of view ;)

<field name="chart_template_id" ref="l10n_za"/>
</record>

<record id="ST1" model="account.tax.template">

This comment has been minimized.

@oco-odoo

oco-odoo Nov 23, 2018

Contributor

Why declaring thoses taxes here, and not with the other ones?

This comment has been minimized.

@h4818

h4818 Feb 13, 2019

Author Contributor

I had a case of requiring to update the data (maybe a related field was missing) - I will try to create in one step.

This comment has been minimized.

@h4818

h4818 Feb 16, 2019

Author Contributor

Important Accounts that are referenced in the chart template below. While less important accounts/taxes are in separate files.

This comment has been minimized.

@jco-odoo

jco-odoo Mar 21, 2019

Contributor

@h4818 Normally the chart template is defined twice. Once without accounts and then with. This way you don't have to redefine accounts. But, it is something we can easily adapt ourselves.

This comment has been minimized.

@h4818

h4818 Mar 21, 2019

Author Contributor

Yes, the chart has to be created before it can be used in the account definition. Hence multiple chart/account definitions. I guess this could be cleaner :)

@jcr-odoo

This comment has been minimized.

Copy link

jcr-odoo commented Feb 13, 2019

@h4818 Hello,
could you update and answer to the questions of OCO-odoo, we can't move forward without it.

Thank you.

@h4818

This comment has been minimized.

Copy link
Contributor Author

h4818 commented Feb 13, 2019

Will make fixes asap

@@ -0,0 +1,10 @@
"id","country_id:id","name","code"

This comment has been minimized.

@adekock11

adekock11 Feb 13, 2019

Contributor

In Odoo V12.0 the res.country.state for ZA is already added in base. Not sure whether it should be added here again as your pull request will probably be targeted toward master. @oco-odoo should we remove the ZA states here or in base?

This comment has been minimized.

@h4818

h4818 Feb 15, 2019

Author Contributor

I have removed the file as it was not being used anyway. Should I modify the PR to target master?

[IMP] l10n_za:
-naming consistency to odoo chart of accounts modules
-replace identifier for the chart template
-remove South African states as it is in base
-credit card account can't be reconciled

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 17, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 17, 2019

@h4818 h4818 changed the base branch from 11.0 to master Feb 18, 2019

@h4818

This comment has been minimized.

Copy link
Contributor Author

h4818 commented Feb 23, 2019

@jcr-odoo @oco-odoo any feedback on the posted changes. Please review and let me know.

@oco-odoo

This comment has been minimized.

Copy link
Contributor

oco-odoo commented Mar 18, 2019

@h4818 Sorry fo the delay, I just added a few more comments; it's nearly okay ;)

@jco-odoo

This comment has been minimized.

Copy link
Contributor

jco-odoo commented Mar 21, 2019

@h4818 We can do the necessary adaptations. Those do not change a lot in the end. Thanks a lot for contributing this.

I have some questions on the side though:

  • Is English common in South-Africa for the accounts or are there a lot of people e.g. using Afrikaans? Because we could extend it with multi_lang and this way have the accounts in all of the languages necessary.
  • Is there a way to define fiscal positions? The taxes seem to make a clear distinction between export or not, so when the partner is outside of South Africa it would automatically e.g. change the taxes from 1A to 2A (I guess). That way you would have 2 fiscal positions. One empty for South Africa and one import/export with all the conversions for taxes and accounts. No idea if you have time to check this.
  • I see that your tax tags correspond to the VAT report 201 (http://www.sars.gov.za/AllDocs/OpsDocs/SARSForms/VAT201%20-%20Vendor%20Declaration%20-%20Sticky%20Notes.pdf) We can create it in Enterprise.
@h4818

This comment has been minimized.

Copy link
Contributor Author

h4818 commented Mar 21, 2019

@jco-odoo to answer your questions:

  • English is the primary language used in South Africa (we do however have 11 official languages, afrikaans being one of them)
  • Yes, I currently use fiscal positions (depending on the customer requirements) to replace standard tax (sales) with zero rate exports - but I have not added this to the localization. Whilst there is no VAT on exports, not all South African implementations may require this. And very easy to setup a fiscal position manually. Should we add?
  • Yes, the tags correspond to the identifiers on the SARS VAT201 form. I have created the report for enterprise l10nza_reports. Should I do a PR on the Enterprise repo?
@jco-odoo

This comment has been minimized.

Copy link
Contributor

jco-odoo commented Mar 21, 2019

@h4818 Thanks for reacting so quickly.

  • For the fiscal positions, if the clients don't use it, it is because they are just working with partners within South Africa? I would love to add it, but with the goal that if the partner is outside of South-Africa, the taxes and accounts automatically adjust.
  • If you can make a pr with the VAT report for enterprise, that would be cool!
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.