[17.0][MIG] hr_planning_resources #5

antoniodavid · 2025-02-04T06:11:56Z

No description provided.

[16.0][ADD] hr_planning_resources [REF][16.0] hr_planning_resources: Refactoring app [16.0][ADD] hr_planning_resources: Add readme [16.0][ADD] hr_planning_resources: Add test-requirements Revert "[16.0][ADD] hr_planning_resources: Add readme" This reverts commit 4073ee9. [16.0][IMP] hr_planning_resources: Refactoring app

rrebollo

First of all, great work! Thank you for your contribution—I really appreciate your efforts. I've shared a few suggestions for your consideration; please feel free to address them as you see fit.

There is one suggestion in particular that I believe could significantly impact the current code: separating the logic for the helpdesk integration. In my opinion, this approach brings valuable benefits—one of them being a smaller scope for this review. 😄

Before proceeding further with my review, I’d love to hear your thoughts on this. Would you be open to making that change? That way, I can conduct the review once.

Thanks again for your contribution—this looks very promising!

rrebollo · 2025-02-05T08:13:05Z

hr_planning_resources/readme/USAGE.md

+5.  In the task, if the "Force recalculation" checkbox is checked, all
+    hours that have elapsed will be calculated.


Could you clarify this a little more?

rrebollo · 2025-02-05T08:13:48Z

hr_planning_resources/__manifest__.py

