Avoid rescuing argument error in next node blocks

[chrisroos]

I don't think it's a good idea to rescue from such a generic exception in our
next_node blocks. It hides real problems in our Ruby code and makes them look
as though the user has entered invalid data in the Smart Answer.

The rescuing of ArgumentError caused me problems twice recently. Once when I was
comparing a Date with a String and once when I was calling a method with
incorrect arguments. This change will mean that errors such as this cause the
application to fail fast.

>> Date.today < '2016-01-01'
ArgumentError: comparison of Date with String failed

>> def foo(arg); end
=> :foo
>> foo()
ArgumentError: wrong number of arguments (0 for 1)

The first two commits ("Raise InvalidResponse when parsing Integers in
Value#parse_input" and "Raise InvalidResponse when parsing Floats in
Value#parse_input") should have removed the only places where we were raising
an ArgumentError in response to bad user input.

We're still raising an ArgumentError from SmartAnswer::Calculators::VatPaymentDeadlines#last_payment_date and #funds_recevied_by. I haven't changed these because I don't believe this error will ever be raised by code in a next_node block. The error can only be raised by #last_payment_date and #funds_received_by if an option other than one of those listed in the multiple_choice options for the :how_do_you_want_to_pay? question is entered, and in that case the SmartAnswer::Question::MultipleChoice#parse_input method will raise an InvalidResponse exception first.

I ran all the regression tests locally against this change and didn't see any errors. That's no guarantee that we won't see problems if/when this is deployed to production.

Expected changes

• None.

[floehopper]

It looks as if there may be other places explicitly raising ArgumentError which might need attention:

• https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/calculators/vat_payment_deadlines.rb#L27
• https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/calculators/vat_payment_deadlines.rb#L50
• https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/question/base.rb#L28

Also I wonder whether it's worth inspecting the message of the legitimately rescued ArgumentErrors c.f. https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/question/date.rb#L75-L79

Apart from the above, this looks great to me! 👍

[chrisroos]

It looks as if there may be other places explicitly raising ArgumentError which might need attention:

• https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/calculators/vat_payment_deadlines.rb#L27

I've already addressed this in the description of this PR. Essentially, I don't think we need to worry about it.

• https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/calculators/vat_payment_deadlines.rb#L50

This is the same as the first example above, so I don't think we need to worry about it. I've updated the description of this PR to include this case as well.

• https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/question/base.rb#L28

This ArgumentError exception will be raised if next_node is called without any arguments which seems OK to me. I'm only interested in exceptions that are raised while evaluating blocks passed to next_node.

[floehopper]

Fair do's. Apologies if I missed the explanation in the PR description first time round.

Look good to me 👍

[chrisroos]

Also I wonder whether it's worth inspecting the message of the legitimately rescued ArgumentErrors c.f. https://github.com/alphagov/smart-answers/blob/master/lib/smart_answer/question/date.rb#L75-L79

I guess my thinking was to replicate the rescue ArgumentError behaviour that was previously in Flow#process. I can see that it'd probably be better to explicitly rescue certain exceptions but I think we could attack that separately as it would lead to a slight change in behaviour.