torchbox / tbxforms Goto Github PK

I can see tox and coverage and a great suite of tests - would it be possible to document how to run the tests locally?

If I get some guidance here I'd be happy to make a PR myself for it. Also, I'd like to add the coverage report to the pipeline, and update contribution guidelines for checking tests are passing still.

On Date fields, previously if I set the default value to a string that’s not a date (for example "Test"), the form throws an error on display:

AttributeError at /form-page/
'str' object has no attribute 'day'

It’s thrown from:

This is because tbxforms expects the value to be a date.

Discussing this with @kbayliss, it seems like the best fix for this would be to add validation of the default values at the point of saving the form. We’ve decided this wasn’t a priority since default values for dates feels pretty niche.

Description

A form that has an optional DateInputField raises a ValueError if user submits the form leaving the date field blank.

Cause

This is caused by

Solution

The solution, unless I am missing something, is to simply check if data_list indeed contains 3 values, before unpacking them

if len(data_list) != 3:
    return None

Any changes to the styles and JS for https://github.com/alphagov/govuk-design-system have not been ported to tbxforms since it was created, so we're ~2yrs out of date.

Django 5.0 added the option for form choices to be defined using enums. Unfortunately, this results in the values being normalised to a plain tuple, which loses the .hint attribute.

The normalisation doesn't happen in all cases, and there are fixed types which bypass it. Perhaps we can extend one of those for Choices to allow it to bypass normalisation.

We have dozens of tests that render a form, field or fieldset and compare the output to a html file. One of the necessary changes that Balazs introduced causes 60+ of those tests to fail, which identifies what is probably the largest roadblock to progress on this project.

Also related to the test failures on #39

It's something we really need to solve as it makes maintenance of tbxforms a real drag - speaking from experience, as I manually edited (with the help of macros/regex in emacs) most of the html files to get the tests passing for the initial release.

Kyle suggested looking into something that works like Jest's snapshot tests, which is a great idea. We don't want to have to manually manage html fixtures, but we do care about the way forms are rendered, so we need to be able to review changes to rendered components and update the fixtures easily. I'm going to get a solution happening this week, which will hopefully make other progress easier.

• see if https://pypi.org/project/pytest-snapshot/ is fit for purpose
• if not, find an alternative

Description

When using Button.primary as a submit button, an AttributeError is raised:

*** AttributeError: 'Button' object has no attribute 'input_type'

Cause

This is caused by instantiating a button with the folowing:

Button.primary(
    name="submit",
    type="submit",
    value=self.action_text or _("Submit"),
)

Solution

I've went with using Submit from crispy_forms.layout as a substitute for Button.primary.

The tbxforms package defines a BaseForm that you would inherit from to use tbxforms in a given project: https://github.com/torchbox/tbxforms/blob/main/tbxforms/forms.py#L14

However, this isn't used for our tests - they define their own BaseForm - https://github.com/torchbox/tbxforms/blob/main/tests/forms.py#L12 - which undermines the entire test suite.

The test suite should at least inherit from the same base form class, even if additional changes are required to run the tests.

(Originally raised by @balazs-endresz here 🙌🏻)

Documentation seems to suggest that @use 'node_modules/tbxforms/style.css'; is used, however I ended up having to use @use 'node_modules/tbxforms/dist/style.css';

We have a use case where we want to provide a validation error with a link to some page. We use format_html()

Currently they are stripped out:

tbxforms defines strict dependencies which severely impacts the package's ability to be integrated with other projects.

