Add class specialisations for using WTForms with the frontend toolkit #400

lfdebrux · 2018-06-04T10:28:16Z

The idea is to make it so that when using WTForms there is an unambiguous and obvious way of linking it up with the frontend toolkit macro.

To be clear that these are specialisations, the class names all have the 'DM' prefix.

Example

Say we want a set of radio buttons. In WTForms you would use RadioField, but now there is a DMRadioField that you can use. You create your form in the same way:

class DMRadioForm(FlaskForm):
    radio = DMRadioField(choices=[...])

The improvement comes when it is time to use it in the Jinja template. Behind the scenes DMRadioField uses a new widget, DMSelectionButtons, that is able to use the frontend toolkit macros. So, to use a DMRadioField object with the selection-buttons macro, the template code would be:

{{ form.radio }}

which (hopefully 🤞) is self-explanatory and easy to write.

You can also use the macros in full yourself, like this:

{%
  with
  name = form.radio.name,
  type = "radio",
  value = form.radio.value,
  error = form.radio.error,
  options = form.radio.options
%}
  {% include "toolkit/forms/selection-buttons.html" %}
{% endwith %}

which is still a bit of an improvement I think.

samuelhwilliams · 2018-06-04T16:15:53Z

Needs a version bump in dmutils/__init__. Either a minor, or a major if we want to say that forms must be updated to this standard when this is pulled in. In the latter case, we'll also need to update the changelog.

I'd probably prefer doing it as a major. 🤔 Even though it's not technically breaking backwards compatibility, we should try to get this standard applied to the frontend apps (as long as it can go in fairly easily). Open to being convinced it should just be a minor though.

lfdebrux · 2018-06-05T09:00:41Z

@samuelhwilliams I'm happy for it to be a major version if you are. I'm happy to start changing frontend apps myself, as far as I'm aware WTForms currently isn't used in many apps.

samuelhwilliams · 2018-06-05T10:47:32Z

I think it's used in 3 apps, so it shouldn't be awful to pull in. Hopefully easier once I get the existing validation masthead consolidation stuff in properly (for supplier and buyer...)

lfdebrux · 2018-06-05T10:56:56Z

Tests have been added, which resulted in some fixes, as tests do

DilwoarH · 2018-06-05T11:40:08Z

dmutils/forms/dm_fields.py

+
+
+class DMBooleanField(DMFieldMixin, wtforms.fields.BooleanField):
+    pass


should we include all of these classes that don't do anything?

So I included specialisations for all classes that I saw being used in the in the app codebase. Although the class body is empty, the fact that it inherits from the mixin means that the new class gets all the features of https://github.com/alphagov/digitalmarketplace-utils/blob/ldeb-wtforms-dmfields/dmutils/forms/dm_fields.py.

I'll add a docstring to the module that explains this.

DilwoarH · 2018-06-05T11:49:43Z

dmutils/forms/dm_fields.py

+                    option['description'] = choice[2]
+
+            # construct a choices argument suitable for wtforms.fields.RadioField
+            choices = [(option['value'], option['label']) for option in self.options]


might be more efficient to do the following:

self.options = [] wtforms_choices = [] for choice in choices: option = {} option['value'] = choice[0] option['label'] = choice[1] if len(choice) > 2 and choice[2]: option['description'] = choice[2] wtforms_choices.append( (option['value'], option['label']) ) super().__init__(label, validators, choices=wtforms_choices, **kwargs)

I'd prefer to keep the two separate as I think it's clearer to read. Is that okay with you?

lfdebrux · 2018-06-05T13:23:32Z

I'm going to do a rebase and force push now to clean up the commit history

galund · 2018-06-08T15:43:46Z

Would like to have a general discussion about this, but am vaguely thinking I'd be in favour of just having these replacing/augmenting forms.fields rather than in their own dm_fields thing. Appreciate others have expressed different views so we should definitely discuss before deciding.

samuelhwilliams · 2018-06-11T09:14:26Z

dmutils/forms/mixins.py

