[19.0][IMP] hr_timesheet_time_control_begin_end: Improve unit amount calculation by handling datetime string conversion#899
Conversation
OCA-git-bot
added
series:19.0
mod:hr_timesheet_time_control_begin_end
|
@NICO-SOLUTIONS, @MohamedOsman7 could you do a quick review... The user base of this module is very small. (probably just me) |
NICO-SOLUTIONS
left a comment
NICO-SOLUTIONS
left a comment
There was a problem hiding this comment.
Your changes cover string values returned by default_get where the ORM expects datetime... 👍
It might also make sense to guard the assignment and sign handling of unit_amount.
CRogos
force-pushed
the
19.0-imp-hr_timehseet_time_control_begin_end
branch
from
bd737ec to
38e0868
Compare
|
@MohamedOsman7 could you review |
| date_time = vals.get("date_time") | ||
| date_time_end = vals.get("date_time_end") | ||
| if isinstance(date_time, str): | ||
| date_time = fields.Datetime.from_string(date_time) |
There was a problem hiding this comment.
Please avoid doing this conversion if not needed
There was a problem hiding this comment.
So you mean the default in this test is wrong and default_date_time should never be a string?
https://github.com/OCA/timesheet/actions/runs/25074397732/job/73463039023?pr=898
There was a problem hiding this comment.
I've now changed the strings to date/datetime type.
CRogos
force-pushed
the
19.0-imp-hr_timehseet_time_control_begin_end
branch
2 times, most recently
from
c53f04c to
243c5c8
Compare
|
This PR has the |
| ctx["default_date"] = datetime.fromisoformat( | ||
| self.env.context.get("default_date_time") | ||
| ).date() | ||
| ctx["default_date"] = self.env.context.get("default_date_time").date() |
There was a problem hiding this comment.
Is this change needed? What happen with timezones?
There was a problem hiding this comment.
In my understanding, the default_date_time should never be a sting and therefore can bypass fromisoformat.
But regarding the timezone you may be right. How about?
| ctx["default_date"] = self.env.context.get("default_date_time").date() | |
| default_date_time = self.env.context.get("default_date_time") | |
| if default_date_time: | |
| ctx["default_date"] = default_date_time.astimezone(ZoneInfo("UTC")).date() |
There was a problem hiding this comment.
I can't say how the conversion should go, sorry.
There was a problem hiding this comment.
I've added a test for _get_default with default_date_time having a timezone and received an error from odoo:
ValueError: Datetime field expects a naive datetime: 2025-06-15 23:00:00+00:00
Therefore I've removed the UTC conversion because the default_date_time may not have timezone information.
Do you have more comments/suggestions/improvements?
CRogos
force-pushed
the
19.0-imp-hr_timehseet_time_control_begin_end
branch
3 times, most recently
from
3d1fac2 to
8315af2
Compare
…ation by handling datetime Co-authored-by: Copilot <copilot@github.com>
CRogos
force-pushed
the
19.0-imp-hr_timehseet_time_control_begin_end
branch
from
8315af2 to
6d74ce4
Compare
pedrobaeza
left a comment
pedrobaeza
left a comment
There was a problem hiding this comment.
OK, let's go!
/ocabot merge patch
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 8a26891. Thanks a lot for contributing to OCA. ❤️ |
5 participants
Compatibility issue with #898