Skip to content

[FIX] hr_holidays_ux: splits multi-month leaves in write and create.#55

Closed
ced-adhoc wants to merge 1 commit intoingadhoc:18.0from
adhoc-dev:18.0-t-96965-ced
Closed

[FIX] hr_holidays_ux: splits multi-month leaves in write and create.#55
ced-adhoc wants to merge 1 commit intoingadhoc:18.0from
adhoc-dev:18.0-t-96965-ced

Conversation

@ced-adhoc
Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings October 6, 2025 21:48
@roboadhoc
Copy link
Copy Markdown

Pull request status dashboard

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements automatic splitting of multi-month leave records in the HR holidays system. The changes enhance user experience by automatically handling leaves that span across multiple months during both creation and modification operations.

  • Refactored the split_days method for improved clarity and efficiency
  • Added automatic leave splitting functionality to both create and write methods
  • Implemented helper methods for date conversion and overlap detection to prevent duplicate records

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread hr_holidays_ux/models/hr_leave.py Outdated
Comment thread hr_holidays_ux/models/hr_leave.py Outdated
Comment thread hr_holidays_ux/models/hr_leave.py Outdated
Comment thread hr_holidays_ux/models/hr_leave.py Outdated
@ced-adhoc ced-adhoc force-pushed the 18.0-t-96965-ced branch 2 times, most recently from b1592e8 to 65120cc Compare October 8, 2025 14:31
@ced-adhoc ced-adhoc requested a review from Copilot October 8, 2025 18:31
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread hr_holidays_ux/models/hr_leave.py Outdated
Comment thread hr_holidays_ux/models/hr_leave.py Outdated
first_vals.update(first_split)

# Update current record with first split period
super().write(first_vals)
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling super().write() inside a loop for individual records can cause issues with the ORM. The write method is designed to handle recordsets, but here it's being called on individual records within the loop. This should be super(HrLeave, record).write(first_vals) or the loop should be restructured to handle the recordset properly.

Suggested change
super().write(first_vals)
super(HrLeave, record).write(first_vals)

Copilot uses AI. Check for mistakes.
Comment thread hr_holidays_ux/models/hr_leave.py Outdated
Comment on lines +225 to +228
super().write(vals)
else:
# No splitting needed, process normally
super().write(vals)
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous issue, calling super().write(vals) within a loop over individual records can cause problems. The write method expects to be called on the entire recordset, not individual records within a loop. Consider restructuring to collect records that don't need splitting and call write once, or use super(HrLeave, record).write(vals).

Copilot uses AI. Check for mistakes.
date_from = self._convert_to_date(date_from)
date_to = self._convert_to_date(date_to)
last_day_of_month = self.get_last_day_of_month(date_from)
return date_to > last_day_of_month
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quizas no son necesarias las variables y se puede hacer directamente la comparación

@mav-adhoc
Copy link
Copy Markdown
Contributor

@roboadhoc r+

roboadhoc pushed a commit that referenced this pull request Nov 5, 2025
closes #55

Signed-off-by: Matias Velazquez <mav@adhoc.com.ar>
@roboadhoc roboadhoc closed this Nov 5, 2025
@roboadhoc roboadhoc deleted the 18.0-t-96965-ced branch November 5, 2025 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants