[60410] Fetch calendar non-working days for the full date range

[cbliard]

When calendar widget displays a range of dates spanning multiple years, non-working days need to be fetched for the entire range or the widget may miss some of them. This is especially true for weeks spanning over two years like from 30-12-2024 to 05-01-2025.

This was detected by having test ./modules/my_page/spec/features/my/my_spent_time_widget_with_a_negative_time_zone_spec.rb:79 failing on new year 2025.

Ticket

https://community.openproject.org/wp/60410

What are you trying to accomplish?

Display non working days in "my spent time" and "calendar" widgets when the week is spanning over 2 years (for instance from 30-12-2024 to 05-01-2025) and there are some non-working days in both years.

Screenshots

Week is from 30-12-2024 to 05-01-2025
Non-working days are 31-12-2024 and 03-01-2025

Before:

After:

What approach did you choose and why?

Use requireNonWorkingYears$ (note the s) instead of requireNonWorkingYear$ to fetch non working days over multiple dates instead of just one date. That's the approach that was already used in frontend/src/app/features/work-packages/components/wp-table/timeline/container/wp-timeline-container.directive.ts.

There is also something that feels wrong with the headers not being in dark gray for non-working days, but it was already like this before.

Merge checklist

• Added/updated tests
  • nope, there was already a test which started failing on the new year 2025
• Added/updated documentation in Lookbook (patterns, previews, etc)
• Tested major browsers (Chrome, Firefox, Edge, ...)

[NobodysNightmare]

There is also something that feels wrong with the headers not being in dark gray for non-working days, but it was already like this before

Should we create a work package for this as well, so that someone can have a look at this too?

I'd agree to ignore this issue for the current PR.

[NobodysNightmare]

The changes look good to me right now. However, I still have thoughts I want to ping back first:

1. Should we add test cases that fail across the year for the broken implementation?
2. Should we either remove dayService.requireNonWorkingYear or at least deprecate it?

Re 2: I am wondering whether there is a way to use this method without repeating the error. IMO I can either try to check a single day and see whether it's a working day or a range of days for non-working days. What doesn't work is taking a single day and then assuming that I gain knowledge about other non-working days "in range", without ever specifying that range (which is exactly what requireNonWorkingYear seems to try to achieve).

[cbliard]

There is also something that feels wrong with the headers not being in dark gray for non-working days, but it was already like this before

Should we create a work package for this as well, so that someone can have a look at this too?

Yes probably. I'll talk about it in today's frontend meeting and create a bug if deemed so.

1. Should we add test cases that fail across the year for the broken implementation?

I hesitated. It's probably better to make that explicit. I'll create one.

2. Should we either remove dayService.requireNonWorkingYear or at least deprecate it?

It's not used anymore? I didn't notice. I have no particular opinion about it.

[NobodysNightmare]

It's not used anymore? I didn't notice. I have no particular opinion about it.

I just checked and it is still used, so I'd probably argue for deprecation. But to bring my point across: I'd expect to find similar issues when looking at the remaining callers of this method, because I don't see how one would use it, without triggering the same issues. The only way would be a caller that's aware of the "year around" logic of the method. Such a caller could be changed to request the correct time range itself.

edit: I don't fully understand where the last remaining caller of the singular version is used, but it looks very affected as well, see https://github.com/opf/openproject/blob/dev/frontend/src/app/shared/components/op-non-working-days-list/op-non-working-days-list.component.ts#L200

edit 2: I found the context and that place is not affected, because it shows exactly the non working days of a single year, so by never "wrapping" it doesn't break, even though it doesn't ever indicate the end time (which it could)

[cbliard]

edit: I don't fully understand where the last remaining caller of the singular version is used, but it looks very affected as well, see https://github.com/opf/openproject/blob/dev/frontend/src/app/shared/components/op-non-working-days-list/op-non-working-days-list.component.ts#L200

That's the /admin/settings/working_days_and_hours page where one can create non-working days, and only one year at a time is displayed there, so the usage seems legit. I would leave it like this.

It looks like this:

[NobodysNightmare]

so the usage seems legit. I would leave it like this.

Most importantly, I think it's out of scope for this PR :)

I'd personally still change it to use the (then) one existing method, and simply pass the desired time range into that method (there is fetchInfo.end as well).

[cbliard]

For the header not being grayed out, bug created here: https://community.openproject.org/projects/openproject/work_packages/60417/activity

[cbliard]

@NobodysNightmare I tried adding a feature test for the week crossing 2 years but failed to add one.

Time can be mocked in the rails process, but the browser still opens on today's date. Then maybe we can click on the "previous week" button until the correct date is displayed but it feels wrong and slow to do it this way.

Probably a unit test would be better to ensure the right calls are made, but that's beyond my abilities.

I give up on adding a test for that.

[NobodysNightmare]

Sad to hear, but fair. In the end we have tests covering it 1 out of 52 times a year ;-)