diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..6dfc09d --- /dev/null +++ b/TODO.md @@ -0,0 +1,54 @@ +## What needs to be done + +### State store inspection + +Would be good to inspect the state store all step attributes and return what is on repository in +a very explanable manner (step attribute and repository value): + +```ruby +state_store.inspect +``` + +### README + +The readme is a mess. + +We need to explain many concepts first: + +* Repository +* State Store +* Step +* Wizard +* Route Strategy +* Steps Processors +* Steps operator +* Inspect +* Logger + +MOST IMPORTANT: Explain data flow: + +* The data flow - devs are confusing +* Explain in ASCII diagram +* Make simple examples + +Explain Navigation flow: + +* DSL Definitions on Wizard#steps_processor +* Method definitions in wizard (although state store is recommended) +* Explain flow_path, saved_path, valid_path +* Explain current_step, next_step, previous_step +* Explain current_step_path, next_step_path, previous_step_path +* Previous path uses flow_path therefore makes sure there are no circular flow + +Explain steps instantiation (if needed): +* flow_steps, saved_steps, valid_steps + +Explain implementing check your answers page: +* Document the CheckAnswersPresenter module +* Show how to create custom presenter classes with row grouping +* Explain format_value override for custom formatting + +We need to enforce the decisions developers needs to make when using the gem and implementing a +wizard. + +Things you need to defined (where to store data? Create one repo for all steps - maybe one - maybe many). diff --git a/lib/dfe/wizard.rb b/lib/dfe/wizard.rb index 1712af3..334f4f6 100644 --- a/lib/dfe/wizard.rb +++ b/lib/dfe/wizard.rb @@ -23,7 +23,7 @@ module DfE # include DfE::Wizard # # def steps_processor - # DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + # DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| # graph.add_node :name, NameStep # graph.add_node :email, EmailStep # graph.add_node :review, ReviewStep @@ -109,6 +109,10 @@ module Core end # @!endgroup + # Module for building Check Your Answers pages + # @api public + autoload :CheckAnswersPresenter, 'dfe/wizard/check_answers_presenter' + # @!group Auto generate Documentation for any wizard module Documentation autoload :Generator, 'dfe/wizard/documentation/generator' @@ -325,7 +329,7 @@ def define_step_attributes_in_state_store # # @example # def steps_processor - # DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + # DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| # graph.add_node :step1, Step1 # graph.root :step1 # end diff --git a/lib/dfe/wizard/behaviours/step_management.rb b/lib/dfe/wizard/behaviours/step_management.rb index 6799367..f3c0053 100644 --- a/lib/dfe/wizard/behaviours/step_management.rb +++ b/lib/dfe/wizard/behaviours/step_management.rb @@ -287,8 +287,8 @@ def step_definitions # Return all attribute names from all step classes # # Flattens the step definitions to extract every attribute defined across - # all steps in the wizard. Used by StateStore's method_missing to determine - # which method calls should access repository data. + # all steps in the wizard. Used by StateStore to generate accessor methods + # for each attribute. # # Attributes are collected from each step class via its `attribute_names` # class method. Steps without attributes contribute nothing to this list. @@ -311,11 +311,11 @@ def step_definitions # wizard.attribute_names.include?("undefined") # => false # # @note Attribute names are collected lazily during wizard initialization. - # This allows StateStore to build method_missing handlers only for + # This allows StateStore to generate accessor methods only for # attributes actually defined in step classes. # # @see #step_definitions For step class objects themselves - # @see StateStore#method_missing Which uses this list to route method calls + # @see StateStore#define_attribute_accessors Which generates methods from this list # @api public def attribute_names step_definitions.flat_map do |_step_id, step_class| diff --git a/lib/dfe/wizard/check_answers_presenter.rb b/lib/dfe/wizard/check_answers_presenter.rb new file mode 100644 index 0000000..429db23 --- /dev/null +++ b/lib/dfe/wizard/check_answers_presenter.rb @@ -0,0 +1,278 @@ +module DfE + module Wizard + # Module for building Check Your Answers pages. + # + # Include this module in your own presenter class to get helper methods + # for building check your answers pages. You control the structure, grouping, + # and formatting - the module provides the building blocks. + # + # @example Create your own check answers presenter + # class RegisterECTCheckAnswers + # include DfE::Wizard::CheckAnswersPresenter + # + # def teacher_details + # [ + # row_for(:review_ect_details, :correct_full_name), + # row_for(:email_address, :email), + # row_for(:start_date, :start_date), + # ] + # end + # + # def programme_details + # [ + # row_for(:programme_type, :training_programme), + # row_for(:lead_provider, :lead_provider_id), + # ] + # end + # + # # Override to customize formatting + # def format_value(attribute, value) + # case attribute + # when :start_date + # value&.to_fs(:govuk_date) + # else + # value + # end + # end + # end + # + # @example In controller + # @check_answers = RegisterECTCheckAnswers.new(@wizard) + # + # @example In view + # <% @check_answers.teacher_details.each do |row| %> + #
<%= row.label %>
+ #
<%= row.formatted_value %>
+ #
<%= link_to "Change", row.change_path %>
+ # <% end %> + # + module CheckAnswersPresenter + # Represents a single row in a check your answers page. + # + # Provides access to raw and formatted values, labels from I18n, + # and change paths for editing. + class Row + # @return [Symbol] the step identifier + attr_reader :step_id + + # @return [Symbol] the attribute name + attr_reader :attribute + + # @return [DfE::Wizard::Step, nil] the step instance + attr_reader :step + + # @return [String] the change path with return_to_review parameter + attr_reader :change_path + + # @return [String, nil] custom label override + attr_reader :custom_label + + # @return [Object, nil] pre-computed formatted value + attr_reader :formatted_value + + def initialize(step_id:, attribute:, step:, change_path:, formatted_value: nil, custom_label: nil) + @step_id = step_id + @attribute = attribute + @step = step + @change_path = change_path + @formatted_value = formatted_value + @custom_label = custom_label + end + + # The raw attribute value from the step. + # + # @return [Object] the unformatted value + def value + step&.public_send(attribute) + end + + # The human-readable label for this attribute. + # + # Uses custom label if provided, otherwise falls back to + # the step's human_attribute_name (which uses I18n). + # + # @return [String] the label text + # + # @example With I18n + # # config/locales/en.yml + # # en: + # # activemodel: + # # attributes: + # # steps/email_step: + # # email: "Email address" + # + # row.label # => "Email address" + def label + return custom_label if custom_label + + if step&.class.respond_to?(:human_attribute_name) + step.class.human_attribute_name(attribute) + else + attribute.to_s.humanize + end + end + + # Check if the row has a value. + # + # @return [Boolean] true if value is present + def value? + value.present? + end + + # Convert to hash for compatibility. + # + # @return [Hash] row data as hash + def to_h + { + step_id: step_id, + attribute: attribute, + label: label, + value: value, + formatted_value: formatted_value, + change_path: change_path, + } + end + end + + # @return [DfE::Wizard] the wizard instance + attr_reader :wizard + + # Initialize the presenter with a wizard. + # + # @param wizard [DfE::Wizard] the wizard instance + def initialize(wizard) + @wizard = wizard + end + + # Steps available for review. Override to customize. + # + # @return [Array] steps to include in review + def reviewable_steps + wizard.flow_steps + end + + # Find a step by ID from reviewable steps. + # + # @param step_id [Symbol] the step identifier + # @return [DfE::Wizard::Step, nil] the step or nil if not found + def find_reviewable_step(step_id) + reviewable_steps.find { |step| step.step_id == step_id } + end + + # Build a row for a step attribute. + # + # Returns a Row object with access to the value, formatted value, + # label, and change path. Returns nil if the step is not in + # reviewable_steps or if the attribute doesn't exist on the step. + # + # @param step_id [Symbol] the step identifier + # @param attribute [Symbol] the attribute name + # @param label [String, nil] custom label (defaults to step's human_attribute_name) + # @param change_step [Symbol, nil] override which step the change link goes to + # + # @return [Row, nil] row object or nil if step/attribute not available + # + # @example Basic row + # row = row_for(:email_address, :email) + # row.value # => "john@example.com" + # row.formatted_value # => "john@example.com" (or custom via format_value override) + # row.label # => "Email" (from I18n or humanized) + # row.change_path # => "/email?return_to_review=email_address" + # + # @example With custom label + # row_for(:start_date, :start_date, label: "ECT start date") + def row_for(step_id, attribute, label: nil, change_step: nil) + step = find_reviewable_step(step_id) + return nil unless step + return nil unless step.respond_to?(attribute) + + raw_value = step.public_send(attribute) + + Row.new( + step_id: step_id, + attribute: attribute, + step: step, + change_path: change_path_for(change_step || step_id), + formatted_value: format_value(attribute, raw_value), + custom_label: label, + ) + end + + # Format a value for display. + # + # Override this method in your presenter to customize how values + # are displayed. By default, returns the raw value unchanged. + # + # @param attribute [Symbol] the attribute name + # @param value [Object] the raw value from the step + # @return [Object] the formatted value for display + # + # @example Custom formatting + # def format_value(attribute, value) + # case attribute + # when :start_date then value&.to_fs(:govuk_date) + # when :working_pattern then value&.humanize + # else value + # end + # end + def format_value(_attribute, value) + value + end + + # Build multiple rows from a step's attributes. + # + # Returns an empty array if the step is not in reviewable_steps. + # Filters out any nil rows (e.g., for non-existent attributes). + # + # @param step_id [Symbol] the step identifier + # @param attributes [Array] attribute names or hashes with options + # + # @return [Array] array of Row objects + # + # @example + # rows_for(:personal_details, [:first_name, :last_name, :email]) + # rows_for(:personal_details, [:first_name, { attribute: :dob, label: "Date of birth" }]) + def rows_for(step_id, attributes) + step = find_reviewable_step(step_id) + return [] unless step + + attributes.filter_map do |attr| + if attr.is_a?(Hash) + row_for(step_id, attr[:attribute], **attr.except(:attribute)) + else + row_for(step_id, attr) + end + end + end + + # Generate the change path for a step. + # + # @param step_id [Symbol] the step to link to + # @return [String] the path with return_to_review parameter + def change_path_for(step_id) + wizard.resolve_step_path(step_id, return_to_review: step_id) + end + + # Get all valid steps that have been completed. + # + # @return [Array] array of valid step instances + def completed_steps + wizard.valid_steps + end + + # Get the flow steps (all steps in current path). + # + # @return [Array] array of step instances in flow order + def flow_steps + wizard.flow_steps + end + + # Access the state store for custom data. + # + # @return [DfE::Wizard::StateStore] the state store + def state_store + wizard.state_store + end + end + end +end diff --git a/lib/dfe/wizard/core/state_store.rb b/lib/dfe/wizard/core/state_store.rb index f416cfa..ccdf46c 100644 --- a/lib/dfe/wizard/core/state_store.rb +++ b/lib/dfe/wizard/core/state_store.rb @@ -17,7 +17,19 @@ module StateStore attr_accessor :step_definitions # @return [Array] All attribute names from all steps - attr_accessor :attribute_names + attr_reader :attribute_names + + # Sets attribute names and generates explicit accessor methods. + # + # Generates real methods for each attribute, making data flow + # explicit and easier to debug. + # + # @param names [Array] all steps attribute names + # @return [void] + def attribute_names=(names) + @attribute_names = names + define_attribute_accessors if step_attributes_methods? + end # Initializes a new state store with the given repository. # @@ -107,21 +119,20 @@ def execute_operation(operation_class:, step:) # Determine if attribute methods generation from steps are enabled # - # Can be overridden in subclasses to disable method_missing-based - # attribute access. + # Can be overridden in subclasses to disable automatic attribute + # accessor generation. # # Useful for strict programming or when attributes should # be accessed through explicit methods only. # - # @return [Boolean] true if method_missing should route attributes - # dynamically + # @return [Boolean] true if accessor methods should be generated # # @example Disable attribute generation in subclass # class CustomStateStore # include DfE::Wizard::Core::StateStore # # def step_attributes_methods? - # false # Disable method_missing + # false # Disable accessor generation # end # end # @@ -130,85 +141,42 @@ def step_attributes_methods? true end - # Route unknown method calls to repository data - # - # Implements dynamic attribute access for all step attributes. - # When a method is called that matches an attribute name, - # this method returns the corresponding value from repository data. - # - # This replaces the need to pre-generate singleton methods for - # every possible attribute. Instead, methods are resolved on-demand - # through method_missing. - # - # ## Call Flow - # - # 1. User calls `state_store.first_name` - # 2. Ruby looks for `first_name` method (not found) - # 3. Ruby calls `method_missing(:first_name)` - # 4. Check if `:first_name` in `attribute_names` - # 5. If yes, return `read[:first_name]` - # 6. If no, call `super` (raise NoMethodError) - # - # ## Performance - # - # First call: method_missing overhead (~2-3x slower than direct method) - # Subsequent calls: same as first (method_missing lookup every time) + # Generate explicit accessor methods for all step attributes. # - # For typical wizards with 5-10 attributes accessed per request, - # the performance impact is negligible. + # This generates real singleton methods for each attribute, making + # data flow explicit: + # - state_store.first_name calls a real method + # - Easier to debug and trace in stack traces + # - Better IDE support for autocompletion # - # @param method_name [Symbol] Name of missing method - # @param args [Array] Arguments passed to method + # Methods are only generated if they don't already exist on the object, + # preserving any custom implementations in subclasses. # - # @return [Object] Value from repository data for the attribute - # - # @raise [NoMethodError] If method name is not a known attribute - # - # @example Accessing attributes - # state_store.attribute_names = [:first_name, :email, :nationality] - # state_store.read = { first_name: "Sarah", email: "sarah@example.com" } - # - # state_store.first_name # => "Sarah" - # state_store.email # => "sarah@example.com" - # state_store.nationality # => nil (not in repository) - # state_store.undefined_attr # => raises NoMethodError - # - # @example Custom methods are not affected - # class StateStore - # include DfE::Wizard::Core::StateStore - # - # def full_name - # "#{first_name} #{last_name}" # Custom implementation - # end - # end + # @return [void] # - # # full_name is called directly, NOT through method_missing - # state_store.full_name # => Custom logic executed + # @example Generated methods + # state_store.attribute_names = [:first_name, :last_name] + # # Generates: + # # def first_name; read[:first_name]; end + # # def last_name; read[:last_name]; end # # @api private - def method_missing(method_name, *args) - if step_attribute?(method_name) - read[method_name] - else - super + def define_attribute_accessors + attribute_names.each do |attr| + attr_sym = attr.to_sym + + next if respond_to?(attr_sym) + + define_singleton_method(attr_sym) { read[attr_sym] } end end - # Check if an attribute name is a known step attribute - # - # Internal helper method used by both {#method_missing} and {#respond_to_missing?} - # to determine whether a method call should be routed to repository data. - # - # ## Logic + # Check if an attribute name is a known step attribute. # # Returns true only if: # 1. Attribute method generation is enabled via {#step_attributes_methods?} # 2. The attribute name exists in the `attribute_names` collection # - # This two-part check allows: - # - Disabling all dynamic attribute access by overriding `step_attributes_methods?` - # - Maintaining custom methods that won't be affected by method_missing - # # @param attribute_name [Symbol, String] Name to check # # @return [Boolean] true if the name is a known step attribute and generation is enabled @@ -221,53 +189,11 @@ def method_missing(method_name, *args) # state_store.step_attribute?(:email) # => true # state_store.step_attribute?(:undefined) # => false # - # @example With generation disabled - # state_store.attribute_names = [:first_name, :email] - # # Override step_attributes_methods? to return false - # - # state_store.step_attribute?(:first_name) # => false (generation disabled) - # state_store.step_attribute?(:email) # => false (generation disabled) - # - # @note This method is called internally by {#method_missing} and {#respond_to_missing?} - # and is rarely called directly by user code. - # - # @see #method_missing Which uses this to route attribute access - # @see #respond_to_missing? Which uses this for introspection # @see #step_attributes_methods? Which enables/disables attribute generation # @api public def step_attribute?(attribute_name) step_attributes_methods? && attribute_names.include?(attribute_name.to_s) end - - # Support respond_to? for dynamic attributes - # - # Enables proper introspection of dynamic attribute methods. - # When `respond_to?` is called with an attribute name, returns true - # if that attribute exists in `attribute_names`. - # - # This is necessary for Rails/Rack compatibility and proper - # duck-typing support. - # - # @param method_name [Symbol, String] Name to check - # @param include_private [Boolean] Include private methods (default: false) - # - # @return [Boolean] true if method_name is a known attribute or exists - # - # @example Checking for attributes - # state_store.attribute_names = [:first_name, :email] - # - # state_store.respond_to?(:first_name) # => true - # state_store.respond_to?(:email) # => true - # state_store.respond_to?(:undefined) # => false - # - # @example Works with Rails parameter helpers - # # Rails uses respond_to? internally - # params.require(:state_store) # Works with respond_to? support - # - # @api public - def respond_to_missing?(method_name, include_private = false) - step_attribute?(method_name) || super - end end end end diff --git a/lib/dfe/wizard/steps_processor/base.rb b/lib/dfe/wizard/steps_processor/base.rb index 4d7a051..d00f610 100644 --- a/lib/dfe/wizard/steps_processor/base.rb +++ b/lib/dfe/wizard/steps_processor/base.rb @@ -27,7 +27,7 @@ module StepsProcessor # end # # @example Using a processor - # processor = Graph.draw(wizard, context: state_store) do |g| + # processor = Graph.draw(wizard, predicate_caller: state_store) do |g| # g.add_node :step1, StepOne # g.add_node :step2, StepTwo # end @@ -493,7 +493,7 @@ def dsl # @raise [ArgumentError] If validation fails # # @example - # processor = Graph.draw(wizard, context: state_store) do |g| + # processor = Graph.draw(wizard, predicate_caller: state_store) do |g| # g.add_node :step1, StepOne # g.add_node :step2, StepTwo # g.root :step1 diff --git a/lib/dfe/wizard/steps_processor/graph.rb b/lib/dfe/wizard/steps_processor/graph.rb index e53f860..cfc437f 100644 --- a/lib/dfe/wizard/steps_processor/graph.rb +++ b/lib/dfe/wizard/steps_processor/graph.rb @@ -18,7 +18,7 @@ module StepsProcessor # Use Graph for multi-path workflows where step order isn't linear. # # @example Simple linear graph - # Graph.draw(wizard) do |g| + # Graph.draw(wizard, predicate_caller: state_store) do |g| # g.add_node :step1, Step1 # g.add_node :step2, Step2 # g.root :step1 @@ -26,7 +26,7 @@ module StepsProcessor # end # # @example With conditional branching - # Graph.draw(wizard) do |g| + # Graph.draw(wizard, predicate_caller: state_store) do |g| # g.add_node :visa_type, VisaTypeStep # g.add_node :student, StudentDetailsStep # g.add_node :work, WorkDetailsStep @@ -37,7 +37,10 @@ module StepsProcessor class Graph < Base # Builds a wizard graph, yielding the DSL so the caller can add nodes/edges. # - # @param wizard [Object] The wizard instance (for method predicates) + # @param wizard [Object] The wizard instance + # @param predicate_caller [Object] Object to call predicate methods on. + # Typically the state_store, so predicates like `:needs_permission?` are called + # on the state store directly, keeping predicate logic in the state store. # @yieldparam dsl [DSL] The graph DSL builder # @return [Graph] # @@ -45,15 +48,15 @@ class Graph < Base # @raise [ArgumentError] If root node not set # # @example - # Graph.draw(wizard) do |g| - # g.add_node :step1, Step1 - # g.root :step1 + # Graph.draw(wizard, predicate_caller: state_store) do |g| + # g.add_node :nationality, NationalityStep + # g.add_conditional_edge from: :nationality, when: :needs_visa?, then: :visa, else: :review # end - def self.draw(wizard) + def self.draw(wizard, predicate_caller:) raise ArgumentError, 'A block must be given to Graph.draw' unless block_given? - graph = new(wizard) - dsl = DSL.new(graph.registry, wizard) + graph = new(wizard, predicate_caller:) + dsl = DSL.new(graph.registry, wizard, predicate_caller:) yield(dsl) unless graph.root_step @@ -66,11 +69,13 @@ def self.draw(wizard) attr_reader :registry, :resolver # @param wizard [Object] The wizard instance + # @param predicate_caller [Object] Object to call predicate methods on # @api public - def initialize(wizard) + def initialize(wizard, predicate_caller:) @wizard = wizard + @predicate_caller = predicate_caller @registry = Registry.new - @resolver = NavigationResolver.new(registry: @registry, wizard:) + @resolver = NavigationResolver.new(registry: @registry, wizard:, predicate_caller:) end # Return the root (starting) step for this graph. @@ -83,14 +88,14 @@ def initialize(wizard) # @raise [ArgumentError] If no root defined # # @example Fixed root - # Graph.draw(wizard) do |g| + # Graph.draw(wizard, predicate_caller: state_store) do |g| # g.add_node :start, StartStep # g.root :start # Always :start # end # graph.root_step # => :start # # @example Dynamic root - # Graph.draw(wizard) do |g| + # Graph.draw(wizard, predicate_caller: state_store) do |g| # g.add_node :simple_path, SimpleStep # g.add_node :complex_path, ComplexStep # g.conditional_root { |state| state.is_complex? ? :complex_path : :simple_path } @@ -100,7 +105,7 @@ def root_step return @registry.root_node if @registry.root_node if @registry.conditional_root_block - @registry.conditional_root_block.call(@wizard.state_store) + @registry.conditional_root_block.call(@predicate_caller) elsif @registry.conditional_root_method @wizard.method(@registry.conditional_root_method).call end @@ -141,7 +146,7 @@ def next_step(step = nil) # graph.previous_step(:step1) # => nil (already at root) # # @example Prevent going back - # Graph.draw(wizard) do |g| + # Graph.draw(wizard, predicate_caller: state_store) do |g| # g.before_previous_step { return nil if payment_locked? } # end def previous_step(step = nil) diff --git a/lib/dfe/wizard/steps_processor/graph/dsl.rb b/lib/dfe/wizard/steps_processor/graph/dsl.rb index d1243e5..409855a 100644 --- a/lib/dfe/wizard/steps_processor/graph/dsl.rb +++ b/lib/dfe/wizard/steps_processor/graph/dsl.rb @@ -9,9 +9,10 @@ class Graph < Base # # @api private class DSL - def initialize(registry, wizard) + def initialize(registry, wizard, predicate_caller:) @registry = registry @wizard = wizard + @predicate_caller = predicate_caller end # Add a step node to the graph. @@ -202,29 +203,42 @@ def add_multiple_conditional_edges(from:, branches:, default: nil, label: nil) # Add custom branching edge for arbitrary logic. # # Use this for complex transitions that don't fit the other patterns. - # The conditional should return the target node ID directly. + # The conditional should return the target step ID directly (not true/false). + # + # When using a Symbol, the method is called on the predicate_caller + # (typically state_store), consistent with other edge types. # # @param from [Symbol] Source node - # @param conditional [Symbol, Proc] Logic returning target node ID + # @param conditional [Symbol, Proc] Logic returning target step ID # @param potential_transitions [Array] Documentation of possible paths # - # @example + # @example With a method on state_store + # # In state_store: + # def determine_payment_path + # case payment_status + # when 'approved' then :confirmation + # when 'pending' then :payment_pending + # else :retry_payment + # end + # end + # + # # In graph definition: # g.add_custom_branching_edge( # from: :payment, - # conditional: proc do |step| - # case step.payment_status - # when :approved then :confirmation - # when :pending then :payment_pending - # when :failed then :retry_payment - # else :error - # end - # end, + # conditional: :determine_payment_path, # potential_transitions: [ # { label: "Payment approved", nodes: [:confirmation] }, # { label: "Payment pending", nodes: [:payment_pending] }, # { label: "Payment failed", nodes: [:retry_payment] } # ] # ) + # + # @example With a proc + # g.add_custom_branching_edge( + # from: :payment, + # conditional: proc { |step| step.payment_status == 'approved' ? :confirmation : :retry }, + # potential_transitions: [...] + # ) def add_custom_branching_edge(from:, conditional:, potential_transitions:) predicate = build_predicate(conditional) @@ -285,19 +299,22 @@ def before_previous_step(method = nil, &block) private - def build_predicate(raw) + # Build a predicate callable. + # + # @param raw [Symbol, Proc] Method name or callable + # @param caller [Object] Object to call the method on (defaults to predicate_caller) + def build_predicate(raw, caller: @predicate_caller) if raw.is_a?(Symbol) - unless @wizard.respond_to?(raw, include_private: true) - raise ArgumentError, "Predicate method :#{raw} not found on #{@wizard.class.name}" + unless caller.respond_to?(raw, include_private: true) + raise ArgumentError, "Predicate method :#{raw} not found on #{caller.class.name}" end - method = @wizard.method(raw) - arity = method.arity + bound_method = caller.method(raw) - if arity.zero? || arity.negative? - proc { method.call } + if bound_method.arity.zero? || bound_method.arity.negative? + proc { bound_method.call } else - proc { |step| method.call(step) } + proc { |step| bound_method.call(step) } end elsif raw.respond_to?(:call) diff --git a/lib/dfe/wizard/steps_processor/graph/navigation_resolver.rb b/lib/dfe/wizard/steps_processor/graph/navigation_resolver.rb index b9de21e..1864724 100644 --- a/lib/dfe/wizard/steps_processor/graph/navigation_resolver.rb +++ b/lib/dfe/wizard/steps_processor/graph/navigation_resolver.rb @@ -12,10 +12,10 @@ class Graph < Base # # @api private class NavigationResolver - def initialize(registry:, wizard:) + def initialize(registry:, wizard:, predicate_caller:) @registry = registry @wizard = wizard - @predicate_caller = @wizard + @predicate_caller = predicate_caller end # Navigate to next step. @@ -60,7 +60,7 @@ def previous_step(target_step = nil) # # @param target_step [Symbol, nil] # @return [Array] - def path_traversal(target_step = @wizard.current_step_name, max_depth: 20) + def path_traversal(target_step = @wizard.current_step_name, max_depth: @registry.nodes.size) root = compute_root_node path = dfs_path(root, target_step, Set.new, max_depth:) @@ -85,9 +85,9 @@ def compute_root_node return @registry.root_node if @registry.root_node if @registry.conditional_root_block - @registry.conditional_root_block.call(@wizard.state_store) + @registry.conditional_root_block.call(@predicate_caller) elsif @registry.conditional_root_method - @predicate_caller.method(@registry.conditional_root_method).call + @wizard.method(@registry.conditional_root_method).call end end diff --git a/spec/dfe/wizard/behaviours/state_management_spec.rb b/spec/dfe/wizard/behaviours/state_management_spec.rb index 6d152a3..9d4f724 100644 --- a/spec/dfe/wizard/behaviours/state_management_spec.rb +++ b/spec/dfe/wizard/behaviours/state_management_spec.rb @@ -31,7 +31,7 @@ include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :personal_details, Steps::PersonalDetails graph.add_node :account_type, Steps::AccountType graph.add_node :company_details, Steps::CompanyDetails @@ -85,10 +85,6 @@ def route_strategy def logger DfE::Wizard::Logger.new(nil) end - - delegate :business?, :individual?, - :verification_email?, :verification_phone?, :verification_id?, - to: :state_store end end @@ -794,7 +790,7 @@ class Review include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :step_one, Steps::PersonalDetails graph.add_node :step_two, Steps::AccountType graph.root :step_one @@ -852,7 +848,7 @@ def logger include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :validate_step, Steps::PersonalDetails graph.add_node :payment_step, Steps::AccountType graph.add_node :review_step, Steps::CompanyDetails @@ -924,7 +920,7 @@ def rollback; end include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :configured_step, Steps::PersonalDetails graph.add_node :unconfigured_step, Steps::AccountType graph.root :configured_step @@ -973,7 +969,7 @@ def logger include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :step_one, Steps::PersonalDetails graph.root :step_one end @@ -1016,7 +1012,7 @@ def logger include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :step_one, Steps::PersonalDetails graph.root :step_one end diff --git a/spec/dfe/wizard/behaviours/step_management_spec.rb b/spec/dfe/wizard/behaviours/step_management_spec.rb index ef9f3ed..2410c92 100644 --- a/spec/dfe/wizard/behaviours/step_management_spec.rb +++ b/spec/dfe/wizard/behaviours/step_management_spec.rb @@ -24,7 +24,7 @@ include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :start, Steps::EflStart graph.add_node :qualification_type, Steps::QualificationType graph.add_node :ielts_details, Steps::IeltsDetails @@ -68,9 +68,6 @@ def route_strategy def logger DfE::Wizard::Logger.new(Rails.logger) end - - delegate :has_qualification?, :is_ielts?, :is_toefl?, :is_other?, - to: :state_store end end diff --git a/spec/dfe/wizard/check_answers_presenter_spec.rb b/spec/dfe/wizard/check_answers_presenter_spec.rb new file mode 100644 index 0000000..5e30c04 --- /dev/null +++ b/spec/dfe/wizard/check_answers_presenter_spec.rb @@ -0,0 +1,315 @@ +RSpec.describe DfE::Wizard::CheckAnswersPresenter do + let(:repository) { DfE::Wizard::Repository::InMemory.new } + let(:state_store) { StateStores::RegisterECTStore.new(repository:) } + let(:wizard) do + RegisterECTWizard.new( + current_step: :check_answers, + state_store:, + current_step_params: {}, + ) + end + + let(:presenter_class) do + Class.new do + include DfE::Wizard::CheckAnswersPresenter + + def teacher_details + [ + row_for(:review_ect_details, :correct_full_name), + row_for(:email_address, :email), + row_for(:start_date, :start_date), + ] + end + + def programme_details + [ + row_for(:programme_type, :training_programme), + row_for(:lead_provider, :lead_provider_id, label: 'Lead provider'), + ] + end + end + end + + subject(:presenter) { presenter_class.new(wizard) } + + before do + state_store.write( + trn: '9999999', + date_of_birth: Date.new(2000, 1, 1), + correct_full_name: 'John Smith', + email: 'john@example.com', + start_date: Date.new(2024, 9, 1), + working_pattern: 'full_time', + school_type: 'state', + training_programme: 'provider_led', + appropriate_body_name: 'Some AB', + lead_provider_id: 'teach_first', + details_correct: 'no', + ) + end + + describe '#initialize' do + it 'stores the wizard' do + expect(presenter.wizard).to eq(wizard) + end + end + + describe '#reviewable_steps' do + it 'defaults to flow_steps' do + expect(presenter.reviewable_steps).to eq(wizard.flow_steps) + end + end + + describe '#find_reviewable_step' do + it 'returns the step when found' do + step = presenter.find_reviewable_step(:email_address) + expect(step).to be_a(Steps::RegisterECT::EmailAddressStep) + end + + it 'returns nil when step not in reviewable steps' do + state_store.write(training_programme: 'school_led') + expect(presenter.find_reviewable_step(:lead_provider)).to be_nil + end + end + + describe '#row_for' do + let(:row) { presenter.row_for(:email_address, :email) } + + it 'returns a Row object' do + expect(row).to be_a(DfE::Wizard::CheckAnswersPresenter::Row) + end + + it 'sets the step_id' do + expect(row.step_id).to eq(:email_address) + end + + it 'sets the attribute' do + expect(row.attribute).to eq(:email) + end + + it 'provides the step instance' do + expect(row.step).to be_a(Steps::RegisterECT::EmailAddressStep) + end + + it 'provides the raw value' do + expect(row.value).to eq('john@example.com') + end + + it 'provides the formatted value' do + expect(row.formatted_value).to eq('john@example.com') + end + + it 'generates change_path with return_to_review' do + expect(row.change_path).to include('return_to_review=email_address') + end + + context 'with custom label' do + let(:row) { presenter.row_for(:email_address, :email, label: 'Teacher email') } + + it 'uses the custom label' do + expect(row.label).to eq('Teacher email') + end + end + + context 'with change_step override' do + let(:row) { presenter.row_for(:email_address, :email, change_step: :start_date) } + + it 'generates change_path for the overridden step' do + expect(row.change_path).to include('return_to_review=start_date') + end + end + + context 'when step is not in reviewable_steps' do + it 'returns nil' do + state_store.write(training_programme: 'school_led') + expect(presenter.row_for(:lead_provider, :lead_provider_id)).to be_nil + end + end + + context 'when attribute does not exist on step' do + it 'returns nil' do + expect(presenter.row_for(:email_address, :nonexistent_attribute)).to be_nil + end + end + end + + describe '#format_value' do + it 'returns the value unchanged by default' do + expect(presenter.format_value(:email, 'john@example.com')).to eq('john@example.com') + end + + context 'when overridden in presenter' do + let(:presenter_class) do + Class.new do + include DfE::Wizard::CheckAnswersPresenter + + def format_value(attribute, value) + case attribute + when :start_date then "Formatted: #{value}" + else value + end + end + end + end + + it 'uses the custom formatting' do + row = presenter.row_for(:start_date, :start_date) + expect(row.formatted_value).to eq("Formatted: #{Date.new(2024, 9, 1)}") + end + + it 'passes through unmatched attributes' do + row = presenter.row_for(:email_address, :email) + expect(row.formatted_value).to eq('john@example.com') + end + end + end + + describe '#rows_for' do + it 'returns multiple rows for a step' do + rows = presenter.rows_for(:review_ect_details, %i[correct_full_name details_correct]) + expect(rows.length).to eq(2) + expect(rows.map(&:attribute)).to eq(%i[correct_full_name details_correct]) + end + + it 'accepts hash options for attributes' do + rows = presenter.rows_for(:review_ect_details, [ + :correct_full_name, + { attribute: :details_correct, label: 'Are details correct?' }, + ]) + + expect(rows[0].label).to eq('Name') # From I18n + expect(rows[1].label).to eq('Are details correct?') + end + + context 'when step is not in reviewable_steps' do + it 'returns empty array' do + state_store.write(training_programme: 'school_led') + expect(presenter.rows_for(:lead_provider, [:lead_provider_id])).to eq([]) + end + end + end + + describe '#change_path_for' do + it 'generates path with return_to_review parameter' do + path = presenter.change_path_for(:email_address) + expect(path).to include('return_to_review=email_address') + end + end + + describe '#completed_steps' do + it 'delegates to wizard.valid_steps' do + expect(presenter.completed_steps).to eq(wizard.valid_steps) + end + end + + describe '#flow_steps' do + it 'delegates to wizard.flow_steps' do + expect(presenter.flow_steps).to eq(wizard.flow_steps) + end + end + + describe '#state_store' do + it 'delegates to wizard.state_store' do + expect(presenter.state_store).to eq(wizard.state_store) + end + end + + describe DfE::Wizard::CheckAnswersPresenter::Row do + let(:step) { wizard.step(:email_address) } + let(:row) do + described_class.new( + step_id: :email_address, + attribute: :email, + step: step, + change_path: '/email_address?return_to_review=email_address', + formatted_value: 'john@example.com', + ) + end + + describe '#value' do + it 'returns the attribute value from the step' do + expect(row.value).to eq('john@example.com') + end + end + + describe '#label' do + it 'uses human_attribute_name from step class' do + expect(row.label).to eq('Email address') # From I18n + end + + context 'with custom_label' do + let(:row) do + described_class.new( + step_id: :email_address, + attribute: :email, + step: step, + change_path: '/email_address', + formatted_value: 'john@example.com', + custom_label: 'Teacher email', + ) + end + + it 'returns the custom label' do + expect(row.label).to eq('Teacher email') + end + end + + context 'without step' do + let(:row) do + described_class.new( + step_id: :email_address, + attribute: :some_attr, + step: nil, + change_path: '/email_address', + formatted_value: nil, + ) + end + + it 'humanizes the attribute name' do + expect(row.label).to eq('Some attr') + end + end + end + + describe '#value?' do + it 'returns true when value is present' do + expect(row.value?).to be true + end + + context 'when value is blank' do + before { state_store.write(email: '') } + + it 'returns false' do + expect(row.value?).to be false + end + end + end + + describe '#to_h' do + it 'returns a hash representation' do + result = row.to_h + + expect(result).to eq( + step_id: :email_address, + attribute: :email, + label: 'Email address', # From I18n + value: 'john@example.com', + formatted_value: 'john@example.com', + change_path: '/email_address?return_to_review=email_address', + ) + end + end + end + + describe 'grouping rows in presenter methods' do + it 'allows grouping rows by section' do + expect(presenter.teacher_details.map(&:attribute)).to eq(%i[correct_full_name email start_date]) + expect(presenter.programme_details.map(&:attribute)).to eq(%i[training_programme lead_provider_id]) + end + + it 'uses custom labels in grouped rows' do + labels = presenter.programme_details.map(&:label) + expect(labels).to include('Lead provider') + end + end +end diff --git a/spec/dfe/wizard/core/state_store_spec.rb b/spec/dfe/wizard/core/state_store_spec.rb index 946ba10..a6f558b 100644 --- a/spec/dfe/wizard/core/state_store_spec.rb +++ b/spec/dfe/wizard/core/state_store_spec.rb @@ -8,7 +8,7 @@ include DfE::Wizard def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :start, DBSSteps::Start graph.add_node :what_service, DBSSteps::WhatService graph.add_node :personal_details, DBSSteps::PersonalDetails @@ -46,8 +46,6 @@ def route_strategy def logger @logger ||= DfE::Wizard::Logger.new(Rails.logger) end - - delegate :has_previous_names?, to: :state_store end end @@ -334,7 +332,7 @@ def step_attributes_methods? end end - describe '#method_missing and #respond_to_missing?' do + describe 'dynamic attribute accessors' do context 'with complete DBS check application' do before do repository.write({ @@ -372,7 +370,7 @@ def step_attributes_methods? state_store.attribute_names = wizard.attribute_names end - it 'responds to all step attributes via method_missing' do + it 'responds to all step attributes' do expect(state_store).to respond_to(:service_type) expect(state_store).to respond_to(:first_name) expect(state_store).to respond_to(:last_name) @@ -381,27 +379,27 @@ def step_attributes_methods? expect(state_store).to respond_to(:passport_number) end - it 'returns correct values via method_missing' do + it 'returns correct values for attributes' do expect(state_store.service_type).to eq('basic') expect(state_store.first_name).to eq('Sarah') expect(state_store.last_name).to eq('Johnson') expect(state_store.email).to eq('sarah.johnson@example.com') end - it 'method_missing reads from current repository data' do + it 'reads from current repository data' do repository.write(repository.read.merge(first_name: 'Emma')) expect(state_store.first_name).to eq('Emma') end - it 'custom predicate methods work with method_missing attributes' do + it 'custom predicate methods work with generated attributes' do expect(state_store.has_previous_names?).to be true expect(state_store.full_name).to eq('Sarah Elizabeth Johnson') expect(state_store.age).to be_between(38, 40) expect(state_store.uk_resident?).to be true end - it 'does not override custom methods with method_missing' do + it 'does not override custom methods with generated accessors' do expect(state_store).to respond_to(:full_name) expect(state_store).to respond_to(:age) expect(state_store.full_name).to include('Sarah') @@ -433,7 +431,7 @@ def step_attributes_methods? expect(state_store.has_previous_names?).to be false end - it 'previous_names_details attributes return nil via method_missing' do + it 'previous_names_details attributes return nil' do expect(state_store.previous_first_name).to be_nil expect(state_store.previous_last_name).to be_nil end @@ -511,7 +509,7 @@ def step_attributes_methods? state_store.attribute_names = wizard.attribute_names end - it 'provides natural attribute access via method_missing' do + it 'provides natural attribute access' do expect(state_store.first_name).to eq('Emma') expect(state_store.email).to eq('emma.williams@example.com') expect(state_store.full_name).to eq('Emma Grace Williams') @@ -532,7 +530,7 @@ def step_attributes_methods? let(:disabled_store) { disabled_store_class.new(repository: repository) } - it 'still responds to method_missing for attributes' do + it 'does not generate attribute accessors' do repository.write({ first_name: 'Test' }) disabled_store.step_definitions = wizard.step_definitions disabled_store.attribute_names = wizard.attribute_names @@ -564,7 +562,7 @@ def first_name let(:collision_store) { collision_store_class.new(repository: repository) } - it 'preserves existing custom method (method_missing not called)' do + it 'preserves existing custom method' do repository.write({ first_name: 'Sarah' }) collision_store.step_definitions = wizard.step_definitions collision_store.attribute_names = wizard.attribute_names diff --git a/spec/dfe/wizard/step_processors/graph_spec.rb b/spec/dfe/wizard/step_processors/graph_spec.rb index 0394aaf..7043ec5 100644 --- a/spec/dfe/wizard/step_processors/graph_spec.rb +++ b/spec/dfe/wizard/step_processors/graph_spec.rb @@ -10,20 +10,13 @@ class WasteExemptionWizard include DfE::Wizard - delegate :is_upper_tier_waste?, - :is_listed_activity?, - :is_non_listed_activity?, - :is_exempt_activity?, - :account_feature_flag_enabled?, - to: :state_store - def initialize @state_store = WasteStateStore.new @current_step_name = :organization_type end def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |g| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |g| # Nodes g.add_node :organization_type, Steps::OrganizationType, label: 'Organization Type' g.add_node :account_login, Steps::AccountLogin, label: 'Account Login' @@ -88,19 +81,6 @@ def logger DfE::Wizard::Logger.new(Rails.logger) end - def determine_status_path(_step_obj) - case state_store.application_status - when :submitted - :review - when :approved - :issue_certificate - when :rejected - :rejection_notice - else - :pending_info - end - end - def conditional_entry_point state_store.is_returning_user? ? :account_login : :organization_type end @@ -147,6 +127,20 @@ def is_exempt_activity? def account_feature_flag_enabled? @account_feature_flag_enabled.present? end + + # Custom branching: determines next step based on application status + def determine_status_path + case application_status + when :submitted + :review + when :approved + :issue_certificate + when :rejected + :rejection_notice + else + :pending_info + end + end end end @@ -187,8 +181,14 @@ class PendingInfo; end context 'when not passing potential root' do before do class GraphWithoutPotentialRoot + include DfE::Wizard + + def initialize + @state_store = Object.new + end + def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :first_page, Object graph.add_node :second_page, Object @@ -271,10 +271,8 @@ def steps_processor class GraphWithSkippedSteps include DfE::Wizard - delegate :user_not_permitted?, :single_provider?, to: :state_store - def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :first_page, Object graph.add_node :second_page, Object, skip_when: :user_not_permitted? graph.add_node :third_page, Object, skip_when: :single_provider? @@ -408,6 +406,14 @@ def single_provider? expect(graph.path_traversal(:nonexistent)).to eq([]) end end + + context 'unreachable due to cycle' do + it 'returns empty array when target is blocked by a cycle' do + # issue_certificate is only reachable if review transitions to it + # but in this test setup, review doesn't transition there + expect(graph.path_traversal(:issue_certificate)).to eq([]) + end + end end describe '#find_step' do @@ -632,3 +638,127 @@ def single_provider? end end end + +RSpec.describe DfE::Wizard::StepsProcessor::Graph, 'Circular Graph Detection' do + # This wizard intentionally has circular edges to test infinite loop prevention + module CircularSteps + class StepA + include DfE::Wizard::Step + attribute :go_to_c, :boolean, default: false + end + + class StepB + include DfE::Wizard::Step + end + + class StepC + include DfE::Wizard::Step + end + end + + class CircularStateStore + include DfE::Wizard::StateStore + + def should_go_to_c? + read[:go_to_c] == true + end + end + + class CircularWizard + include DfE::Wizard + + def initialize + @state_store = CircularStateStore.new + @current_step_name = :step_a + end + + def steps_processor + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |g| + g.add_node :step_a, CircularSteps::StepA + g.add_node :step_b, CircularSteps::StepB + g.add_node :step_c, CircularSteps::StepC + + g.root :step_a + + # Circular: A -> B -> C -> A (when condition is met) + g.add_edge from: :step_a, to: :step_b + g.add_conditional_edge( + from: :step_b, + when: :should_go_to_c?, + then: :step_c, + else: :step_a, # This creates a cycle back to A + ) + g.add_edge from: :step_c, to: :step_a # This also creates a cycle + end + end + + def logger + nil + end + + def route_strategy + nil + end + end + + let(:wizard) { CircularWizard.new } + let(:graph) { wizard.steps_processor } + + describe '#path_traversal with circular edges' do + context 'when graph has cycles' do + before do + wizard.state_store.write(go_to_c: false) + end + + it 'returns empty array when target is unreachable due to cycle' do + # step_c is unreachable when go_to_c is false (B goes back to A) + # The visited set prevents infinite loops + expect(graph.path_traversal(:step_c)).to eq([]) + end + + it 'finds path when target is reachable before cycle' do + path = graph.path_traversal(:step_b) + expect(path).to eq(%i[step_a step_b]) + end + end + + context 'when following the cycle path' do + before do + wizard.state_store.write(go_to_c: true) + end + + it 'returns empty array for nonexistent step even with cycles' do + # A -> B -> C -> A (cycle) + # visited set stops traversal when it revisits A + expect(graph.path_traversal(:nonexistent)).to eq([]) + end + + it 'finds path to step_c through the conditional' do + path = graph.path_traversal(:step_c) + expect(path).to eq(%i[step_a step_b step_c]) + end + end + end + + describe '#next_step with circular edges' do + it 'returns the next step even in circular graph' do + expect(graph.next_step(:step_a)).to eq(:step_b) + end + + it 'follows conditional back to start without issues' do + wizard.state_store.write(go_to_c: false) + expect(graph.next_step(:step_b)).to eq(:step_a) + end + end + + describe 'max_depth uses step count' do + it 'limits traversal to number of steps in graph' do + # The circular wizard has 3 steps, so max_depth should be 3 + expect(graph.step_definitions.size).to eq(3) + + # Even with cycles, traversal is bounded by step count + path = graph.path_traversal(:step_b) + expect(path.length).to be <= 3 + end + end +end diff --git a/spec/dfe/wizard/test/r_spec_matchers_spec.rb b/spec/dfe/wizard/test/r_spec_matchers_spec.rb index 54a60b5..bf9dda4 100644 --- a/spec/dfe/wizard/test/r_spec_matchers_spec.rb +++ b/spec/dfe/wizard/test/r_spec_matchers_spec.rb @@ -356,7 +356,7 @@ def step(step_name) end def steps_processor - @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self) do |g| + @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |g| g.add_node :step_a, Class.new g.add_node :step_b, Class.new g.root :step_a @@ -432,7 +432,7 @@ def step(step_name) end def steps_processor - @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self) do |g| + @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |g| g.add_node :step_a, Class.new g.add_node :step_b, Class.new g.conditional_root(potential_root: [:step_b]) { :step_b } @@ -471,7 +471,7 @@ def step(step_name) end def steps_processor - @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self) do |g| + @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |g| g.add_node :entry, Class.new g.add_node :form, Class.new g.add_node :review, Class.new diff --git a/spec/dummy-app-wizard-specs/register_ect_wizard_spec.rb b/spec/dummy-app-wizard-specs/register_ect_wizard_spec.rb index a8f31f6..d4bbf41 100644 --- a/spec/dummy-app-wizard-specs/register_ect_wizard_spec.rb +++ b/spec/dummy-app-wizard-specs/register_ect_wizard_spec.rb @@ -144,7 +144,7 @@ end it 'follows normal flow when CYA is unreachable' do - allow(wizard).to receive_messages(in_trs?: true, prohibited_from_teaching?: true) + allow(state_store).to receive_messages(in_trs?: true, prohibited_from_teaching?: true) expect(wizard).to have_next_step(:cannot_register_ect) end @@ -246,7 +246,7 @@ let(:current_step) { :cannot_register_ect } before do - allow(wizard).to receive_messages(in_trs?: true, prohibited_from_teaching?: true) + allow(state_store).to receive_messages(in_trs?: true, prohibited_from_teaching?: true) end it 'returns empty path when target is blocked' do @@ -329,7 +329,7 @@ let(:current_step) { :cannot_register_ect } before do - allow(wizard).to receive_messages(in_trs?: true, prohibited_from_teaching?: true) + allow(state_store).to receive_messages(in_trs?: true, prohibited_from_teaching?: true) end it { expect(wizard).to have_previous_step(:find_ect) } @@ -340,8 +340,8 @@ before do state_store.write(trn: '1111111') - allow(wizard).to receive(:find_ect_transitions).and_return(:national_insurance_number) - allow(wizard).to receive(:national_insurance_number_transitions).and_return(:not_found) + allow(state_store).to receive(:find_ect_transitions).and_return(:national_insurance_number) + allow(state_store).to receive(:national_insurance_number_transitions).and_return(:not_found) end it { expect(wizard).to have_previous_step(:national_insurance_number) } diff --git a/spec/features/register_ect_wizard_spec.rb b/spec/features/register_ect_wizard_spec.rb index 5eedb91..f03f835 100644 --- a/spec/features/register_ect_wizard_spec.rb +++ b/spec/features/register_ect_wizard_spec.rb @@ -12,7 +12,7 @@ 'Working pattern' => 'Full time', 'Appropriate body' => 'Example Appropriate Body', 'Training programme' => 'Provider led', - 'Lead provider' => 'Teach first', + 'Lead provider' => 'Teach First', ) and_i_confirm_details diff --git a/spec/rails-dummy/app/controllers/register_ect/check_answers_controller.rb b/spec/rails-dummy/app/controllers/register_ect/check_answers_controller.rb index 5fab314..78fa043 100644 --- a/spec/rails-dummy/app/controllers/register_ect/check_answers_controller.rb +++ b/spec/rails-dummy/app/controllers/register_ect/check_answers_controller.rb @@ -1,4 +1,7 @@ module RegisterECT class CheckAnswersController < WizardController + def new + @review = RegisterECTReview.new(@wizard) + end end end diff --git a/spec/rails-dummy/app/presenters/register_ect_review.rb b/spec/rails-dummy/app/presenters/register_ect_review.rb new file mode 100644 index 0000000..f243869 --- /dev/null +++ b/spec/rails-dummy/app/presenters/register_ect_review.rb @@ -0,0 +1,58 @@ +class RegisterECTReview + include DfE::Wizard::CheckAnswersPresenter + + def teacher_details + [ + row_for(:review_ect_details, :correct_full_name), + row_for(:find_ect, :trn), + row_for(:email_address, :email), + row_for(:start_date, :start_date), + row_for(:working_pattern, :working_pattern), + ] + end + + def programme_details + [ + appropriate_body_row, + row_for(:programme_type, :training_programme), + row_for(:lead_provider, :lead_provider_id), + ].compact + end + + def format_value(attribute, value) + case attribute + when :start_date + value&.to_fs(:govuk_date) + when :working_pattern, :training_programme + value&.humanize + when :lead_provider_id + lead_provider_name(value) + else + value + end + end + + private + + def appropriate_body_row + step_id = state_store.school_independent? ? :independent_school_appropriate_body : :state_school_appropriate_body + + Row.new( + step_id: step_id, + attribute: :appropriate_body, + step: nil, + change_path: change_path_for(step_id), + formatted_value: state_store.appropriate_body_text, + custom_label: 'Appropriate body', + ) + end + + def lead_provider_name(provider_id) + providers = { + 'teach_first' => 'Teach First', + 'ambition' => 'Ambition Institute', + 'edt' => 'Education Development Trust', + } + providers[provider_id] || provider_id&.humanize + end +end diff --git a/spec/rails-dummy/app/views/register_ect/check_answers/new.html.erb b/spec/rails-dummy/app/views/register_ect/check_answers/new.html.erb index 43baf8b..7e3d246 100644 --- a/spec/rails-dummy/app/views/register_ect/check_answers/new.html.erb +++ b/spec/rails-dummy/app/views/register_ect/check_answers/new.html.erb @@ -1,4 +1,4 @@ -<%= govuk_back_link href: @wizard.previous_step_path || register_ect_find_ect_path %> +<%= govuk_back_link href: @wizard.previous_step_path(fallback: register_ect_find_ect_path) %>
@@ -6,62 +6,34 @@

