From 0092ce122c7e582bf352855e721ead088acbd092 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 26 Feb 2020 16:58:33 +0000 Subject: [PATCH 01/22] Bring in Jinja and Sass for checkboxes component --- app/assets/stylesheets/govuk-frontend/_all.scss | 1 + gulpfile.js | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/govuk-frontend/_all.scss b/app/assets/stylesheets/govuk-frontend/_all.scss index 9b8755367..6776fb95a 100644 --- a/app/assets/stylesheets/govuk-frontend/_all.scss +++ b/app/assets/stylesheets/govuk-frontend/_all.scss @@ -26,6 +26,7 @@ $govuk-assets-path: "/static/"; @import 'components/button/_button'; @import 'components/details/_details'; @import 'components/radios/_radios'; +@import 'components/checkboxes/_checkboxes'; @import "utilities/all"; @import "overrides/all"; diff --git a/gulpfile.js b/gulpfile.js index 60984c140..d8e098977 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -65,7 +65,12 @@ const copy = { 'footer', 'back-link', 'details', - 'button' + 'button', + 'error-message', + 'fieldset', + 'hint', + 'label', + 'checkboxes' ]; let done = 0; From 7b288ea51a5714a500e6b8e8d34f430a477ddb7e Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 2 Apr 2020 18:06:01 +0100 Subject: [PATCH 02/22] Add govukCheckboxesField govukCheckboxesField subclasses SelectMultipleField and overwrites how it renders HTML to let us use the GOVUK Checkboxes component while retaining all the functionality of WTForms fields. Based on work on github.com/richardjpope/recourse: https://github.com/richardjpope/recourse/blob/master/recourse/forms.py#L6 --- app/main/forms.py | 68 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 7 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 985f50bc6..cadda8a37 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -3,7 +3,7 @@ from datetime import datetime, timedelta from itertools import chain import pytz -from flask import request +from flask import Markup, render_template, request from flask_login import current_user from flask_wtf import FlaskForm as Form from flask_wtf.file import FileAllowed @@ -492,17 +492,71 @@ class RegisterUserFromOrgInviteForm(StripWhitespaceForm): auth_type = HiddenField('auth_type', validators=[DataRequired()]) -PermissionsAbstract = type("PermissionsAbstract", (StripWhitespaceForm,), { - permission: BooleanField(label) for permission, label in permissions -}) - - BroadcastPermissionsAbstract = type("BroadcastPermissionsAbstract", (StripWhitespaceForm,), { permission: BooleanField(label) for permission, label in broadcast_permissions }) -class BasePermissionsForm(StripWhitespaceForm): +# based on work done by @richardjpope: https://github.com/richardjpope/recourse/blob/master/recourse/forms.py#L6 +class govukCheckboxesField(SelectMultipleField): + + def __init__(self, label='', validators=None, param_extensions=None, **kwargs): + super(govukCheckboxesField, self).__init__(label, validators, **kwargs) + # default choices to a single True Boolean + if 'choices' not in kwargs: + self.choices = [('y', label)] + self.param_extensions = param_extensions + + # self.__call__ renders the HTML for the field by: + # 1. delegating to self.meta.render_field which + # 2. calls field.widget + # this bypasses that by making self.widget a method with the same interface as widget.__call__ + def widget(self, field, **kwargs): + items = [] + + # error messages + error_message = None + if field.errors: + error_message = {"text": " ".join(field.errors).strip()} + + # convert options to ones govuk understands + for option in field: + items.append({ + "name": option.name, + "id": option.id, + "text": option.label.text, + "value": option.data, + "checked": option.checked + }) + + params = { + 'idPrefix': field.id, + 'name': field.name, + 'errorMessage': error_message, + 'items': items + } + + # add HTML to group items if more than one + if len(items) > 1: + params.update({ + "fieldset": { + "attributes": {"id": field.name}, + "legend": { + "text": field.label.text, + "classes": "govuk-fieldset__legend--s" + } + } + }) + + # extend default params with any sent in + if self.param_extensions: + params.update(self.param_extensions) + + return Markup( + render_template('vendor/govuk-frontend/components/checkboxes/template.njk', params=params)) + + +class PermissionsForm(StripWhitespaceForm): def __init__(self, all_template_folders=None, *args, **kwargs): super().__init__(*args, **kwargs) self.folder_permissions.choices = [] From 38ad2e7e8657168cdf4e203a2370caf778731df3 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 23 Apr 2020 12:12:55 +0100 Subject: [PATCH 03/22] Add mixin & field to make collapsible checkboxes Allows checkboxes to be collapsed so they take up less space in the page. The collapsed state includes a live summary tracking which of them are selected. Includes changes to the JS for collapsible checkboxes to make it work with the GOVUK Checkboxes component HTML. --- .../javascripts/collapsibleCheckboxes.js | 14 +++++---- app/main/forms.py | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/collapsibleCheckboxes.js b/app/assets/javascripts/collapsibleCheckboxes.js index 11c14a2e4..0333b8dc3 100644 --- a/app/assets/javascripts/collapsibleCheckboxes.js +++ b/app/assets/javascripts/collapsibleCheckboxes.js @@ -5,7 +5,7 @@ function Summary (module) { this.module = module; - this.$el = module.$formGroup.find('.selection-summary'); + this.$el = module.$formGroup.find('.selection-summary').first(); this.fieldLabel = module.fieldLabel; this.total = module.total; this.addContent(); @@ -25,6 +25,7 @@ if (this.fieldLabel === 'folder') { this.$text.addClass('selection-summary__text--folders'); } this.$el.append(this.$text); + this.module.$formGroup.find('.govuk-hint').remove(); }; Summary.prototype.update = function(selection) { let template; @@ -86,12 +87,13 @@ .focus(); }; CollapsibleCheckboxes.prototype.start = function(component) { - this.$formGroup = $(component); - this.$fieldset = this.$formGroup.find('fieldset'); + this.$component = $(component); + this.$formGroup = this.$component.find('.govuk-form-group').first(); + this.$fieldset = this.$formGroup.find('fieldset').first(); this.$checkboxes = this.$fieldset.find('input[type=checkbox]'); - this.fieldLabel = this.$formGroup.data('fieldLabel'); + this.fieldLabel = this.$component.data('fieldLabel'); this.total = this.$checkboxes.length; - this.legendText = this.$fieldset.find('legend').text().trim(); + this.legendText = this.$fieldset.find('legend').first().text().trim(); this.expanded = false; this.addHeadingHideLegend(); @@ -113,7 +115,7 @@ }; CollapsibleCheckboxes.prototype.getSelection = function() { return this.$checkboxes.filter(':checked').length; }; CollapsibleCheckboxes.prototype.addHeadingHideLegend = function() { - const headingLevel = this.$formGroup.data('heading-level') || '2'; + const headingLevel = this.$component.data('heading-level') || '2'; this.$heading = $(`${this.legendText}`); this.$fieldset.before(this.$heading); diff --git a/app/main/forms.py b/app/main/forms.py index cadda8a37..2f28673ba 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -556,6 +556,36 @@ class govukCheckboxesField(SelectMultipleField): render_template('vendor/govuk-frontend/components/checkboxes/template.njk', params=params)) +# Extends fields using the govukCheckboxesField interface to wrap their render in HTML needed by the collapsible JS +class govukCollapsibleCheckboxesMixin: + def __init__(self, label='', validators=None, field_label='', param_extensions=None, **kwargs): + + self.field_label = field_label + + def widget(self, field, **kwargs): + + # add a blank hint to act as an ARIA live-region + if self.param_extensions is not None: + self.param_extensions.update( + {"hint": {"html": "
"}}) + else: + self.param_extensions = \ + {"hint": {"html": "
"}} + + # wrap the checkboxes HTML in the HTML needed by the collapisble JS + return Markup( + f'
' + f' {super(govukCollapsibleCheckboxesMixin, self).widget(field, **kwargs)}' + f'
' + ) + + +class govukCollapsibleCheckboxesField(govukCollapsibleCheckboxesMixin, govukCheckboxesField): + pass + + class PermissionsForm(StripWhitespaceForm): def __init__(self, all_template_folders=None, *args, **kwargs): super().__init__(*args, **kwargs) From 3ebb58219d98ff27d6f6f6ad5faca510fc1e295b Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 23 Apr 2020 12:15:24 +0100 Subject: [PATCH 04/22] Add govukCollapsibleNestedCheckboxesField Includes: 1. changes to make NestedFieldMixin work with new fields and CSS for nested checkboxes 2. adds custom version of GOVUK checkboxes component to allow us to: - add classes to elements currently inaccessible - wrap the checkboxes in a list - add child checkboxes to each checkbox (making tree structures possible through recursion Change 2. should be pushed upstream to the GOVUK Design System as a proposal for changes to the GOVUK Checkboxes component. --- .../stylesheets/components/checkboxes.scss | 28 ++++ app/main/forms.py | 89 ++++++++++-- .../forms/fields/checkboxes/macro.njk | 4 + .../forms/fields/checkboxes/template.njk | 134 ++++++++++++++++++ 4 files changed, 241 insertions(+), 14 deletions(-) create mode 100644 app/templates/forms/fields/checkboxes/macro.njk create mode 100644 app/templates/forms/fields/checkboxes/template.njk diff --git a/app/assets/stylesheets/components/checkboxes.scss b/app/assets/stylesheets/components/checkboxes.scss index 3986c65bf..094c37d7a 100644 --- a/app/assets/stylesheets/components/checkboxes.scss +++ b/app/assets/stylesheets/components/checkboxes.scss @@ -1,3 +1,7 @@ +// Taken from https://github.com/alphagov/govuk-frontend/blob/v2.13.0/src/components/checkboxes/_checkboxes.scss +$govuk-touch-target-size: 44px; +$govuk-checkboxes-size: 40px; + .selection-summary { .selection-summary__text { @@ -65,6 +69,30 @@ } +.govuk-form-group--nested { + + $border-thickness: $govuk-touch-target-size - $govuk-checkboxes-size; + $border-indent: $govuk-touch-target-size / 2; + + position: relative; + + // To equalise the spacing between the line and the top/bottom of + // the radio + margin-top: govuk-spacing(1) + ($border-thickness / 2); + margin-bottom: govuk-spacing(1) * -1; + padding-left: govuk-spacing(2) + 2; + + &:before { + content: ""; + position: absolute; + bottom: 0; + left: $border-indent * -1; + width: $border-thickness; + height: 100%; + background: $govuk-border-colour; + } +} + .selection-content { margin-bottom: govuk-spacing(4); diff --git a/app/main/forms.py b/app/main/forms.py index 2f28673ba..b57701aa3 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -17,6 +17,7 @@ from notifications_utils.recipients import ( normalise_phone_number, validate_phone_number, ) +from werkzeug.utils import cached_property from wtforms import ( BooleanField, DateField, @@ -322,13 +323,16 @@ class RadioFieldWithNoneOption(FieldWithNoneOption, RadioField): class NestedFieldMixin: + def children(self): + # start map with root option as a single child entry child_map = {None: [option for option in self if option.data == self.NONE_OPTION_VALUE]} # add entries for all other children for option in self: + # assign all options with a NONE_OPTION_VALUE (not always None) to the None key if option.data == self.NONE_OPTION_VALUE: child_ids = [ folder['id'] for folder in self.all_template_folders @@ -344,6 +348,47 @@ class NestedFieldMixin: return child_map + # to be used as the only version of .children once radios are converted + @cached_property + def _children(self): + return self.children() + + def get_items_from_options(self, field): + items = [] + + for option in self._children[None]: + item = self.get_item_from_option(option) + if option.data in self._children: + item['children'] = self.render_children(field.name, option.label.text, self._children[option.data]) + items.append(item) + + return items + + def render_children(self, name, label, options): + params = { + "name": name, + "fieldset": { + "legend": { + "text": label, + "classes": "govuk-visually-hidden" + } + }, + "formGroup": { + "classes": "govuk-form-group--nested" + }, + "asList": True, + "items": [] + } + for option in options: + item = self.get_item_from_option(option) + + if len(self._children[option.data]): + item['children'] = self.render_children(name, option.label.text, self._children[option.data]) + + params['items'].append(item) + + return render_template('forms/fields/checkboxes/template.njk', params=params) + class NestedRadioField(RadioFieldWithNoneOption, NestedFieldMixin): pass @@ -500,6 +545,8 @@ BroadcastPermissionsAbstract = type("BroadcastPermissionsAbstract", (StripWhites # based on work done by @richardjpope: https://github.com/richardjpope/recourse/blob/master/recourse/forms.py#L6 class govukCheckboxesField(SelectMultipleField): + render_as_list = False + def __init__(self, label='', validators=None, param_extensions=None, **kwargs): super(govukCheckboxesField, self).__init__(label, validators, **kwargs) # default choices to a single True Boolean @@ -507,30 +554,34 @@ class govukCheckboxesField(SelectMultipleField): self.choices = [('y', label)] self.param_extensions = param_extensions + def get_item_from_option(self, option): + return { + "name": option.name, + "id": option.id, + "text": option.label.text, + "value": str(option.data), # to protect against non-string types like uuids + "checked": option.checked + } + + def get_items_from_options(self, field): + return [self.get_item_from_option(option) for option in field] + # self.__call__ renders the HTML for the field by: # 1. delegating to self.meta.render_field which # 2. calls field.widget # this bypasses that by making self.widget a method with the same interface as widget.__call__ def widget(self, field, **kwargs): - items = [] # error messages error_message = None if field.errors: error_message = {"text": " ".join(field.errors).strip()} - # convert options to ones govuk understands - for option in field: - items.append({ - "name": option.name, - "id": option.id, - "text": option.label.text, - "value": option.data, - "checked": option.checked - }) + # returns either a list or a hierarchy of lists + # depending on how get_items_from_options is implemented + items = self.get_items_from_options(field) params = { - 'idPrefix': field.id, 'name': field.name, 'errorMessage': error_message, 'items': items @@ -545,21 +596,23 @@ class govukCheckboxesField(SelectMultipleField): "text": field.label.text, "classes": "govuk-fieldset__legend--s" } - } + }, + "asList": self.render_as_list }) # extend default params with any sent in - if self.param_extensions: + if self.param_extensions.keys(): params.update(self.param_extensions) return Markup( - render_template('vendor/govuk-frontend/components/checkboxes/template.njk', params=params)) + render_template('forms/fields/checkboxes/macro.njk', params=params)) # Extends fields using the govukCheckboxesField interface to wrap their render in HTML needed by the collapsible JS class govukCollapsibleCheckboxesMixin: def __init__(self, label='', validators=None, field_label='', param_extensions=None, **kwargs): + super(govukCollapsibleCheckboxesMixin, self).__init__(label, validators, param_extensions, **kwargs) self.field_label = field_label def widget(self, field, **kwargs): @@ -586,6 +639,14 @@ class govukCollapsibleCheckboxesField(govukCollapsibleCheckboxesMixin, govukChec pass +# govukCollapsibleCheckboxesMixin adds an ARIA live-region to the hint and wraps the render in HTML needed by the +# collapsible JS +# NestedFieldMixin puts the items into a tree hierarchy, pre-rendering the sub-trees of the top-level items +class govukCollapsibleNestedCheckboxesField(govukCollapsibleCheckboxesMixin, NestedFieldMixin, govukCheckboxesField): + NONE_OPTION_VALUE = None + render_as_list = True + + class PermissionsForm(StripWhitespaceForm): def __init__(self, all_template_folders=None, *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/app/templates/forms/fields/checkboxes/macro.njk b/app/templates/forms/fields/checkboxes/macro.njk new file mode 100644 index 000000000..c91452053 --- /dev/null +++ b/app/templates/forms/fields/checkboxes/macro.njk @@ -0,0 +1,4 @@ +{%- macro govukCheckboxes(params) %} + {%- include "./template.njk" -%} +{%- endmacro %} +{{ govukCheckboxes(params) }} diff --git a/app/templates/forms/fields/checkboxes/template.njk b/app/templates/forms/fields/checkboxes/template.njk new file mode 100644 index 000000000..2ac9cf68b --- /dev/null +++ b/app/templates/forms/fields/checkboxes/template.njk @@ -0,0 +1,134 @@ +{% from "components/error-message/macro.njk" import govukErrorMessage -%} +{% from "components/fieldset/macro.njk" import govukFieldset %} +{% from "components/hint/macro.njk" import govukHint %} +{% from "components/label/macro.njk" import govukLabel %} + + +{#- Copied from https://github.com/alphagov/govuk-frontend/blob/v2.13.0/src/components/checkboxes/template.njk + Changes: + - `classes` option added to `item` allow custom classes on the `.govuk-checkboxes__item` element + - `classes` option added to `item.hint` allow custom classes on the `.govuk-hint` element (added to GOVUK Frontend in v3.5.0 - remove when we update) + - `asList` option added the root `params` object to allow setting of the `.govuk-checkboxes` and `.govuk-checkboxes__item` element types + - `children` option added to `item` allowing the sending in of prerendered child checkboxes (allowing the creation of tree structures through recursion) -#} +{#- If an id 'prefix' is not passed, fall back to using the name attribute + instead. We need this for error messages and hints as well -#} +{% set idPrefix = params.idPrefix if params.idPrefix else params.name %} + +{#- a record of other elements that we need to associate with the input using + aria-describedby – for example hints or error messages -#} +{% set describedBy = params.describedBy if params.describedBy else "" %} +{% if params.fieldset.describedBy %} + {% set describedBy = params.fieldset.describedBy %} +{% endif %} + +{#- set the types of element used for the checkboxes and their group based on + whether asList is set -#} +{% if params.asList %} + {% set groupElement = 'ul' %} + {% set groupItemElement = 'li' %} +{% else %} + {% set groupElement = 'div' %} + {% set groupItemElement = 'div' %} +{% endif %} + +{% set isConditional = false %} +{% for item in params.items %} + {% if item.conditional %} + {% set isConditional = true %} + {% endif %} +{% endfor %} + +{#- fieldset is false by default -#} +{% set hasFieldset = true if params.fieldset else false %} + +{#- Capture the HTML so we can optionally nest it in a fieldset -#} +{% set innerHtml %} +{% if params.hint %} + {% set hintId = idPrefix + '-hint' %} + {% set describedBy = describedBy + ' ' + hintId if describedBy else hintId %} + {{ govukHint({ + id: hintId, + classes: params.hint.classes, + attributes: params.hint.attributes, + html: params.hint.html, + text: params.hint.text + }) | indent(2) | trim }} +{% endif %} +{% if params.errorMessage %} + {% set errorId = idPrefix + '-error' %} + {% set describedBy = describedBy + ' ' + errorId if describedBy else errorId %} + {{ govukErrorMessage({ + id: errorId, + classes: params.errorMessage.classes, + attributes: params.errorMessage.attributes, + html: params.errorMessage.html, + text: params.errorMessage.text, + visuallyHiddenText: params.errorMessage.visuallyHiddenText + }) | indent(2) | trim }} +{% endif %} + <{{ groupElement }} class="govuk-checkboxes {%- if params.classes %} {{ params.classes }}{% endif %}" + {%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %} + {%- if isConditional %} data-module="checkboxes"{% endif -%}> + {% for item in params.items %} + {% set id = item.id if item.id else idPrefix + "-" + loop.index %} + {% set name = item.name if item.name else params.name %} + {% set conditionalId = "conditional-" + id %} + {% set hasHint = true if item.hint.text or item.hint.html %} + {% set itemHintId = id + "-item-hint" if hasHint else "" %} + {% set itemDescribedBy = describedBy if not hasFieldset else "" %} + {% set itemDescribedBy = (itemDescribedBy + " " + itemHintId) | trim %} + <{{ groupItemElement }} class="govuk-checkboxes__item {%- if item.classes %} {{ item.classes }}{% endif %}"> + + {{ govukLabel({ + html: item.html, + text: item.text, + classes: 'govuk-checkboxes__label' + (' ' + item.label.classes if item.label.classes), + attributes: item.label.attributes, + for: id + }) | indent(6) | trim }} + {%- if hasHint %} + {{ govukHint({ + id: itemHintId, + classes: 'govuk-checkboxes__hint' + (' ' + item.hint.classes if item.hint.classes), + attributes: item.hint.attributes, + html: item.hint.html, + text: item.hint.text + }) | indent(6) | trim }} + {%- endif %} + {%- if item.children %} + {{ item.children | safe }} + {%- endif %} + {% if params.asList and item.conditional %} +
+ {{ item.conditional.html | safe }} +
+ {% endif %} + + {% if not params.asList and item.conditional %} +
+ {{ item.conditional.html | safe }} +
+ {% endif %} + {% endfor %} + +{% endset -%} + +
+{% if params.fieldset %} + {% call govukFieldset({ + describedBy: describedBy, + classes: params.fieldset.classes, + attributes: params.fieldset.attributes, + legend: params.fieldset.legend + }) %} + {{ innerHtml | trim | safe }} + {% endcall %} +{% else %} + {{ innerHtml | trim | safe }} +{% endif %} +
From 3f79881864f7b25289d7fdfe178aa6cf19f848d1 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 28 Apr 2020 17:20:09 +0100 Subject: [PATCH 05/22] Fix nested checkboxes with single top-level node Nested checkboxes with a single top-level node will only have one item in their `items` list. This is because the other choices are children of that list item. This means we need to check the `choices` attribute, which lists all the checkboxes, to see if they should be marked as a group (by being wrapped in a `
`) or not. --- app/main/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index b57701aa3..e054d9e4d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -588,7 +588,7 @@ class govukCheckboxesField(SelectMultipleField): } # add HTML to group items if more than one - if len(items) > 1: + if len(field.choices) > 1: params.update({ "fieldset": { "attributes": {"id": field.name}, From 38cc90a24b14ca78ec148eafcca1f5e05d1d5c73 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 7 May 2020 15:30:20 +0100 Subject: [PATCH 06/22] Add govukCheckboxField for single checkboxes Single checkboxes are distinct because: - they don't need to be wrapped in a `
` - they are a subclass of BooleanField so their data is either True or False --- app/main/forms.py | 99 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 81 insertions(+), 18 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e054d9e4d..28311e5b5 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -542,6 +542,73 @@ BroadcastPermissionsAbstract = type("BroadcastPermissionsAbstract", (StripWhites }) +class govukCheckboxField(BooleanField): + + def __init__(self, label='', validators=None, param_extensions=None, **kwargs): + super(govukCheckboxField, self).__init__(label, validators, false_values=None, **kwargs) + self.param_extensions = param_extensions + + def extend_params(self, params, extensions): + items = None + param_items = len(params['items']) if 'items' in params else 0 + + # split items off from params to make it a pure dict + if 'items' in extensions: + items = extensions['items'] + del extensions['items'] + + # merge dicts + params.update(extensions) + + # merge items + if items: + if 'items' not in params: + params['items'] = items + else: + for idx, _item in enumerate(items): + if idx >= param_items: + params['items'].append(items[idx]) + else: + params['items'][idx].update(items[idx]) + + # self.__call__ renders the HTML for the field by: + # 1. delegating to self.meta.render_field which + # 2. calls field.widget + # this bypasses that by making self.widget a method with the same interface as widget.__call__ + def widget(self, field, param_extensions=None, **kwargs): + + # error messages + error_message = None + if field.errors: + error_message = {"text": " ".join(field.errors).strip()} + + params = { + 'name': field.name, + 'errorMessage': error_message, + 'items': [ + { + "name": field.name, + "id": field.id, + "text": field.label.text, + "value": 'y', + "checked": field.data + } + ] + + } + + # extend default params with any sent in during instantiation + if self.param_extensions: + self.extend_params(params, self.param_extensions) + + # add any sent in though use in templates + if param_extensions: + self.extend_params(params, param_extensions) + + return Markup( + render_template('forms/fields/checkboxes/macro.njk', params=params)) + + # based on work done by @richardjpope: https://github.com/richardjpope/recourse/blob/master/recourse/forms.py#L6 class govukCheckboxesField(SelectMultipleField): @@ -549,9 +616,6 @@ class govukCheckboxesField(SelectMultipleField): def __init__(self, label='', validators=None, param_extensions=None, **kwargs): super(govukCheckboxesField, self).__init__(label, validators, **kwargs) - # default choices to a single True Boolean - if 'choices' not in kwargs: - self.choices = [('y', label)] self.param_extensions = param_extensions def get_item_from_option(self, option): @@ -583,26 +647,25 @@ class govukCheckboxesField(SelectMultipleField): params = { 'name': field.name, + "fieldset": { + "attributes": {"id": field.name}, + "legend": { + "text": field.label.text, + "classes": "govuk-fieldset__legend--s" + } + }, + "asList": self.render_as_list, 'errorMessage': error_message, 'items': items } - # add HTML to group items if more than one - if len(field.choices) > 1: - params.update({ - "fieldset": { - "attributes": {"id": field.name}, - "legend": { - "text": field.label.text, - "classes": "govuk-fieldset__legend--s" - } - }, - "asList": self.render_as_list - }) + # extend default params with any sent in during instantiation + if self.param_extensions: + self.extend_params(params, self.param_extensions) - # extend default params with any sent in - if self.param_extensions.keys(): - params.update(self.param_extensions) + # add any sent in though use in templates + if param_extensions: + self.extend_params(params, param_extensions) return Markup( render_template('forms/fields/checkboxes/macro.njk', params=params)) From 2092a045479c5a368285eb857ca7b45260fd13f1 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 7 May 2020 15:44:13 +0100 Subject: [PATCH 07/22] Split common checkbox methods off into mixin --- app/main/forms.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 28311e5b5..499c53210 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -542,11 +542,7 @@ BroadcastPermissionsAbstract = type("BroadcastPermissionsAbstract", (StripWhites }) -class govukCheckboxField(BooleanField): - - def __init__(self, label='', validators=None, param_extensions=None, **kwargs): - super(govukCheckboxField, self).__init__(label, validators, false_values=None, **kwargs) - self.param_extensions = param_extensions +class govukCheckboxesMixin: def extend_params(self, params, extensions): items = None @@ -571,6 +567,13 @@ class govukCheckboxField(BooleanField): else: params['items'][idx].update(items[idx]) + +class govukCheckboxField(govukCheckboxesMixin, BooleanField): + + def __init__(self, label='', validators=None, param_extensions=None, **kwargs): + super(govukCheckboxField, self).__init__(label, validators, false_values=None, **kwargs) + self.param_extensions = param_extensions + # self.__call__ renders the HTML for the field by: # 1. delegating to self.meta.render_field which # 2. calls field.widget @@ -610,7 +613,7 @@ class govukCheckboxField(BooleanField): # based on work done by @richardjpope: https://github.com/richardjpope/recourse/blob/master/recourse/forms.py#L6 -class govukCheckboxesField(SelectMultipleField): +class govukCheckboxesField(govukCheckboxesMixin, SelectMultipleField): render_as_list = False From 830aeae7b8b83b5263c2e59c2731fd65166bbd9c Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 2 Apr 2020 18:08:50 +0100 Subject: [PATCH 08/22] Update permissions page Includes adding filtering to the user permissions data. Classes extending BasePermissionsForm have their user permissions handled by permissions_field which stores its data in a list. This replaces the previous approach of having a BooleanField for each role. Because permissions_field.data is taken directly from POST data, it needs extra guarding against values not present in whatever roles model the class is based on (ie. broadcast_permissions). --- app/main/forms.py | 66 +++++++++++++------ .../views/manage-users/permissions.html | 14 +--- 2 files changed, 49 insertions(+), 31 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 499c53210..c7f5234ca 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -537,11 +537,6 @@ class RegisterUserFromOrgInviteForm(StripWhitespaceForm): auth_type = HiddenField('auth_type', validators=[DataRequired()]) -BroadcastPermissionsAbstract = type("BroadcastPermissionsAbstract", (StripWhitespaceForm,), { - permission: BooleanField(label) for permission, label in broadcast_permissions -}) - - class govukCheckboxesMixin: def extend_params(self, params, extensions): @@ -713,7 +708,23 @@ class govukCollapsibleNestedCheckboxesField(govukCollapsibleCheckboxesMixin, Nes render_as_list = True -class PermissionsForm(StripWhitespaceForm): +# guard against data entries that aren't a role in permissions +def filter_by_permissions(valuelist): + if valuelist is None: + return None + else: + return [entry for entry in valuelist if any(entry in role for role in permissions)] + + +# guard against data entries that aren't a role in broadcast_permissions +def filter_by_broadcast_permissions(valuelist): + if valuelist is None: + return None + else: + return [entry for entry in valuelist if any(entry in role for role in broadcast_permissions)] + + +class BasePermissionsForm(StripWhitespaceForm): def __init__(self, all_template_folders=None, *args, **kwargs): super().__init__(*args, **kwargs) self.folder_permissions.choices = [] @@ -723,7 +734,9 @@ class PermissionsForm(StripWhitespaceForm): (item['id'], item['name']) for item in ([{'name': 'Templates', 'id': None}] + all_template_folders) ] - folder_permissions = NestedCheckboxesField('Folders this team member can see') + folder_permissions = govukCollapsibleNestedCheckboxesField( + 'Folders this team member can see', + field_label='folder') login_authentication = RadioField( 'Sign in using', @@ -735,34 +748,49 @@ class PermissionsForm(StripWhitespaceForm): validators=[DataRequired()] ) - @property - def permissions(self): - return {field.id for field in self.permissions_fields if field.data is True} + permissions_field = govukCheckboxesField( + 'Permssions', + filters=[filter_by_permissions], + choices=[ + (value, label) for value, label in permissions + ], + param_extensions={ + "hint": {"text": "All team members can see sent messages."} + } + ) @property - def permissions_fields(self): - return ( - getattr(self, permission) for permission, field in self.__dict__.items() - if isinstance(field, BooleanField) - ) + def permissions(self): + return set(self.permissions_field.data) @classmethod def from_user(cls, user, service_id, **kwargs): return cls( **kwargs, **{ - role: user.has_permission_for_service(service_id, role) - for role in roles.keys() + "permissions_field": [ + role for role in roles.keys() if user.has_permission_for_service(service_id, role)] }, login_authentication=user.auth_type ) -class PermissionsForm(PermissionsAbstract, BasePermissionsForm): +class PermissionsForm(BasePermissionsForm): pass -class BroadcastPermissionsForm(BroadcastPermissionsAbstract, BasePermissionsForm): +class BroadcastPermissionsForm(BasePermissionsForm): + + permissions_field = govukCheckboxesField( + 'Permssions', + choices=[ + (value, label) for value, label in broadcast_permissions + ], + filters=[filter_by_broadcast_permissions], + param_extensions={ + "hint": {"text": "All team members can see sent messages."} + } + ) @property def permissions(self): diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index e6ee7637d..51169c37a 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -1,20 +1,10 @@ {% from "components/checkbox.html" import checkbox, checkboxes_nested %} {% from "components/radios.html" import radio, radios, conditional_radio_panel %} -
- - Permissions - - - All team members can see sent messages. - - {% for field in form.permissions_fields %} - {{ checkbox(field) }} - {% endfor %} -
+{{ form.permissions_field }} {% if form.folder_permissions.all_template_folders %} - {{ checkboxes_nested(form.folder_permissions, form.folder_permissions.children(), hide_legend=True, collapsible_opts={ 'field': 'folder' }) }} + {{ form.folder_permissions }} {% elif user and user.platform_admin %}

Platform admin users can access all template folders. From 3956d4f5fab238bc1c0bd783f6f1e0e70bda56a8 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 9 Apr 2020 20:56:17 +0100 Subject: [PATCH 09/22] Update manage folder page --- app/main/forms.py | 4 +++- app/templates/views/templates/manage-template-folder.html | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index c7f5234ca..cac7e250f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1895,7 +1895,9 @@ class TemplateFolderForm(StripWhitespaceForm): (item.id, item.name) for item in all_service_users ] - users_with_permission = MultiCheckboxField('Team members who can see this folder') + users_with_permission = govukCollapsibleCheckboxesField( + 'Team members who can see this folder', + field_label='folder') name = StringField('Folder name', validators=[DataRequired(message='Cannot be empty')]) diff --git a/app/templates/views/templates/manage-template-folder.html b/app/templates/views/templates/manage-template-folder.html index ddc20df80..6c817014b 100644 --- a/app/templates/views/templates/manage-template-folder.html +++ b/app/templates/views/templates/manage-template-folder.html @@ -26,7 +26,7 @@ {% call form_wrapper(action=url_for('main.manage_template_folder', service_id=current_service.id, template_folder_id=template_folder_id)) %} {{ textbox(form.name, width='1-1') }} {% if current_user.has_permissions("manage_service") and form.users_with_permission.all_service_users %} - {{ checkboxes(form.users_with_permission, collapsible_opts={ 'field': 'team member' }) }} + {{ form.users_with_permission }} {% endif %} {{ page_footer( From 03240b21d57d1d3ff434192a72ba13f83ed344f2 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 9 Apr 2020 11:51:11 +0100 Subject: [PATCH 10/22] Update templates page Includes: - changes to the govukCheckboxesField class to allow params to be extended at render time - updates to templates and folders CSS --- .../stylesheets/components/message.scss | 34 +++++--- app/main/forms.py | 40 +++++++++- .../views/templates/_template_list.html | 79 ++++++++++++------- 3 files changed, 108 insertions(+), 45 deletions(-) diff --git a/app/assets/stylesheets/components/message.scss b/app/assets/stylesheets/components/message.scss index 3d63ff455..14e02ebf7 100644 --- a/app/assets/stylesheets/components/message.scss +++ b/app/assets/stylesheets/components/message.scss @@ -24,6 +24,9 @@ } } +$message-text-left-spacing: 22px; +$message-type-bottom-spacing: govuk-spacing(4); + .message { &-name { @@ -63,23 +66,14 @@ } &-type { - color: $secondary-text-colour; - margin: 0 0 govuk-spacing(4) 0; + color: $govuk-secondary-text-colour; + margin: 0 0 $message-type-bottom-spacing 0; + padding-left: 0; pointer-events: none; } } -#template-list { - - margin-top: govuk-spacing(6); - - &.top-gutter-5px { - margin-top: 5px; - } - -} - .template-list { &-item { @@ -123,6 +117,22 @@ } + &-hint, + &-label { + padding-left: $message-text-left-spacing; + } + + &-label { + padding-top: 0px; + padding-bottom: 0px; + } + + // Fix for GOVUK Frontend selector with high precendence + // https://github.com/alphagov/govuk-frontend/blob/v2.13.0/src/components/hint/_hint.scss + &-label:not(.govuk-label--m):not(.govuk-label--l):not(.govuk-label--xl)+.template-list-item-hint { + margin-bottom: $message-type-bottom-spacing; + } + } &-folder { diff --git a/app/main/forms.py b/app/main/forms.py index cac7e250f..2fb9ee3c4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -628,11 +628,34 @@ class govukCheckboxesField(govukCheckboxesMixin, SelectMultipleField): def get_items_from_options(self, field): return [self.get_item_from_option(option) for option in field] + def extend_params(self, params, extensions): + items = None + param_items = len(params['items']) if 'items' in params else 0 + + # split items off from params to make it a pure dict + if 'items' in extensions: + items = extensions['items'] + del extensions['items'] + + # merge dicts + params.update(extensions) + + # merge items + if items: + if 'items' not in params: + params['items'] = items + else: + for idx, _item in enumerate(items): + if idx >= param_items: + params['items'].append(items[idx]) + else: + params['items'][idx].update(items[idx]) + # self.__call__ renders the HTML for the field by: # 1. delegating to self.meta.render_field which # 2. calls field.widget # this bypasses that by making self.widget a method with the same interface as widget.__call__ - def widget(self, field, **kwargs): + def widget(self, field, param_extensions=None, **kwargs): # error messages error_message = None @@ -2002,9 +2025,18 @@ class TemplateAndFoldersSelectionForm(Form): return self.move_to_new_folder_name.data return None - templates_and_folders = MultiCheckboxField('Choose templates or folders', validators=[ - required_for_ops('move-to-new-folder', 'move-to-existing-folder') - ]) + templates_and_folders = govukCheckboxesField( + 'Choose templates or folders', + validators=[required_for_ops('move-to-new-folder', 'move-to-existing-folder')], + choices=[], # added to keep order of arguments, added properly in __init__ + param_extensions={ + "fieldset": { + "legend": { + "classes": "govuk-visually-hidden" + } + } + } + ) # if no default set, it is set to None, which process_data transforms to '__NONE__' # this means '__NONE__' (self.ALL_TEMPLATES option) is selected when no form data has been submitted # set default to empty string so process_data method doesn't perform any transformation diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index 5bcb03501..42483d33a 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -21,37 +21,58 @@ {% endif %}

{% else %} -
+ `; - formGroup = document.querySelector('.form-group'); + wrapper = document.querySelector('.selection-wrapper'); + formGroup = wrapper.querySelector('.govuk-form-group'); fieldset = formGroup.querySelector('fieldset'); - checkboxesContainer = fieldset.querySelector('.checkboxes-nested'); + checkboxesContainer = fieldset.querySelector('.govuk-checkboxes'); checkboxes = checkboxesContainer.querySelectorAll('input[type=checkbox]'); }); @@ -141,6 +144,12 @@ describe('Collapsible fieldset', () => { }); + test("removes the hint", () => { + + expect(document.querySelector('.govuk-hint')).toBeNull(); + + }); + }); test('has the right summary text when started with no checkboxes selected', () => { @@ -187,7 +196,7 @@ describe('Collapsible fieldset', () => { test("the summary doesn't have a folder icon if fields aren't called 'folder'", () => { - formGroup.dataset.fieldLabel = 'team member'; + wrapper.dataset.fieldLabel = 'team member'; // start module window.GOVUK.modules.start(); @@ -277,35 +286,74 @@ describe('Collapsible fieldset', () => { describe("the footer (that wraps the button)", () => { - beforeEach(() => { + describe("is inserted", () => { - // track calls to sticky JS - window.GOVUK.stickAtBottomWhenScrolling.recalculate = jest.fn(() => {}); + test("after the fieldset", () => { - // start module - window.GOVUK.modules.start(); + // start module + window.GOVUK.modules.start(); - // show the checkboxes - helpers.triggerEvent(formGroup.querySelector('.govuk-button'), 'click'); + // show the checkboxes + helpers.triggerEvent(formGroup.querySelector('.govuk-button'), 'click'); + + expect(formGroup.querySelector('.selection-footer').previousElementSibling.nodeName).toBe('FIELDSET'); + + }); + + test("after the root fieldset if the checkboxes are nested", () => { + + // add a nested list of checkboxes to the first checkbox item + const nestedCheckboxes = document.createElement('div'); + nestedCheckboxes.className = 'govuk-form-group govuk-form-group--nested'; + nestedCheckboxes.innerHTML = _checkboxes(11, 20); + checkboxesContainer.querySelector('.govuk-checkboxes__item').appendChild(nestedCheckboxes); + + // start module + window.GOVUK.modules.start(); + + // show the checkboxes + helpers.triggerEvent(formGroup.querySelector('.govuk-button'), 'click'); + + expect(formGroup.querySelector('.selection-footer').previousElementSibling.nodeName).toBe('FIELDSET'); + + }); }); - test("is made sticky when the fieldset is expanded", () => { + describe("its stickiness", () => { - expect(formGroup.querySelector('.selection-footer').classList.contains('js-stick-at-bottom-when-scrolling')).toBe(true); - expect(window.GOVUK.stickAtBottomWhenScrolling.recalculate.mock.calls.length).toBe(1); + beforeEach(() => { + + // track calls to sticky JS + window.GOVUK.stickAtBottomWhenScrolling.recalculate = jest.fn(() => {}); + + // start module + window.GOVUK.modules.start(); + + // show the checkboxes + helpers.triggerEvent(formGroup.querySelector('.govuk-button'), 'click'); + + }); + + test("is added when the fieldset is expanded", () => { + + expect(formGroup.querySelector('.selection-footer').classList.contains('js-stick-at-bottom-when-scrolling')).toBe(true); + expect(window.GOVUK.stickAtBottomWhenScrolling.recalculate.mock.calls.length).toBe(1); + + }); + + test("is removed when the fieldset is collapsed", () => { + + // click the button to collapse the fieldset + helpers.triggerEvent(formGroup.querySelector('.govuk-button'), 'click'); + + expect(formGroup.querySelector('.selection-footer').classList.contains('js-stick-at-bottom-when-scrolling')).toBe(false); + expect(window.GOVUK.stickAtBottomWhenScrolling.recalculate.mock.calls.length).toBe(2); + + }); }); - test("has its stickiness removed when the fieldset is collapsed", () => { - - // click the button to collapse the fieldset - helpers.triggerEvent(formGroup.querySelector('.govuk-button'), 'click'); - - expect(formGroup.querySelector('.selection-footer').classList.contains('js-stick-at-bottom-when-scrolling')).toBe(false); - expect(window.GOVUK.stickAtBottomWhenScrolling.recalculate.mock.calls.length).toBe(2); - - }); }); describe("when the selection changes", () => { @@ -339,7 +387,7 @@ describe('Collapsible fieldset', () => { test("if fields are called 'folders'", () => { - formGroup.dataset.fieldLabel = 'folder'; + wrapper.dataset.fieldLabel = 'folder'; checkFirstCheckbox(); @@ -359,7 +407,7 @@ describe('Collapsible fieldset', () => { test("if fields are called 'team member'", () => { - formGroup.dataset.fieldLabel = 'team member'; + wrapper.dataset.fieldLabel = 'team member'; checkFirstCheckbox(); @@ -379,7 +427,7 @@ describe('Collapsible fieldset', () => { test("if fields are called 'arbitrary thing'", () => { - formGroup.dataset.fieldLabel = 'arbitrary thing'; + wrapper.dataset.fieldLabel = 'arbitrary thing'; checkFirstCheckbox(); @@ -403,7 +451,7 @@ describe('Collapsible fieldset', () => { test("if fields are called 'folder'", () => { - formGroup.dataset.fieldLabel = 'folder'; + wrapper.dataset.fieldLabel = 'folder'; checkAllCheckboxes(); @@ -423,7 +471,7 @@ describe('Collapsible fieldset', () => { test("if fields are called 'team member'", () => { - formGroup.dataset.fieldLabel = 'team member'; + wrapper.dataset.fieldLabel = 'team member'; checkAllCheckboxes(); @@ -447,7 +495,7 @@ describe('Collapsible fieldset', () => { test("if fields are called 'folder'", () => { - formGroup.dataset.fieldLabel = 'folder'; + wrapper.dataset.fieldLabel = 'folder'; checkAllCheckboxesButTheLast(); @@ -466,7 +514,7 @@ describe('Collapsible fieldset', () => { test("if fields are called 'team member'", () => { - formGroup.dataset.fieldLabel = 'team member'; + wrapper.dataset.fieldLabel = 'team member'; checkAllCheckboxesButTheLast(); From 0f9e4c813aeae4f3794374aabeba055804169333 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 26 May 2020 14:51:16 +0100 Subject: [PATCH 14/22] Change HTML for template list items Moves the link out of the label and increases the hit-size for the checkbox. The intention is to reduce the chance of clicking the wrong thing by accident. This includes a TODO in the checkboxes component template code. The item meta needs to be associated with the checkbox input by use of `aria-describedby` but this needs changes in govuk-frontend-jinja to happen. --- .../stylesheets/components/message.scss | 29 +++++++++++++------ .../forms/fields/checkboxes/template.njk | 2 ++ .../views/templates/_template_list.html | 28 +++++++++++------- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/app/assets/stylesheets/components/message.scss b/app/assets/stylesheets/components/message.scss index c4dc7bff4..d783f7d5b 100644 --- a/app/assets/stylesheets/components/message.scss +++ b/app/assets/stylesheets/components/message.scss @@ -24,13 +24,13 @@ } } -$message-text-left-spacing: 22px; +$govuk-checkboxes-size: 40px; +$govuk-checkboxes-label-padding-left-right: govuk-spacing(3); $message-type-bottom-spacing: govuk-spacing(4); .message { &-name { - @include bold-24; margin: 0; a { @@ -78,6 +78,10 @@ $message-type-bottom-spacing: govuk-spacing(4); &-item { + &-with-checkbox { + padding-left: $govuk-checkboxes-size + $govuk-checkboxes-label-padding-left-right; + } + &-hidden-by-default { display: none; } @@ -105,14 +109,12 @@ $message-type-bottom-spacing: govuk-spacing(4); } - &-hint, &-label { - padding-left: $message-text-left-spacing; - } - - &-label { - padding-top: 0px; - padding-bottom: 0px; + position: absolute; + left: 0; + width: $govuk-checkboxes-size + $govuk-checkboxes-label-padding-left-right; + height: 100%; + padding: 0; } // Fix for GOVUK Frontend selector with high precendence @@ -121,6 +123,15 @@ $message-type-bottom-spacing: govuk-spacing(4); margin-bottom: $message-type-bottom-spacing; } + &-hint { + padding-left: 0; + } + + } + + &-folder, + &-template { + @include govuk-font($size: 24, $weight: bold, $line-height: 1.25); } &-folder { diff --git a/app/templates/forms/fields/checkboxes/template.njk b/app/templates/forms/fields/checkboxes/template.njk index 2ac9cf68b..ddd83e88c 100644 --- a/app/templates/forms/fields/checkboxes/template.njk +++ b/app/templates/forms/fields/checkboxes/template.njk @@ -78,6 +78,7 @@ {% set itemDescribedBy = describedBy if not hasFieldset else "" %} {% set itemDescribedBy = (itemDescribedBy + " " + itemHintId) | trim %} <{{ groupItemElement }} class="govuk-checkboxes__item {%- if item.classes %} {{ item.classes }}{% endif %}"> + {%- if item.before %}{{ item.before }}{% endif -%} {% endif %} + {%- if item.after %}{{ item.after }}{% endif -%} {% if not params.asList and item.conditional %}
diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index 2121a011e..0c7f4125c 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -25,7 +25,7 @@ {% for item in template_list %} - {% set item_label_content %} + {% set item_link_content %} {% for ancestor in item.ancestors %} {{- format_item_name(ancestor.name) -}} @@ -42,26 +42,34 @@ {% endif %} {% endset %} + {% set label_content %} + {{ format_item_name(item.name) }} + {% endset %} + + {% set item_meta %} + + {{ item.hint }} + + {% endset %} + {# create the item config now to include the label content -#} + {# TODO: "attributes": { "aria-describedby": item.id ~ "-hint" } needs to be added but govuk-frontend-jinja doesn't currently support this -#} {% set checkbox_config = { - "html": item_label_content, + "html": label_content, "label": { - "classes": "template-list-item-label govuk-!-font-size-24 govuk-!-font-weight-bold" + "classes": "template-list-item-label", }, "id": "templates-or-folder-" ~ item.id, - "hint": { - "text": item.hint, - "classes": "template-list-item-hint" - }, - "classes": "template-list-item {}".format( - "template-list-item-hidden-by-default" if item.ancestors else "template-list-item-without-ancestors") + "classes": "template-list-item template-list-item-with-checkbox {}".format( + "template-list-item-hidden-by-default" if item.ancestors else "template-list-item-without-ancestors"), + "after": item_link_content ~ item_meta } %} {% set _ = checkboxes_data.append(checkbox_config) %} {% if not current_user.has_permissions('manage_templates') %}

- {{ item_label_content }} + {{ item_link_content }}

{{ item.hint }} From ee03753187c418036ea6089b55903d601cf8f8cb Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 9 Jun 2020 16:36:52 +0100 Subject: [PATCH 15/22] Make template-list checkbox label text match link The checkboxes need an accessible name that identifies the folder/template and this needs to include their full path to avoid duplication. There's a lot of debate about how to write out breadcrumb/path syntax so this just puts all the words together under the assumption that the folder naming will describe the path (and to introduce as little extra semantics as possible to start with). --- .../views/templates/_template_list.html | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index 0c7f4125c..f17fcfb07 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -1,12 +1,15 @@ {% from "components/message-count-label.html" import folder_contents_count, message_count_label %} -{% macro format_item_name(name) -%} +{% macro format_item_name(name, separators=True) -%} {%- if name is string -%} {{- name -}} {%- else -%} {%- for part in name -%} - {{- format_item_name(part) -}} - {%- if not loop.last %} {% endif -%} + {{- format_item_name(part, separators) -}} + {%- if not loop.last -%} + {%- if separators %} + {%- else %} {% endif -%} + {% endif -%} {%- endfor -%} {% endif %} {%- endmacro %} @@ -33,17 +36,20 @@ {% endfor %} {% if item.is_folder %} - {{ format_item_name(item.name) }} + {{- format_item_name(item.name) -}} {% else %} - {{ format_item_name(item.name) }} + {{- format_item_name(item.name) -}} {% endif %} {% endset %} {% set label_content %} - {{ format_item_name(item.name) }} + + {%- for ancestor in item.ancestors %}{{ format_item_name(ancestor.name, separators=False) }} {% endfor -%} + {{ format_item_name(item.name, separators=False) -}} + {% endset %} {% set item_meta %} From 64cc9f25209cfb22fa46af3fcd5041d5b5aae254 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 9 Jun 2020 16:44:05 +0100 Subject: [PATCH 16/22] Make tests work with new template list HTML Adds the extra text added to each checkbox label. It's a copy of the text of the link in the same list item which does add a lot of duplication to the test data. This reformats a lot of the test data, stacking it to separate out the duplicate items. --- tests/app/main/views/test_template_folders.py | 247 +++++++++++------- tests/app/main/views/test_templates.py | 2 +- 2 files changed, 158 insertions(+), 91 deletions(-) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 91da73454..b70a3cf79 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -54,29 +54,43 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): {}, ['Email', 'Text message', 'Letter'], [ - 'folder_one 2 folders', - 'folder_one folder_one_one 1 template, 1 folder', - 'folder_one folder_one_one folder_one_one_one 1 template', - 'folder_one folder_one_one folder_one_one_one sms_template_nested Text message template', - 'folder_one folder_one_one letter_template_nested Letter template', - 'folder_one folder_one_two Empty', - 'folder_two Empty', - 'sms_template_one Text message template', - 'sms_template_two Text message template', - 'email_template_one Email template', - 'email_template_two Email template', - 'letter_template_one Letter template', - 'letter_template_two Letter template', + 'folder_one folder_one 2 folders', + ('folder_one folder_one_one ' + 'folder_one folder_one_one ' + '1 template, 1 folder'), + ('folder_one folder_one_one folder_one_one_one ' + 'folder_one folder_one_one folder_one_one_one ' + '1 template'), + ('folder_one folder_one_one folder_one_one_one sms_template_nested ' + 'folder_one folder_one_one folder_one_one_one sms_template_nested ' + 'Text message template'), + ('folder_one folder_one_one letter_template_nested ' + 'folder_one folder_one_one letter_template_nested ' + 'Letter template'), + ('folder_one folder_one_two ' + 'folder_one folder_one_two ' + 'Empty'), + 'folder_two folder_two Empty', + ('sms_template_one ' + 'sms_template_one ' + 'Text message template'), + ('sms_template_two ' + 'sms_template_two ' + 'Text message template'), + 'email_template_one email_template_one Email template', + 'email_template_two email_template_two Email template', + 'letter_template_one letter_template_one Letter template', + 'letter_template_two letter_template_two Letter template', ], [ - 'folder_one 2 folders', - 'folder_two Empty', - 'sms_template_one Text message template', - 'sms_template_two Text message template', - 'email_template_one Email template', - 'email_template_two Email template', - 'letter_template_one Letter template', - 'letter_template_two Letter template', + 'folder_one folder_one 2 folders', + 'folder_two folder_two Empty', + 'sms_template_one sms_template_one Text message template', + 'sms_template_two sms_template_two Text message template', + 'email_template_one email_template_one Email template', + 'email_template_two email_template_two Email template', + 'letter_template_one letter_template_one Letter template', + 'letter_template_two letter_template_two Letter template', ], [ 'folder_one', @@ -102,29 +116,39 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): {'template_type': 'all'}, ['Email', 'Text message', 'Letter'], [ - 'folder_one 2 folders', - 'folder_one folder_one_one 1 template, 1 folder', - 'folder_one folder_one_one folder_one_one_one 1 template', - 'folder_one folder_one_one folder_one_one_one sms_template_nested Text message template', - 'folder_one folder_one_one letter_template_nested Letter template', - 'folder_one folder_one_two Empty', - 'folder_two Empty', - 'sms_template_one Text message template', - 'sms_template_two Text message template', - 'email_template_one Email template', - 'email_template_two Email template', - 'letter_template_one Letter template', - 'letter_template_two Letter template', + 'folder_one folder_one 2 folders', + ('folder_one folder_one_one ' + 'folder_one folder_one_one ' + '1 template, 1 folder'), + ('folder_one folder_one_one folder_one_one_one ' + 'folder_one folder_one_one folder_one_one_one ' + '1 template'), + ('folder_one folder_one_one folder_one_one_one sms_template_nested ' + 'folder_one folder_one_one folder_one_one_one sms_template_nested ' + 'Text message template'), + ('folder_one folder_one_one letter_template_nested ' + 'folder_one folder_one_one letter_template_nested ' + 'Letter template'), + ('folder_one folder_one_two ' + 'folder_one folder_one_two ' + 'Empty'), + 'folder_two folder_two Empty', + 'sms_template_one sms_template_one Text message template', + 'sms_template_two sms_template_two Text message template', + 'email_template_one email_template_one Email template', + 'email_template_two email_template_two Email template', + 'letter_template_one letter_template_one Letter template', + 'letter_template_two letter_template_two Letter template', ], [ - 'folder_one 2 folders', - 'folder_two Empty', - 'sms_template_one Text message template', - 'sms_template_two Text message template', - 'email_template_one Email template', - 'email_template_two Email template', - 'letter_template_one Letter template', - 'letter_template_two Letter template', + 'folder_one folder_one 2 folders', + 'folder_two folder_two Empty', + 'sms_template_one sms_template_one Text message template', + 'sms_template_two sms_template_two Text message template', + 'email_template_one email_template_one Email template', + 'email_template_two email_template_two Email template', + 'letter_template_one letter_template_one Letter template', + 'letter_template_two letter_template_two Letter template', ], [ 'folder_one', @@ -150,17 +174,23 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): {'template_type': 'sms'}, ['All', 'Email', 'Letter'], [ - 'folder_one 1 folder', - 'folder_one folder_one_one 1 folder', - 'folder_one folder_one_one folder_one_one_one 1 template', - 'folder_one folder_one_one folder_one_one_one sms_template_nested Text message template', - 'sms_template_one Text message template', - 'sms_template_two Text message template', + 'folder_one folder_one 1 folder', + ('folder_one folder_one_one ' + 'folder_one folder_one_one ' + '1 folder'), + ('folder_one folder_one_one folder_one_one_one ' + 'folder_one folder_one_one folder_one_one_one ' + '1 template'), + ('folder_one folder_one_one folder_one_one_one sms_template_nested ' + 'folder_one folder_one_one folder_one_one_one sms_template_nested ' + 'Text message template'), + 'sms_template_one sms_template_one Text message template', + 'sms_template_two sms_template_two Text message template', ], [ - 'folder_one 1 folder', - 'sms_template_one Text message template', - 'sms_template_two Text message template', + 'folder_one folder_one 1 folder', + 'sms_template_one sms_template_one Text message template', + 'sms_template_two sms_template_two Text message template', ], [ 'folder_one', @@ -179,15 +209,21 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): {'template_folder_id': PARENT_FOLDER_ID}, ['Email', 'Text message', 'Letter'], [ - 'folder_one_one 1 template, 1 folder', - 'folder_one_one folder_one_one_one 1 template', - 'folder_one_one folder_one_one_one sms_template_nested Text message template', - 'folder_one_one letter_template_nested Letter template', - 'folder_one_two Empty', + 'folder_one_one folder_one_one 1 template, 1 folder', + ('folder_one_one folder_one_one_one ' + 'folder_one_one folder_one_one_one ' + '1 template'), + ('folder_one_one folder_one_one_one sms_template_nested ' + 'folder_one_one folder_one_one_one sms_template_nested ' + 'Text message template'), + ('folder_one_one letter_template_nested ' + 'folder_one_one letter_template_nested ' + 'Letter template'), + 'folder_one_two folder_one_two Empty', ], [ - 'folder_one_one 1 template, 1 folder', - 'folder_one_two Empty', + 'folder_one_one folder_one_one 1 template, 1 folder', + 'folder_one_two folder_one_two Empty', ], [ 'folder_one_one', @@ -205,12 +241,16 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], [ - 'folder_one_one 1 folder', - 'folder_one_one folder_one_one_one 1 template', - 'folder_one_one folder_one_one_one sms_template_nested Text message template', + 'folder_one_one folder_one_one 1 folder', + ('folder_one_one folder_one_one_one ' + 'folder_one_one folder_one_one_one ' + '1 template'), + ('folder_one_one folder_one_one_one sms_template_nested ' + 'folder_one_one folder_one_one_one sms_template_nested ' + 'Text message template'), ], [ - 'folder_one_one 1 folder', + 'folder_one_one folder_one_one 1 folder', ], [ 'folder_one_one', @@ -240,13 +280,15 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): {'template_folder_id': CHILD_FOLDER_ID}, ['Email', 'Text message', 'Letter'], [ - 'folder_one_one_one 1 template', - 'folder_one_one_one sms_template_nested Text message template', - 'letter_template_nested Letter template', + 'folder_one_one_one folder_one_one_one 1 template', + ('folder_one_one_one sms_template_nested ' + 'folder_one_one_one sms_template_nested ' + 'Text message template'), + 'letter_template_nested letter_template_nested Letter template', ], [ - 'folder_one_one_one 1 template', - 'letter_template_nested Letter template', + 'folder_one_one_one folder_one_one_one 1 template', + 'letter_template_nested letter_template_nested Letter template', ], [ 'folder_one_one_one', @@ -266,10 +308,10 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): {'template_folder_id': GRANDCHILD_FOLDER_ID}, ['Email', 'Text message', 'Letter'], [ - 'sms_template_nested Text message template', + 'sms_template_nested sms_template_nested Text message template', ], [ - 'sms_template_nested Text message template', + 'sms_template_nested sms_template_nested Text message template', ], [ 'sms_template_nested', @@ -1462,45 +1504,70 @@ def test_show_custom_error_message( ( {}, [ - ['folder_A', '1 template, 2 folders'], - ['folder_E folder_F folder_G', '1 template'], - ['email_template_root', 'Email template'], + ['folder_A', 'folder_A', '1 template, 2 folders'], + ['folder_E folder_F folder_G', + 'folder_E', 'folder_F', 'folder_G', + '1 template'], + ['email_template_root', 'email_template_root', 'Email template'], ], [ - ['folder_A', '1 template, 2 folders'], - ['folder_A', 'folder_C', '1 template'], - ['folder_A', 'folder_C', 'sms_template_C', 'Text message template'], - ['folder_A', 'folder_D', 'Empty'], - ['folder_A', 'sms_template_A', 'Text message template'], - ['folder_E folder_F folder_G', '1 template'], - ['folder_E folder_F folder_G', 'email_template_G', 'Email template'], - ['email_template_root', 'Email template'], + ['folder_A', 'folder_A', '1 template, 2 folders'], + ['folder_A folder_C', + 'folder_A', 'folder_C', + '1 template'], + ['folder_A folder_C sms_template_C', + 'folder_A', 'folder_C', 'sms_template_C', + 'Text message template'], + ['folder_A folder_D', + 'folder_A', 'folder_D', + 'Empty'], + ['folder_A sms_template_A', + 'folder_A', 'sms_template_A', + 'Text message template'], + ['folder_E folder_F folder_G', + 'folder_E', 'folder_F', 'folder_G', + '1 template'], + ['folder_E folder_F folder_G email_template_G', + 'folder_E', 'folder_F', 'folder_G', 'email_template_G', + 'Email template'], + ['email_template_root', 'email_template_root', 'Email template'], ], None, ), ( {'template_type': 'email'}, [ - ['folder_E folder_F folder_G', '1 template'], - ['email_template_root', 'Email template'], + ['folder_E folder_F folder_G', + 'folder_E', 'folder_F', 'folder_G', + '1 template'], + ['email_template_root', 'email_template_root', 'Email template'], ], [ - ['folder_E folder_F folder_G', '1 template'], - ['folder_E folder_F folder_G', 'email_template_G', 'Email template'], - ['email_template_root', 'Email template'], + ['folder_E folder_F folder_G', + 'folder_E', 'folder_F', 'folder_G', + '1 template'], + ['folder_E folder_F folder_G email_template_G', + 'folder_E', 'folder_F', 'folder_G', 'email_template_G', + 'Email template'], + ['email_template_root', 'email_template_root', 'Email template'], ], None, ), ( {'template_type': 'sms'}, [ - ['folder_A', '1 template, 1 folder'], + ['folder_A', 'folder_A', '1 template, 1 folder'], ], [ - ['folder_A', '1 template, 1 folder'], - ['folder_A', 'folder_C', '1 template'], - ['folder_A', 'folder_C', 'sms_template_C', 'Text message template'], - ['folder_A', 'sms_template_A', 'Text message template'], + ['folder_A', 'folder_A', '1 template, 1 folder'], + ['folder_A folder_C', + 'folder_A', 'folder_C', + '1 template'], + ['folder_A folder_C sms_template_C', + 'folder_A', 'folder_C', 'sms_template_C', + 'Text message template'], + ['folder_A sms_template_A', 'folder_A', 'sms_template_A', + 'Text message template'], ], None, ), diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index a3cf74d41..e67ffa759 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -279,7 +279,7 @@ def test_should_show_live_search_if_service_has_lots_of_folders( ) count_of_templates_and_folders = len(page.select('#template-list .govuk-label')) - count_of_folders = len(page.select('.template-list-folder:first-child')) + count_of_folders = len(page.select('.template-list-folder:first-of-type')) count_of_templates = count_of_templates_and_folders - count_of_folders assert len(page.select('.live-search')) == 1 From 01f84d544370653c319794eb177b2c93a70ddd43 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 9 Jul 2020 12:04:18 +0100 Subject: [PATCH 17/22] Convert checkboxes for broadcast areas Includes removal of MultiCheckboxField due to it no longer being used elsewhere in this file. --- app/main/forms.py | 10 +++------- app/templates/views/broadcast/areas.html | 5 ++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 0134a0274..e34eeda40 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -38,7 +38,6 @@ from wtforms import ( ) from wtforms.fields.html5 import EmailField, SearchField, TelField from wtforms.validators import URL, DataRequired, Length, Optional, Regexp -from wtforms.widgets import CheckboxInput, ListWidget from app import format_thousands from app.main.validators import ( @@ -118,11 +117,6 @@ def get_next_days_until(until): ] -class MultiCheckboxField(SelectMultipleField): - widget = ListWidget(prefix_label=False) - option_widget = CheckboxInput() - - class RadioField(WTFormsRadioField): def __init__( @@ -2135,11 +2129,13 @@ class AcceptAgreementForm(StripWhitespaceForm): class BroadcastAreaForm(StripWhitespaceForm): - areas = MultiCheckboxField('Choose areas to broadcast to') + areas = govukCheckboxesField('Choose areas to broadcast to') def __init__(self, choices, *args, **kwargs): super().__init__(*args, **kwargs) self.areas.choices = choices + self.areas.render_as_list = True + self.areas.param_extensions = {'fieldset': {'legend': {'classes': 'govuk-visually-hidden'}}} @classmethod def from_library(cls, library): diff --git a/app/templates/views/broadcast/areas.html b/app/templates/views/broadcast/areas.html index 5020b587c..9ed0a38e8 100644 --- a/app/templates/views/broadcast/areas.html +++ b/app/templates/views/broadcast/areas.html @@ -1,6 +1,5 @@ {% from "components/page-header.html" import page_header %} {% from "components/page-footer.html" import sticky_page_footer %} -{% from "components/checkbox.html" import checkboxes %} {% from "components/form.html" import form_wrapper %} {% from "components/live-search.html" import live_search %} @@ -17,10 +16,10 @@ back_link=url_for('.choose_broadcast_library', service_id=current_service.id, broadcast_message_id=broadcast_message.id), )}} - {{ live_search(target_selector='.multiple-choice', show=show_search_form, form=search_form, label='Search by name') }} + {{ live_search(target_selector='.govuk-checkboxes__item', show=show_search_form, form=search_form, label='Search by name') }} {% call form_wrapper() %} - {{ checkboxes(form.areas, hide_legend=True) }} + {{ form.areas }} {{ sticky_page_footer('Add to broadcast') }} {% endcall %} From ca9b8a8ca35461fcd78f23b8895ed70def34a082 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 14 Jul 2020 10:34:53 +0100 Subject: [PATCH 18/22] Add analytics error tracking to checkbox fields The existing macros added data attributes to any error message displayed which communicated the error to Google Analytics (if the user had given consent). This re-implements that functionality. --- app/main/forms.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e34eeda40..c0ffc6e68 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -572,7 +572,14 @@ class govukCheckboxField(govukCheckboxesMixin, BooleanField): # error messages error_message = None if field.errors: - error_message = {"text": " ".join(field.errors).strip()} + error_message = { + "attributes": { + "data-module": "track-error", + "data-error-type": field.errors[0], + "data-error-label": field.name + }, + "text": " ".join(field.errors).strip() + } params = { 'name': field.name, @@ -654,7 +661,14 @@ class govukCheckboxesField(govukCheckboxesMixin, SelectMultipleField): # error messages error_message = None if field.errors: - error_message = {"text": " ".join(field.errors).strip()} + error_message = { + "attributes": { + "data-module": "track-error", + "data-error-type": field.errors[0], + "data-error-label": field.name + }, + "text": " ".join(field.errors).strip() + } # returns either a list or a hierarchy of lists # depending on how get_items_from_options is implemented From e266b11ee2b2876d93040130d45b3b5414699660 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 31 Jul 2020 10:20:44 +0100 Subject: [PATCH 19/22] Fix tests for pages using govukCheckboxField These fields used to use govukCheckboxesField and so stored their data in a list. They were since migrated to govukCheckboxField, which extends BooleanField and so keeps its data as a boolean value. --- tests/app/main/views/test_manage_users.py | 8 ++-- tests/app/main/views/test_service_settings.py | 46 +++++++++---------- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 0540eda03..69502ece8 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1101,9 +1101,11 @@ def test_user_cant_invite_themselves( service_id=SERVICE_ONE_ID, _data={ 'email_address': active_user_with_permissions['email_address'], - 'send_messages': 'y', - 'manage_service': 'y', - 'manage_api_keys': 'y', + 'permissions_field': [ + 'send_messages', + 'manage_service', + 'manage_api_keys' + ] }, _follow_redirects=True, _expected_status=200, diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 39475af5c..d7a1b3da3 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2299,8 +2299,8 @@ def test_incorrect_sms_sender_input( @pytest.mark.parametrize('reply_to_addresses, data, api_default_args', [ ([], {}, True), - (create_multiple_email_reply_to_addresses(), {"is_default": []}, False), - (create_multiple_email_reply_to_addresses(), {"is_default": ["y"]}, True) + (create_multiple_email_reply_to_addresses(), {}, False), + (create_multiple_email_reply_to_addresses(), {"is_default": "y"}, True) ]) def test_add_reply_to_email_address_sends_test_notification( mocker, client_request, reply_to_addresses, data, api_default_args @@ -2420,8 +2420,8 @@ def test_add_reply_to_email_address_fails_if_notification_not_delivered_in_45_se @pytest.mark.parametrize('letter_contact_blocks, data, api_default_args', [ ([], {}, True), # no existing letter contact blocks - (create_multiple_letter_contact_blocks(), {"is_default": []}, False), - (create_multiple_letter_contact_blocks(), {"is_default": ["y"]}, True) + (create_multiple_letter_contact_blocks(), {}, False), + (create_multiple_letter_contact_blocks(), {"is_default": "y"}, True) ]) def test_add_letter_contact( letter_contact_blocks, @@ -2496,8 +2496,8 @@ def test_add_letter_contact_when_coming_from_template( @pytest.mark.parametrize('sms_senders, data, api_default_args', [ ([], {}, True), - (create_multiple_sms_senders(), {"is_default": []}, False), - (create_multiple_sms_senders(), {"is_default": ["y"]}, True) + (create_multiple_sms_senders(), {}, False), + (create_multiple_sms_senders(), {"is_default": "y"}, True) ]) def test_add_sms_sender( sms_senders, @@ -2563,10 +2563,10 @@ def test_default_box_doesnt_show_on_first_letter_sender( @pytest.mark.parametrize('reply_to_address, data, api_default_args', [ - (create_reply_to_email_address(is_default=True), {"is_default": ["y"]}, True), - (create_reply_to_email_address(is_default=True), {"is_default": []}, True), - (create_reply_to_email_address(is_default=False), {"is_default": []}, False), - (create_reply_to_email_address(is_default=False), {"is_default": ["y"]}, True) + (create_reply_to_email_address(is_default=True), {"is_default": "y"}, True), + (create_reply_to_email_address(is_default=True), {}, True), + (create_reply_to_email_address(is_default=False), {}, False), + (create_reply_to_email_address(is_default=False), {"is_default": "y"}, True) ]) def test_edit_reply_to_email_address_sends_verification_notification_if_address_is_changed( reply_to_address, @@ -2591,10 +2591,10 @@ def test_edit_reply_to_email_address_sends_verification_notification_if_address_ @pytest.mark.parametrize('reply_to_address, data, api_default_args', [ - (create_reply_to_email_address(), {"is_default": ["y"]}, True), - (create_reply_to_email_address(), {"is_default": []}, True), - (create_reply_to_email_address(is_default=False), {"is_default": []}, False), - (create_reply_to_email_address(is_default=False), {"is_default": ["y"]}, True) + (create_reply_to_email_address(), {"is_default": "y"}, True), + (create_reply_to_email_address(), {}, True), + (create_reply_to_email_address(is_default=False), {}, False), + (create_reply_to_email_address(is_default=False), {"is_default": "y"}, True) ]) def test_edit_reply_to_email_address_goes_straight_to_update_if_address_not_changed( reply_to_address, @@ -2651,7 +2651,7 @@ def test_add_edit_reply_to_email_address_goes_straight_to_update_if_address_not_ message=error_message )] ) - data = {"is_default": ["y"], 'email_address': "reply_to@example.com"} + data = {"is_default": "y", 'email_address': "reply_to@example.com"} page = client_request.post( url, service_id=SERVICE_ONE_ID, @@ -2751,10 +2751,10 @@ def test_delete_reply_to_email_address( @pytest.mark.parametrize('letter_contact_block, data, api_default_args', [ - (create_letter_contact_block(), {"is_default": ["y"]}, True), - (create_letter_contact_block(), {"is_default": []}, True), - (create_letter_contact_block(is_default=False), {"is_default": []}, False), - (create_letter_contact_block(is_default=False), {"is_default": ["y"]}, True) + (create_letter_contact_block(), {"is_default": "y"}, True), + (create_letter_contact_block(), {}, True), + (create_letter_contact_block(is_default=False), {}, False), + (create_letter_contact_block(is_default=False), {"is_default": "y"}, True) ]) def test_edit_letter_contact_block( letter_contact_block, @@ -2828,10 +2828,10 @@ def test_delete_letter_contact_block( @pytest.mark.parametrize('sms_sender, data, api_default_args', [ - (create_sms_sender(), {"is_default": ["y"], "sms_sender": "test"}, True), - (create_sms_sender(), {"is_default": [], "sms_sender": "test"}, True), - (create_sms_sender(is_default=False), {"is_default": [], "sms_sender": "test"}, False), - (create_sms_sender(is_default=False), {"is_default": ["y"], "sms_sender": "test"}, True) + (create_sms_sender(), {"is_default": "y", "sms_sender": "test"}, True), + (create_sms_sender(), {"sms_sender": "test"}, True), + (create_sms_sender(is_default=False), {"sms_sender": "test"}, False), + (create_sms_sender(is_default=False), {"is_default": "y", "sms_sender": "test"}, True) ]) def test_edit_sms_sender( sms_sender, From f3222296140fecde6d752624cba649c86a757125 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 31 Jul 2020 15:06:03 +0100 Subject: [PATCH 20/22] Change tests to check permissions_field name A comment on this group of changes mentioned the 'name' attribute of checkboxes for the permissions_field should be checked as well as the 'value' attribute: https://github.com/alphagov/notifications-admin/pull/3535#discussion_r460869797 This adds checks to support that point. --- tests/app/main/views/test_manage_users.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 69502ece8..ea8433e19 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -288,11 +288,15 @@ def test_service_without_caseworking_doesnt_show_admin_vs_caseworker( service_id=SERVICE_ONE_ID, **extra_args ) - assert page.select('input[type=checkbox]')[0]['value'] == 'view_activity' - assert page.select('input[type=checkbox]')[1]['value'] == 'send_messages' - assert page.select('input[type=checkbox]')[2]['value'] == 'manage_templates' - assert page.select('input[type=checkbox]')[3]['value'] == 'manage_service' - assert page.select('input[type=checkbox]')[4]['value'] == 'manage_api_keys' + permission_checkboxes = page.select('input[type=checkbox]') + + for idx in range(len(permission_checkboxes)): + assert permission_checkboxes[idx]['name'] == 'permissions_field' + assert permission_checkboxes[0]['value'] == 'view_activity' + assert permission_checkboxes[1]['value'] == 'send_messages' + assert permission_checkboxes[2]['value'] == 'manage_templates' + assert permission_checkboxes[3]['value'] == 'manage_service' + assert permission_checkboxes[4]['value'] == 'manage_api_keys' @pytest.mark.parametrize('endpoint, extra_args', [ @@ -320,11 +324,11 @@ def test_broadcast_service_only_shows_relevant_permissions( **extra_args ) assert [ - field['value'] for field in page.select('input[type=checkbox]') + (field['name'], field['value']) for field in page.select('input[type=checkbox]') ] == [ - 'send_messages', - 'manage_templates', - 'manage_service', + ('permissions_field', 'send_messages'), + ('permissions_field', 'manage_templates'), + ('permissions_field', 'manage_service'), ] @@ -433,6 +437,7 @@ def test_should_show_page_for_one_user( for index, expected in enumerate(expected_checkboxes): expected_input_value, expected_checked = expected + assert checkboxes[index]['name'] == 'permissions_field' assert checkboxes[index]['value'] == expected_input_value assert checkboxes[index].has_attr('checked') == expected_checked From e3c434bb8fc3683616ee86eff129ad5973c4c8bd Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 31 Jul 2020 15:09:34 +0100 Subject: [PATCH 21/22] Change permissions tests to support old API User permissions were handled by a group of BooleanFields but introducing the new checkboxes changed this to just one field that stores its data in a list. It was mentioned in a comment that there could be a situation, when the instances roll, where clients are using the old fields but POSTing to a server running the new code. https://github.com/alphagov/notifications-admin/pull/3535#discussion_r460872903 This introduces tests for that situation. --- tests/app/main/views/test_manage_users.py | 77 +++++++++++++++++++---- 1 file changed, 64 insertions(+), 13 deletions(-) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index ea8433e19..82790baac 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -514,6 +514,22 @@ def test_should_not_show_page_for_non_team_member( {}, set(), ), + ( # should be able to handle permissions being sent as booleans, until changeover to a list is complete + { + 'view_activity': 'y', + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + 'manage_api_keys': 'y', + }, + { + 'view_activity', + 'send_messages', + 'manage_templates', + 'manage_service', + 'manage_api_keys', + } + ), ]) def test_edit_user_permissions( client_request, @@ -584,6 +600,20 @@ def test_edit_user_permissions( 'view_activity', } ), + ( # should be able to handle permissions being sent as booleans, until changeover to a list is complete + { + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + 'manage_api_keys': 'y', + }, + { + 'view_activity', + 'send_messages', + 'manage_service', + 'manage_templates', + } + ), ]) def test_edit_user_permissions_for_broadcast_service( client_request, @@ -828,9 +858,10 @@ def test_should_show_folder_permission_form_if_service_has_folder_permissions_en assert len(folder_checkboxes) == 3 -@pytest.mark.parametrize('email_address, gov_user', [ - ('test@example.gov.uk', True), - ('test@example.com', False) +@pytest.mark.parametrize('email_address, gov_user, old_permissions', [ + ('test@example.gov.uk', True, False), + ('test@example.com', False, False), + ('test@example.gov.uk', True, True) ]) def test_invite_user( client_request, @@ -839,6 +870,7 @@ def test_invite_user( sample_invite, email_address, gov_user, + old_permissions, mock_get_template_folders, mock_get_organisations, ): @@ -848,19 +880,25 @@ def test_invite_user( mocker.patch('app.models.user.InvitedUsers.client_method', return_value=[sample_invite]) mocker.patch('app.models.user.Users.client_method', return_value=[active_user_with_permissions]) mocker.patch('app.invite_api_client.create_invite', return_value=sample_invite) + data = {'email_address': email_address} + if old_permissions: + data['view_activity'] = 'y' + data['send_messages'] = 'y' + data['manage_templates'] = 'y' + data['manage_service'] = 'y' + data['manage_api_keys'] = 'y' + else: + data['permissions_field'] = [ + 'view_activity', + 'send_messages', + 'manage_templates', + 'manage_service', + 'manage_api_keys', + ] page = client_request.post( 'main.invite_user', service_id=SERVICE_ONE_ID, - _data={ - 'email_address': email_address, - 'permissions_field': [ - 'view_activity', - 'send_messages', - 'manage_templates', - 'manage_service', - 'manage_api_keys', - ] - }, + _data=data, _follow_redirects=True, ) assert page.h1.string.strip() == 'Team members' @@ -965,6 +1003,19 @@ def test_invite_user_with_email_auth_service( 'view_activity', }, ), + ( # should be able to handle permissions being sent as booleans, until changeover to a list is complete + { + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + }, + { + 'view_activity', + 'send_messages', + 'manage_templates', + 'manage_service', + }, + ) )) def test_invite_user_to_broadcast_service( client_request, From 75bac87a4db0fa6df48b9c62af659b40c0e2e9aa Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 31 Jul 2020 10:31:39 +0100 Subject: [PATCH 22/22] Make permissions forms handle old/new params --- app/main/forms.py | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index c0ffc6e68..4301861fb 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -755,6 +755,20 @@ def filter_by_broadcast_permissions(valuelist): return [entry for entry in valuelist if any(entry in role for role in broadcast_permissions)] +# Included to support both versions of how user permissions are handled in permissions forms +# Remove when changeover to new version (permissions_field) is complete +PermissionsAbstract = type("PermissionsAbstract", (StripWhitespaceForm,), { + permission: BooleanField(label) for permission, label in permissions +}) + + +# Included to support both versions of how user permissions are handled in permissions forms +# Remove when changeover to new version (permissions_field) is complete +BroadcastPermissionsAbstract = type("BroadcastPermissionsAbstract", (StripWhitespaceForm,), { + permission: BooleanField(label) for permission, label in broadcast_permissions +}) + + class BasePermissionsForm(StripWhitespaceForm): def __init__(self, all_template_folders=None, *args, **kwargs): super().__init__(*args, **kwargs) @@ -790,9 +804,25 @@ class BasePermissionsForm(StripWhitespaceForm): } ) + # Modified to support both versions of how user permissions are handled in permissions forms + # Remove when changeover to new version (permissions_field) is complete @property def permissions(self): - return set(self.permissions_field.data) + permissions_field_data = set(self.permissions_field.data) + permissions_fields_data = {field.id for field in self.permissions_fields if field.data is True} + if len(permissions_field_data) == 0 and len(permissions_fields_data) != 0: + return permissions_fields_data + else: + return permissions_field_data + + # Included to support both versions of how user permissions are handled in permissions forms + # Remove when changeover to new version (permissions_field) is complete + @property + def permissions_fields(self): + return ( + getattr(self, permission) for permission, field in self.__dict__.items() + if isinstance(field, BooleanField) + ) @classmethod def from_user(cls, user, service_id, **kwargs): @@ -806,11 +836,15 @@ class BasePermissionsForm(StripWhitespaceForm): ) -class PermissionsForm(BasePermissionsForm): +# Included to support both versions of how user permissions are handled in permissions forms +# Remove when changeover to new version (permissions_field) is complete +class PermissionsForm(PermissionsAbstract, BasePermissionsForm): pass -class BroadcastPermissionsForm(BasePermissionsForm): +# Included to support both versions of how user permissions are handled in permissions forms +# Remove when changeover to new version (permissions_field) is complete +class BroadcastPermissionsForm(BroadcastPermissionsAbstract, BasePermissionsForm): permissions_field = govukCheckboxesField( 'Permssions',