@@ -0,0 +1,33 @@
+{
+    "name": "HR Resource Planner",
+    "summary": "",


Please add a summary

rrebollo · 2025-02-05T08:22:34Z

hr_planning_resources/__manifest__.py

+    "depends": [
+        "project",
+        "web_timeline",
+        "helpdesk_mgmt",


My suggestion is to move helpdesk integration to another addon.

rrebollo · 2025-02-05T08:34:07Z

hr_planning_resources/models/hr_task_mixin.py

+            "project.project": [("project_id", "=", self.id)],
+            "helpdesk.ticket": [("ticket_id", "=", self.id)],
+        }
+        domain = domain_map.get(self._name, [("task_id", "=", self.id)])


I think prefixing task_id (obviously this should be done in field definition) with hr_ will be better. task_id collide with the built-in project.task immediately in my thoughts.

rrebollo · 2025-02-05T08:35:35Z

hr_planning_resources/models/hr_task_recurrency.py

+
+from odoo import _, api, fields, models
+from odoo.exceptions import ValidationError
+from odoo.tools.date_utils import get_timedelta


👍 for using helper date utils, nice

rrebollo · 2025-02-05T08:54:00Z

hr_planning_resources/models/hr_task.py

+        return datetime.combine(fields.Date.context_today(self), time.min)
+
+    def _default_date_end(self):
+        return datetime.combine(fields.Date.context_today(self), time.max)


Suggested change

return datetime.combine(fields.Date.context_today(self), time.max)

return fields.Datetime.end_of(fields.Date.context_today(self), 'day')

rrebollo · 2025-02-05T09:00:38Z

hr_planning_resources/models/hr_task.py

+        return datetime.combine(fields.Date.context_today(self), time.max)
+
+    def _get_default_employee(self):
+        return self.env["hr.employee"].search([("user_id", "=", self.env.uid)], limit=1)


I think you should match active company in this domain too

rrebollo · 2025-02-05T09:04:20Z

hr_planning_resources/models/hr_task.py

+from odoo import _, api, fields, models
+
+TASK_TYPES = [
+    ("task", _("Task")),


Suggested change

("task", _("Task")),

("task", "Task"),

You don't need to mark for translation selection labels

rrebollo · 2025-02-05T09:04:48Z

hr_planning_resources/models/hr_task.py

+
+TASK_TYPES = [
+    ("task", _("Task")),
+    ("project", _("Project")),


rrebollo · 2025-02-05T09:05:00Z

hr_planning_resources/models/hr_task.py

+TASK_TYPES = [
+    ("task", _("Task")),
+    ("project", _("Project")),
+    ("ticket", _("Ticket")),


acsonefho

Nice job! 💪
Sorry but I didn't finish the review but I put some comments :)

acsonefho · 2025-02-20T06:18:30Z

hr_planning_resources/data/hr_task_ir_cron.xml

@@ -0,0 +1,22 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<odoo>


Should be noupdate="1"

acsonefho · 2025-02-20T06:19:17Z

hr_planning_resources/models/helpdesk_ticket.py

+
+    def _compute_hr_task_count(self):
+        for record in self:
+            ticket_count = self.env["hr.task"].search_count(


Please use read_group (before the for) to do only a single query 🙏

Or add a task_ids o2m and then you can just do ticket_count = len(record.task_ids)

Also, consider moving this to a separate glue module with the Helpdesk app.
You don't want to force that app on the users.

acsonefho · 2025-02-20T06:21:44Z

hr_planning_resources/models/helpdesk_ticket.py

+
+    @api.model_create_multi
+    def create(self, vals_list):
+        if self.env.context.get("default_user_id", False):


This context key should already put this value (on the form view) as a default. And maybe the user change it (and you re-force it?)

acsonefho · 2025-02-20T06:23:01Z

hr_planning_resources/models/hr_leave.py

+    return time.strftime(get_lang(env).time_format)
+
+
+def format_date(env, date):


You already have this kind of tool into Odoo:

fields.Date.to_string(...) fields.Date.from_string(...) fields.Datetime.to_string(...) fields.Datetime.from_string(...)

acsonefho · 2025-02-20T06:28:56Z

hr_planning_resources/models/hr_leave.py

+        def localize(date):
+            return (
+                utc.localize(date)
+                .astimezone(timezone(self.env.user.tz or "UTC"))


You already have fields.Datetime.context_timestamp(...) who is doing a part of your code

acsonefho · 2025-02-20T06:42:02Z

hr_planning_resources/models/hr_task.py

+        return (
+            self.env.user.tz
+            or self.employee_id.tz
+            or self.employee_id.tz


dreispt · 2025-06-02T15:31:22Z

hr_planning_resources/__init__.py

@@ -0,0 +1,2 @@
+from . import models
+from . import wizard


Remember the singular naming convention for module names.
This should probably be hr_planning_resource.

dreispt · 2025-06-02T15:37:07Z

hr_planning_resources/models/hr_leave.py

+                ("date_to", ">=", date_from),
+            ],
+            order="date_from",
+        )


Preferably you have a method just to build and return the domain, and then do the search in the main business logic, instead of already returning the recordset.
Doing so is more friendly to extensions.

dreispt · 2025-06-02T15:38:09Z

hr_planning_resources/models/hr_leave.py

+        )
+
+    def _group_leaves_by_employee(self, calendar_leaves, employee_ids):
+        leaves = defaultdict(list)


Can't leaves be a recordset?

dreispt · 2025-06-02T15:47:39Z

This app might have similar goals to the Shift Planning module started at OCA/hr#1473
This modules has as a design goal to be compatible with the EE Planning App.

antoniodavid added 4 commits February 3, 2025 00:10

[16.0][IMP] hr_planning_resources: Pre-commit

98844a5

[ADD][16.0] hr_planning_resources: Remove test-requirements

afd8906

[IMP] hr_planning_resources: pre-commit auto fixes

bc18975

antoniodavid force-pushed the 17.0-mig-hr_planning_resources branch from 549c4b4 to aab5f77 Compare February 5, 2025 04:28

[ADD][17.0] hr_planning_resources

411e569

antoniodavid force-pushed the 17.0-mig-hr_planning_resources branch from aab5f77 to 411e569 Compare February 5, 2025 04:31

rrebollo reviewed Feb 5, 2025

View reviewed changes

[ADD][17.0] hr_planning_resources: Icon change

53b83c5

acsonefho suggested changes Feb 20, 2025

View reviewed changes

dreispt reviewed Jun 2, 2025

View reviewed changes

		5. In the task, if the "Force recalculation" checkbox is checked, all
		hours that have elapsed will be calculated.

	return datetime.combine(fields.Date.context_today(self), time.max)
	return fields.Datetime.end_of(fields.Date.context_today(self), 'day')

		@@ -0,0 +1,22 @@
		<?xml version="1.0" encoding="UTF-8" ?>
		<odoo>

		return time.strftime(get_lang(env).time_format)


		def format_date(env, date):

Uh oh!

[17.0][MIG] hr_planning_resources #5

Are you sure you want to change the base?

[17.0][MIG] hr_planning_resources #5

Uh oh!

Conversation

antoniodavid commented Feb 4, 2025

Uh oh!

rrebollo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acsonefho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreispt commented Jun 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants