Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy automatic detection of possible next nodes #2343

Merged
merged 18 commits into from Mar 10, 2016

Conversation

Projects
None yet
2 participants
@floehopper
Copy link
Contributor

commented Mar 8, 2016

Previously the possible node keys returned by a next_node block had to be specified explicitly using the permitted keyword argument. This PR introduces a mechanism for automatically detecting the possible node keys by parsing the source code in the next_node block.

In order to make use of this mechanism, you should set the permitted keyword argument to :auto and wrap the node keys returned by the next_node block with calls to #question or #outcome as appropriate. See the example below for details.

Note that if you fail to wrap the node key, you will see an exception when that node is chosen much as you would've done if a node not in the list of permitted next nodes was chosen. Note also that at the moment the #question and #outcome methods are identical i.e. it doesn't actually matter which you use. However, I think using them improves the readability of the code and if we thought it was useful, we could later introduce a check that the chosen node is of the correct type.

Before

multiple_choice :question? do
  option :option_one
  option :option_two

  permitted_next_nodes = [
    :solution_one,
    :solution_two
  ]

  next_node(permitted: permitted_next_nodes) do |response|
    case response
    when :option_one
      :solution_one
    when :option_two
      :solution_two
    end
  end
end

After

multiple_choice :question? do
  option :option_one
  option :option_two

  next_node(permitted: :auto) do |response|
    case response
    when :option_one
      outcome :solution_one
    when :option_two
      outcome :solution_two
    end
  end
end

This work follows on from #2311 and makes use of the changes in #2319 & #2340.

However, in contrast to #2311, the automatic detection of possible next nodes is now only triggered when a flow is being visualised via the GraphPresenter. Also this implementation does not use throw & catch. Hopefully this allays the concerns about a possible deterioration in performance.

Unfortunately the logic in SmartAnswer::Question::Base is more complicated than I would like, but much of that complexity is due to having to support both the old and new styles of next_node blocks. I'm confident that it'll be possible to simplify the logic considerably once we've converted all the flows over to the new style. We should also be able to remove the permitted: :auto option once all the flows have been converted.

I did contemplate adding a new engine/integration test for the new behaviour. However, in the end I decided that updating all the fixtures for the existing engine/integration tests gave enough coverage and was more in keeping with the current state of affairs. Also I was pleased to see that the GraphPresenterTest uses one of these fixture flows - now that the fixture has been updated, the fact that the test still passes gives me confidence that the automatic detection of possible next nodes is working OK.

I have not updated any published flows to use the new style next_node blocks in this PR. I'm imagining doing that in one or more subsequent PRs. It should be easy to do flow-by-flow or even question-by-question, because of the support for the old style syntax.

For further details, please see the individual commit notes.

Trello card

@chrisroos chrisroos self-assigned this Mar 9, 2016

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2016

I think we might have problems using this approach if we also want to use next to return early from the next_node block. Fortunately, it looks as though the only Smart Answer using next is check-uk-visa, and the work that @erikse did in PR #2310 should mean that we can easily remove those uses of next.

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2016

This all looks good to me. It took me quite a while to understand what was going on but I think I've got it now! As you've already mentioned, the code should be easier to understand once we've removed support for supplying the permitted next nodes via the permitted argument.

Great work, @floehopper!

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

@chrisroos: Can you explain why we might have problems using this approach in flows which use next to exit early from a next_node block? I just changed the first such occurrence in check-uk-visa to:

if calculator.study_visit? || calculator.work_visit?
  next question(:staying_for_how_long?)
end

And ran its regression test and they all passed. Am I missing something?

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

@chrisroos: Thanks for reviewing. I think I've addressed all your comments. It'd be great if you could review the latest state of play so I can try to get this merged tomorrow.

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

Can you explain why we might have problems using this approach in flows which use next to exit early from a next_node block?

@floehopper: Sorry - my mistake! When I was trying this branch locally I ran into problems visualising the check-uk-visa flow after updating it to use the question and outcome methods. I obviously didn't investigate properly because I've just done the same thing again and the visualisation works as expected. Sorry for the confusion.

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

Your changes look good to me, @floehopper.

floehopper added some commits Mar 7, 2016

Introduce `permitted: :auto` option for next_node
For the moment, using this option sets `permitted_next_nodes` to be an empty
array. While this isn't immediately useful, it paves the way for automatically
detecting possible next nodes within a `next_node` block.

The new option is currently unusable, because if `permitted_next_nodes` is an
empty array then any result from the `next_node` block will result in an
exception since it won't be in the list of `permitted_next_nodes`.
Introduce optional syntactic sugar for next_node
This change just introduces an alternative way to define the result of a
`next_node` block. However, it will also make it easier to automatically detect
possible next nodes within a `next_node` block by parsing the source code. I
think the new style is also slightly more readable.

Original style:

```
next_node(permitted: [:next_question, :done]) do |response|
  if response == 'skip'
    :done
  else
    :next_question
  end
```

New style:

```
next_node(permitted: [:next_question, :done]) do |response|
  if response == 'skip'
    outcome :done
  else
    question :next_question
  end
```

Note that I've used `Object#dup` to "unfreeze" the `state` so we can
`extend` it with the next node block methods. This follows the pattern elsewhere
in the code where the `state` is duplicated and re-frozen whenever it is
modified. I've added a unit test to ensure the `state` which we use to
`instance_exec` the next node block is frozen.
Introduce parser for identifying method invocations
Although this is not currently used, I plan to use this to automatically
detect possible next nodes in a `next_node` block.

Although this is a fairly general solution, it does not currently handle
methods with negative arity i.e. it will just ignore invocations of them.

