Fix multiple captures and their impact on nested field error links #301

jsugarman · 2021-07-01T07:58:15Z

What

Fix multiple captures and their impact on nested field error links

prevent the overhead of render calls being made multiple times
fix the side-effect of incrementing nested field-error ids, thereby breaking links between the error summary and the fields

Why

We have a fairly complex nested field structure for a few of our forms
and error linking using this formbuilder was not working, but works against
this branch.

On investigation I found that partials for the nested fields were being Rendered multiple times.
This did not actually render the fields multiple times, which I still do not understand
why not, but did increment the id of the nested fields.

Cause

1. Fieldset class plus super class capture the called block
Digging into the code I found that the RadioButtonsFieldset and CheckBoxesFieldset
inherit from the Base class which captures the block.call themselves AND via calls to
super().

Because super(arg1,arg2,etc) is called, the block is passed implicitly
whether you supply it as an arg to super, &block, or not. This
results in the block being called and captured twice capture { block.call },
once for the fieldset and once by superclass Base.

In the case of a block being passed that renders a partial it
means that rails renders the partial twice which has the
side effect of incrementing the ids of fields.

fixed by removing the fieldset level capture and relying on inheritance
2. fieldset_item block capture
Ironically, out of a change we asked for, and you kindly supplied, there is an additional
capture { block.call } in the FieldsetItem trait. which we are also dependent on.

fixed by passing the captured content directly

Example

A simplified version with only one fieldset might look like this

# my_nested_fields_partial
    = f.fields_for :details, @details do |ff|
        = ff.govuk_text_field :name

# my_form_partial
= f.govuk_radio_buttons_fieldset :provider, do
    if some_condition
      = render partial: 'my_nested_fields_partial'

The resulting html
my_nested_fields_partial is called twice, but only displays content once??, but does increment field-error ids so:
id of the name field is provider-details-attributes-1-name-field-error while the summary error generated for it correctly links to provider-details-attributes-0-name-field-error

Real example

We have fieldsets inside fieldsets plus conditional blocks which causes exponential id incrementation issues, so that our first nested field ends up with an id of 15 instead of 0, the second with an id of 31.

Additional

Possibly problematic block captures exist in other classes that inherit from the Base class and could result in similar issues being experienced. lib/govuk_design_system_formbuilder/elements/submit.rb in particular. I have not changed this in this PR however as I have not experienced problems with this myself.

Inherits from base which captures the `block.call` already. Because `super(arg1,arg2,etc)` is called, the block is passed implicitly whether you supply it as an arg to super, `&block`, or not. This results in the block being called and captured twice `capture { block.call }`, once for the fieldset and once by superclass `Base`. In the case of a block being passed that renders a partial it means the partial will be rendered twice for example. ``` f.govuk_radio_buttons_fieldset do = render partial: 'my_radio_button_partial' => gets rendered twice, at least ```

Inherits from base which captures the `block.call` already. Because `super(arg1,arg2,etc)` is called, the block is passed implicitly whether you supply it as an arg to super, `&block`, or not. This results in the block being called and captured twice `capture { block.call }`, once the fieldset and once for the superclass, `Base`. In the case of a block being passed that renders a partial it means the partial will be rendered twice for example. ``` f.govuk_check_boxes_fieldset do = render partial: 'my_check_box_partial' => gets rendered twice, at least ```

jsugarman · 2021-07-01T08:00:53Z

Hi @peteryates, hopefully this PR makes sense. happy to discuss here or via slack, as usual

peteryates · 2021-07-01T08:31:07Z

Thanks @jsugarman! Yeah, this looks good to me and your solution is definitely easier to follow. Will take a closer look later and get this merged and released.

peteryates · 2021-07-01T08:52:04Z

spec/support/shared/shared_block_examples.rb

+      before { allow(helper).to receive(:render) }
+
+      let(:example_block) { proc { helper.render(html: '<b>rendered</b>') } }
+
+      specify do
+        subject
+        expect(helper).to have_received(:render).once
+      end


I think reordering the setup makes this a tiny bit easier to follow

Suggested change

before { allow(helper).to receive(:render) }

let(:example_block) { proc { helper.render(html: '<b>rendered</b>') } }

specify do

subject

expect(helper).to have_received(:render).once

end

let(:example_block) { proc { helper.render(html: '<b>rendered</b>') } }

before { allow(helper).to receive(:render) }

before { subject }

specify { expect(helper).to have_received(:render).once }

Do you prefer to keep the befores separate?

or

before do allow(helper).to receive(:render) subject end

have applied as separate befores for now as above

peteryates · 2021-07-01T08:54:08Z