+    '''
+    def __init__(self, label=None, validators=None, hint=None, **kwargs):
+        self.hint = hint
+        self.question = label


It feels like this could get out of sync with the WTForm native label attribute. I think there are at least a few forms that update the label based on some dynamic context when the form is initialized.

Can we do a proxy on self.label.text using the @property decorator rather than copy the value across?

samuelhwilliams · 2018-06-11T09:50:39Z

dmutils/forms/mixins.py


        super().__init__(label=label, validators=validators, **kwargs)

+    @property
+    def question(self):
+        return self.label


This should probably be a proxy on self.label.text specifically. Using self.label will emit the text inside HTML <label> tags, which we probably wouldn't want in most cases.

Ah yes that makes sense

galund · 2018-06-11T10:41:51Z

dmutils/forms/fields.py

@@ -17,7 +17,7 @@
 class StripWhitespaceStringField(StringField):


So the suggestion now is that all of the implementation here should be moved into dm_fields classes; these classes would inherit from the dm_fields ones and would be deprecated.

Do we want to stick a DeprecationWarning on them?

Yeah, reckon a warning emitted by the constructor is the right thing here? And move any tests over from the old thing to the new thing.

samuelhwilliams · 2018-06-11T16:52:21Z

dmutils/forms/dm_fields.py

+                option['value'] = choice[0]
+                option['label'] = choice[1]
+                if len(choice) > 2 and choice[2]:
+                    option['description'] = choice[2]


It feels like we probably need to append the constructed option dict to self.options, unless I'm missing something magical. 🤔 I can't see where it would otherwise get persisted here.

samuelhwilliams · 2018-06-11T16:57:32Z

tests/forms/test_fields.py

+    (StripWhitespaceStringFieldForm(), StripWhitespaceStringField),
+))
+def test_form_with_field(form, field_class):
+    assert isinstance(form.field, field_class)


I'm not sure I understand what this is testing.

To be honest I don't remember.

I will take the test out.

galund · 2018-06-27T13:01:47Z

dmutils/forms/dm_fields.py

+
+
+class DMBooleanField(DMFieldMixin, wtforms.fields.BooleanField):
+    pass


From my experience in Crown-Commercial-Service/digitalmarketplace-buyer-frontend#798 we are missing some detail here required to make this work, specifically we need options I think.

Got it, will add 👍

Changes have been made

lfdebrux · 2018-07-02T14:54:39Z

I've just rebased the changes on master, and also added options to DMBooleanField as @galund suggested, and made this PR into a true breaking change by completely removing the old field code.

galund

Seems good to me.

I'd still have been tempted to keep both versions of the classes for a little while rather than have a breaking change, but doing it this way is fine, as long as you've got a clear idea of how much work will be needed across all the apps, and have gotten that agreed with @benvand.

We create 'DM' versions of common WTForms fields that have the DMFieldMixin in their MRO. We also remove our non-DM versions of custom fields, so this is a breaking change

This commit adds widgets for WTForms that render field html based on Jinja templates. By default the templates used are those in digitalmarketplace-frontend-toolkit.

risicle · 2018-08-06T10:57:48Z

dmutils/forms/mixins.py

+For more examples of these mixins in action, see `dmutils/forms/dm_fields.py`.
+
+For more about mixins in general, see this `online article`_,
+or the very excellent `Python Cookbook`_ by David Beazley.


insert amazon affiliate link

Also available at all good bookstores 😛

risicle · 2018-08-06T11:01:34Z

Would there be a neat way of adding a method to these field instances which will do all of the template-calling for us? So we could just do {{ someform.somefield.get_dmfrontend_widget() }}?

Or even just override wtforms standard way of doing this - __html__() or something?

lfdebrux · 2018-08-06T11:23:34Z

@risicle that functionality is already there, I'll update the documentation to reflect that

risicle

Neat

katstevens

Awesome stuff.

Do we know (roughly) the impact of the breaking changes - that is, how many apps currently use StripWhitespaceStringField, EmailField etc and how extensively?

lfdebrux · 2018-08-06T12:25:45Z

@katstevens 6 apps in total:

1. admin-frontend
2. brief-responses-frontend
3. supplier-frontend
4. buyer-frontend
5. briefs-frontend
6. user-frontend

I've already done the work for buyer-frontend here

lfdebrux requested review from samuelhwilliams, DilwoarH and brenetic June 4, 2018 10:28

lfdebrux force-pushed the ldeb-wtforms-dmfields branch 2 times, most recently from 9ba47fc to cb61614 Compare June 4, 2018 14:43

DilwoarH previously requested changes Jun 5, 2018

View reviewed changes

lfdebrux force-pushed the ldeb-wtforms-dmfields branch 2 times, most recently from 7202ed2 to 5815b65 Compare June 6, 2018 12:59

lfdebrux mentioned this pull request Jun 8, 2018

Tell us about your contract page Crown-Commercial-Service/digitalmarketplace-buyer-frontend#779

Merged

samuelhwilliams reviewed Jun 11, 2018

View reviewed changes

galund reviewed Jun 11, 2018

View reviewed changes

samuelhwilliams reviewed Jun 11, 2018

View reviewed changes

lfdebrux force-pushed the ldeb-wtforms-dmfields branch 2 times, most recently from 996632c to 3f75626 Compare June 15, 2018 09:05

galund mentioned this pull request Jun 27, 2018

"End search" becomes "before you export" Crown-Commercial-Service/digitalmarketplace-buyer-frontend#798

Merged

galund reviewed Jun 27, 2018

View reviewed changes

lfdebrux force-pushed the ldeb-wtforms-dmfields branch from 3f75626 to a42e3af Compare July 2, 2018 14:51

lfdebrux force-pushed the ldeb-wtforms-dmfields branch 2 times, most recently from b2d0cf8 to 22ebeb9 Compare July 2, 2018 15:58

galund approved these changes Jul 4, 2018

View reviewed changes

lfdebrux force-pushed the ldeb-wtforms-dmfields branch 4 times, most recently from 6360b10 to 0b1b296 Compare July 13, 2018 14:15

lfdebrux added 7 commits August 6, 2018 10:22

Add a mixin that make it easier to use WTForms with frontend toolkit

8ac5800

Add DM wrappers for the most commonly used WTForms fields

e637309

We create 'DM' versions of common WTForms fields that have the DMFieldMixin in their MRO. We also remove our non-DM versions of custom fields, so this is a breaking change

Add default hints

9a683f6

Add type attribute to form fields

dde2e98

Add DMSelectFieldMixin for making DMRadioField

72116ea

Add widgets for rendering WTForm fields from Jinja

7ce10f6

This commit adds widgets for WTForms that render field html based on Jinja templates. By default the templates used are those in digitalmarketplace-frontend-toolkit.

Add the ability to suppress question labels on form widgets

e99deab

lfdebrux force-pushed the ldeb-wtforms-dmfields branch from 0b1b296 to a8ce893 Compare August 6, 2018 09:28

lfdebrux requested review from risicle, katstevens, Wynndow and benvand and removed request for brenetic August 6, 2018 10:47

risicle reviewed Aug 6, 2018

View reviewed changes

Version bump (major)

84902c5

lfdebrux force-pushed the ldeb-wtforms-dmfields branch from a8ce893 to 84902c5 Compare August 6, 2018 11:31

risicle approved these changes Aug 6, 2018

View reviewed changes

katstevens approved these changes Aug 6, 2018

View reviewed changes

lfdebrux merged commit ff95fbc into master Aug 6, 2018

lfdebrux deleted the ldeb-wtforms-dmfields branch August 6, 2018 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add class specialisations for using WTForms with the frontend toolkit #400

Add class specialisations for using WTForms with the frontend toolkit #400

lfdebrux commented Jun 4, 2018 •

edited

Loading

samuelhwilliams commented Jun 4, 2018

lfdebrux commented Jun 5, 2018

samuelhwilliams commented Jun 5, 2018

lfdebrux commented Jun 5, 2018

DilwoarH Jun 5, 2018

lfdebrux Jun 5, 2018

DilwoarH Jun 5, 2018

lfdebrux Jun 5, 2018

lfdebrux commented Jun 5, 2018

galund commented Jun 8, 2018

samuelhwilliams Jun 11, 2018

lfdebrux Jun 11, 2018

samuelhwilliams Jun 11, 2018

lfdebrux Jun 11, 2018

galund Jun 11, 2018

lfdebrux Jun 11, 2018

galund Jun 11, 2018

lfdebrux Jun 12, 2018

samuelhwilliams Jun 11, 2018

lfdebrux Jun 12, 2018

samuelhwilliams Jun 11, 2018

lfdebrux Jun 12, 2018

galund Jun 27, 2018 •

edited

Loading

lfdebrux Jun 27, 2018

lfdebrux commented Jul 2, 2018

galund left a comment

risicle Aug 6, 2018

lfdebrux Aug 6, 2018 •

edited

Loading

risicle commented Aug 6, 2018

lfdebrux commented Aug 6, 2018

risicle left a comment

katstevens left a comment

lfdebrux commented Aug 6, 2018 •

edited

Loading



		class DMBooleanField(DMFieldMixin, wtforms.fields.BooleanField):
		pass

		@@ -17,7 +17,7 @@
		class StripWhitespaceStringField(StringField):

Add class specialisations for using WTForms with the frontend toolkit #400

Add class specialisations for using WTForms with the frontend toolkit #400

Conversation

lfdebrux commented Jun 4, 2018 • edited Loading

Example

samuelhwilliams commented Jun 4, 2018

lfdebrux commented Jun 5, 2018

samuelhwilliams commented Jun 5, 2018

lfdebrux commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfdebrux commented Jun 5, 2018

galund commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galund Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfdebrux commented Jul 2, 2018

galund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfdebrux Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

risicle commented Aug 6, 2018

lfdebrux commented Aug 6, 2018

risicle left a comment

Choose a reason for hiding this comment

katstevens left a comment

Choose a reason for hiding this comment

lfdebrux commented Aug 6, 2018 • edited Loading

lfdebrux commented Jun 4, 2018 •

edited

Loading

galund Jun 27, 2018 •

edited

Loading

lfdebrux Aug 6, 2018 •

edited

Loading

lfdebrux commented Aug 6, 2018 •

edited

Loading