The `invocations` method returns an array of hashes each of which has two
keys. The `:method` key references the method object in the
`methods_of_interest` passed into the constructor. The `:arg_nodes` key
references an AST fragment representing the arguments with which the method
was called.

This commit introduces explicit dependencies on the `parser`, `ast` & `method_source` gems. However, `parser` was already an implicit dependency via `rubocop` and `ast` was already
an implicit dependency via `rubocop` and thence `parser`. `method_source` was already an
implicit dependency via `pry`. Thus no new actual dependencies have been introduced.

I've included `AST::Sexp` into the test in order to simplify the code when
asserting against a specific AST fragment.
Introduce parser for identifying possible next nodes
This makes use of the recently introduced, more general parser,
`MethodInvocationParser` to identify invocations of the syntactic sugar
methods on `NextNodeBlock::InstanceMethods`. I plan to use this to
automatically identify possible next nodes in a `next_node` block.
Automatically identify permitted next nodes
This makes use of `SmartAnswer::Question::NextNode::Parser` to identify calls
to the syntactic sugar methods in
`SmartAnswer::Question::NextNode::InstanceMethods` i.e. `#question` and
`#outcome`.

Currently there's nothing forcing all return values to be wrapped in a call
to one of these methods, but I plan to address that in subsequent commits.
Move permitted next node auto-detection into reader
This is just a refactoring, i.e. there should be no change in behaviour.
Hence there are no changes to the tests.

Now that the automatic detection of permitted next nodes happens inside the
reader method, I've had to start calling the reader method from within
`Question::Base` in a few places.

My plan for future commits is to remove calls to the reader method from
within `Question::Base`. The only other places which call this method
are in `GraphPresenter`.

This will mean that the automatic detection of permitted next nodes only
happens when strictly necessary i.e. when visualising the flow and not
during normal user interaction.

The benefit of doing this is that we shouldn't need to worry about the
performance of the automatic detection code.
Relax permitted next node check when `permitted: :auto`
This change removes one of the places which was invoking the
`permitted_next_nodes` reader method from within `Question::Base`. We now
access the `@permitted_next_nodes` instance variable directly, avoiding
needlessly triggering the automatic detection code in the reader method.

Although it's nicer to fail-fast at flow-definition-time if no nodes are
returned via the syntactic sugar methods, an exception will still be raised
at flow-execution-time, because the chosen next node will not be in the
(empty) list of permitted next nodes.

I did contemplate moving the check into the `permitted_next_nodes` reader
method, but since I'm aiming for that to only be invoked from the
`GraphPresenter`, I'm not sure that would gain us much.

I also contemplated removing the check altogether (i.e. when auto-detection
is disabled). However, I think it's useful enough that the extra complexity
is worth it.
Use more idiomatic Rails method: Object#present?
I'm planning to change `NextNodeBlock::InstanceMethods#question` and `#outcome`
to return a new class wrapping the node key symbol to represent the fact that
it's been returned via one of these methods.

By using `Object#present?` here, we'll be able to keep this condition unchanged
by making sure the new class implements `#present?` appropriately.
Alternative check for next node when `permitted: :auto`
Previously we always checked whether the node key returned from `next_node`
was in the list of permitted next nodes. I think that the main point of this
check was to ensure that the list of permitted next nodes stayed up-to-date
with the possible return values from `next_node` for each question, so that
the data used for visualisation is up-to-date.

The point of this commit is to avoid having to invoke the permitted next node
automatic detection code just in order to perform this check. I'm avoiding
that by calling the `@permitted_next_nodes` instance variable direct instead
of the reader method. Now the only place which uses the reader method is the
`GraphPresenter` used in visualisation.

The syntactic sugar methods, `question` & `outcome` now return the node key
wrapped in a new class, `NextNodeBlock::PermittedNodeKey` which allows us to
detect whether the node key returned from `next_node` has come from one of
the syntactic sugar methods. By raising an exception if it isn't we can be
sure that the automatic detection parsing will identify that node key as a
possible next node and hence ensure that the data used for visualisation is
kept up-to-date.

I've inherited `PermittedNodeKey` from `SimpleDelegator` so that to almost all
intents and purposes it "looks" just like a symbol. However, I noticed that
the `nil?` method is not delegated automatically. I've added unit tests to
assert the behaviour of methods which are being used in
`Question::Base#next_node_for` i.e. `present?`, `to_sym` & `to_s` (String
interpolation in exception message). Since Rails implements `Object#present?`
as the inverse of `Object#blank?`, I've tested the latter as well.

Although the code in `Question::Base` is now considerably more complicated than
I would like, much of that complexity is related to dealing with the "legacy"
style of `next_node` blocks, i.e. where permitted next nodes are explicitly
passed in. Since the idea is that this will go away, I'm confident that it'll
be possible to simplify the code significantly when we've converted all the
flows to use the new style of `next_node` blocks.
Use `permitted: :auto` in graph fixture
Note that this is used in the `GraphPresenterTest`. I've looked at the coverage
provided by this test and it gives me confidence that the visualisation still
works even when permitted next nodes are detected automatically.
@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2016

@chrisroos: Thanks for reviewing again.

I'm going to rebase this against master, apply the fixup & squash commits, and force-push.

@floehopper floehopper force-pushed the lazy-automatic-detection-of-possible-next-nodes branch from 4da22e5 to 20205f7 Mar 10, 2016

floehopper added a commit that referenced this pull request Mar 10, 2016

Merge pull request #2343 from alphagov/lazy-automatic-detection-of-po…
…ssible-next-nodes

Lazy automatic detection of possible next nodes

@floehopper floehopper merged commit 327f209 into master Mar 10, 2016

1 check passed

default "Build #4363 succeeded on Jenkins"
Details

@floehopper floehopper deleted the lazy-automatic-detection-of-possible-next-nodes branch Mar 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.