Teacher details

- <% teacher_details_reviewable_attributes = %w[correct_full_name trn email start_date working_pattern] %> - <%= govuk_summary_list do |summary_list| %> - <% @wizard.flow_steps.each do |step| %> - <% step.attributes.each do |attribute_name, attribute_value| %> - <% next unless attribute_name.in?(teacher_details_reviewable_attributes) %> - <%= summary_list.with_row do |row| %> - <%= row.with_key { step.class.human_attribute_name(attribute_name) } %> - - <% if attribute_name == "working_pattern" %> - <%= row.with_value { attribute_value.humanize } %> - <% else %> - <%= row.with_value { attribute_value.is_a?(Date) ? attribute_value.to_fs(:govuk_date) : attribute_value.to_s } %> - <% end %> - <%= row.with_action( - text: "Change", - href: @wizard.resolve_step_path(step.step_id, return_to_review: step.step_id), - visually_hidden_text: attribute_name - ) %> - <% end %> + <% @review.teacher_details.each do |row| %> + <%= summary_list.with_row do |r| %> + <%= r.with_key { row.label } %> + <%= r.with_value { row.formatted_value } %> + <%= r.with_action( + text: "Change", + href: row.change_path, + visually_hidden_text: row.label.downcase + ) %> <% end %> <% end %> <% end %>

