Skip to content

Conversation

@samueljlieber
Copy link
Contributor

@samueljlieber samueljlieber commented Jul 8, 2022

Task ID: #2908400

@robodoo
Copy link
Collaborator

robodoo commented Jul 8, 2022

@samueljlieber samueljlieber requested a review from mivu-odoo July 8, 2022 14:59
@samueljlieber samueljlieber self-assigned this Jul 8, 2022
@StraubCreative StraubCreative removed the request for review from mivu-odoo July 8, 2022 17:01
@samueljlieber
Copy link
Contributor Author

Hey @StraubCreative 😄
This doc is ready for a preliminary review. Please let me know if you have any suggestions for improvement!

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @samueljlieber

Here's the first batch of changes I'd like to see. A lot of the suggestions revolve mostly around the written content itself, with only a few RST corrections, namely:

  • Grammar, wording, and sentence structure
  • Suggestions to improve written context (and not rely so much on images)
  • Improvements around content placement i.e. text goes before images.

cc: @vbe-odoo

@samueljlieber
Copy link
Contributor Author

Hi @StraubCreative

Thanks for all of the info in each suggestion! I made all of the requested changes except for items that need content/information from the l10n team. I left those items unresolved and I will work with the team to get the content :)

cc: @vbe-odoo

@StraubCreative
Copy link
Contributor

StraubCreative commented Aug 25, 2022

Looks good @samueljlieber. Very quick work 🙂

Two errors I found which are contributing to the failed ci/doc check.

  1. content/applications/finance/accounting/fiscal_localizations/localizations/argentina.rst:: WARNING: Anonymous hyperlink mismatch: 1 references but 0 targets. See "backrefs" attribute for IDs.

  2. content/applications/finance/accounting/fiscal_localizations/localizations/argentina.rst:: WARNING: image file not readable: applications/finance/accounting/fiscal_localizations/localizations/argentina/bank-account-relation-error.png

One is a hyperlink issue (not properly formatted). The other is a failed image reference (e.g. image missing, file path wrong in RST, etc.)

Be sure to run make html before every push to avoid failed CI checks like this ✅

@samueljlieber
Copy link
Contributor Author

samueljlieber commented Aug 30, 2022

Hi @vbe-odoo,
I removed the sentence referencing the common errors section that currently doesn't exist. I also updated the Get AFIP Certificate link :)

@StraubCreative's remaining content suggestions:

  1. Sentence is unfinished
  2. Sentences don't go deep enough on context
  3. Needs more written content
  4. Missing written content
  5. Missing written content
  6. Missing written content

@samueljlieber
Copy link
Contributor Author

Hi @gza-odoo 👋 thank you for making these suggestions!
@StraubCreative I made the changes, ready for another review. Thank you! :)

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Great first round of revisions everyone. The doc looks much better and more readable than where it started 😄

Thank you @samueljlieber for the speedy updates and @gza-odoo for the content suggestions.

Here's the next batch of changes which mostly include some more grammar/formatting suggestions in trying to tighten things up.

I think after these are implemented, we can open the PR up to the Accounting team 👍

@samueljlieber
Copy link
Contributor Author

Commit 88a5326 updated document per ZST's suggestions (Thank you 🙏). Remaining suggestions:

@StraubCreative
Copy link
Contributor

We need just two quick things here @gza-odoo @vbe-odoo
Please see @samueljlieber comment above (click the blue links in the list which will take you to each comment describing what's needed).

After those I think we can open the PR to Accounting team 🚀

@samueljlieber
Copy link
Contributor Author

Hi @StraubCreative, I updated the two points in my previous comment with the comments the team provided, thank you @vbe-odoo and @gza-odoo!

Regarding the typo that says assignes - that is no longer present 👍

Also regarding @gza-odoo's comment I made a small change for context clarity:
type of document that does not detail the taxes. They are included in the total amount.
to
type of document that does not detail the taxes, since the taxes are included in the total amount.
Please let me know if this is incorrect :)

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

@gza-odoo thank you for the content updates!

@samueljlieber nice catch with the capitalizations in the list items. Let's go one step further and remove the nested colons from the list.

After this is edited we can make the PR live for Accounting team to review 🚀

@gza-odoo
Copy link

I'm ok with what you suggest @samueljlieber :)

@samueljlieber
Copy link
Contributor Author

Hi @StraubCreative, your edits have been made 👍

@StraubCreative
Copy link
Contributor

Thanks @samueljlieber for the quick changes 🙏

I'm going to open this up to Accounting for review now.

Note:
On the next batch of change requests, let's fix the commit message to [IMP] l10n: AR update documentation so it's country-specific and reflects the original commit for the PR.

@samueljlieber
Copy link
Contributor Author

Thank you @toaa-odoo 😀 your suggestions have been implemented in 15b7bab.

Hi @vbe-odoo and @gza-odoo can you help me determine how we should label Argentinian/Argentinean throughout the document per @toaa-odoo's point:

