Skip to content

Conversation

mpascuall
Copy link

This module raises an error when attempting to create a new FSM Order if
the selected start date falls within a worker's non-working or leave period.

It also removes the group restriction on the Time Off smart button
so it becomes visible without enabling debug mode.

cc https://github.com/APSL 8363
@miquelalzanillas @lbarry-apsl @javierobcn @peluko00 @BernatObrador @ppyczko please review

@mpascuall mpascuall force-pushed the 17.0-add-fieldservice_check_worker_availability branch 3 times, most recently from 614cd5c to 90009bc Compare April 22, 2025 10:46
Copy link
Contributor

@peluko00 peluko00 left a comment

Choose a reason for hiding this comment

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

LGTM, code review and tested in runboat

@mpascuall mpascuall force-pushed the 17.0-add-fieldservice_check_worker_availability branch 2 times, most recently from db19b67 to 6fe76c8 Compare April 22, 2025 12:36
Copy link
Contributor

@BernatObrador BernatObrador left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@ppyczko ppyczko left a comment

Choose a reason for hiding this comment

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

While testing this module, I noticed that it's still possible to create an FSM order that overlaps with a time off entry. The current validation only checks the Scheduled Start (ETA), but doesn't account for the entire scheduled duration of the order.

For example, consider this time off entry:

imagen

If I then create an FSM order starting at 8:45h with a scheduled duration of 4 hours (ending at 12:45h), no error is raised—even though the FSM order overlaps the entire time off period (which goes from 9:00h to 11:00h).

imagen

The same issue occurs even if the order only partially overlaps the time off. For example, in another test, the time off entry again spans from 9:00h to 11:00h, and the FSM order overlaps it by 1 hour and 45 minutes.

imagen

@mpascuall mpascuall force-pushed the 17.0-add-fieldservice_check_worker_availability branch from 6fe76c8 to fd9872b Compare April 23, 2025 09:00
@mpascuall
Copy link
Author

While testing this module, I noticed that it's still possible to create an FSM order that overlaps with a time off entry. The current validation only checks the Scheduled Start (ETA), but doesn't account for the entire scheduled duration of the order.

For example, consider this time off entry:

imagen

If I then create an FSM order starting at 8:45h with a scheduled duration of 4 hours (ending at 12:45h), no error is raised—even though the FSM order overlaps the entire time off period (which goes from 9:00h to 11:00h).

imagen

The same issue occurs even if the order only partially overlaps the time off. For example, in another test, the time off entry again spans from 9:00h to 11:00h, and the FSM order overlaps it by 1 hour and 45 minutes.

imagen

Hi! Many thanks, could you review it again please?

Comment on lines 32 to 57
raise ValidationError(
_(
"%(name)s has a registered leave on "
"this date %(date)s.\nReason: %(reason)s"
)
% {
"name": person.name,
"date": scheduled_date_dt_utc.strftime("%Y-%m-%d %H:%M"),
"reason": overlapping_leave.name,
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested functionality and it works great. As a last suggestion, could you please modify this ValidationError message to be more accurate?

Right now, it only shows the start time of the FSM order, which can be misleading in cases where the order overlaps partially with a leave (e.g., FSM order scheduled from 13:30h to 15:30h, leave from 14:00h to 15:00h). The message that shows is:

imagen

But the leave is registered from 14:00h to 15:00h.

To make the overlap clearer, maybe we could include both the start and end time of the order and the leave. Something like this, for example:

Suggested change
raise ValidationError(
_(
"%(name)s has a registered leave on "
"this date %(date)s.\nReason: %(reason)s"
)
% {
"name": person.name,
"date": scheduled_date_dt_utc.strftime("%Y-%m-%d %H:%M"),
"reason": overlapping_leave.name,
}
)
raise ValidationError(
_(
"%(name)s has a registered leave from %(leave_start)s to %(leave_end)s, "
"which overlaps with the scheduled time of this order "
"(from %(order_start)s to %(order_end)s).\nReason: %(reason)s"
)
% {
"name": person.name,
"leave_start": overlapping_leave.date_from.strftime("%Y-%m-%d %H:%M"),
"leave_end": overlapping_leave.date_to.strftime("%Y-%m-%d %H:%M"),
"order_start": scheduled_date_dt_utc.strftime("%Y-%m-%d %H:%M"),
"order_end": scheduled_date_end_dt_utc.strftime("%Y-%m-%d %H:%M"),
"reason": overlapping_leave.name,
}
)

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, could you review it now?

@mpascuall mpascuall force-pushed the 17.0-add-fieldservice_check_worker_availability branch from fd9872b to 70874c5 Compare April 24, 2025 11:02
Copy link
Contributor

@ppyczko ppyczko left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved ready to merge stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants