[16.0][ADD] hr_timesheet_hours_billed #570

solo4games · 2023-02-18T18:51:09Z

This module allows to specify billed amount of time in timesheet while still keeping original time tracked. Additional field "Hours Billed" and "Approved" "Approved by" and "Approved on" are added to timesheet. These fields are visible only for "Timesheets: Administrator" group

hr_timesheet_hours_billed/__manifest__.py

hr_timesheet_hours_billed/models/hours_billed.py

hr_timesheet_hours_billed/models/so_line_hours_billed.py

hr_timesheet_hours_billed/models/hours_billed.py

hr_timesheet_hours_billed/views/hours_billed_view.xml

geomer198

LGTM!
PR Name must be have format [verion][ADD] module_name

Exmaple: [16.0][ADD] hr_timesheet_hours_billed

hr_timesheet_hours_billed/models/hours_billed.py

ivs-cetmix

File names must follow OCA rules

hr_timesheet_hours_billed/models/hours_billed.py

hr_timesheet_hours_billed/models/account_analytic_line.py

GabbasovDinar

Code LGTM!

hr_timesheet_hours_billed/models/account_analytic_line.py

ivs-cetmix · 2023-07-04T12:21:23Z

hr_timesheet_hours_billed/models/account_analytic_line.py

@@ -29,7 +29,8 @@ def _inverse_approved(self):
    def create(self, vals_list):
        for vals in vals_list:
            if not vals.get("unit_amount_billed"):
-                vals["unit_amount_billed"] = vals["unit_amount"]
+                unit_amount = vals.get("unit_amount")
+                vals["unit_amount_billed"] = 0.0 if not unit_amount else unit_amount


You can replace these 2 lines with a single one:
vals["unit_amount_billed"] = vals.get("unit_amount", 0.0)

ivs-cetmix

LGTM

OCA-git-bot · 2023-07-05T10:59:59Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

OCA-git-bot · 2023-07-05T11:00:01Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

ivs-cetmix · 2023-10-17T19:30:08Z

Hey @OCA/project-service-maintainers any chances to have this merged?

simahawk

Tech review only. LG overall, some minor glitches.

simahawk · 2023-10-18T05:12:35Z

hr_timesheet_hours_billed/__manifest__.py

+    "category": "Timesheet",
+    "website": "https://github.com/OCA/timesheet",
+    "maintainers": ["solo4games", "CetmixGitDrone"],
+    "author": "Odoo Community Association (OCA), Cetmix",


pls keep OCA at the end of the list

simahawk · 2023-10-18T05:15:15Z

hr_timesheet_hours_billed/models/account_analytic_line.py

+    def _inverse_approved(self):
+        user = self.env.user
+        now = fields.Datetime.now()
+        for record in self.filtered(lambda rec: rec.approved):


Rule of thumb: if you don't need a variable w/ the result of filtered to play with later, avoid this approach because you loop twice (possibly on the same recordset):

Suggested change

for record in self.filtered(lambda rec: rec.approved):

for record in self:

if not rec.approved:

continue

simahawk · 2023-10-18T05:17:19Z

hr_timesheet_hours_billed/models/sale_order_line.py

+        "analytic_line_ids.approved",
+    )
+    def _compute_qty_delivered(self):
+        """This method compute the delivered quantity of the SO lines:


This sounds more like a comment. A relevant info would be that you added some more field to depends

I need to write after this text something like
"Added analytic_line_ids.so_line, analytic_line_ids.unit_amount_billed, ..."?

simahawk · 2023-10-18T05:20:49Z

hr_timesheet_hours_billed/models/sale_order_line.py

+        """Compute and write the delivered quantity of current SO lines,
+        based on their related analytic lines.
+        :param additional_domain: domain to restrict AAL to include in computation
+        (required since timesheet is an AAL with a project ...)
+        """


Doesn't seem to write. Plus, having a get method that writes can be misleading and sometime dangerous.
Regarding the format of docstring, I always recommend to follow https://peps.python.org/pep-0257/#multi-line-docstrings (which is kind of the same guideline we have for commit messages).

Suggested change

"""Compute and write the delivered quantity of current SO lines,

based on their related analytic lines.

:param additional_domain: domain to restrict AAL to include in computation

(required since timesheet is an AAL with a project ...)

"""

"""Retrieve delivered quantity by line.

$longer-explanation-here-if-you-like

:param additional_domain: domain to restrict AAL to include in computation

(required since timesheet is an AAL with a project ...)