I noticed in screenshot content/applications/finance/accounting/fiscal_localizations/localizations/argentina/select-fiscal-package.png that Argentinian is spelled 'Argentinean localization'. Both are correct ways to spell the adjective, but if you were multiple working on this, you might have spelled it differently within the module. Check among yourselves that you indeed used the same spelling throughout.

Thank you! ⭐️

@vbe-odoo
Copy link
Contributor

vbe-odoo commented Oct 17, 2022

Hi @vbe-odoo and @gza-odoo can you help me determine how we should label Argentinian/Argentinean throughout the document per @toaa-odoo's point:

I noticed in screenshot content/applications/finance/accounting/fiscal_localizations/localizations/argentina/select-fiscal-package.png that Argentinian is spelled 'Argentinean localization'. Both are correct ways to spell the adjective, but if you were multiple working on this, you might have spelled it differently within the module. Check among yourselves that you indeed used the same spelling throughout.

Hi @samueljlieber I think we should use this one "Argentinean localization" as is the one that we have in the database and on the screenshots, as both are OK, let's keep the one we have been using when installing l10n_ar in a database, as it will more difficult to change the name there (and on the screenshots) than in this DOC.

Let me know what are your thoughts on this.
Thanks!
cc' @gza-odoo @fvz-odoo @ren-odoo @toaa-odoo

@samueljlieber
Copy link
Contributor Author

@vbe-odoo I think your suggestion is the way to go - I will update the document to use "Argentinean localization". Thanks!

@samueljlieber
Copy link
Contributor Author

Commit a02776e to update Argentinian to Argentinean throughout the document 🙂

CC: @toaa-odoo

Copy link
Contributor

@toaa-odoo toaa-odoo left a comment

Choose a reason for hiding this comment

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

Great work, you won't hear from me again! :)

Tagging @jcs-odoo for final review.

@toaa-odoo toaa-odoo requested review from a team and jcs-odoo and removed request for jcs-odoo October 19, 2022 14:33
@xpl-odoo xpl-odoo requested review from jcs-odoo and removed request for a team November 28, 2022 13:43
@xpl-odoo
Copy link
Contributor

xpl-odoo commented Nov 28, 2022

@jcs-odoo wanted to review it

@StraubCreative
Copy link
Contributor

53b451c addressed merge conflict on prior commit a02776e after #2983 went into effect.

Ready for your review with all checks passing @jcs-odoo

Copy link
Contributor

@jcs-odoo jcs-odoo left a comment

Choose a reason for hiding this comment

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

Apologies for this huge delay 🙈

I didn't go into detail for the whole doc but in general, I think this doc could refer more to the rest of the doc. Normally, we would add doc links or custom anchors to specific headings in "see also" sections.

Also, I think that there are too many images, but it's also okay like that for now. I saw that they were not compressed with pngquant and that you forgot to delete the old images that are not useful anymore.

I already applied these changes and a few typos I caught here and there. I'll push them on this branch in a few minutes


- `VIDEO WEBINAR OF A COMPLETE DEMO <https://youtu.be/c41-8cVaYAI>`_.
- `ECOMMERCE <https://youtu.be/5gUi2WWfRuI>`_.
- `VIDEO WEBINAR V15 <https://www.youtube.com/watch?v=_H1HbU-wKVg>`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using versions in the content. The version switcher in the doc indicates the user the version of the current page, but typing this in the page means that it is ported to other versions and requires more maintenance of the doc.

Also, we should write "Odoo 15" instead of "v15"

Comment on lines 48 to 49
Configure your company
~~~~~~~~~~~~~~~~~~~~~~
Copy link
Contributor

Choose a reason for hiding this comment

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

These headings' level seem weird.
I don't see why "configure your company" and "chart of accounts" are listed as a sublevel of "install the argentinean.

Suggested change
Configure your company
~~~~~~~~~~~~~~~~~~~~~~
Configure your company
----------------------

Comment on lines 13 to 46
Introduction
============

The Argentinean localization has been improved and extended in Odoo v13, in this version the next
modules are available:
The following modules are necessary for all databases that require Argentinean localization:

- **l10n_ar**: This module add accounting features for the Argentinian localization, which represent
the minimal configuration needed for a company to operate in Argentina and under the AFIP
(Administración Federal de Ingresos Públicos) regulations and guidelines.
- **l10n_ar**: adds accounting features for the Argentinean localization, which represent the
minimal configuration needed for a company to operate in Argentina and under the AFIP
(Administración Federal de Ingresos Públicos) regulations and guidelines;

- **l10n_ar_reports**: Add VAT Book report which is a legal requirement in Argentine and that holds
the VAT detail info of sales or purchases recorded on the journal entries. This module includes as
well the VAT summary report that is used to analyze the invoice
- **l10n_ar_reports**: adds the VAT Book report which is a legal requirement in Argentina and that
holds detailed VAT information of sales or purchases recorded on the journal entries. This module
includes the VAT summary report as well, which is used to analyze the invoice;

