Skip to content

Conversation

@william-andre
Copy link
Contributor

No description provided.

@william-andre william-andre requested a review from a team as a code owner November 24, 2021 16:51
@robodoo
Copy link
Collaborator

robodoo commented Nov 24, 2021

@william-andre william-andre force-pushed the 15.0-accounting-localization--IEL-wan branch 6 times, most recently from 5b47a88 to e6d6464 Compare November 24, 2021 19:13
Copy link

@qdp-odoo qdp-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor cosmetic proposal/question. The rest is fine by me and can be improved latter on.

@qdp-odoo
Copy link

qdp-odoo commented Nov 25, 2021

@odoo/doc-review let us know your opinion on the changes proposed in 8753a51

edit: changed history
14d4676
915e122
271252e
86f7266

Copy link
Contributor

@oco-odoo oco-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Twice's the charm ! 🥳


.. note::

Recommended **xmlid** for the record is `chart_template`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny that the example above doesn't respect that :p Should we even bother recommanding anything here ? It actually makes no difference from a technical point of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is annoying to have l10n_xx.l10nxx_chart_template. (quite repetitive) Would be easier if it was uniform.

.. seealso::
- :ref:`Account References <reference/account_account>`
- :doc:`/applications/finance/accounting/payables`
- :doc:`/applications/finance/accounting/receivables`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why linking the doc about payable and receivable accounts here? To me, they have nothing to do with the creation of an l10n module/CoA.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They help devs to understand the account types when they have to chose the right ones

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had hopes it would explain those user types better, but it does not seem to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, not convinced; to me it's giving a little too much details regarding what we are explaining here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the link to the doc, maybe you'll find it more relevant

account_tax_report_line -> account_account_tag [label="Creates (+ and -)"];
account_tax_report_line -> res_country [label="0..1"];
account_account_tag -> res_country [label="0..1"];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's missing account.tax.report.