An article (https://iscinumpy.dev/post/bound-version-constraints/) suggests removing upper bounds where possible, and the argument is very compelling.

TLDR:

Only add a cap if a dependency is known to be incompatible or there is a high (>75%) chance of it being incompatible in its next release.

As such, we should remove as many upper bounds as possible.

Originally raised by @balazs-endresz 🙌 .

At the moment I can add my own classes to the form itself and to any form field, if I use a Layout I can also add arbitrary attributes if needed. But it seems I can't change the label class unless I write a custom template.

It would be good to have a way to do this - a lot of the headaches with form styling comes when we need to nest CSS, so anything we can do to avoid this is good.

The following merged PRs to crispy-forms-gds can be pulled into our package:

• wildfish/crispy-forms-gds#59

• wildfish/crispy-forms-gds#76:

  • we will need to check the CSS class name for the divider markup
  • see https://design-system.service.gov.uk/components/radios/ - the divider should be used to separate answers that are conceptually different

From @thibaudcolas:

This doesn’t seem to be working. Looking at the code, I think this is because the [hidden=true] selector won’t work, should be [hidden].

In addition,

• Clearing the value on submit and reset inputs won’t be very desirable. It seems unlikely for us to use either, but I’d still prefer we don’t remove their label
• I haven’t been able to test it but I don’t think clearing a radio or checkbox works like the code suggests - you’d need to find which input is checked, then .checked = false.
• For select – the node.type I get is select-one, and I think it would be better to set selectedIndex to 0.

Using either TimeField or DateTimeField results in an unstyled/default field (i.e. one that does not look like the GOV.UK Design System).

This is because the GOV.UK Design System does not contain a pattern for time fields or datetime fields.

While there's no official pattern for these fields, alphagov/govuk-design-system-backlog#173 contains some suggestions on how to work around this until official support is (hopefully) added, which could be implemented within tbxforms as an interim solution.

Changelog: https://github.com/django-crispy-forms/django-crispy-forms/blob/main/CHANGELOG.md#20a1-2023-01-18. I don't think the template pack changes affect us, so just review the other items.

Review django-crispy-forms/django-crispy-forms#296 for background on removing the safe filter.

Suggest starting from Balazs's branch: #28

We'll need to push our minimum supported Django version to 3.2 the current LTS release.

• pyproject.toml is updated
• tests are passing

See:

• cookiecutter-wagtail-package's test action
• cookiecutter-wagtail-package's tox.ini
• Dan's comments on #24

It looks like pytest is the test runner used on this this project so we'll have to make sure tox calls pytest.

• Tox runs the tests for Pull Requests
• Tox runs the tests locally
• pytest is used as test runner
• pytest can be run locally on its own

https://github.com/torchbox/tbxforms/blob/main/tbxforms/static/sass/tbxforms.scss nests all the BEM classes created for the various form elements inside a .tbxforms class. This means that if you want to override the tbxforms styles, you have to write more nested code in the override.

.tbxforms   .tbxforms-error-message {
...
}

Is it possible to just remove the nesting in tbxforms.scss?

From @thibaudcolas:

The fields’ aria-describedby has the help text first, error messages second, while in the template we show the error messages first. I think both should match. I like the idea of having errors first, so the beginning of the field’s description is "Error:". Means we don’t need to additionally have aria-invalid="true".

Feedback from one of our projects to consider – currently we have text and number fields. It’s not necessarily clear what to use for phone numbers, though the GDS has guidance on this.

It’s not super clear to me whether this is an issue with this package or Django more generally or Wagtail forms specifically, but thought I’d open it there for further consideration.

Feedback from one of our projects. For us to consider as a feature to add directly in this package?

We built an anchor link navigation block which appears under the h1 of the form if you submit it with errors - with anchor links to the specific fields with errors.
You can see this in action if you submit a form on their site with all fields empty: https://www.rnib.org.uk/living-with-sight-loss/community-connection-and-wellbeing/sight-loss-counselling/online-counselling-form/

Building those kinds of error summaries is commonly considered an accessibility improvement (cognitive, mechanic).

This project’s Sass variables for color use the british colour spelling:

This is to match the govuk-frontend naming, which clashes with our usual style of using American spelling for Sass/CSS variables. Our style is due to the CSS language using the American spelling for CSS keywords (background-color).

results in OverflowError: signed integer is greater than maximum errors

Initially reported in #7. The conditional fields don’t work for me in IE11. There is ES6+ syntax used in the published code, which creates a syntax error in IE11:

class TbxForms {
  static selector() {
    return "form.tbxforms";
  }

It looks like Vite (esbuild) unfortunately doesn’t support compiling down to ES5 (what we’d need for IE11 compatibility), the build.target only goes down to es2015 (ES6).

The simplest way to fix this would be to rewrite the JS code to not use the class syntax (which isn’t that good to start with, and doesn’t do much for this project – just make a function initTbxForms(node) {}, and a separate clearInput function. I think this should work as long as esbuild doesn’t do any minification. We’ll need to make sure no other part of the code uses ES6+ syntax either (template literals, arrow functions, const, etc.).

Alternatively – we could switch over to Babel for the production build. For a single file like this, the setup is very straightforward with Rollup. See for example https://github.com/thibaudcolas/draftjs-filters/blob/main/rollup.config.js.

Note due to this I haven’t been able to test the things I was worried about initially (dispatchEvent).

Comment from @thibaudcolas:

tbxforms/tbxforms/forms.py

Line 3 in 381c250

from wagtail.contrib.forms.forms import FormBuilder

Do we really want Wagtail to be a dependency of tbxforms? It feels a bit odd to me, and makes the template pack somewhat harder to reuse and maintain since anyone using this will have to make sure their dependencies are compatible with those of Wagtail. Could we instead conditionally have this code (and the import) only run if wagtail.contrib.forms is in INSTALLED_APPS?

While tbxforms supports wagtail forms, it should not be a requirement for installing or using tbxforms.

We should add unit tests for TBXFORMS_ALLOW_HTML_LABEL, TBXFORMS_ALLOW_HTML_HELP_TEXT and TBXFORMS_ALLOW_HTML_BUTTON to prevent future regression.

From @thibaudcolas :

The templates make a lot of use of |safe, which makes me feel wary at the very least, and will be a source of XSS vulnerabilities when forms are defined via user input (the Wagtail form builder in particular).

Testing the current implementation of BaseWagtailFormBuilder and the templates,

• This makes it possible do an XSS injection via the fields’ label
• The buttons label also allows XSS injection (input.value|safe in button.html)
• The injection via help_text is prevented by WAGTAILFORMS_HELP_TEXT_ALLOW_HTML – so we’re good, but it’d be nice if similarly usage of |safe for help text was configurable within the template pack (rather than trusting it’s done upstream).
• For sites that make it possible to do fieldsets and legends (with implementations like wagtailstreamforms), I’m pretty sure there would be more injection opportunities.

So – I’d suggest:

1. Removing the |safe for anything that doesn’t strictly need it
2. For things that do need it, checking whether there might be more bespoke sanitisation we could introduce
3. Documenting which things we’d expect implementers of the template pack to sanitise themselves

The current implementation matches how Crispy Forms work out of the box (https://github.com/django-crispy-forms/django-crispy-forms/search?q=%7Csafe), though Crispy Forms doesn't assume the forms are editable via a CMS whereas ours could be.

Follow-up to #5, this time focusing on things I spotted in our integration.

Wagtail functionality

There is Wagtail-specific functionality here:

Do we really want Wagtail to be a dependency of tbxforms? It feels a bit odd to me, and makes the template pack somewhat harder to reuse and maintain since anyone using this will have to make sure their dependencies are compatible with those of Wagtail. Could we instead conditionally have this code (and the import) only run if wagtail.contrib.forms is in INSTALLED_APPS?

XSS injection vectors

The templates make a lot of use of |safe, which makes me feel wary at the very least, and will be a source of XSS vulnerabilities when forms are defined via user input (the Wagtail form builder in particular).

Testing the current implementation of BaseWagtailFormBuilder and the templates,

• This makes it possible do an XSS injection via the fields’ label
• The buttons label also allows XSS injection (input.value|safe in button.html)
• The injection via help_text is prevented by WAGTAILFORMS_HELP_TEXT_ALLOW_HTML – so we’re good, but it’d be nice if similarly usage of |safe for help text was configurable within the template pack (rather than trusting it’s done upstream).
• For sites that make it possible to do fieldsets and legends (with implementations like wagtailstreamforms), I’m pretty sure there would be more injection opportunities.

So – I’d suggest:

1. Removing the |safe for anything that doesn’t strictly need it
2. For things that do need it, checking whether there might be more bespoke sanitisation we could introduce
3. Documenting which things we’d expect implementers of the template pack to sanitise themselves

IE11 compatibility

The conditional fields don’t work for me in IE11. There is ES6+ syntax used in the published code, which creates a syntax error in IE11:

class TbxForms {
  static selector() {
    return "form.tbxforms";
  }

It looks like Vite (esbuild) unfortunately doesn’t support compiling down to ES5 (what we’d need for IE11 compatibility), the build.target only goes down to es2015 (ES6).

The simplest way to fix this would be to rewrite the JS code to not use the class syntax (which isn’t that good to start with, and doesn’t do much for this project – just make a function initTbxForms(node) {}, and a separate clearInput function. I think this should work as long as esbuild doesn’t do any minification. We’ll need to make sure no other part of the code uses ES6+ syntax either (template literals, arrow functions, const, etc.).

Alternatively – we could switch over to Babel for the production build. For a single file like this, the setup is very straightforward with Rollup. See for example https://github.com/thibaudcolas/draftjs-filters/blob/main/rollup.config.js.

Note due to this I haven’t been able to test the things I was worried about initially (dispatchEvent).

Conditional fields

I could spot a number of issues with this:

Reliance on JS

• The way fields are implemented, they will be visible if JS fails to execute on the page. It’s unclear to me whether this is intended or not.
• Due to this approach, when the page loads the conditional parts of the form are visible for a moment. I think it would be better if the fields were hidden on page load.

Clear hidden fields on submit

This doesn’t seem to be working. Looking at the code, I think this is because the [hidden=true] selector won’t work, should be [hidden].

In addition,

• Clearing the value on submit and reset inputs won’t be very desirable. It seems unlikely for us to use either, but I’d still prefer we don’t remove their label
• I haven’t been able to test it but I don’t think clearing a radio or checkbox works like the code suggests - you’d need to find which input is checked, then .checked = false.
• For select – the node.type I get select-one, and I think it would be better to set selectedIndex to 0.

ARIA

• The fields’ aria-describedby has the help text first, error messages second, while in the template we show the error messages first. I think both should match. I like the idea of having errors first, so the beginning of the field’s description is "Error:". Means we don’t need to additionally have aria-invalid="true".

Styles

• We should use the American spelling color for the API / variables, so it matches CSS. We tend to avoid british spellings for things that have corresponding CSS keywords or property names.