"""

simahawk · 2023-10-18T05:22:23Z

hr_timesheet_hours_billed/models/sale_order_line.py

+            uom.id: uom for uom in self.env["uom.uom"].browse(product_uom_ids)
+        }
+        for item in data:
+            if not item["product_uom_id"]:


can't this check be part of the domain?

I think not, because this is contains tuple, so this will be hard to check it from domain, but I can check approved in domain

simahawk · 2023-10-18T05:23:14Z

hr_timesheet_hours_billed/models/sale_order_line.py

+            so_line = lines_map[so_line_id]
+            result.setdefault(so_line_id, 0.0)
+            uom = product_uom_map.get(item["product_uom_id"][0])
+            if item.get("approved"):


same here as it seems non approved lines and lines w/o uom are totally ignored here

simahawk · 2023-10-18T05:25:32Z

hr_timesheet_hours_billed/tests/test_hours_billed.py

+@tagged("-at_install", "post_install")
+class TestCommonHourBilled(TestCommonSaleTimesheet):
+    # @classmethod
+    def setUp(self):


Please move this to setUpClass and disable tracking unless you need it for functional purposes

@classmethod def setUpClass(cls): super().setUpClass() cls.env = cls.env(context=dict(cls.env.context, tracking_disable=True))

This module allows to specify billed amount of time in timesheet while still keeping original time tracked. Additional field "Hours Billed" and "Approved" "Approved by" and "Approved on" are added to timesheet. These fields are visible only for "Timesheets: Administrator" group

leemannd

Hello, if I understand correctly, this module allows to:

specify an invoicable amount
adds fields to manage the approval status of the analytic lines

The base features of rounding are already present into sale_timesheet_rounding
https://github.com/OCA/timesheet/tree/15.0/sale_timesheet_rounded

I think it would be better if you port the already existing module and add a new one with the features of approval

solo4games · 2023-10-31T08:46:39Z

Hello, if I understand correctly, this module allows to:

specify an invoicable amount

adds fields to manage the approval status of the analytic lines

The base features of rounding are already present into sale_timesheet_rounding https://github.com/OCA/timesheet/tree/15.0/sale_timesheet_rounded

I think it would be better if you port the already existing module and add a new one with the features of approval

I can not understand what the meaning of using sale_timesheet_rounding module, because, if I understand correctly, there is only rounding mechanism, but in hr_timesheet_hours_billed module we use approval mechanism to invoice only "approved" timesheets.

ivs-cetmix · 2023-10-31T11:10:45Z

Hello, if I understand correctly, this module allows to:

specify an invoicable amount

adds fields to manage the approval status of the analytic lines

The base features of rounding are already present into sale_timesheet_rounding https://github.com/OCA/timesheet/tree/15.0/sale_timesheet_rounded

I think it would be better if you port the already existing module and add a new one with the features of approval

Thank you for the hint! Sounds reasonable. We will check this.

leemannd · 2023-11-01T12:46:40Z

Hello @solo4games ,
From my understanding, you're doing two things into your module.

Adding a field Hours Billed unit_amount_billed.
This field is pre-populated from unit_amount.
Then you can update the time to be billed, invoiced to the customer while keeping intact unit_amount

This feature possibility is fully covered in sale_timesheet_rounding.

The module, will add unit_amount_rounded. That is prepopulated at creation with the value of unit_amount.
If you want you can add a setup into your project, to have either:

unit_amount_rounded == unit_amount
- see https://github.com/OCA/timesheet/blob/15.0/sale_timesheet_rounded/models/account_analytic_line.py#L26
unit_amount_rounded => unit_amount + rounding to a factor, or to 15 minutes
- see => https://github.com/OCA/timesheet/blob/15.0/sale_timesheet_rounded/models/project_project.py#L6

This means it covers the possibility to keep track of the real time (for HR purpose), to maybe reduce manually the time to be invoiced or event better, to round it by default if your policy on support is to round hours to a given amount of time.

The only thing that is not handled into the module we created, is the strict handling of the approval that you're adding.

So my proposition is to migrate the module sale_timesheet_rounded and to create a second one that handles the validation.

github-actions · 2024-03-31T12:24:07Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

solo4games marked this pull request as draft February 18, 2023 18:51

ivs-cetmix suggested changes Feb 18, 2023

View reviewed changes

hr_timesheet_hours_billed/models/hours_billed.py Outdated Show resolved Hide resolved

hr_timesheet_hours_billed/models/hours_billed.py Outdated Show resolved Hide resolved

geomer198 reviewed Feb 22, 2023

View reviewed changes

hr_timesheet_hours_billed/views/hours_billed_view.xml Outdated Show resolved Hide resolved

geomer198 approved these changes Feb 22, 2023

View reviewed changes

solo4games force-pushed the 16.0-t2367-hr_timesheet_hours_billed-add_hours_billed branch from 2f3dc42 to 8ab7b42 Compare March 6, 2023 09:08

GabbasovDinar suggested changes Mar 7, 2023

View reviewed changes

hr_timesheet_hours_billed/models/hours_billed.py Outdated Show resolved Hide resolved

hr_timesheet_hours_billed/models/hours_billed.py Outdated Show resolved Hide resolved

hr_timesheet_hours_billed/models/hours_billed.py Outdated Show resolved Hide resolved

ivs-cetmix suggested changes Mar 7, 2023

View reviewed changes

solo4games changed the title ~~[16.0][ADD] hr_timesheet_hours_billed:hours_billed~~ [16.0][ADD] hr_timesheet_hours_billed Mar 9, 2023

GabbasovDinar suggested changes May 16, 2023

View reviewed changes

hr_timesheet_hours_billed/models/account_analytic_line.py Outdated Show resolved Hide resolved

GabbasovDinar suggested changes May 20, 2023

View reviewed changes

hr_timesheet_hours_billed/models/account_analytic_line.py Outdated Show resolved Hide resolved

hr_timesheet_hours_billed/models/account_analytic_line.py Outdated Show resolved Hide resolved

GabbasovDinar approved these changes May 22, 2023

View reviewed changes

ivs-cetmix suggested changes May 22, 2023

View reviewed changes

hr_timesheet_hours_billed/models/account_analytic_line.py Outdated Show resolved Hide resolved

hr_timesheet_hours_billed/models/account_analytic_line.py Outdated Show resolved Hide resolved

ivs-cetmix suggested changes May 24, 2023

View reviewed changes

hr_timesheet_hours_billed/models/account_analytic_line.py Outdated Show resolved Hide resolved

hr_timesheet_hours_billed/models/account_analytic_line.py Outdated Show resolved Hide resolved

solo4games marked this pull request as ready for review June 28, 2023 18:50

solo4games force-pushed the 16.0-t2367-hr_timesheet_hours_billed-add_hours_billed branch 2 times, most recently from 27d023b to d7e7969 Compare July 3, 2023 21:44

ivs-cetmix suggested changes Jul 4, 2023

View reviewed changes

solo4games force-pushed the 16.0-t2367-hr_timesheet_hours_billed-add_hours_billed branch 2 times, most recently from 5c2132d to 81b8fae Compare July 4, 2023 12:32

ivs-cetmix approved these changes Jul 5, 2023

View reviewed changes

OCA-git-bot added approved ready to merge labels Jul 5, 2023

simahawk requested changes Oct 18, 2023

View reviewed changes

OCA-git-bot removed approved ready to merge labels Oct 18, 2023

[IMP] hr_timesheet_hours_billed: rewrite code with help of review

24ef59c

solo4games force-pushed the 16.0-t2367-hr_timesheet_hours_billed-add_hours_billed branch from c3124d9 to 24ef59c Compare October 30, 2023 05:58

[FIX] hr_timesheet_hours_billed: fix pre-commit and tests

2e2b08f

leemannd suggested changes Oct 31, 2023

View reviewed changes

ivs-cetmix mentioned this pull request Nov 1, 2023

[ADD]test_task_packed_picking: add module DemchukM/oca#5

Open

[REF] hr_timesheet_hours_billed: Code is recatored.

7f62ba5

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Mar 31, 2024

github-actions bot closed this May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][ADD] hr_timesheet_hours_billed #570

[16.0][ADD] hr_timesheet_hours_billed #570

solo4games commented Feb 18, 2023

geomer198 left a comment

ivs-cetmix left a comment

GabbasovDinar left a comment

ivs-cetmix Jul 4, 2023

ivs-cetmix left a comment

OCA-git-bot commented Jul 5, 2023

OCA-git-bot commented Jul 5, 2023

ivs-cetmix commented Oct 17, 2023

simahawk left a comment

simahawk Oct 18, 2023

simahawk Oct 18, 2023

simahawk Oct 18, 2023

solo4games Oct 18, 2023

solo4games Oct 19, 2023

simahawk Oct 18, 2023

simahawk Oct 18, 2023

solo4games Oct 18, 2023

simahawk Oct 18, 2023

simahawk Oct 18, 2023

leemannd left a comment

solo4games commented Oct 31, 2023

ivs-cetmix commented Oct 31, 2023

leemannd commented Nov 1, 2023

github-actions bot commented Mar 31, 2024

-        for record in self.filtered(lambda rec: rec.approved):
+        for record in self:
+            if not rec.approved:
+                continue

[16.0][ADD] hr_timesheet_hours_billed #570

[16.0][ADD] hr_timesheet_hours_billed #570

Conversation

solo4games commented Feb 18, 2023

geomer198 left a comment

Choose a reason for hiding this comment

ivs-cetmix left a comment

Choose a reason for hiding this comment

GabbasovDinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivs-cetmix left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Jul 5, 2023

OCA-git-bot commented Jul 5, 2023

ivs-cetmix commented Oct 17, 2023

simahawk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leemannd left a comment

Choose a reason for hiding this comment

solo4games commented Oct 31, 2023

ivs-cetmix commented Oct 31, 2023

leemannd commented Nov 1, 2023

github-actions bot commented Mar 31, 2024