From e06c1f5daa4454f6550c0ffd57275fa7467954a3 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 14 Jan 2021 21:05:23 +0000 Subject: [PATCH 1/4] Fix bug with extend_params function The OrganisationAgreementSignedForm class has a bug causing it to render different HTML when the page loads to when you subsequently refresh it. This commit proposes a change to the extend_params function to fix it. extend_params, is used by the OrganisationAgreementSignedForm, as well as all the other WTForms field classes we added to wrap GOVUK Frontend components. Fixing it should therefore fix any similar bugs with them. All of these fields send a dict of configuration data to the GOVUK Frontend component when they call it, at render time. This dict is 'JSON-like', meaning it's values can be all the primitives as well as lists and dicts. This also means it can go quite deep. Extending the default configuration The classes have a default dict of this data kept privately in the params variable. They let you change it by passing in an argument called param_extensions on instantiation, after that, through an attribute of the same name and at render time as the same argument (in templates). The extend_params function The param_extensions dict is used as a collection of changes to make to the default params dict. The changes are applied by the extend_params function. Its code deletes part of the param_extensions, a side effect that didn't seem a problem because it isn't used after the function has run. The bug The bug was only with the part of the HTML that got its data from the part of the param_extensions dict that was deleted by extend_params. The class with the bug set param_extensions when the field is instantiated, as part of its parent form definition. My guess is that param_extensions was stored in memory, as part of the form class, and reused when the page refreshed. At that point, extend_params had deleted part of its data, causing the bug. --- app/main/forms.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 8d0b14427..92333c47d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -649,16 +649,19 @@ def extend_params(params, extensions): # merge dicts merge_jsonlike(params, extensions) + # tidy up + extensions['items'] = items + # merge items if items: if 'items' not in params: - params['items'] = items + params['items'] = extensions['items'] else: - for idx, _item in enumerate(items): + for idx, _item in enumerate(extensions['items']): if idx >= param_items: - params['items'].append(items[idx]) + params['items'].append(extensions['items'][idx]) else: - params['items'][idx].update(items[idx]) + params['items'][idx].update(extensions['items'][idx]) def govuk_checkbox_field_widget(self, field, param_extensions=None, **kwargs): From 23d391e3fc3c348a718eb7c611285b936631f170 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 21 Jan 2021 16:49:12 +0000 Subject: [PATCH 2/4] Change how merge_jsonlike treats lists Current behaviour is to check item-against-item and merge based on whether items match, irrelevant of position. This doesn't produce the results we need for our usecases (merging data to send to GOVUK Frontend components). We actually want: - items to be compared based on their position - new primitive items at the same position to overwrite existing ones - dicts or lists at the same position to be merged For example, Starting with this list: [{"name": "option-1", "value": "1"}] Merging in this list: [{"hint": {"text": "Choose one option"}}] You currently get this: [ {"name": "option-1", "value": "1"}, {"hint": {"text": "Choose one option"}} ] We want to get this: [ { "name": "option-1", "value": "1", "hint": {"text": "Choose one option"} } ] --- app/utils.py | 9 +++++++-- tests/app/test_utils.py | 14 ++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/app/utils.py b/app/utils.py index 41cdc29d1..75f301b72 100644 --- a/app/utils.py +++ b/app/utils.py @@ -651,8 +651,13 @@ def merge_jsonlike(source, destination): return True def merge_lists(source, destination): - for item in destination: - if item not in source: + last_dest_idx = len(destination) - 1 + for idx, item in enumerate(destination): + if idx <= last_dest_idx: + # assign destination value if can't be merged into source + if merge_items(source[idx], destination[idx]) is False: + source[idx] = destination[idx] + else: source.append(item) def merge_dicts(source, destination): diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 4246b5037..b869e9a6a 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -614,18 +614,20 @@ def test_get_sample_template_returns_template(template_type): ({"a": "b"}, {"c": "d"}, {"a": "b", "c": "d"}), # dicts with nested dict, both under same key, additive behaviour: ({"a": {"b": "c"}}, {"a": {"e": "f"}}, {"a": {"b": "c", "e": "f"}}), - # same key in both dicts, value is a string, destination supersedes source: + # same key in both dicts, value is a string, destination supercedes source: ({"a": "b"}, {"a": "c"}, {"a": "c"}), + # lists with same length but different items, destination supercedes source: + (["b", "c", "d"], ["b", "e", "f"], ["b", "e", "f"]), # lists in dicts behave as top level lists - ({"a": ["b", "c", "d"]}, {"a": ["b", "e", "f"]}, {"a": ["b", "c", "d", "e", "f"]}), - # lists with same string in both result in a list of unique values - (["a", "b", "c", "d"], ["d", "e", "f"], ["a", "b", "c", "d", "e", "f"]), + ({"a": ["b", "c", "d"]}, {"a": ["b", "e", "f"]}, {"a": ["b", "e", "f"]}), + # lists with same string in both, at different positions, result in duplicates keeping their positions + (["a", "b", "c", "d"], ["d", "e", "f"], ["d", "e", "f", "d"]), # lists with same dict in both result in a list with one instance of that dict ([{"b": "c"}], [{"b": "c"}], [{"b": "c"}]), # if dicts in lists have different values, they are not merged - ([{"b": "c"}], [{"b": "e"}], [{"b": "c"}, {"b": "e"}]), + ([{"b": "c"}], [{"b": "e"}], [{"b": "e"}]), # merge a dict with a null object returns that dict (does not work the other way round) - ({"a": {"b": "c"}}, None, {"a": {"b": "c"}}), + ({"a": {"b": "c"}}, None, {"a": {"b": "c"}}) ]) def test_merge_jsonlike_merges_jsonlike_objects_correctly(source_object, destination_object, expected_result): merge_jsonlike(source_object, destination_object) From 1059cf4d81ef96b593d94e342dfcef5bbfafe98c Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 22 Jan 2021 15:40:28 +0000 Subject: [PATCH 3/4] Remove extend_params in favour of merge_jsonlike A comment on the pull request for this branch pointed out that it's not clear why the 'items' list is deleted and then reassigned in extend_params: https://github.com/alphagov/notifications-admin/pull/3770#pullrequestreview-573067465 The simple reason is that we want to use merge_jsonlike to merge params and param_extensions (passed in as extensions) but merge_jsonlike doesn't merge lists correctly. I realised that if we just make merge_jsonlike merge lists correctly, we can use it for everything extend_params does. This commit does that, and replaces all calls to extend_params with merge_jsonlike. Because extend_params is used across many form field classes, and so many pages, I took the following precautions after making those changes: 1. found every use of param_extensions 2. looked at the merges onto params that each would cause and deduped them to a final list of 6(!) 3. tested pages containing fields from that list 4. added new testcases to the merge_jsonlike tests for any merges that exist in our codebase but not in our tests --- app/main/forms.py | 39 ++++++--------------------------------- tests/app/test_utils.py | 10 +++++++++- 2 files changed, 15 insertions(+), 34 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 92333c47d..cd4617f31 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -637,33 +637,6 @@ class RegisterUserFromOrgInviteForm(StripWhitespaceForm): auth_type = HiddenField('auth_type', validators=[DataRequired()]) -def extend_params(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 - merge_jsonlike(params, extensions) - - # tidy up - extensions['items'] = items - - # merge items - if items: - if 'items' not in params: - params['items'] = extensions['items'] - else: - for idx, _item in enumerate(extensions['items']): - if idx >= param_items: - params['items'].append(extensions['items'][idx]) - else: - params['items'][idx].update(extensions['items'][idx]) - - def govuk_checkbox_field_widget(self, field, param_extensions=None, **kwargs): # error messages @@ -695,11 +668,11 @@ def govuk_checkbox_field_widget(self, field, param_extensions=None, **kwargs): # extend default params with any sent in during instantiation if self.param_extensions: - extend_params(params, self.param_extensions) + merge_jsonlike(params, self.param_extensions) # add any sent in though use in templates if param_extensions: - extend_params(params, param_extensions) + merge_jsonlike(params, param_extensions) return Markup( render_template('forms/fields/checkboxes/macro.njk', params=params)) @@ -751,11 +724,11 @@ def govuk_checkboxes_field_widget(self, field, wrap_in_collapsible=False, param_ # extend default params with any sent in during instantiation if self.param_extensions: - extend_params(params, self.param_extensions) + merge_jsonlike(params, self.param_extensions) # add any sent in though use in templates if param_extensions: - extend_params(params, param_extensions) + merge_jsonlike(params, param_extensions) if wrap_in_collapsible: @@ -805,11 +778,11 @@ def govuk_radios_field_widget(self, field, param_extensions=None, **kwargs): # extend default params with any sent in during instantiation if self.param_extensions: - extend_params(params, self.param_extensions) + merge_jsonlike(params, self.param_extensions) # add any sent in though use in templates if param_extensions: - extend_params(params, param_extensions) + merge_jsonlike(params, param_extensions) return Markup( render_template('components/radios/template.njk', params=params)) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index b869e9a6a..b198cf1b0 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -616,6 +616,8 @@ def test_get_sample_template_returns_template(template_type): ({"a": {"b": "c"}}, {"a": {"e": "f"}}, {"a": {"b": "c", "e": "f"}}), # same key in both dicts, value is a string, destination supercedes source: ({"a": "b"}, {"a": "c"}, {"a": "c"}), + # nested dict added to new key of dict, additive behaviour: + ({"a": "b"}, {"c": {"d": "e"}}, {"a": "b", "c": {"d": "e"}}), # lists with same length but different items, destination supercedes source: (["b", "c", "d"], ["b", "e", "f"], ["b", "e", "f"]), # lists in dicts behave as top level lists @@ -626,8 +628,14 @@ def test_get_sample_template_returns_template(template_type): ([{"b": "c"}], [{"b": "c"}], [{"b": "c"}]), # if dicts in lists have different values, they are not merged ([{"b": "c"}], [{"b": "e"}], [{"b": "e"}]), + # if nested dicts in lists have different keys, additive behaviour + ([{"b": "c"}], [{"d": {"e": "f"}}], [{"b": "c", "d": {"e": "f"}}]), # merge a dict with a null object returns that dict (does not work the other way round) - ({"a": {"b": "c"}}, None, {"a": {"b": "c"}}) + ({"a": {"b": "c"}}, None, {"a": {"b": "c"}}), + # double nested dicts, new adds new Boolean key: value, additive behaviour + ({"a": {"b": {"c": "d"}}}, {"a": {"b": {"e": True}}}, {"a": {"b": {"c": "d", "e": True}}}), + # double nested dicts, both have same key, different values, destination supercedes source + ({"a": {"b": {"c": "d"}}}, {"a": {"b": {"c": "e"}}}, {"a": {"b": {"c": "e"}}}) ]) def test_merge_jsonlike_merges_jsonlike_objects_correctly(source_object, destination_object, expected_result): merge_jsonlike(source_object, destination_object) From 67392e97ee05bed9f0ec98beb133b10789cdf5de Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 26 Jan 2021 12:16:08 +0000 Subject: [PATCH 4/4] Fix issue with looping in list merging The last_dest_idx variable should always have been tracking the last index in the source list. The original intention, implemented incorrectly, was to just append any items which source has no item at that index. --- app/utils.py | 4 ++-- tests/app/test_utils.py | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/utils.py b/app/utils.py index 75f301b72..554ccc83c 100644 --- a/app/utils.py +++ b/app/utils.py @@ -651,9 +651,9 @@ def merge_jsonlike(source, destination): return True def merge_lists(source, destination): - last_dest_idx = len(destination) - 1 + last_src_idx = len(source) - 1 for idx, item in enumerate(destination): - if idx <= last_dest_idx: + if idx <= last_src_idx: # assign destination value if can't be merged into source if merge_items(source[idx], destination[idx]) is False: source[idx] = destination[idx] diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index b198cf1b0..1f5cd0c8e 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -630,6 +630,8 @@ def test_get_sample_template_returns_template(template_type): ([{"b": "c"}], [{"b": "e"}], [{"b": "e"}]), # if nested dicts in lists have different keys, additive behaviour ([{"b": "c"}], [{"d": {"e": "f"}}], [{"b": "c", "d": {"e": "f"}}]), + # if dicts in destination list but not source, they just get added to end of source + ([{"a": "b"}], [{"a": "b"}, {"a": "b"}, {"c": "d"}], [{"a": "b"}, {"a": "b"}, {"c": "d"}]), # merge a dict with a null object returns that dict (does not work the other way round) ({"a": {"b": "c"}}, None, {"a": {"b": "c"}}), # double nested dicts, new adds new Boolean key: value, additive behaviour