Skip to content

Org: Add possibility of adding year in holidays.#2593

Open
cyrillkuettel wants to merge 1 commit into
masterfrom
ogc-2385-feiertage-als-volles-datum-mit-jahr
Open

Org: Add possibility of adding year in holidays.#2593
cyrillkuettel wants to merge 1 commit into
masterfrom
ogc-2385-feiertage-als-volles-datum-mit-jahr

Conversation

@cyrillkuettel

Copy link
Copy Markdown
Contributor

Please fill in the commit message below and work through the checklist. You can delete parts that are not needed, e.g. the optional description, the link to a ticket or irrelevant options of the checklist.

Commit message

Org: Add possibility of adding year in holidays.

TYPE: Feature
LINK: OGC-2385

Checklist

  • I have performed a self-review of my code
  • I have tested my code thoroughly by hand
  • I have added tests for my changes/features

@linear

linear Bot commented Jul 21, 2026

Copy link
Copy Markdown

OGC-2385

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.78788% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.51%. Comparing base (4678f5e) to head (941bd6b).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/onegov/org/forms/settings.py 75.00% 4 Missing ⚠️
src/onegov/org/models/swiss_holidays.py 82.35% 3 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/onegov/org/models/swiss_holidays.py 85.83% <82.35%> (-1.21%) ⬇️
src/onegov/org/forms/settings.py 74.46% <75.00%> (+1.20%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4678f5e...941bd6b. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

other_holidays = TextAreaField(
label=_('Other holidays'),
description=('31.10 - Halloween'),
description=('31.10 - Halloween / 31.10.2025 - Halloween'),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would keep the old description intact, since this it was displayed as a placeholder and you can't have two entries on the same line.

If you want a more explanatory/verbose description, you're better off doing something like organisation_hierarchy where you give an example with a markdown code block. Long descriptions are rendered as markdown.

Comment on lines +185 to +188
for dt in self._other_dated:
if dt.year == year:
yield dt, self._other_dated[dt]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
for dt in self._other_dated:
if dt.year == year:
yield dt, self._other_dated[dt]
for dt, descriptions in self._other_dated.items():
if dt.year == year:
yield dt, descriptions

This is also true for the loop above, but we probably should just iterate through the items, instead of the keys

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also this is kind of an edge-case, but if you define two holidays, one with a year and one without, that cover the same day of the year, you will now emit the same date twice, instead of one date that combines the description of the two. This is especially relevant if both variants have the same description.

In all we properly de-duplicate the dates, so this method should probably behave the same. It might then be also worth simplifying all by reusing other instead of duplicating those two loops, even if it is less efficient.

But we could also consider different ways to encode the same information, like changing _other to dict[tuple[int, int], dict[int | None, set[str]] (i.e. defaultdict(lambda: defaultdict(set))), you would then iterate through _other like before, but instead of directly getting descriptions you construct them for a given year by combining descriptions.get(None, set()) | descriptions.get(year, set()).

That way you retain the old single loop structure, you have to do a little bit of extra work to get the descriptions for a given year, but you have to do less work to de-duplicate the descriptions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay thanks I'll see what I can do.

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.

2 participants