[19.0][MIG] hr_attendance by hbrunn · Pull Request #5705 · OCA/OpenUpgrade

hbrunn · 2026-06-09T13:32:49Z

No description provided.

hbrunn · 2026-06-09T13:33:02Z

/ocabot migration hr_attendance

Depends on :

hr : [19.0][MIG] hr+dependencies #5566
barcodes : [19.0][MIG] stock, stock_account #5547
base_geolocalize : [19.0][MIG] base_geolocalize: nothing to do #5731

hbrunn · 2026-06-23T09:23:28Z

@legalsylvain done in #5745, now setting our whole discussion as off-topic

legalsylvain · 2026-06-23T10:00:40Z

@legalsylvain done in #5745, now setting our whole discussion as off-topic

thanks a lot for this Job. I manually run the script a last time for the V19. new 19 PR should be handled by your script.

pedrobaeza · 2026-06-24T06:47:05Z

+---Models in module 'hr_attendance'---
+obsolete model hr.attendance.overtime (renamed to hr.attendance.overtime.line)
+new model hr.attendance.overtime.line (renamed from hr.attendance.overtime)
+


Why adding these empty lines that makes the reading harder? I think the comment should be together with the analysis lines block.

well I think the empty lines improve readability. If that's so important to you, add it to https://oca.github.io/OpenUpgrade/090_contribute.html#how-to-contribute-new-migration-scripts and I'll bulk update the whole branch, as I've been doing this for a long time

pedrobaeza · 2026-06-24T06:57:17Z

+
+# NOTHING TO DO
+
+hr_attendance / hr.attendance            / in_location (char)            : NEW


Reorganize this line into in_city DEL comment for seeing both together and to understand the change.

hard disagree about reordering lines in the work file. I want to be able to say diff upgrade_analysis* to easily verify all lines from the generated file are in the work file (=only whitespace or comments in the diff)

pedrobaeza · 2026-06-24T07:03:36Z

+    # date is required in v19, fill with create_date if empty, possibly wrong
+    env.cr.execute(
+        f"""
+        UPDATE hr_attendance_overtime_line SET date=(


Why not get it the same from the nearest attendance record?

how do you propose to find the nearest record? create_date? that's just as prone to filling in a wrong date here.

I consider overtime lines without date garbage data which people should clean up, and do the update only to satisfy the non-null constraint

pedrobaeza · 2026-06-24T07:05:06Z

+SQL_EMPLOYEE2TZ = """
+(
+    SELECT
+    hr_employee.id employee_id,


I find you new SQL style a bit difficult to read, and very vertical scroll intensive. Either indent the lines after the command (SELECT, FROM, etc), or put them in the same line as always.

It's not so new, but indeed was lacking indentation

Based on the link, this formatting does not follow the recommended style https://docs.telemetry.mozilla.org/concepts/sql_style.html#join-conditions

Suggested change

hr_employee.id employee_id,

FROM

hr_employee

JOIN

resource_resource

ON hr_employee.resource_id=resource_resource.id

LEFT JOIN

resource_calendar

ON resource_resource.calendar_id=resource_calendar.id

JOIN

res_company

ON hr_employee.company_id=res_company.id

LEFT JOIN

resource_calendar company_resource_calendar

ON res_company.resource_calendar_id=company_resource_calendar.id

pedrobaeza · 2026-06-24T07:05:49Z

+        WHERE
+        hr_attendance.employee_id=hr_attendance_overtime_line.employee_id
+        AND
+        hr_attendance.date=hr_attendance_overtime_line.date


This part may not fit according current code, and even more, we may have more than one record for the same date.

which code do you mean? and indeed this can lead to updating the same record multiple times, but how does this matter?

carlos-lopez-tecnativa

TT58628

carlos-lopez-tecnativa · 2026-07-09T18:49:41Z

+SQL_EMPLOYEE2TZ = """
+(
+    SELECT
+    hr_employee.id employee_id,


Based on the link, this formatting does not follow the recommended style https://docs.telemetry.mozilla.org/concepts/sql_style.html#join-conditions

Suggested change

hr_employee.id employee_id,

FROM

hr_employee

JOIN

resource_resource

ON hr_employee.resource_id=resource_resource.id

LEFT JOIN

resource_calendar

ON resource_resource.calendar_id=resource_calendar.id

JOIN

res_company

ON hr_employee.company_id=res_company.id

LEFT JOIN

resource_calendar company_resource_calendar

ON res_company.resource_calendar_id=company_resource_calendar.id

carlos-lopez-tecnativa · 2026-07-15T12:11:19Z

+    default_rule.employer_tolerance = main_company.overtime_company_threshold / 60.0
+    default_rule.employee_tolerance = main_company.overtime_employee_threshold / 60.0
+    for company in env["res.company"].search([]):
+        rule = default_rule
+        if (
+            default_rule.employer_tolerance != company.overtime_company_threshold
+            or default_rule.employee_tolerance != company.overtime_employee_threshold
+        ):
+            rule = default_rule.copy(
+                {
+                    "employer_tolerance": company.overtime_company_threshold / 60.0,
+                    "employee_tolerance": company.overtime_employee_threshold / 60.0,
+                    "ruleset_id": default_rule.ruleset_id.copy(
+                        {
+                            "company_id": company.id,
+                            "rule_ids": [
+                                Command.link(other_rule.copy().id)
+                                for other_rule in default_rule.ruleset_id.rule_ids
+                                if other_rule != default_rule
+                            ],
+                        }
+                    ).id,
+                }
+            )


I don't agree with this logic.

Why are you setting the fields on the default rule and then copying that rule to every company?

I don't think the fields should be set on the default rule.

If the default rule does not have a company_id set, why should it take the values from the main company?

this results in the default rule without company applying to all companies that have the same settings as the main company, which probably is the most common case. you only get specific rules for companies that have different settings than the main company.

what would your alternative do?

this results in the default rule without company applying to all companies that have the same settings as the main company, which probably is the most common case. you only get specific rules for companies that have different settings than the main company.

what would your alternative do?

Option 1: If you create or duplicate the rule for the main_company, then don't set these fields on the default rule. Otherwise, we assume that the values from the main company are the same for all other companies. In other words, don't modify the default rule; instead, create a new rule for each company that has these fields set.

Option 2: If the default rule without a company_id is intended to preserve the field values from the main_company, then don't create or duplicate another rule for the main_company.

Example:

My Company has these fields set.
C2 has these fields set, but with values different from My Company.
C3 does not have these fields set.

With Option 1, I would expect the following:

Don't modify the default rule, so it can be used by C3.
Don't create a rule for C3, because the default rule is shared.
Create a dedicated rule for My Company and C2, with the specific values configured for each company.

What I see as incorrect behavior:

A rule is created for C3, even though this company does not have these fields set.
The default rule receives the same values as My Company, but another rule is also created for My Company. Instead, it should use the default rule rather than creating a new one.

I prefer Option 1. What do you think?

i believe i implemented option 2, and i think that's the good one. would expect no new rule for main company in your example

i believe i implemented option 2, and i think that's the good one. would expect no new rule for main company in your example

OK, if you want to use Option 2, that's fine. However, in this case a new rule is still created for My Company. Please check my screenshot; this is a database migrated from v18.

The rule is created because, in the if statement, you compare values that have been divided by 60 when assigned to the default rule, while the values on the company fields are not divided by 60. As a result, they are never equal, and the code always enters the block that copies the rule.

default_rule.employer_tolerance != company.overtime_company_threshold

It should be:

default_rule.employer_tolerance != (company.overtime_company_threshold / 60.0)

thanks, your code would never create a new rule though. I think d698ab8 gets it right

Sorry, but after testing the latest changes, I'm not convinced by the results.

Following my example:

My Company has these fields set.

C2 has these fields set, but with values different from My Company.

C3 does not have these fields set.

Currently, a new rule is created for C3, but I would expect no rule to be created for this company.

The default rule (without company_id set) takes the values from main_company, but this rule is shared. As a result, other companies inherit values from main_company incorrectly.

I made these changes locally, and the behavior is correct according to my expectations:

No rule is created for C3.

An explicit rule is created for every company that has these fields set.

The default rule remains untouched.

I think this behavior is more correct because the default rule remains shared and is used by companies that do not have specific values configured, while companies with customized values get their own dedicated rules.

well, I disagree this is the most natural expectation. I think we should treat everything that's different from the main company the exception, and whatever the main company does should be the default for new companies

Okay, while another person can review this and decide whether the values configured on main_company can be used by other companies.

Please avoid creating rules for companies that do not have these fields set, because the default rule without a company_id should be used instead.

In my example, no rule should be created for C3.

Could you apply this change?

we need this rule because otherwise the default one will be active. remember the goal here is to get v19 to behave in a similar way it was in v18

[MIG] hr_attendance

6622a2b

hbrunn added this to the 19.0 milestone Jun 9, 2026

OCA-git-bot mentioned this pull request Jun 9, 2026

Migration to version 19.0 #5363

Open

This comment was marked as duplicate.

Sign in to view

legalsylvain added the Blocked by dependency label Jun 9, 2026

legalsylvain mentioned this pull request Jun 9, 2026

[19.0][OU-ADD] hr_timesheet_attendance: Nothing to do #5690

Open

This comment was marked as off-topic.

Sign in to view

legalsylvain added Dependency OK and removed Blocked by dependency labels Jun 23, 2026

pedrobaeza requested changes Jun 24, 2026

View reviewed changes

fixup! [MIG] hr_attendance

279edb6

OCA-git-bot added mod:openupgrade_scripts Module openupgrade_scripts series:19.0 labels Jun 30, 2026

carlos-lopez-tecnativa requested changes Jul 10, 2026

View reviewed changes

fixup! [MIG] hr_attendance

813e1e2

carlos-lopez-tecnativa requested changes Jul 15, 2026

View reviewed changes

fixup! [MIG] hr_attendance

d698ab8


		# NOTHING TO DO

		hr_attendance / hr.attendance / in_location (char) : NEW

-    hr_employee.id employee_id,
+    FROM
+        hr_employee
+    JOIN
+        resource_resource
+        ON hr_employee.resource_id=resource_resource.id
+    LEFT JOIN
+        resource_calendar
+        ON resource_resource.calendar_id=resource_calendar.id
+    JOIN
+        res_company
+        ON hr_employee.company_id=res_company.id
+    LEFT JOIN
+        resource_calendar company_resource_calendar
+        ON res_company.resource_calendar_id=company_resource_calendar.id

Uh oh!

Uh oh!

Conversation

hbrunn commented Jun 9, 2026

Uh oh!

hbrunn commented Jun 9, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as duplicate.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

hbrunn commented Jun 23, 2026

Uh oh!

legalsylvain commented Jun 23, 2026

Uh oh!

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!

Uh oh!

carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hbrunn commented Jun 9, 2026 •

edited by github-actions Bot

Loading