- **l10n_ar_edi**: This module includes all technical and functional requirements to generate
Electronic Invoice via web service, based on the AFIP regulations.
- **l10n_ar_edi**: includes all technical and functional requirements to generate Electronic
Invoices via web service, based on the AFIP regulations.

The following module is optional and should be installed only if it meets a specific organization
requirement:

- **l10n_ar_website_sale**: allows the user to see Identification Type and AFIP Responsibility in
the eCommerce checkout form in order to create electronic invoices.

Configuration
=============

Install the Argentinean localization modules
--------------------------------------------

For this, go to *Apps* and search for Argentina. Then click *Install* for the first two modules.
Go to :guilabel:`Apps` and search for `l10n_ar` either in the apps/module list or by using the
search box. Then click :guilabel:`Install` for all of the Argentinean Modules.

.. image:: argentina/argentina01.png
.. image:: argentina/install-argentinian-l10-modules.png
:align: center
:alt: Install Argentinean localization modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • This is not an introduction and is rather part of the configuration.
  • I suggest using the same format as the latest localization pages to indicate which module to install

No need to explain everything everytime in the doc. It is best to add a link to the doc that explains how to install a module rather than explaining it on every page, with a screenshot.

I suggest replacing line 13 to 46 with this (and deleting the screenshot)

Configuration
=============

Modules installation
--------------------

:ref:`Install <general/install>` the following modules to get all the features of the Argentinean
localization:

.. list-table::
   :header-rows: 1
   :widths: 25 25 50

   * - Name
     - Technical name
     - Description
   * - :guilabel:`Argentina - Accounting`
     - `l10n_ar`
     - Default :doc:`fiscal localization package <../overview/fiscal_localization_packages>`, which
       represents the minimal configuration to operate in Argentina under the :abbr:`AFIP
       (Administración Federal de Ingresos Públicos)` regulations and guidelines.
   * - :guilabel:`Argentinean Accounting Reports`
     - `l10n_ar_reports`
     - VAT Book report and VAT summary report.
   * - :guilabel:`Argentinean Electronic Invoicing`
     - `l10n_ar_edi`
     - Includes all technical and functional requirements to generate electronic invoices via web
       service, based on the AFIP regulations.
   * - :guilabel:`Argentinean eCommerce`
     - `l10n_ar_website_sale`
     - (optional) Allows the user to see Identification Type and AFIP Responsibility in the
       eCommerce checkout form in order to create electronic invoices.

@jcs-odoo
Copy link
Contributor

jcs-odoo commented Dec 5, 2022

Pinging for merge already so we can move forward. We can still improve afterwards if needed :)

@jcs-odoo jcs-odoo requested a review from a team December 5, 2022 14:19
@StraubCreative
Copy link
Contributor

Thanks for the review @jcs-odoo
It seems like some of your suggestions would benefit from the new model/structure introduced in #2659.
So we'll push that once next and then take all of your feedback into a new PR once both are merged.

cc: @samueljlieber

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.

Comment on lines +89 to +90
Localization` and choose either :guilabel:`Prueba (Testing)` or :guilabel:`Produccion (Production)`
.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Localization` and choose either :guilabel:`Prueba (Testing)` or :guilabel:`Produccion (Production)`
.
Localization` and choose either :guilabel:`Prueba (Testing)` or :guilabel:`Produccion (Production)`.

:alt: A list showing less common Argentinean tax options, which are labeled as inactive in Odoo
by default.

.. _document-types:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would conflict with any other document-types anchor added later in the doc, which is likely to happen.

Suggested change
.. _document-types:
.. _localizations/argentina/document-types:

Argentina case: AFIP).
In some Latin American countries, like Argentina, some accounting transactions such as invoices and
vendor bills are classified by document types defined by the governmental fiscal authorities. In
Argentina, the `AFIP <https://www.afip.gob.ar/>`__ is the governmental fiscal authority that
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use single underscores in links.

Suggested change
Argentina, the `AFIP <https://www.afip.gob.ar/>`__ is the governmental fiscal authority that
Argentina, the `AFIP <https://www.afip.gob.ar/>`_ is the governmental fiscal authority that

- Condition applied based on the type of Issues and Receiver (ex. Type of fiscal regimen of
the buyer and type of fiscal regimen of the vendor)
- The journal entry related to the invoice (if the journal uses documents);
- The onditions applied based on the type of issuer and receiver (e.g., the type of fiscal regime of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The onditions applied based on the type of issuer and receiver (e.g., the type of fiscal regime of
- The conditions applied based on the type of issuer and receiver (e.g., the type of fiscal regime of

:align: center
For auditing and troubleshooting purposes, it is possible to obtain detailed information of an
invoice number that has been previously sent to the AFIP. To retrieve this information, go into
:doc:`Developer Mode <../../../../general/developer_mode>`, then go to the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either:

  • use doc and start from the root to limit the number of ../: /applications/general/developer_mode
  • use ref (exceptionally) with the target developer-mode.

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.

9 participants