Skip to content

Bugfix/swi 63 handle empty policy area#1933

Open
Tschuppi81 wants to merge 13 commits into
masterfrom
bugfix/swi-63-handle-empty-policy-area
Open

Bugfix/swi 63 handle empty policy area#1933
Tschuppi81 wants to merge 13 commits into
masterfrom
bugfix/swi-63-handle-empty-policy-area

Conversation

@Tschuppi81
Copy link
Copy Markdown
Contributor

SWI: Ignore empty policy area

TYPE: Bugfix
LINK: swi-63

Checklist

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

@Tschuppi81 Tschuppi81 requested a review from Daverball July 25, 2025 13:23
@Tschuppi81
Copy link
Copy Markdown
Contributor Author

Is there a way to influence the converters to handle this case?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 25, 2025

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.35%. Comparing base (03a3b42) to head (1aa3c36).
⚠️ Report is 4 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/onegov/swissvotes/models/policy_area.py 92.85% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/onegov/swissvotes/collections/votes.py 98.91% <100.00%> (+45.12%) ⬆️
src/onegov/swissvotes/converters.py 100.00% <100.00%> (ø)
src/onegov/swissvotes/forms/search.py 100.00% <100.00%> (+55.95%) ⬆️
src/onegov/swissvotes/path.py 97.91% <100.00%> (+24.58%) ⬆️
src/onegov/swissvotes/models/policy_area.py 98.71% <92.85%> (+53.40%) ⬆️

... and 107 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03a3b42...1aa3c36. 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.

@Daverball
Copy link
Copy Markdown
Member

Daverball commented Jul 29, 2025

Is there a way to influence the converters to handle this case?

Yes, you can write a custom converter for the policy area format, so any values that don't conform to the format get dropped.

Ideally we change the collection to accept list[PolicyArea] instead and write a converter for PolicyArea that verifies the format, split the string over . and make sure each component is numeric and starts with the previous component.

Copy link
Copy Markdown
Member

@Daverball Daverball left a comment

Choose a reason for hiding this comment

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

This is closer to what I suggested, but it's not quite there yet.

Comment thread src/onegov/core/filters.py
Comment thread src/onegov/swissvotes/collections/votes.py Outdated
Comment thread src/onegov/swissvotes/converters.py
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

Fixes a crash in the SwissVotes /votes view when the policy_area query parameter contains empty values by introducing a dedicated policy-area URL converter/validator and adjusting the collection/form code to work with PolicyArea objects.

Changes:

  • Add PolicyArea URL encode/decode support (dropping empty/invalid values) and use it for the /votes path converter.
  • Change SwissVoteCollection.policy_area from list[str] to list[PolicyArea] and update filtering + form model application accordingly.
  • Extend/update test suite to cover converter behavior and the “empty policy area” request scenario.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/onegov/user/test_auth.py Adds a gitleaks:allow marker for the Yubikey secret in tests.
tests/onegov/swissvotes/test_views_votes.py Adds regression test ensuring /votes doesn’t crash with an empty policy_area parameter.
tests/onegov/swissvotes/test_forms.py Updates search form tests to use PolicyArea objects for policy_area.
tests/onegov/swissvotes/test_converters.py Adds tests for policy-area URL encoding/decoding behavior.
tests/onegov/swissvotes/test_collections.py Updates collection tests to pass/expect PolicyArea objects for policy_area.
src/onegov/swissvotes/path.py Switches /votes query conversion for policy_area to a custom converter and drops empty entries.
src/onegov/swissvotes/models/policy_area.py Adds from_url_param validation and __hash__ for PolicyArea.
src/onegov/swissvotes/forms/search.py Ensures the form uses string values while the model uses PolicyArea objects.
src/onegov/swissvotes/converters.py Introduces policy_area_converter for Morepath URL param conversion.
src/onegov/swissvotes/collections/votes.py Updates filtering logic to consume PolicyArea objects directly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/onegov/swissvotes/models/policy_area.py
Comment thread src/onegov/swissvotes/path.py Outdated
Comment thread tests/onegov/swissvotes/test_views_votes.py Outdated
Comment thread tests/onegov/swissvotes/test_converters.py Outdated
Comment on lines +85 to +87
filtered_policy_area = [
p for p in policy_area if p
] if policy_area else None
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 don't think this step is necessary. I'm pretty sure morepath will discard any values that convert to None for list converters. Please verify. If it doesn't do that it might be necessary for the converter to raise ValueError when it doesn't see a valid PolicyArea.

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.

3 participants