Programme details

- <% programme_details_reviewable_attributes = %w[lead_provider_id training_programme] %> - <%= govuk_summary_list do |summary_list| %> - <% @wizard.flow_steps.each do |step| %> - <% step.attributes.each do |attribute_name, attribute_value| %> - <% next unless attribute_name.in?(programme_details_reviewable_attributes) %> - <%= summary_list.with_row do |row| %> - <%= row.with_key { step.class.human_attribute_name(attribute_name) } %> - <%= row.with_value { attribute_value.humanize } %> - <%= row.with_action( - text: "Change", - href: @wizard.resolve_step_path(step.step_id, return_to_review: step.step_id), - visually_hidden_text: attribute_name - ) %> - <% end %> + <% @review.programme_details.each do |row| %> + <%= summary_list.with_row do |r| %> + <%= r.with_key { row.label } %> + <%= r.with_value { row.formatted_value } %> + <%= r.with_action( + text: "Change", + href: row.change_path, + visually_hidden_text: row.label.downcase + ) %> <% end %> <% end %> - - <%= summary_list.with_row do |row| %> - <%= row.with_key { "Appropriate body" } %> - <%= row.with_value { @wizard.state_store.appropriate_body_text } %> - <%= row.with_action( - text: "Change", - href: @wizard.resolve_step_path( - @wizard.state_store.school_independent? ? :independent_school_appropriate_body : :state_school_appropriate_body, - return_to_review: @wizard.state_store.school_independent? ? :independent_school_appropriate_body : :state_school_appropriate_body - ), - visually_hidden_text: "appropriate body" - ) %> - <% end %> <% end %> <%= form_with(model: @wizard.current_step, diff --git a/spec/rails-dummy/app/wizards/a_levels_requirements_wizard.rb b/spec/rails-dummy/app/wizards/a_levels_requirements_wizard.rb index 233033f..853f65e 100644 --- a/spec/rails-dummy/app/wizards/a_levels_requirements_wizard.rb +++ b/spec/rails-dummy/app/wizards/a_levels_requirements_wizard.rb @@ -3,13 +3,8 @@ class ALevelsRequirementsWizard attr_reader :course - delegate :any_a_levels?, - :add_another_a_level?, - :has_remaining_a_levels?, - to: :state_store - def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :what_a_level_is_required, Steps::WhatALevelIsRequired graph.add_node :add_a_level_to_a_list, Steps::AddALevelToAList graph.add_node :remove_a_level_subject_confirmation, Steps::RemoveALevelSubjectConfirmation diff --git a/spec/rails-dummy/app/wizards/add_course_wizard.rb b/spec/rails-dummy/app/wizards/add_course_wizard.rb index 44e82ca..d32fc31 100644 --- a/spec/rails-dummy/app/wizards/add_course_wizard.rb +++ b/spec/rails-dummy/app/wizards/add_course_wizard.rb @@ -1,22 +1,8 @@ class AddCourseWizard include DfE::Wizard - delegate :further_education?, - :applications_open_feature_flag_inactive?, - :design_technology?, - :modern_languages?, - :physics?, - :teacher_degree_apprenticeship?, - :single_accredited_provider_or_self_accredited?, - :fee_based?, - :can_sponsor_student_visa?, - :can_sponsor_skilled_worker_visa?, - :no_visa_sponsorship?, - :visa_deadline_required?, - to: :state_store - def steps_processor - @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node(:level, Steps::Courses::Level) graph.add_node(:subjects, Steps::Courses::Subjects) diff --git a/spec/rails-dummy/app/wizards/assign_mentor_wizard.rb b/spec/rails-dummy/app/wizards/assign_mentor_wizard.rb index 66027fd..a151d23 100644 --- a/spec/rails-dummy/app/wizards/assign_mentor_wizard.rb +++ b/spec/rails-dummy/app/wizards/assign_mentor_wizard.rb @@ -1,10 +1,8 @@ class AssignMentorWizard include DfE::Wizard - delegate :lead_provider_will_not_provide?, to: :state_store - def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :who_will_be_the_mentor, Steps::WhoWillBeTheMentor graph.add_node :can_receive_mentor_training, Steps::CanReceiveMentorTraining graph.add_node :which_lead_provider, Steps::WhichLeadProvider diff --git a/spec/rails-dummy/app/wizards/personal_information_wizard.rb b/spec/rails-dummy/app/wizards/personal_information_wizard.rb index 54010e6..fed67bf 100644 --- a/spec/rails-dummy/app/wizards/personal_information_wizard.rb +++ b/spec/rails-dummy/app/wizards/personal_information_wizard.rb @@ -1,12 +1,8 @@ class PersonalInformationWizard include DfE::Wizard - delegate :needs_permission_to_work_or_study?, - :right_to_work_or_study?, - to: :state_store - def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :name_and_date_of_birth, Steps::NameAndDateOfBirth graph.add_node :nationality, Steps::Nationality graph.add_node :right_to_work_or_study, Steps::RightToWorkOrStudy diff --git a/spec/rails-dummy/app/wizards/register_ect_wizard.rb b/spec/rails-dummy/app/wizards/register_ect_wizard.rb index 5a2c0f7..29845c5 100644 --- a/spec/rails-dummy/app/wizards/register_ect_wizard.rb +++ b/spec/rails-dummy/app/wizards/register_ect_wizard.rb @@ -1,20 +1,8 @@ class RegisterECTWizard include DfE::Wizard - delegate :in_trs?, - :matches_trs_dob?, - :active_at_school?, - :induction_exempt?, - :induction_failed?, - :prohibited_from_teaching?, - :induction_completed?, - :cant_use_email?, - :school_independent?, - :provider_led?, - to: :state_store - def steps_processor - DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :cannot_register_ect, Steps::RegisterECT::CannotRegisterECTStep graph.add_node :cant_use_email, Steps::RegisterECT::CantUseEmailStep graph.add_node :check_answers, Steps::RegisterECT::CheckAnswersStep @@ -121,24 +109,4 @@ def previous_step_override :check_answers if current_step_name == target && valid_path_to?(:check_answers) end - - def find_ect_transitions - return :trn_not_found unless in_trs? - return :national_insurance_number unless matches_trs_dob? - return :already_active_at_school if active_at_school? - return :induction_completed if induction_completed? - return :induction_exempt if induction_exempt? - return :induction_failed if induction_failed? - return :cannot_register_ect if prohibited_from_teaching? - - :review_ect_details - end - - def national_insurance_number_transitions - return :not_found unless in_trs? - return :induction_completed if induction_completed? - return :induction_exempt if induction_exempt? - - :review_ect_details - end end diff --git a/spec/rails-dummy/app/wizards/state_stores/register_ect_store.rb b/spec/rails-dummy/app/wizards/state_stores/register_ect_store.rb index 1ba85ac..2667f2c 100644 --- a/spec/rails-dummy/app/wizards/state_stores/register_ect_store.rb +++ b/spec/rails-dummy/app/wizards/state_stores/register_ect_store.rb @@ -57,6 +57,28 @@ def appropriate_body_text appropriate_body_name.presence || 'Not provided' end + # Custom branching: determines next step after find_ect + def find_ect_transitions + return :trn_not_found unless in_trs? + return :national_insurance_number unless matches_trs_dob? + return :already_active_at_school if active_at_school? + return :induction_completed if induction_completed? + return :induction_exempt if induction_exempt? + return :induction_failed if induction_failed? + return :cannot_register_ect if prohibited_from_teaching? + + :review_ect_details + end + + # Custom branching: determines next step after national_insurance_number + def national_insurance_number_transitions + return :not_found unless in_trs? + return :induction_completed if induction_completed? + return :induction_exempt if induction_exempt? + + :review_ect_details + end + private def trn diff --git a/spec/support/work_experience_wizard.rb b/spec/support/work_experience_wizard.rb index 5244202..d5ede64 100644 --- a/spec/support/work_experience_wizard.rb +++ b/spec/support/work_experience_wizard.rb @@ -98,7 +98,7 @@ class WorkExperienceWizard include DfE::Wizard def steps_processor - @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self) do |graph| + @steps_processor ||= DfE::Wizard::StepsProcessor::Graph.draw(self, predicate_caller: state_store) do |graph| graph.add_node :work_title, Steps::WorkTitle graph.add_node :employer, Steps::Employer graph.add_node :start_date, Steps::StartDate