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

[IMP] accounting/l10n_co: Update documentation for DIAN module #11315

Closed
wants to merge 1 commit into from

Conversation

masi-odoo
Copy link
Contributor

This commit improves the official documentation to support the strategy to focus on the new DIAN electronic invoicing module on V18 (leaving notes and references for clients still using Carvajal in this version)

Drive Folder with images: https://drive.google.com/drive/folders/11cXF1gXxwws_P9hUslnSJcfjheEbBJZN

@robodoo
Copy link
Collaborator

robodoo commented Nov 1, 2024

Pull request status dashboard

@samueljlieber samueljlieber changed the base branch from 18.0 to 17.0 November 4, 2024 21:33
@samueljlieber samueljlieber changed the base branch from 17.0 to 18.0 November 4, 2024 21:33
Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

Hi @masi-odoo thank you for your work on this PR! I added your images and made some technical changes regarding our RST guidelines, as well as expanded on some instructions for more context.

Please review my changes and suggestions and let me know if there is anything else you would like to edit.

Here is the built documentation for my changes: http://runbot136.odoo.com/runbot/static/build/70201010-18-0/logs/build/html/applications/finance/fiscal_localizations/colombia.html

@C3POdoo C3POdoo requested a review from a team November 5, 2024 14:35
Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

Thank you @masi-odoo for your feedback on my suggestions, I implemented all! I am approving this PR and passing it over to the review team :)

@samueljlieber
Copy link
Contributor

Thanks @masi-odoo, the report images have been updated in 882aa14 :)

@masi-odoo
Copy link
Contributor Author

Awesome @samueljlieber. I think we are all set for the next review then!

Thanks again!

Copy link
Contributor

@afma-odoo afma-odoo left a comment

Choose a reason for hiding this comment

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

Hi @masi-odoo! Thank you for your work on this Mexican localization page! I added some comments/suggestions. Please take a look at it and let me know if you have any questions. Thanks!

@masi-odoo
Copy link
Contributor Author

Hello @afma-odoo thank you so much for the review! It certainly improves the documentation

@samueljlieber Is it possible you can take a look at the comments I added to review best practices?

Everything else I marked it with a 👍 because I agree with the improvements.

Let me know if I can help in something!

Copy link
Contributor

@afma-odoo afma-odoo left a comment

Choose a reason for hiding this comment

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

@samueljlieber Thank you for the update. Please let me know if you have any questions ;)

@afma-odoo
Copy link
Contributor

@samueljlieber There seems to be a problem with headings, as some checks (documentation & documentation_guidelines) were not successful:

  • "Invoice creation" and "Sending electronic invoices" headings are not visible in HTML.
  • The heading 5 format is used ("Identification information", "Fiscal information", ...) after the heading 3 format ("Master data configuration"). Please make sure the order follows the one available on the documentation guideline page. Thanks!

@samueljlieber
Copy link
Contributor

Thank you @afma-odoo for pointing out these changes that I missed, sorry about that! All have been addressed in dc1ddd8.

Thanks again!

Copy link
Contributor

@afma-odoo afma-odoo left a comment

Choose a reason for hiding this comment

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

Thank you @samueljlieber for the updates. I force-pushed some changes to fix minor issues. I approve the PR and add the be-doc review.
Thanks again!

@afma-odoo afma-odoo requested a review from a team November 15, 2024 11:10
Copy link
Contributor

@auva-odoo auva-odoo left a comment

Choose a reason for hiding this comment

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

Hello @masi-odoo I pushed some changes, coud you please check?
Main changes done:

  • I think it makes sense to group the Electronic invoice credentials and DIAN environment configuration sections together since the configurations are actually done in the same place.
  • Same for the Identification information and Fiscal information under *Master data: unless I'm mistaken, those are both related to contacts so I would group them together and clarify exactly what is needed from the user. I pushed a suggestion, can you please check it's correct?
    -I also rewrote some sections for better clarity, focusing on what the user needs to do directly and reducing repetition by adding cross-references where applicable.
  • I don't understand the example for the invoice sequence (in the Invoice creation section, on line 325) --> I have not fixed this yet. Could you please check this?

Thank you!

@masi-odoo
Copy link
Contributor Author

Hello @auva-odoo I agree with the changes you made to the documentation, thanks for these improvements!

This commit improves the official documentation to support the strategy to focus on the new DIAN electronic invoicing module on V18 (leaving notes and references for clients still using Carvajal in this version)
@auva-odoo
Copy link
Contributor

auva-odoo commented Nov 22, 2024

Thanks @masi-odoo! and thank you @samueljlieber and @afma-odoo for your work 🙂

@robodoo r+

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.

5 participants