Skip to content

[16.0] [ADD] volunteer_holiday#578

Draft
Genepi314 wants to merge 27 commits into16.0from
16.0-add-volunteer_holiday
Draft

[16.0] [ADD] volunteer_holiday#578
Genepi314 wants to merge 27 commits into16.0from
16.0-add-volunteer_holiday

Conversation

@Genepi314
Copy link
Copy Markdown

Description

Add holidays for companies and leaves for volunteers.

Odoo task (if applicable)

Checklist before approval

  • Tests are present (or not needed).
  • Credits/copyright have been changed correctly.
  • Change log snippet is present.
  • (If a new module) Moving this to OCA has been considered.

@remytms remytms changed the title 16.0 add volunteer holiday [16.0] [ADD] volunteer_holiday Mar 2, 2026
Comment on lines +100 to +114
if (
(
shift_start_date >= holiday_start_date
and shift_start_date <= holiday_end_date
)
or (
shift_end_date >= holiday_start_date
and shift_start_date <= holiday_end_date
)
or (
shift_start_date <= holiday_start_date
and shift_end_date >= holiday_end_date
)
):
return True
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Simply write:

Suggested change
if (
(
shift_start_date >= holiday_start_date
and shift_start_date <= holiday_end_date
)
or (
shift_end_date >= holiday_start_date
and shift_start_date <= holiday_end_date
)
or (
shift_start_date <= holiday_start_date
and shift_end_date >= holiday_end_date
)
):
return True
return (
(
shift_start_date >= holiday_start_date
and shift_start_date <= holiday_end_date
)
or (
shift_end_date >= holiday_start_date
and shift_start_date <= holiday_end_date
)
or (
shift_start_date <= holiday_start_date
and shift_end_date >= holiday_end_date
)
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, I didn't know about this simplified syntax. It is done.

Copy link
Copy Markdown
Collaborator

@remytms remytms left a comment

Choose a reason for hiding this comment

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

First partial review.

Comment on lines +8 to +18
<!-- Après test, passer en noupdate=1 -->
<record id="ir_cron_cancel_holiday_shift" model="ir.cron">
<field name="name">Cancel Generated Shifts During Company Holidays</field>
<field name="model_id" ref="model_volunteer_company_holiday" />
<field name="state">code</field>
<field name="code">model._cancel_holiday_shift()</field>
<field name="interval_number">24</field>
<field name="interval_type">hours</field>
<field name="numbercall">-1</field>
<field name="doall" eval="False" />
</record>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Apply your comment here. :)

<field name="name">Christmas Holidays</field>
<field
name="start_date"
eval="(datetime.today().replace(month=12, day=24)).strftime('%Y-%m-%d')"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Starting with python 3.9 (odoo 16.0 uses 3.10), prefer using isformat() instead of strftime for standard iso formatting.

See method here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After some tests on these cases, neither strftime() nor isoformat() seems to be required. On my side, loading works fine without any formatting. That said, I'm not sure of all the details behind it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

After some tests on these cases, neither strftime() nor isoformat() seems to be required. On my side, loading works fine without any formatting. That said, I'm not sure of all the details behind it.

Indeed, it is not needed. Thank you, I removed the strftime() in all demo files :-)

/>
<field
name="end_date"
eval="(datetime.today().replace(month=12, day=25)).strftime('%Y-%m-%d')"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

isoformat()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed all strftime() after aydrpm's suggestion :-)

<field name="name">Store Inventory</field>
<field
name="start_date"
eval="(datetime.today().replace(month=7, day=1)).strftime('%Y-%m-%d')"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

isoformat

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed all strftime() after aydrpm's suggestion :-)

Comment on lines +34 to +35
# Retirer la limite de temps, regarder les shifts futurs
def _cancel_holiday_shift(self, time_in_months=3):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that you should apply your comment, and remove the time limit. Just check all shifts, and see if they are overlapping a holiday.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that for now it’s ok if the method starts at now() and all future shifts.

Because, we don’t want to change the past.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Just corrected it in the the commit "Refactor _cancel_holiday_shift..."

Comment on lines +68 to +71
# Create dictionary key-company for list of values-shifts
shifts_by_company = {}
for shift in confirmed_future_generated_shifts_in_range:
shifts_by_company.setdefault(shift.company_id, []).append(shift)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just order them by company_id when using the search() method.

