-
Notifications
You must be signed in to change notification settings - Fork 10.7k
[IMP] l10n: Chile new features #4317
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
Conversation
samueljlieber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vbe-odoo 👋
I opened this PR as a draft for your review as well as to work out some edits 🙂
I had to update all of the image filenames from what was written in the Google Doc to match our documentation guidelines, please refer to this document to see the filename changes also I had to update a few of the alt tags on images because there were duplicates.
I resized and compressed all of the images but there are a few that could be retaken to be more legible:
- locate-propuesta-f28-report.png (chile_edi_49_SP.png)
- credit-note-document-type.png (chile_edi_29_SP.png)
- chilean-cafs.png (chile_edi_15_SP.png)
Please let me know your thoughts and if you have any other changes 🙂 Thank you!
6ef14ff to
1ef55d1
Compare
|
Fixed build error in 1ef55d1. |
1ef55d1 to
2bc449e
Compare
|
In 2bc449e I updated images provided by @masi-odoo and performed a full technical review of the PR. Opening this PR up for final review @StraubCreative 🙂 |
toaa-odoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samueljlieber, this is a long PR, thanks for you work! I made a few comments and suggestions (which are up to you), however there is one recurrent point:
- When explaining that the user should enable one feature or change a field, I think it would be good to also explain where the user can find this feature. Considering it is a localization, we can assume this would be a first set-up, and possibly new users without the knowledge of where to find what.
Don't hesitate if you have any questions, and again, thanks a lot for this (very) long PR! :)
2bc449e to
df03044
Compare
|
Hi @toaa-odoo, thank you so much for the in-depth review! In df03044 I made the changes you commented/suggested for each of the resolved conversations. The following are still open because I need @masi-odoo help with the content:
I will push up another commit with these last few items, and then I will request your review again. Thank you! 🙂 |
df03044 to
65abb18
Compare
|
Hi @toaa-odoo this PR is ready for another look, @masi-odoo (thank you! 🙂) and I addressed the remaining suggestions you had in commit 65abb18. Thank you all for the continued effort on this PR 🙏 |
65abb18 to
b366846
Compare
jcs-odoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samueljlieber , I'm just passing by :)
Some images seem not compressed.
Have a good day!
b366846 to
768220b
Compare
cfdac02 to
eeae645
Compare
|
@xpl-odoo can you review this, please? I just changed slightly the structure... You can check my changes and see if they make sense, otherwise, feel free to revert them. Thank you a lot! |
|
Hi @samueljlieber! |
aa781e6 to
631d8c1
Compare
|
Thank you @xpl-odoo for your comments! I worked with @masi-odoo to implement them in 631d8c1. I will now tag @odoo/us-doc-review for a final look 🙏 |
|
cc: @erla-odoo CL l10n documentation 😄 |
StraubCreative
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really great improvement to what we had before. Well done 👏
For the sake of time I need to pause my review here for the moment. Below you'll find a few things I caught which I'll be pushing up the changes for in another commit. They are as follows:
- specificity around headings and some body content
- center-aligned all images
- h3 --> h2 formatting for "configuration" headings since it makes them visually easier to read and find and more closely follows the Document Structure guidelines. I left Usage and testing and Financial reporting sections collapsed, as they were since they are advanced reading that can be clicked open as needed by the reader.
- admonition block added by one of our l10n experts regarding email servers
I think we could use another pass around specificity of language and navigational information.
- There are times where there are good UI instructions, but it's unclear where we actually are in Odoo
- There are times where we're skipping a step or not actually saying what things are.
Example:
Open the :menuselection:`Contacts` app to do so and fill in the following fields:
You can't actually do this instruction above without first specifying to open a Contacts record and then editing the record's the form. 😅
I'm going to pull in some other writers on my team to review the remaining contents and help close this out.
Again, great work, we're almost done I think 👍
| Tributarios Electrónicos)` incoming email server. To do so, click :guilabel:`Configure DTE Incoming | ||
| Email`. Then, click :guilabel:`New` to add a server and fill in the following fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just spoke to a CL l10n expert here in SF office. We need this important admonition here as we receive a lot of support tickets in relation to this cc: @erla-odoo
| Tributarios Electrónicos)` incoming email server. To do so, click :guilabel:`Configure DTE Incoming | |
| Email`. Then, click :guilabel:`New` to add a server and fill in the following fields: | |
| Tributarios Electrónicos)` incoming email server. | |
| .. important:: | |
| In order to receive your SII documents, it's necessary to set up your own email server. | |
| More information on how to do this can be found in this documentation: | |
| :doc:`../../general/email_communication/email_servers` | |
| Begin by, clicking :guilabel:`Configure DTE Incoming Email`. Then, click :guilabel:`New` to add a | |
| server and fill in the following fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StraubCreative the instructions in this section are for setting up an Incoming Email Server, I am not sure if linking to the email_servers.rst doc is correct since the instructions in the DTE incoming email server section are specific to l10n_CL.
I agree that an Important admonition is required to highlight this step, but the instructions are already within this section, no?
cc: @masi-odoo @erla-odoo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could leave it without the link, I suggested having it because some people do want more information regarding setting up the email servers so having the link would lower the amount of tickets submitted for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erla-odoo I will leave the link then! I wanted to make sure the link was relevant! 🙂 Thanks!
631d8c1 to
39d00d8
Compare
|
39d00d8: addressed CRs from last review Feel free to Resolve conversation on individual change requests as you read them (since they should be fixed now) |
larm-odoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello- I have some suggestions just to clarify the information a bit.
|
Thanks for the help @larm-odoo ! |
8cc501a to
afa8fa5
Compare
|
Thank you @larm-odoo! Hi @Felicious can you please review, Lara got to about line 300, so feel free to start there 🙏 |
Felicious
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samueljlieber! Just finished adding comments to lines 225 - 744. Once I saw I had to install the Delivery guide module, I figured it was time to stop for today.
Great job on this thicc-y doc-- I only had minor wording suggestions and a few places where I wasn't sure where we were, that I pointed out in my comments. I can probably tackle another chunk on another day, but for now, my brain is checked out. It's amazing how you made all of these changes and added all of these new screenshots. The doc is in a great place! The sections are clear and the writing flows well between the sections. I can tell you put in a lot of effort 😊
b7a10e7 to
66bfeb2
Compare
|
Thank you for the review @Felicious! I implemented most of your suggestions, all were very helpful 🙂 |
StraubCreative
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samueljlieber @masi-odoo @vbe-odoo
Approving with comments so we can get this in the hands of Chilean users. We can deploy the updates as they are, however I have a number of items to look at, listed below, perhaps more fitting for a separate PR. The most consistent items I saw revolved around grammar and lean content (the usual suspects 😅)
Your call @samueljlieber 🤙
@robodoo delegate=samueljlieber
| - Taxes | ||
| - Default Account Payable | ||
| - Default Account Receivable | ||
| - Transfer Accounts | ||
| - Conversion Rate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these get gui or menu labels?
| information required by it. | ||
|
|
||
| A list of daily reports is displayed with all daily DTE sent to SII. | ||
| Contact configurations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no written content here?
Co-authored-by: masi-odoo <masi@odoo.com> Co-authored-by: StraubCreative <zst@odoo.com> Co-authored-by: larm-odoo <121518652+larm-odoo@users.noreply.github.com> Co-authored-by: Felicia Kuan <feku@odoo.com>
66bfeb2 to
f83484b
Compare
|
Thank you for the thorough review @StraubCreative, I implemented all of the resolved suggestions in f83484b and left the ones that I will need to work on in another PR unresolved. c.c. @masi-odoo @robodoo r+ |
|
@samueljlieber this pull request has forward-port PRs awaiting action (not merged or closed): |
Task: #3296044
This PR improves the 16.0 Chilean Localization documentation.