Skip to content

Conversation

@larm-odoo
Copy link
Contributor

New doc for new section of HR, focused on the configuration section.

@robodoo
Copy link
Collaborator

robodoo commented Apr 24, 2023

@C3POdoo C3POdoo requested review from a team April 24, 2023 19:18
@larm-odoo larm-odoo assigned larm-odoo and unassigned larm-odoo May 2, 2023
@larm-odoo larm-odoo requested a review from a team May 2, 2023 17:17
Copy link
Contributor

@meng-odoo meng-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 @larm-odoo! Just finished reviewing this. I had suggestions for where information could be added, mostly at the beginning of sections/paragraphs to introduce the topic and explain important vocabulary words and how they relate to Odoo in particular.

Also, something I just checked but didn't comment on in my review: all headings should be in sentence case!

Please let me know if you have any questions!

@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from ddc45dd to cbb0ea9 Compare May 5, 2023 14:39
@larm-odoo
Copy link
Contributor Author

Hi @meng-odoo! I made all the edits/suggestions, and added a bit more information/text in the areas you suggested.

@larm-odoo larm-odoo requested a review from meng-odoo May 5, 2023 14:40
Copy link
Contributor

@meng-odoo meng-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 @larm-odoo, great job adding info to this doc and making those edits! I had a bunch of small wording suggestions, and a couple more places where I think information could be added. I know this doc already covers so much, but we still want to go through and make sure we explain as much as possible, so that someone reading this doc will have all their questions answered :)

Please let me know if you have any questions!

meng-odoo

This comment was marked as duplicate.

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.

Hi @larm-odoo
doc structure review:

For the app-level page, could you add a small explanation about what the app is +a link to Odoo Tutorials, like on this page, please?
https://github.com/odoo/documentation/blob/16.0/content/applications/hr/attendances.rst?plain=1

The best would be to put the main, or basic content, on that app-level page.

And for the commit message and PR title, please change the tag to [ADD].

Thanks and have a good day

@larm-odoo larm-odoo changed the title [NEW] Payroll: adding new configuration doc [ADD] Payroll: adding new configuration doc May 12, 2023
@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from cbb0ea9 to 5183b97 Compare May 15, 2023 21:10
@larm-odoo
Copy link
Contributor Author

Thanks @jcs-odoo - made the edits as suggested, and it seems to be working like the example you gave. @meng-odoo - I also made your edits. After checking with Zac, two spots we are leaving as a 'This is for Belgium, and will be updated when the app is updated' explanation, so that is helpful!

@larm-odoo larm-odoo requested review from jcs-odoo and meng-odoo May 15, 2023 21:12
@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from 5183b97 to 2f4c82a Compare May 16, 2023 17:58
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.

Hi @larm-odoo

Thanks for changing the app page :)
Ideally, the "main" content of the app can go there, or maybe a page describing a default flow, redirecting to all the children pages.

Also, the build failed and indicates there is a missing image. It's the same warning message as when you build the documentation on your computer.

image
image

as for the ci/documentation_guidelines test
image
it indicates you need to add a blank line at the end of the configuration.rst file. This is needed to help git when there are later additions that come at the end of the page.

Have a good day :)

@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from 2f4c82a to 8278187 Compare May 17, 2023 20:42
@larm-odoo
Copy link
Contributor Author

Thanks for the info @jcs-odoo! I fixed the errors for the build, it looks good now. As for the main article, I am going to have an overview doc added after this one, which will link to this. So I will have a lot more info/a walk through for this section. I am just starting with the 'meat'. Does that explain how I made this doc/landing page?

Copy link
Contributor

@meng-odoo meng-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 @larm-odoo, this doc is looking great! I just had a few more small suggestions/wording changes, then this should be good to go :)

@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from 8278187 to 1f76ac7 Compare May 23, 2023 17:51
@larm-odoo larm-odoo requested review from jcs-odoo and meng-odoo May 23, 2023 17:53
Copy link
Contributor

@meng-odoo meng-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 @larm-odoo, this doc looks great :) Thanks for all your work on this! I had one single comment this round. Please go ahead and push this doc to the next step after this!

@larm-odoo
Copy link
Contributor Author

larm-odoo commented May 23, 2023 via email

@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from 1f76ac7 to 4f00199 Compare May 26, 2023 12:37
@larm-odoo larm-odoo requested a review from samueljlieber May 26, 2023 12:38
@larm-odoo larm-odoo requested a review from samueljlieber May 31, 2023 14:13
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 @larm-odoo 👋 thank you for your continued effort on this PR. I have some more technical changes after taking another look. Please see below and let me know if you have any questions. Please tag me again once this PR is ready 🙂 Thank you!

Comment on lines 45 to 53
the *Timesheets* application, a :guilabel:`Work Entry Type` needs to be selected. The list of
:guilabel:`Work Entry Types` is automatically created based on localization settings set in the
database.

To view the current work entry types available.Go to :menuselection:`Payroll --> Configuration -->
Work Entry Types`

Each work entry type has a code to aid in the creation of payslips, and ensure all taxes and fees
are correctly entered.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is referring to the records and not the page "Work Entry Types", and while the page heading is Work Entry Types, the focus of the sentence is on the records... so I don't think we need any formatting. But I am curious on @StraubCreative's thoughts here 👇

p.s. also missing a comma, a space before 'go' and a period.

Suggested change
the *Timesheets* application, a :guilabel:`Work Entry Type` needs to be selected. The list of
:guilabel:`Work Entry Types` is automatically created based on localization settings set in the
database.
To view the current work entry types available.Go to :menuselection:`Payroll --> Configuration -->
Work Entry Types`
Each work entry type has a code to aid in the creation of payslips, and ensure all taxes and fees
are correctly entered.
the *Timesheets* application, a :guilabel:`Work Entry Type` needs to be selected. The list of work
entry types is automatically created based on localization settings set in the database.
To view the current work entry types available, go to :menuselection:`Payroll --> Configuration -->
Work Entry Types`.
Each work entry type has a :guilabel:`Code` to aid in the creation of payslips, and ensure all taxes
and fees are correctly entered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah these are tough, since I feel the need to explain the why, then the how, and I am using the terms in the UI. I also wonder what @StraubCreative says on this.

Copy link
Contributor Author

@larm-odoo larm-odoo Jun 1, 2023

Choose a reason for hiding this comment

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

Yeah these are tough, since I feel the need to explain the why, then the how, and I am using the terms in the UI. I also wonder what @StraubCreative says on this. I'll leave this as unresolved so we see it.

@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from e03492c to d9a0868 Compare June 1, 2023 17:12
@larm-odoo larm-odoo requested a review from samueljlieber June 1, 2023 17:12
@larm-odoo
Copy link
Contributor Author

All changes done, @samueljlieber ! Ready for you again =)

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 @larm-odoo! Just a couple small changes and then you can pass this along to final review. I am approving now be please make these changes before tagging 🙂 great work!

@larm-odoo larm-odoo force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from d9a0868 to 9f8c7ff Compare June 5, 2023 17:58
@larm-odoo larm-odoo requested review from StraubCreative and removed request for a team June 5, 2023 18:02
@larm-odoo
Copy link
Contributor Author

Hi @StraubCreative - this is ready for you, now!

Copy link
Contributor

@meng-odoo meng-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 @larm-odoo! I did a final review of this doc and it looks good to me :) Thanks for your work on this! I'm going to go ahead and tag @StraubCreative for merge.

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 doc @larm-odoo. Very thorough on an otherwise complex topic, keep it up 👍

I think based on new 2023 guideline we'll want to move all of the content from configuration.rst to payroll.rst to minimize clicks for the user and to avoid pages with lean content. Will make a Task and assign in Project.

@robodoo r+

@robodoo
Copy link
Collaborator

robodoo commented Aug 4, 2023

@larm-odoo @StraubCreative unable to stage: merge conflict

@samueljlieber samueljlieber force-pushed the 14.0-Payroll-new-configuration-doc-larm branch from 9f8c7ff to 604e7b5 Compare August 4, 2023 21:52
@StraubCreative
Copy link
Contributor

@robodoo retry

@robodoo
Copy link
Collaborator

robodoo commented Aug 4, 2023

I'm sorry, @StraubCreative: retry makes no sense when the PR is not in error.

@StraubCreative
Copy link
Contributor

@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.

7 participants