-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[IMP] accounting: add Hong Kong payroll #8204
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
hi @odoo/payroll-doc-review , I let you handle this one as the content is only local payroll specific, no need for accounting localization review :) |
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- after reviewing the doc, there are just a few universal things to keep in mind. Anything that is on the UI, especially that is clicked, should have a guilabel. Also, it is best practice to avoid using future tense, and the second person. For example: "You can click...." "This will allow you...", etc. I am unable to make comments on the grayed out sections of the doc, but there were places there as well that need to have these adjustments. Also, anytime an application is referenced, it should be in italics instead of bold (Attendances not Attendances). Also, ensure anything that is in a guilabel is exactly how it appears on the screen- that includes the names of applications, all fields, buttons, etc. I hope this review was helpful!
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
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 a lot of information in this doc! I can see how it has improved along the way. My review is ready- if you have any questions, please let me know. Someo of these edits are just suggestions, so let me know if you need any clarifiction.
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
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.
Document is in great sape- I mostly fixed all the build errors in this review. There were a lotof extra spaces at the ends of lines, and ther eneeds to be a blank line at the end of the doc as well. All changes are techinical- and I am approving now, since once these are implemented, this doc can go to the next stage.
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
HI @larm-odoo , commited the extra spaces fixes. Thanks for taking the time to review the documentation. Appreciate the feedback! |
Always happy to help! |
9fc023e
to
c64bdc4
Compare
Hello @larm-odoo. I fixed the remaining formatting issue and all green now. Could you help to r+ this PR so we could merge it? Thanks! |
Hello @larm-odoo , any updates on this PR? |
@robodoo r+ |
I'm sorry, @larm-odoo. I'm afraid I can't do that. |
Apologies @maya-odoo - I missed pushing this- this is in the que now! I'll make sure this goes live all the way to master. |
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.
@maya-odoo -- hello. So, during my Final Review, I noticed a lot of repeated mistakes and formatting errors that simply must be changed before this can move to the next stage of the process. I have pointed them out in my comments below -- most of which need to be implemented throughout the doc -- so, if I only mentioned it once, it's because it would be quite tedious to point out every instance. That said, please implement all the necessary changes that I left in my feedback throughout the entire doc, to ensure the formatting and uniformity are in line. once you've done that, please tag me again for another review. thanks.
Since there are so many comments and changes needed, I am pausing my Final Review for now, and will pick things back up once you've made all the necessary adjustments.
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
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 @maya-odoo and all, thank you for your hard work on this PR and the payroll improvements to the Hong Kong doc!
In my technical review of this PR, I found it easier to push up a second commit with my suggestions, as there are a large number of formatting and syntax changes to be made. Please take a look at the commit that I will push up after this review. Below are a summary of the changes I made:
- Format the required modules as a table, to follow the similar format of other fiscal localization docs.
- List the modules that are automatically installed alongside the required modules.
- Link to the Install Apps and Modules documentation.
- Resize and compress all images with PNGquant.
- Remove a couple "unnecessary" images, as the written content provides clear instructions.
- Fix/improve admonitions and seealso's
As well, the Attendances & hourly wage seems to be missing some instruction, or at least a navigation instruction. I left a ?
in that section where more detail is needed 🙂
Please feel free to comment any changes, or make them yourself on this PR. After you have had a chance to review my commit and comments, I would like to pass this to @ksc-odoo for a final content review before we merge.
I appreciate your continued effort on this PR, and am excited to get this information out for HK l10n! Thank you!
c64bdc4
to
51b22b8
Compare
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.
Hey there, @maya-odoo -- just finished my final review. approving now, because the handful of feedback I have left is super minor and should be easy to take care of. Once you implement the necessary adjustments, feel free to tag @samueljlieber again for a Tech Review. Thanks!
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
content/applications/finance/fiscal_localizations/hong_kong.rst
Outdated
Show resolved
Hide resolved
All final touches are done, @samueljlieber ready for your final review. Thanks! |
Impacted Version: - 17.0 and above This commit improve below features: - Add Hong Kong payroll documentation
f91a191
to
faeaf0a
Compare
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 @maya-odoo, thank you so much for your hard work on this PR @maya-odoo and all! Your improvements look great to me. In faeaf0a I fixed any lines over 100 chars, compressed images, and squashed all commits. This PR is good to go 👍 thanks again for your work!
@robodoo r+
Impacted Version:
This commit improve below features: