[16.0][MIG] hr_holidays_public_overtime

[pcastelovigo]

Using oca-port

[pcastelovigo]

test failing because of #230

[marcos-mendez]

Automated Review -- Tests Failed

1. Root cause of the test failure

The test failure occurs because the database connection fails during the Odoo startup, likely due to an incorrect or missing database configuration in the test environment (Runboat). This is not directly caused by the module code but indicates a misconfiguration in the CI setup or test runner.

---

2. Suggested fix

The root cause is a database connection error, not a code issue. However, to improve robustness, ensure that the test environment uses a valid database configuration. The code itself is correct and follows OCA conventions.

No code changes are required to fix the test failure, but you may want to verify that odoo.conf or the CI environment has correct DB settings.

---

3. Additional code issues

There is one real bug in the code:

❌ hr_holidays_public_overtime/models/hr_attendance.py

def _update_overtime(self, employee_attendance_dates=None):
    ...
    employee_attendance_dates = defaultdict(
        employee_attendance_dates.default_factory,
        {
            employee.with_context(
                exclude_public_holidays=True, employee_id=employee.id
            ): dates
            for employee, dates in employee_attendance_dates.items()
        },
    )

Issue: This line assumes employee_attendance_dates is a defaultdict, but it's actually a regular dict passed from self._get_attendances_dates(). Using .default_factory on a regular dict will raise an AttributeError.

Fix:
Replace the line:

employee_attendance_dates = defaultdict(
    employee_attendance_dates.default_factory,
    {
        employee.with_context(
            exclude_public_holidays=True, employee_id=employee.id
        ): dates
        for employee, dates in employee_attendance_dates.items()
    },
)

With:

employee_attendance_dates = defaultdict(
    list,
    {
        employee.with_context(
            exclude_public_holidays=True, employee_id=employee.id
        ): dates
        for employee, dates in employee_attendance_dates.items()
    },
)

This ensures that the defaultdict is initialized correctly, avoiding runtime errors.

---

4. Test improvements

The current test in test_hr_holidays_public_attendance.py is minimal and only tests a single case.

✅ Suggested improvements:

1. Use SavepointCase or TransactionCase for proper test isolation and database rollback.
2. Add test cases for:
   • Attendance on a public holiday that is not a weekend → should count as overtime.
   • Attendance on a public holiday that is a weekend → should not count as overtime (unless configured otherwise).
   • Multiple attendances on same day → overtime should accumulate.
   • Attendance on a non-public holiday → should not count as overtime.
3. Test with different calendar configurations (e.g., employee with custom calendar).
4. Test that exclude_public_holidays=True is respected in the context when calculating overtime.

Example test improvement:

def test_overtime_on_public_holiday(self):
    # Setup public holiday
    public_holiday = self.env['hr.holidays.public'].create({
        'name': 'Test Holiday',
        'date': '1994-10-14',
        'year_id': self.env['hr.holidays.public.year'].create({'year': 1994}).id,
    })
    # Create attendance
    attendance = self.env['hr.attendance'].create({
        'employee_id': self.employee.id,
        'check_in': '1994-10-14 12:00:00',
        'check_out': '1994-10-14 14:00:00',
    })
    # Check overtime was calculated
    overtime = self.env['hr.attendance.overtime'].search([
        ('date', '=', '1994-10-14'),
        ('employee_id', '=', self.employee.id),
    ])
    self.assertEqual(overtime.duration, 2)

This follows OCA testing patterns by using TransactionCase or SavepointCase and ensuring full coverage of edge cases.

---

⏰ PR Aging Alert

This PR by @pcastelovigo has been open for 112 days (3 months).
🔴 Zero human reviews in 112 days. This contributor invested their time to improve this module. The PSC owes them at least a response — even a "needs changes" is better than silence.
💤 No activity for 102 days. Has this PR been forgotten?

Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)

---

Reciprocal Review Request

Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):

My open PRs across OCA:

• server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
• server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
• hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
• stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
• stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
• stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
• purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix

Reviewing each other's work helps the whole community move forward. Thank you!

---

Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b

[pcastelovigo]

@hbrunn