Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/onegov/core/filters.py
Copy link
Member

Choose a reason for hiding this comment

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

I think you should merge master, this looks like changes that happened a while ago on master, so they should not be included in the diff.

Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
import os
import rcssmin # type:ignore[import-untyped]

from webassets.filter import Filter, register_filter # type:ignore
from webassets.filter.datauri import ( # type:ignore[import-untyped]
from webassets.filter import Filter, register_filter
from webassets.filter.datauri import (
CSSDataUri, CSSUrlRewriter)
from dukpy.webassets import BabelJSX # type:ignore[import-untyped]
from dukpy import jsx_compile # type:ignore[import-untyped]
Expand Down Expand Up @@ -49,10 +49,10 @@ def setup(self) -> None:
self.transformer = jsx_compile


register_filter(JsxFilter)
register_filter(JsxFilter) # type:ignore[no-untyped-call]


class DataUriFilter(CSSDataUri): # type:ignore[misc]
class DataUriFilter(CSSDataUri):
""" Overrides the default datauri filter to work around this issue:

https://github.com/miracle2k/webassets/issues/387
Expand All @@ -67,26 +67,28 @@ def input(self, _in: IO[str], out: IO[str], **kw: Any) -> None:
self.source_path = self.keywords['source_path']
self.output_path = self.keywords['output_path']

return super(CSSUrlRewriter, self).input(_in, out, **kw)
return super(CSSUrlRewriter, self).input(_in, out, **kw) # type:ignore[no-untyped-call]

@property
def source_url(self) -> str:
assert self.ctx is not None
return self.ctx.resolver.resolve_source_to_url(
self.ctx, self.keywords['source_path'], self.keywords['source'])

@property
def output_url(self) -> str:
assert self.ctx is not None
return self.ctx.resolver.resolve_output_to_url(
self.ctx, self.keywords['output'])


register_filter(DataUriFilter)
register_filter(DataUriFilter) # type:ignore[no-untyped-call]


class RCSSMinFilter(Filter): # type:ignore[misc]
class RCSSMinFilter(Filter):
""" Adds the rcssmin filter (not yet included in webassets) """

name = 'custom-rcssmin'
name = 'custom-rcssmin' # type:ignore[assignment]

def setup(self) -> None:
self.rcssmin = rcssmin
Expand All @@ -95,4 +97,4 @@ def output(self, _in: IO[str], out: IO[str], **kw: Any) -> None:
out.write(self.rcssmin.cssmin(_in.read()))


register_filter(RCSSMinFilter)
register_filter(RCSSMinFilter) # type:ignore[no-untyped-call]
4 changes: 4 additions & 0 deletions src/onegov/swissvotes/collections/votes.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ def __init__(
sort_by: str | None = None,
sort_order: str | None = None
) -> None:
# ignore empty policy areas swi-63
if policy_area and '' in policy_area:
policy_area = [i for i in policy_area if i]

Comment on lines +74 to +77
Copy link
Member

Choose a reason for hiding this comment

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

This check should go away if the converter is doing its job

super().__init__(page)
self.app = app
self.session = app.session()
Expand Down
50 changes: 50 additions & 0 deletions src/onegov/swissvotes/converters.py
Copy link
Member

Choose a reason for hiding this comment

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

This seems a little bit too complex for a converter, I would choose two simple converter functions like we do for things like datetime_converter and use the base class. Converter decode functions are allowed to raise exceptions IIRC, but you could also add simple try catch that returns None if splitting the components and turning them into integers fails.

The other issue is that you're converting from str to str. My idea was to convert to PolicyArea and move some of the validation into the PolicyArea constructor, this makes the API less error-prone to use in general, since the collection will now expect a PolicyArea instance rather than str, which has already been validated, with str it's still possible to pass in bogus data from somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not sure if this will actually work, since Converter.__init__ will replace your decode/encode methods with whatever you passed in when constructing the instance, which is str.

Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
from __future__ import annotations
import re
from typing import TYPE_CHECKING

import morepath


if TYPE_CHECKING:
PolicyAreaConverterBase = morepath.Converter[str]
else:
PolicyAreaConverterBase = morepath.Converter


class PolicyAreaListConverter(PolicyAreaConverterBase):

def verify_format(self, s: str) -> bool:
# verify is a number or in '1.13.136' format,
# no alphanumeric character allowed
return bool(re.fullmatch(r'\d+(\.\d+)*', s))

def verify_components(self, s: str) -> bool:
# verify that componenet starts with previous component if
# split over `.` valid '1.12.123' but invalid '1.22.123'
components = s.split('.')
if len(components) == 1:
return True

for component in components[1:]:
if not component.startswith(
components[components.index(component) - 1]):
return False

return True

def validate(self, s: str) -> bool:
""" Basically drops invalid policy areas. """
if not s:
return False

return self.verify_format(s) and self.verify_components(s)

def decode(self, s: str) -> list[str]: # type: ignore[override]
if not s:
return []
return [item for item in s if self.validate(item)]

def encode(self, l: list[str]) -> list[str]: # type: ignore[override]
if not l:
return []
return [item for item in l if item]
3 changes: 2 additions & 1 deletion src/onegov/swissvotes/path.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from onegov.swissvotes.app import SwissvotesApp
from onegov.swissvotes.collections import SwissVoteCollection
from onegov.swissvotes.collections import TranslatablePageCollection
from onegov.swissvotes.converters import PolicyAreaListConverter
from onegov.swissvotes.models import Principal
from onegov.swissvotes.models import SwissVote
from onegov.swissvotes.models import SwissVoteFile
Expand Down Expand Up @@ -54,7 +55,7 @@ def get_locale(app: SwissvotesApp, locale: str) -> SiteLocale | None:
'to_date': extended_date_converter,
'legal_form': [int],
'result': [int],
'policy_area': [str],
'policy_area': [PolicyAreaListConverter(str)],
'term': str,
'full_text': bool,
'position_federal_council': [int],
Expand Down
2 changes: 1 addition & 1 deletion stubs/more/webassets/directives.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ from collections.abc import Callable, Collection, Iterator, Mapping, Sequence
from typing_extensions import Self, TypeAlias

from dectate import Action
from webassets import Bundle, Environment # type: ignore[import-untyped]
from webassets import Bundle, Environment

_Filter: TypeAlias = str | Collection[str] | None

Expand Down
2 changes: 1 addition & 1 deletion stubs/more/webassets/tweens.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ from urllib.parse import unquote as unquote

from more.webassets.core import IncludeRequest
from morepath.request import Request, Response
from webassets import Environment # type: ignore[import-untyped]
from webassets import Environment

CONTENT_TYPES: set[str]
METHODS: set[str]
Expand Down
4 changes: 2 additions & 2 deletions tests/onegov/swissvotes/test_collections.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def test_votes_default(swissvotes_app):
to_date=4,
legal_form=5,
result=6,
policy_area=7,
policy_area=['7'],
term=8,
full_text=9,
position_federal_council=10,
Expand All @@ -114,7 +114,7 @@ def test_votes_default(swissvotes_app):
assert votes.to_date == 4
assert votes.legal_form == 5
assert votes.result == 6
assert votes.policy_area == 7
assert votes.policy_area == ['7']
assert votes.term == 8
assert votes.full_text == 9
assert votes.position_federal_council == 10
Expand Down
37 changes: 37 additions & 0 deletions tests/onegov/swissvotes/test_converters.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
from onegov.swissvotes.converters import PolicyAreaListConverter


def test_policy_area_converter():
converter = PolicyAreaListConverter(str)

assert converter.decode(['']) == []
assert converter.decode([None]) == []
assert converter.decode([]) == []
assert converter.decode(['1']) == ['1']
assert converter.decode(['1', '4']) == ['1', '4']
assert converter.decode(['1', '4', '8', '10']) == ['1', '4', '8', '10']
assert converter.decode(['1', '', '8', '22']) == ['1', '8', '22']
assert converter.decode(['1', '', '', '22']) == ['1', '22']

assert converter.encode(None) == []
assert converter.encode('') == []
assert converter.encode([]) == []
assert converter.encode(['1']) == ['1']
assert converter.encode(['21']) == ['21']
assert converter.encode(['1', '4']) == ['1', '4']
assert converter.encode(['1', '4', '8', '10']) == ['1', '4', '8', '10']

assert converter.decode(['1.12']) == ['1.12']
assert converter.decode(['1.12.121']) == ['1.12.121']
assert converter.decode(['4.42.421']) == ['4.42.421']
assert converter.decode(['10.102']) == ['10.102']
assert converter.decode(['10.103.1035']) == ['10.103.1035']
assert converter.decode(['12.125.1251']) == ['12.125.1251']
assert converter.decode(['1.12.123.1231']) == ['1.12.123.1231']

# invalid policy area(s)
assert converter.decode(['z']) == []
assert converter.decode(['1,12,121']) == []
assert converter.decode(['1.32.121']) == []
assert converter.decode(['4.92.421']) == []
assert converter.decode(['a.a2.a21']) == []
13 changes: 13 additions & 0 deletions tests/onegov/swissvotes/test_views_votes.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,3 +281,16 @@ def test_view_update_external_resources(mfg, sa, bs, swissvotes_app):

assert '15 hinzugefügt, 17 geändert, 19 gelöscht' in manage
assert 'Quellen konnten nicht aktualisiert werden: 4, 8, 9' in manage


def test_view_votes_empty_policy_area(swissvotes_app):
""" Ensure that the votes view does not crash when the policy area is empty
"""
client = Client(swissvotes_app)
client.get('/locale/de_CH').follow()

page = client.get('/votes')
assert page.status_code == 200

page = client.get('/votes?term=&policy_area=9&policy_area=')
assert page.status_code == 200