lib/govuk_design_system_formbuilder/containers/check_boxes_fieldset.rb

@@ -5,6 +5,8 @@ class CheckBoxesFieldset < Base
      include Traits::Hint

      def initialize(builder, object_name, attribute_name, hint:, legend:, caption:, small:, classes:, form_group:, multiple:, &block)
+        raise LocalJumpError, 'no block given' unless block_given?


Immediately erroring here is a huge improvement. I tend to use fail instead of raise where there's no intention of rescuing.

…ired check box and radio button fieldsets were handling passing of blocks either explicitly (check_boxes_fieldset) or implicitly (radio_buttons_fieldset). This handles blocks being passed to superclass explcitly for both for consitency. A couple of specs broke indicating current functionality expects an error to be raised when no block is passed the helpers. I have handled this by raising an error explcitly for these objects, and by raising a `LocalJumpError`. `LocalJumpError`s are raised when yield is called in a method which is not supplied with a block, so is, perhaps, a more idiomatic error?! `block_given?` is used to test whether block is provided to avoid side affects as it does not call the block, [I believe](https://ruby-doc.org/core-2.7.0/Kernel.html#method-i-block_given-3F)

The `capture { block.call }` in the `conditional_content` method causes blocks passed to `govuk_radio_buttons_fieldset` or `govuk_check_boxes_fieldset` to be rendered twice. This is because the `Traits::FieldsetItem` is included in `FieldsetRadioButton` and `FieldsetCheckBox`, which inherit from `Base` and call super(), which captures the block content already. ``` @block_content = capture { block.call } if block_given? ``` This change assumes that `Traits::FieldsetItem` will only ever be included in Objects that already capture block content and can therefore pass their content directly.

…dset This shares the test logic that the fieldsets require and locks down the behaviour: - a block is required for fieldsets - a block should only be called/yeilded to once - a block which calls render only ends up calling render once The last two are fundamentally the same and both are testing implmentation detail, which is not ideal. The checking that render is only called once is closer to the use case and behaviour I am trying to define but perhaps there is a better way to test that in a black-box fashion.

Couple of Lint/UnusedMethodArgument cop violations plus is more consistent with how we are passing named blocks in `RadioButtonsFieldset` and `CheckBoxesFieldset`.

peteryates

Definite improvement 👍🏽

jsugarman · 2021-07-01T11:03:49Z

Great, many thanks @peteryates . I assume you have to merge, especially as the test coverage check always fails when the PR is from a fork.

jsugarman added 2 commits June 29, 2021 17:12

peteryates reviewed Jul 1, 2021

View reviewed changes

jsugarman added 4 commits July 1, 2021 10:54

Fix rubocop violations and pass blocks explicitly

fd025f4

Couple of Lint/UnusedMethodArgument cop violations plus is more consistent with how we are passing named blocks in `RadioButtonsFieldset` and `CheckBoxesFieldset`.

jsugarman force-pushed the fix-multiple-captures branch from 486eeec to fd025f4 Compare July 1, 2021 10:02

peteryates approved these changes Jul 1, 2021

View reviewed changes

peteryates merged commit 028a55f into x-govuk:master Jul 1, 2021

jsugarman mentioned this pull request Jul 1, 2021

[CFP-158] Fix govuk_design_system_formbuilder nested error linking ministryofjustice/Claim-for-Crown-Court-Defence#4080

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiple captures and their impact on nested field error links #301

Fix multiple captures and their impact on nested field error links #301

jsugarman commented Jul 1, 2021 •

edited

Loading

jsugarman commented Jul 1, 2021

peteryates commented Jul 1, 2021

peteryates Jul 1, 2021

jsugarman Jul 1, 2021

jsugarman Jul 1, 2021

peteryates Jul 1, 2021

peteryates left a comment

jsugarman commented Jul 1, 2021

Fix multiple captures and their impact on nested field error links #301

Fix multiple captures and their impact on nested field error links #301

Conversation

jsugarman commented Jul 1, 2021 • edited Loading

What

Why

Cause

Example

Real example

Additional

jsugarman commented Jul 1, 2021

peteryates commented Jul 1, 2021

peteryates Jul 1, 2021

Choose a reason for hiding this comment

jsugarman Jul 1, 2021

Choose a reason for hiding this comment

jsugarman Jul 1, 2021

Choose a reason for hiding this comment

peteryates Jul 1, 2021

Choose a reason for hiding this comment

peteryates left a comment

Choose a reason for hiding this comment

jsugarman commented Jul 1, 2021

jsugarman commented Jul 1, 2021 •

edited

Loading