From c4fe90834e5f2777bad091f50bbb78a6af8a4ea4 Mon Sep 17 00:00:00 2001 From: Lance Edgar Date: Sat, 28 Dec 2024 18:56:04 -0600 Subject: [PATCH 1/4] fix: exclude FK fields by default, for model forms e.g. `person_uuid` and such --- src/wuttaweb/util.py | 39 +++++++++++++++++++++++++++++++++------ tests/test_util.py | 23 +++++++++++++++++------ 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/wuttaweb/util.py b/src/wuttaweb/util.py index 0697f03..f634c00 100644 --- a/src/wuttaweb/util.py +++ b/src/wuttaweb/util.py @@ -32,6 +32,7 @@ import uuid as _uuid import warnings import sqlalchemy as sa +from sqlalchemy import orm import colander from webhelpers2.html import HTML, tags @@ -478,24 +479,36 @@ def render_csrf_token(request, name='_csrf'): return HTML.tag('div', tags.hidden(name, value=token, id=None), style='display:none;') -def get_model_fields(config, model_class=None): +def get_model_fields(config, model_class, include_fk=False): """ Convenience function to return a list of field names for the given - model class. + :term:`data model` class. This logic only supports SQLAlchemy mapped classes and will use that to determine the field listing if applicable. Otherwise this returns ``None``. - """ - if not model_class: - return + :param config: App :term:`config object`. + + :param model_class: Data model class. + + :param include_fk: Whether to include foreign key column names in + the result. They are excluded by default, since the + relationship names are also included and generally preferred. + + :returns: List of field names, or ``None`` if it could not be + determined. + """ try: mapper = sa.inspect(model_class) except sa.exc.NoInspectionAvailable: return - fields = [prop.key for prop in mapper.iterate_properties] + if include_fk: + fields = [prop.key for prop in mapper.iterate_properties] + else: + fields = [prop.key for prop in mapper.iterate_properties + if not prop_is_fk(mapper, prop)] # nb. we never want the continuum 'versions' prop app = config.get_app() @@ -505,6 +518,20 @@ def get_model_fields(config, model_class=None): return fields +def prop_is_fk(mapper, prop): + """ """ + if not isinstance(prop, orm.ColumnProperty): + return False + + prop_columns = [col.name for col in prop.columns] + for rel in mapper.relationships: + rel_columns = [col.name for col in rel.local_columns] + if rel_columns == prop_columns: + return True + + return False + + def make_json_safe(value, key=None, warn=True): """ Convert a Python value as needed, to ensure it is compatible with diff --git a/tests/test_util.py b/tests/test_util.py index 6946d65..b8c7ba7 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -11,6 +11,8 @@ from fanstatic import Library, Resource from pyramid import testing from wuttjamaican.conf import WuttaConfig +from wuttjamaican.testing import ConfigTestCase + from wuttaweb import util as mod @@ -463,14 +465,10 @@ class TestGetFormData(TestCase): self.assertEqual(data, {'foo2': 'baz'}) -class TestGetModelFields(TestCase): - - def setUp(self): - self.config = WuttaConfig() - self.app = self.config.get_app() +class TestGetModelFields(ConfigTestCase): def test_empty_model_class(self): - fields = mod.get_model_fields(self.config) + fields = mod.get_model_fields(self.config, None) self.assertIsNone(fields) def test_unknown_model_class(self): @@ -482,6 +480,19 @@ class TestGetModelFields(TestCase): fields = mod.get_model_fields(self.config, model.Setting) self.assertEqual(fields, ['name', 'value']) + def test_include_fk(self): + model = self.app.model + + # fk excluded by default + fields = mod.get_model_fields(self.config, model.User) + self.assertNotIn('person_uuid', fields) + self.assertIn('person', fields) + + # fk can be included + fields = mod.get_model_fields(self.config, model.User, include_fk=True) + self.assertIn('person_uuid', fields) + self.assertIn('person', fields) + def test_avoid_versions(self): model = self.app.model From 171e9f7488a9a6861f7bb5163943d31f7027a8ef Mon Sep 17 00:00:00 2001 From: Lance Edgar Date: Sat, 28 Dec 2024 20:33:56 -0600 Subject: [PATCH 2/4] fix: add schema node type, widget for "money" (currency) fields --- src/wuttaweb/forms/schema.py | 22 +++++++++++++++++++++ src/wuttaweb/forms/widgets.py | 37 +++++++++++++++++++++++++++++++++++ tests/forms/test_schema.py | 9 +++++++++ tests/forms/test_widgets.py | 31 +++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+) diff --git a/src/wuttaweb/forms/schema.py b/src/wuttaweb/forms/schema.py index b5e7667..7591a7a 100644 --- a/src/wuttaweb/forms/schema.py +++ b/src/wuttaweb/forms/schema.py @@ -155,6 +155,28 @@ class WuttaEnum(colander.Enum): return widgets.SelectWidget(**kwargs) +class WuttaMoney(colander.Money): + """ + Custom schema type for "money" fields. + + This is a subclass of :class:`colander:colander.Money`, but uses + the custom :class:`~wuttaweb.forms.widgets.WuttaMoneyInputWidget` + by default. + + :param request: Current :term:`request` object. + """ + + def __init__(self, request, *args, **kwargs): + super().__init__(*args, **kwargs) + self.request = request + self.config = self.request.wutta_config + self.app = self.config.get_app() + + def widget_maker(self, **kwargs): + """ """ + return widgets.WuttaMoneyInputWidget(self.request, **kwargs) + + class WuttaSet(colander.Set): """ Custom schema type for :class:`python:set` fields. diff --git a/src/wuttaweb/forms/widgets.py b/src/wuttaweb/forms/widgets.py index 0fa8773..2541195 100644 --- a/src/wuttaweb/forms/widgets.py +++ b/src/wuttaweb/forms/widgets.py @@ -41,6 +41,7 @@ in the namespace: """ import datetime +import decimal import os import colander @@ -194,6 +195,42 @@ class WuttaDateTimeWidget(DateTimeInputWidget): return super().serialize(field, cstruct, **kw) +class WuttaMoneyInputWidget(MoneyInputWidget): + """ + Custom widget for "money" fields. This is used by default for + :class:`~wuttaweb.forms.schema.WuttaMoney` type nodes. + + The main purpose of this widget is to leverage + :meth:`~wuttjamaican:wuttjamaican.app.AppHandler.render_currency()` + for the readonly display. + + This is a subclass of + :class:`deform:deform.widget.MoneyInputWidget` and uses these + Deform templates: + + * ``moneyinput`` + + :param request: Current :term:`request` object. + """ + + def __init__(self, request, *args, **kwargs): + super().__init__(*args, **kwargs) + self.request = request + self.config = self.request.wutta_config + self.app = self.config.get_app() + + def serialize(self, field, cstruct, **kw): + """ """ + readonly = kw.get('readonly', self.readonly) + if readonly: + if cstruct in (colander.null, None): + return "" + cstruct = decimal.Decimal(cstruct) + return self.app.render_currency(cstruct) + + return super().serialize(field, cstruct, **kw) + + class FileDownloadWidget(Widget): """ Widget for use with :class:`~wuttaweb.forms.schema.FileDownload` diff --git a/tests/forms/test_schema.py b/tests/forms/test_schema.py index 7b15660..564e8c2 100644 --- a/tests/forms/test_schema.py +++ b/tests/forms/test_schema.py @@ -80,6 +80,15 @@ class TestWuttaEnum(WebTestCase): self.assertIsInstance(widget, widgets.SelectWidget) +class TestWuttaMoney(WebTestCase): + + def test_widget_maker(self): + enum = self.app.enum + typ = mod.WuttaMoney(self.request) + widget = typ.widget_maker() + self.assertIsInstance(widget, widgets.WuttaMoneyInputWidget) + + class TestObjectRef(DataTestCase): def setUp(self): diff --git a/tests/forms/test_widgets.py b/tests/forms/test_widgets.py index e324458..71f0dc0 100644 --- a/tests/forms/test_widgets.py +++ b/tests/forms/test_widgets.py @@ -1,6 +1,7 @@ # -*- coding: utf-8; -*- import datetime +import decimal from unittest.mock import patch import colander @@ -107,6 +108,36 @@ class TestWuttaDateTimeWidget(WebTestCase): self.assertEqual(result, '2024-12-12 13:49+0000') +class TestWuttaMoneyInputWidget(WebTestCase): + + def make_field(self, node, **kwargs): + # TODO: not sure why default renderer is in use even though + # pyramid_deform was included in setup? but this works.. + kwargs.setdefault('renderer', deform.Form.default_renderer) + return deform.Field(node, **kwargs) + + def make_widget(self, **kwargs): + return mod.WuttaMoneyInputWidget(self.request, **kwargs) + + def test_serialize(self): + node = colander.SchemaNode(WuttaDateTime()) + field = self.make_field(node) + widget = self.make_widget() + amount = decimal.Decimal('12.34') + + # editable widget has normal text input + result = widget.serialize(field, str(amount)) + self.assertIn(' Date: Sat, 28 Dec 2024 20:34:15 -0600 Subject: [PATCH 3/4] fix: use app handler to render error string, when progress fails --- pyproject.toml | 2 +- src/wuttaweb/progress.py | 5 ++++- tests/test_progress.py | 7 +++++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 0442f5b..75ad01b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,7 +44,7 @@ dependencies = [ "pyramid_tm", "waitress", "WebHelpers2", - "WuttJamaican[db]>=0.19.0", + "WuttJamaican[db]>=0.19.1", "zope.sqlalchemy>=1.5", ] diff --git a/src/wuttaweb/progress.py b/src/wuttaweb/progress.py index 759c2da..047be83 100644 --- a/src/wuttaweb/progress.py +++ b/src/wuttaweb/progress.py @@ -92,6 +92,9 @@ class SessionProgress(ProgressBase): """ def __init__(self, request, key, success_msg=None, success_url=None, error_url=None): + self.request = request + self.config = self.request.wutta_config + self.app = self.config.get_app() self.key = key self.success_msg = success_msg self.success_url = success_url @@ -137,7 +140,7 @@ class SessionProgress(ProgressBase): """ self.session.load() self.session['error'] = True - self.session['error_msg'] = str(error) + self.session['error_msg'] = self.app.render_error(error) self.session['error_url'] = error_url or self.error_url self.session.save() diff --git a/tests/test_progress.py b/tests/test_progress.py index 8bfce3c..a1e3908 100644 --- a/tests/test_progress.py +++ b/tests/test_progress.py @@ -5,6 +5,8 @@ from unittest import TestCase from pyramid import testing from beaker.session import Session as BeakerSession +from wuttjamaican.testing import ConfigTestCase + from wuttaweb import progress as mod @@ -31,10 +33,11 @@ class TestGetProgressSession(TestCase): self.assertEqual(session.id, 'mockid.progress.foo') -class TestSessionProgress(TestCase): +class TestSessionProgress(ConfigTestCase): def setUp(self): - self.request = testing.DummyRequest() + self.setup_config() + self.request = testing.DummyRequest(wutta_config=self.config) self.request.session.id = 'mockid' def test_error_url(self): From 84ab93108158c8ea4cd2b0c7269cbc229b38fd29 Mon Sep 17 00:00:00 2001 From: Lance Edgar Date: Sat, 28 Dec 2024 21:08:10 -0600 Subject: [PATCH 4/4] fix: include grid filters for all column properties of model class by default anyway. previous logic started from `grid.columns` and then only included column properties, but now we start from the model class itself and let sa-utils figure out the default list --- pyproject.toml | 1 + src/wuttaweb/grids/base.py | 22 ++++++++++------------ tests/grids/test_base.py | 11 +++++++++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 75ad01b..073ed88 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,6 +42,7 @@ dependencies = [ "pyramid_fanstatic", "pyramid_mako", "pyramid_tm", + "SQLAlchemy-Utils", "waitress", "WebHelpers2", "WuttJamaican[db]>=0.19.1", diff --git a/src/wuttaweb/grids/base.py b/src/wuttaweb/grids/base.py index ba402ce..b9f0de7 100644 --- a/src/wuttaweb/grids/base.py +++ b/src/wuttaweb/grids/base.py @@ -32,6 +32,7 @@ from collections import namedtuple, OrderedDict import sqlalchemy as sa from sqlalchemy import orm +from sqlalchemy_utils import get_columns import paginate from paginate_sqlalchemy import SqlalchemyOrmPage @@ -1116,19 +1117,16 @@ class Grid: filters = filters or {} if self.model_class: - # TODO: i tried using self.get_model_columns() here but in - # many cases that will be too aggressive. however it is - # often the case that the *grid* columns are a subset of - # the unerlying *table* columns. so until a better way - # is found, we choose "too few" instead of "too many" - # filters here. surely must improve it at some point. - for key in self.columns: - if key in filters: + # nb. i first tried self.get_model_columns() but my notes + # say that was too aggressive in many cases. then i tried + # using the *subset* of self.columns, just the ones which + # corresponded to a property on the model class. and now + # i am using sa-utils to give the "true" column list.. + for col in get_columns(self.model_class): + if col.key in filters: continue - prop = getattr(self.model_class, key, None) - if (prop and hasattr(prop, 'property') - and isinstance(prop.property, orm.ColumnProperty)): - filters[prop.key] = self.make_filter(prop) + prop = getattr(self.model_class, col.key) + filters[prop.key] = self.make_filter(prop) return filters diff --git a/tests/grids/test_base.py b/tests/grids/test_base.py index 786d038..f03aad2 100644 --- a/tests/grids/test_base.py +++ b/tests/grids/test_base.py @@ -982,6 +982,17 @@ class TestGrid(WebTestCase): self.assertEqual(filters['value'], 42) self.assertEqual(myfilters['value'], 42) + # filters for all *true* columns by default, despite grid.columns + with patch.object(mod.Grid, 'make_filter'): + # nb. filters are MagicMock instances + grid = self.make_grid(model_class=model.User, + columns=['username', 'person']) + filters = grid.make_backend_filters() + self.assertIn('username', filters) + self.assertIn('active', filters) + # nb. relationship not included by default + self.assertNotIn('person', filters) + def test_make_filter(self): model = self.app.model