We might also want to add account.tax.carryover.line to the drawing, if we really want something complete (not sure it's not too much here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it good like this?

Invoicing (`account`)
---------------------

This is the main module for Invoicing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@william-andre william-andre force-pushed the 15.0-accounting-localization--IEL-wan branch 2 times, most recently from 2122ff6 to 197fd11 Compare November 26, 2021 12:30
Copy link
Collaborator

@Feyensv Feyensv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the idea of an autofield documenter, but the rest of the "tech" changes do not bring much and break current compatibility & maintainability.

Somebody fetching only the documentation repository must be able to build the documentation (which is not the case with your changes).

Also, in all honesty, the current doc on "how to build a localization" is not a howto, but more a kind of "raw checklist" in its current state.
Unless you are already knowledgeable in the inner details of the accounting modules, I have the feeling that you are not really receiving much more help by reading the doc than by reading a localization module directly.

Did you ask newbies in your team to give their POV on this doc?
As developers, we easily believe everything is easy to understand in our own scope and to oversimplify the our explanations.

"Good news: you are not alone on this incomprehension. Bad news: you have to figure it out a bit." ---> What's the use of a documentation if it says "We leave you in your own mess" ?

Comment on lines 7 to 11
MODEL_REFERENCE = {
'account.account.type': 'addons/account/data/data_account_type.xml',
'res.country': 'odoo/addons/base/data/res_country_data.xml',
'res.currency': 'odoo/addons/base/data/res_currency_data.xml',
}
Copy link
Collaborator

@Feyensv Feyensv Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this should be avoided. A sphinx extension should be independent of the local code.
Specify this in the config if really needed, but for me, this kind of reference is not very useful and easily broken.
The dev can easily grep and find the file generating the base data.

Furthermore, the static reference <https://github.com/odoo/odoo/blob/15.0/{reference} below doesn't consider the current version of the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dev can easily grep and find the file generating the base data.

Let's delete the docs then? Devs can find everything from the source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it in the config

Comment on lines 22 to 28
odoo.addons.account.models.account_account_tag.AccountAccountTag,
odoo.addons.account.models.account_tax_report.AccountTaxReportLine,
odoo.addons.account.models.chart_template.AccountAccountTemplate,
odoo.addons.account.models.chart_template.AccountChartTemplate,
odoo.addons.account.models.chart_template.AccountFiscalPositionTemplate,
odoo.addons.account.models.chart_template.AccountGroupTemplate,
odoo.addons.account.models.chart_template.AccountTaxTemplate,
Copy link
Collaborator

@Feyensv Feyensv Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said before, a sphinx extension should be independent and not contain this kind of parameter list.

I'm thinking of a better way to support the custom Odoo Architecture, without this "hack" of a list of classes.
At worst, we drop those cross references. If the file hierarchy is clean, I believe it's still better than this custom list 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the reference system using :ref: and .. autoodooclass:

Comment on lines 39 to 83
annotation[attrname] = dict
field = self.object
if field.type == 'many2one':
annotation[attrname] = int
elif field.type in ('one2many', 'many2many'):
annotation[attrname] = List[odoo.fields.Command]
elif field.type in ('selection', 'reference', 'char', 'text', 'html'):
annotation[attrname] = str
elif field.type == 'boolean':
annotation[attrname] = bool
elif field.type in ('float', 'monetary'):
annotation[attrname] = float
elif field.type == 'integer':
annotation[attrname] = int
elif field.type == 'date':
annotation[attrname] = datetime.date
elif field.type == 'datetime':
annotation[attrname] = datetime.datetime
Copy link
Collaborator

@Feyensv Feyensv Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
annotation[attrname] = dict
field = self.object
if field.type == 'many2one':
annotation[attrname] = int
elif field.type in ('one2many', 'many2many'):
annotation[attrname] = List[odoo.fields.Command]
elif field.type in ('selection', 'reference', 'char', 'text', 'html'):
annotation[attrname] = str
elif field.type == 'boolean':
annotation[attrname] = bool
elif field.type in ('float', 'monetary'):
annotation[attrname] = float
elif field.type == 'integer':
annotation[attrname] = int
elif field.type == 'date':
annotation[attrname] = datetime.date
elif field.type == 'datetime':
annotation[attrname] = datetime.datetime
annotation[attrname] = self.object.__class__

You can directly reference the documented Odoo Field class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? No? That would return the fields class, not the type that we are expecting to input for that field.
When you do

sell.env['res.users'].write({'name': something})

You don't expect something to be an instance of fields.Char, but str

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I expect something to be a char but we're documenting Odoo fields. I want the users to know what's behind a field, not only as value, but also as behavior, implied orm logic, choices of values, ...

If the display of a selection, reference, char, text & html fields is the same, then the autofield documenter doesn't do its job right...
Furthermore, we are in the dev doc, I expect the readers to know how to set the different types of fields (and if they don't, clicking on the class reference will redirect them to the needed documentation, even if some improvements are probably needed in the orm documentation).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely disagree with that. I can add some better typing using the python module with the same name for fields if you want, but I am sure that I don't want fields.Char.
The end user, which might not be a real dev with a lot of experience, but just someone looking to do some RPC calls or to import/export data with the tools, doesn't care about the details of the field.

Comment on lines 5 to 9
=======

.. autoclass:: odoo.addons.account.models.chart_template.AccountAccountTemplate

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide either an introduction in the different rst files, or in the source code for the classes.

Raw tech content without any text should be avoided as much as possible.
It would be way better to have an introduction on the concept/meaning behind the tech model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will add some in the source code. But branches are not synchronized, and we need more time for that part.

Comment on lines 15 to 17
On installing the `account <https://github.com/odoo/odoo/tree/15.0/addons/account>`__ module, the localization module corresponding to the country code of the company is installed automatically.
In case of no country code set or no localization module found, the `l10n_generic_coa <https://github.com/odoo/odoo/tree/15.0/addons/l10n_generic_coa>`__ (US) localization module is installed by default.
Check `post init hook <https://github.com/odoo/odoo/blob/15.0/addons/account/__init__.py>`__ for details.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use static references to github code, it won't be maintained and updated with the documentation versions.
The code is easy to find, this documentation targets developers, it's not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't target devs. And trust me, it is not that easy to find.

To quote you:

Did you ask newbies in your team to give their POV on this doc?
As developers, we easily believe everything is easy to understand in our own scope and to oversimplify the our explanations.

And yes. I asked newbies.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it doesn't target devs, why is it in the developer/ section then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It targets devs, but some are more "hardcore" than others. We often have PRs of ppl who can barely use a version control system for instance. Or devs du dimanche who want to help building a localization for their country, who know how to write xml but are afraid of digging in the code.

:language: csv
:end-at: ch_coa_1171

CSV is prefered but you may use XML format instead.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little note on the perf diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is none (tested one or two years ago, after 13.0).
It is just for readability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, csv records are created in batch, which isn't the case for xml records 🤔

@AntoineVDV AntoineVDV self-requested a review November 29, 2021 10:47
Comment on lines 22 to 19
.. autofield:: bank_account_code_prefix
.. autofield:: cash_account_code_prefix
.. autofield:: transfer_account_code_prefix
Copy link
Contributor

@yelizariev yelizariev Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, you lost my notes about those fields. The information from autofield is not enough

Current version

image

My version

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will create a PR that adds help on the fields. It wouldn't be synchronized anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 28 to 25
.. autofield:: account_journal_suspense_account_id
.. autofield:: account_journal_payment_debit_account_id
.. autofield:: account_journal_payment_credit_account_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same about suspense accounts. Don't make conspiracy about it. People should know the truth

Current version

image

My version

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will create a PR that adds help on the fields. It wouldn't be synchronized anyway.

Copy link
Contributor

@yelizariev yelizariev Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why won't it be synchronized ? You plan to add help to master branch, right?
We either need such PR to odoo/odoo v15 or keep fields description in odoo/documentation v15 for a while (and replace it with generated docs in v16). I don't like the idea to hide the knowledge and enlighten new developers during special ceremony with accounting gurus only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't synchronized because runbot team says it is too difficult to do to have a batch containing both the code and the doc

Comment on lines 157 to 168
.. example::
`addons/l10n_ch/data/l10n_ch_chart_post_data.xml <https://github.com/odoo/odoo/blob/15.0/addons/l10n_ch/data/l10n_ch_chart_post_data.xml>`__.

.. literalinclude:: {ODOO_RELPATH}/addons/l10n_ch/data/l10n_ch_chart_post_data.xml
:language: xml
:start-at: l10nch_chart_template
:end-at: </record>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example gives 7 fields only, while it's 21 fields

  <!-- Receivable/payable accounts. -->
  <field name="property_account_receivable_id" ref="..."/>
  <field name="property_account_payable_id" ref="..."/>

  <!-- Default Income/Expense Account for a Product Template. -->
  <field name="property_account_income_id" ref="..."/>
  <field name="property_account_expense_id" ref="..."/>

  <!-- Default Income/Expense Account for a Product Category. -->
  <field name="property_account_income_categ_id" ref="..."/>
  <field name="property_account_expense_categ_id" ref="..."/>

  <!-- Cash loss/gain accounts. They correspond profit_account_id / loss_account_id fields -->
  <field name="default_cash_difference_income_account_id" ref="..."/>
  <field name="default_cash_difference_expense_account_id" ref="..."/>

  <!-- Loss/gain exchange rate accounts. -->
  <field name="income_currency_exchange_account_id" ref="..."/>
  <field name="expense_currency_exchange_account_id" ref="..."/>

  <!-- Stock valuation accounts. -->
  <field name="property_stock_account_input_categ_id" ref="..."/>
  <field name="property_stock_account_output_categ_id" ref="..."/>
  <field name="property_stock_valuation_account_id" ref="..."/>

  <!-- Tax accounts to  balance current/advance tax payments. -->
  <!-- It's used in `env['account.generic.tax.report']._generate_tax_closing_entries` -->
  <field name="property_tax_payable_account_id" ref="..."/>
  <field name="property_tax_receivable_account_id" ref="..."/>
  <field name="property_advance_tax_payment_account_id" ref="..."/>


  <!-- Base Tax Received Account -->
  <!-- Corresponds to `account_cash_basis_base_account_id` field. -->
  <!-- It's used for taxes with `tax_exigibility` equal to `on_payment` -->
  <field name="property_cash_basis_base_account_id" ref="..."/>

  <!-- POS account. -->
  <field name="default_pos_receivable_account_id" ref="..."/>

  <!-- Payment accounts. -->
  <!--
       Suspense account is used to register an unreconiled bank entries created via bank statement.
       Once it's reconciled, the suspense account in `account.move` will be replaced with a proper account.
       Suspense account is an exceptional case when account is supposed to be replaced in the transaction.
       The new account depends on `account.payment` records. There are two cases
       1. `account.payment' exists before reconciliation aka Blue lines in reconciliation wizard
       2. `account.payment' is created on reconciliation aka regular Black lines in reconciliation wizard

       For case n.1 the new account is Outstanding Receipts/Payments account and the workflow is following:

       Initial transactions:
       * Invoice/bill: Payable/Receivable vs Expenses/Sales
       * Payment:             Outstanding vs Payable/Receivable
       After creating bank statement
       * Bank entry:                 Bank vs Suspense
       After reconciliation:
       * Bank entry:                 Bank vs Outstanding

       For case n.2 the new account is Payable/Receivable account from invoice/bill and the workflow is following:

       Initial transactions:
       * Invoice/bill: Payable/Receivable vs Expenses/Sales
       After creating bank statement
       * Bank entry:                 Bank vs Suspense
       After reconciliation:
       * Bank entry:                 Bank vs Payable/Receivable
  -->
  <field name="account_journal_suspense_account_id" ref="..."/>
  <!-- Outstanding Receipts Account -->
  <field name="account_journal_payment_debit_account_id" ref="..."/>
  <!-- Outstanding Payments Account -->
  <field name="account_journal_payment_credit_account_id" ref="..."/>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current version

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you can't put them all at the same time, you first need to create the accounts, look below in the HowTo, where the steps are done in the right order

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably misunderstand me. I'm talking about post-data file, not about the first file account_chart_template_data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have the reference for that, no need to put an example with every single account set. I'm not even sure it exists.

Copy link
Contributor

@jco-odoo jco-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot still missing. The transfer and suspense accounts would be good to explain in a more functional documentation as well. (I like the way it is explained by @yelizariev and you can not just add that easily in the code)

.. seealso::
- :ref:`Account References <reference/account_account>`
- :doc:`/applications/finance/accounting/payables`
- :doc:`/applications/finance/accounting/receivables`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had hopes it would explain those user types better, but it does not seem to.


Next settings for the chart of accounts are set in a separate file, because we need to provide `list of accounts <#accounts>`__ first. In `data/account_chart_post_data.xml` we set some default accounts:

.. todo add reference to account_id in CoA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still todo? (or can we refer to chart_template.py?)

@william-andre william-andre force-pushed the 15.0-accounting-localization--IEL-wan branch 4 times, most recently from 0f05b0f to 3ad6130 Compare December 1, 2021 11:39
@qdp-odoo
Copy link

qdp-odoo commented Dec 2, 2021

@Feyensv all good for us, can you give a r+ ?

thanks

@AntoineVDV AntoineVDV force-pushed the 15.0-accounting-localization--IEL-wan branch from 65c8975 to 9f01bd6 Compare May 4, 2022 10:41
@C3POdoo C3POdoo requested a review from a team May 4, 2022 10:42
@AntoineVDV AntoineVDV removed the request for review from a team May 4, 2022 10:45
william-andre and others added 4 commits May 6, 2022 11:37
Also add a condition on some directives to ignore them when we have no
relative/absolute path.
This new directive is generating documentation from Odoo fields. This
can be used to build documentation about business classes.
This will help developpers import/export data and build localization
modules for instance.
Co-Authored-By: William Andre <wan@odoo.com>
Co-Authored-By: Ivan Yelizariev <iel@odoo.com>
@william-andre william-andre force-pushed the 15.0-accounting-localization--IEL-wan branch from 9f01bd6 to 87dd6f6 Compare May 6, 2022 09:40
Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for confirmation that it will build fine on odoo.com before merging.

@AntoineVDV
Copy link
Collaborator

@robodoo r+

@william-andre
Copy link
Contributor Author

@robodoo rebase-ff ? 🙏

@robodoo
Copy link
Collaborator

robodoo commented May 6, 2022

I'm sorry, @william-andre. You can't method=rebase-ff.

@jco-odoo
Copy link
Contributor

jco-odoo commented May 6, 2022

@AntoineVDV

@robodoo
Copy link
Collaborator

robodoo commented May 6, 2022

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward

@AntoineVDV
Copy link
Collaborator

@robodoo rebase-ff

@robodoo
Copy link
Collaborator

robodoo commented May 6, 2022

Merge method set to rebase and fast-forward

robodoo pushed a commit that referenced this pull request May 6, 2022
Also add a condition on some directives to ignore them when we have no
relative/absolute path.

Part-of: #1334
robodoo pushed a commit that referenced this pull request May 6, 2022
This new directive is generating documentation from Odoo fields. This
can be used to build documentation about business classes.
This will help developpers import/export data and build localization
modules for instance.

Part-of: #1334
robodoo pushed a commit that referenced this pull request May 6, 2022
@robodoo robodoo closed this in 2da1f72 May 6, 2022
@robodoo robodoo temporarily deployed to merge May 6, 2022 16:53 Inactive
AntoineVDV pushed a commit that referenced this pull request May 9, 2022
Also add a condition on some directives to ignore them when we have no
relative/absolute path.

Part-of: #1334
AntoineVDV pushed a commit that referenced this pull request May 9, 2022
This new directive is generating documentation from Odoo fields. This
can be used to build documentation about business classes.
This will help developpers import/export data and build localization
modules for instance.

Part-of: #1334
AntoineVDV pushed a commit that referenced this pull request May 9, 2022
@fw-bot fw-bot deleted the 15.0-accounting-localization--IEL-wan branch May 20, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.