Also, as there is less holiday than shifts, it’s better to do so with holiday, or even fetch holiday for the company when needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I used itertools as you suggested recently and ordered the holidays by company. I'm not sure I did it perfectly right though, so don't hesitate to tell me if something is still off.

Comment on lines +73 to +88
for holiday in future_company_holidays_in_range:
same_company_shifts = shifts_by_company.get(holiday.company_id, [])
for shift in same_company_shifts:
if shift.state != "canceled" and self._shift_covers_holiday(
shift.start_time,
shift.end_time,
holiday.start_date,
holiday.end_date,
):
shift.sudo().write(
{
"stage_id": self.env.ref(
"volunteer.volunteer_shift_stage_canceled"
).id
}
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If your shifts are ordered by company_id, you can use itertools.groupby() to iterate through all the shifts, company by company. Then you can fetch the holiday of the comany, each time you face a new company.

Using itertools.groupby() is more efficient for such a task.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

See comment above.

volunteer_leave_ids = fields.One2many(
comodel_name="volunteer.volunteer.leave",
inverse_name="volunteer_id",
string="Leave",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here for the user interface you can use plural form.

Suggested change
string="Leave",
string="Leaves",

class VolunteerVolunteerLeave(models.Model):
_name = "volunteer.volunteer.leave"
_description = "Volunteer Leave"
_order = "start_date"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_order = "start_date"
_order = "start_date desc, volunteer_id, id"

Same as for company.holiday.

Copy link
Copy Markdown
Author

@Genepi314 Genepi314 Mar 25, 2026

Choose a reason for hiding this comment

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

I just did this for volunteer leaves. But I am not sure I understand what you mean for company holidays. Do you mean you also want it to be displayed with _order = "start_date desc" ? Or do you mean I should also add "company_id, id" ?

I didn't put the "desc" after _order = "start_date" because of the difference in the views. Volunteer Leaves has a view that I couldn't get filtered (the one nested in volunteer_volunteer_view, as a page in the notebook), so I thought it would be less tedious to get the most distant in the future as first to appear on the view, rather than the most distant in the past.

This does not apply to company holidays, as they're filtered automatically to show only the future ones. Therefore I thought it would be best to see the closest next one, rather than the most distant in the future one.

But anyway, this is the way I see it. If you don't like it I'll change it to "start_date desc" for company holidays too :-)

Copy link
Copy Markdown
Collaborator

@remytms remytms left a comment

Choose a reason for hiding this comment

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

Partial review.

class VolunteerCompanyHoliday(models.Model):
_name = "volunteer.company.holiday"
_description = "Company Holidays"
_order = "start_date"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

up for this comment.

for company, grouped_holidays_by_company in groupby(
future_company_holidays, key=lambda holiday: holiday.company_id.id
):
grouped_holidays = list(grouped_holidays_by_company)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

grouped_holiday_by_company is a iterator. You loop only once over it. So there is no need to convert it to a list.

Comment on lines +74 to +76
for company, grouped_holidays_by_company in groupby(
future_company_holidays, key=lambda holiday: holiday.company_id.id
):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for company, grouped_holidays_by_company in groupby(
future_company_holidays, key=lambda holiday: holiday.company_id.id
):
for company_id, grouped_holidays_by_company in groupby(
future_company_holidays, key=lambda holiday: holiday.company_id
):

I found this more readable in terms of Odoo nomenclature.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see, it is changed.

Comment on lines +21 to +47
def _compute_overlap_holiday(self):
"""Compute if shift overlaps with any company holiday period."""
date_today = datetime.today().date()
Holiday = self.env["volunteer.company.holiday"]
future_shifts = self.env["volunteer.shift"].search(
[("end_time", ">=", datetime.today())]
)
for shift in future_shifts:
found_overlap = False
this_company_future_holidays = self.env["volunteer.company.holiday"].search(
[
("end_date", ">=", date_today),
("company_id", "=", shift.company_id.id),
]
)
for holiday in this_company_future_holidays:
if Holiday._shift_covers_holiday(
shift.start_time,
shift.end_time,
holiday.start_date,
holiday.end_date,
):
found_overlap = True
shift.overlaps_holiday = True

if not found_overlap:
shift.overlaps_holiday = False
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here you should not retrieve all shift and then check them. The compute function must only run on record in self.

So just group self by company_id. Then loop over the shift and fetch company holiday when needed.

Then you must set a value for overlaps_holiday for each record in self.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I wrote this one a bit quickly, so sorry for the lack of optimization already.
I have some questions here:

  1. Should I actually use an iterator like in company_holidays to go through holidays and shifts more efficiently ?
  2. About setting a value for overlaps_holiday, I thought it wasn't needed, as it has a default value set to false already. Should I still set it to false again in the compute function ?

all_companies = self.env["res.company"].search([])
for company in all_companies:
nb_days = company.nb_days_before_leave_end
message_body = ("Your leave ends in {} days.").format(nb_days)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here import the _ function and use it for you string like this:

_("Your ...")

This way, your string can be translated.

Comment on lines +57 to +61
all_future_volunteer_leaves = (
self.env["volunteer.volunteer.leave"]
.sudo()
.search([("end_date", ">=", date.today())])
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if sudo() is needed, as we can run the cron with a user that has the correct rights to do that.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

True, I changed it in company holidays but not in volunteer leaves. Thanks for telling me.

leave.start_date,
leave.end_date,
):
participation.sudo().write({"registration_state": "canceled"})
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Idem here for sudo().

comodel_name="res.company",
string="Company",
default=lambda self: self.env.user.company_id,
required=True,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here we can remove the required=True for company_id. As if it is false it means that it can be used by all company.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed it there.

But would that be true for all company_id fields then ? Like in volunteer.company.holiday for example ? Or is it different for this one ? I don't know how it would actually work with the cancel_holiday_shift function, as for now the domain used to retrieve shifts from the same company has a "=" operator for companies, and not a "in"...

Copy link
Copy Markdown
Collaborator

@remytms remytms left a comment

Choose a reason for hiding this comment

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

Some little changes.

Comment on lines +9 to +46
<record id="ir_cron_cancel_holiday_shift" model="ir.cron">
<field
name="name"
>Volunteer: Cancel Generated Shifts During Company Holidays</field>
<field name="model_id" ref="model_volunteer_company_holiday" />
<field name="state">code</field>
<field name="code">model._cancel_holiday_shift()</field>
<field name="interval_number">24</field>
<field name="interval_type">hours</field>
<field name="numbercall">-1</field>
<field name="doall" eval="False" />
</record>

<record id="ir_cron_cancel_volunteer_leave_participation" model="ir.cron">
<field
name="name"
>Volunteer: Cancel Participations During Volunteer Leaves</field>
<field name="model_id" ref="model_volunteer_volunteer_leave" />
<field name="state">code</field>
<field name="code">model._cancel_volunteer_leave_participation()</field>
<field name="interval_number">24</field>
<field name="interval_type">hours</field>
<field name="numbercall">-1</field>
<field name="doall" eval="False" />
</record>

<record id="ir_cron_send_leave_end_notification" model="ir.cron">
<field
name="name"
>Notification: Send a Reminder to Volunteers Before their Leave Ends</field>
<field name="model_id" ref="model_volunteer_volunteer" />
<field name="state">code</field>
<field name="code">model._send_notification_end_leave()</field>
<field name="interval_number">24</field>
<field name="interval_type">hours</field>
<field name="numbercall">-1</field>
<field name="doall" eval="False" />
</record>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here we should set the admin user as user_id.

"maintainers": ["remytms"],
"license": "AGPL-3",
"application": False,
"depends": ["base", "volunteer", "mail"],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"depends": ["base", "volunteer", "mail"],
"depends": ["volunteer", "mail"],

<field name="name">Volunteer Leaves List</field>
<field name="model">volunteer.volunteer.leave</field>
<field name="arch" type="xml">
<tree default_order="start_date asc">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No need to put default_order if it is not different than the one in _order of the model.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Here it is different in the model and in the view. Here is the code in the model:
_order = "start_date desc, volunteer_id, id"
This is because of the notebook page in volunteer_volunteer_view in which no default filter is available, where as in the volunteer_volunteer_leave_view, there is a default filter applied to see only future volunteer leaves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants