Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions app/models/strata/flows/question_page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,33 @@ def completed?(record)
record.valid?(@name.to_sym)
end

def started?(record)
@fields.any? do |field|
field_names = field.is_a?(Hash) ? field.keys : [ field ]
field_names.any? do |name|
attribute_name = started_field_name(record, name)
value = record.public_send(attribute_name) if attribute_name
Comment thread
MichaelCrawfordNava marked this conversation as resolved.
# An explicit boolean false is a real user-supplied answer (e.g. a "No"
# on a yes/no question), but `false.present?` is falsy in Rails.
value == false || value.present?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value == false doesn't distinguish a user-supplied false from an ActiveRecord column default of false — same shape of bug this PR is fixing, one indirection away.

spec/dummy/db/schema.rb:59 already declares sample_application_forms.reviewed with default: false. ActiveRecord applies column defaults to in-memory records, so SampleApplicationForm.new.reviewed == false. No current flow lists :reviewed against SampleApplicationForm, so this is latent today — but the moment a consumer adds question_page :reviewed (or any boolean-default field) to a flow, the containing task will render "Continue" before the user has done anything.

Consider checking record.attribute_changed?(name) / record.changed_attributes instead, or only applying the false-is-meaningful rule to attributes whose declared default is nil. A spec asserting started? is false on a fresh AR record with attribute :x, :boolean, default: false would lock the behavior in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I not familiar with attribute_changed but it looks like it's more for in-memory tracking https://api.rubyonrails.org/classes/ActiveModel/Dirty.html

From one of the examples:

person.save
person.changed?      # => false
person.name_changed? # => false

But I do think that's a bug - if a column has a default false value, then the changed value would return as true.
Now I'm thinking this inferring from data values is just a game of whack-a-mole unless we're more explicit in saving whether tasks have been started in the DB.

end
end
end

def started_field_name(record, name)
attribute_name = name.to_s
return attribute_name if record.respond_to?(attribute_name)

# Fields for accepts_nested_attributes_for associations are declared as
# :foo_attributes (matching the foo_attributes= setter used by form
# params), but only the bare association reader (record.foo) exists.
# Only strip the suffix when probing a nested-attributes field.
return unless attribute_name.end_with?("_attributes")

association_name = attribute_name.delete_suffix("_attributes")
association_name if record.respond_to?(association_name)
end

def edit_pathname
"edit_#{@name}"
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/strata/flows/task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def initialize(name, depends_on: nil, pages: [])
end

def started?(record)
@pages.any? { |page| page.completed?(record) }
@pages.any? { |page| page.started?(record) }
end

def completed?(record)
Expand Down
10 changes: 10 additions & 0 deletions spec/dummy/app/previews/sample_task_list_preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@ def depends_on_no_tasks_completed
render Strata::Flows::TaskListComponent.new(flow:, show_step_label: true)
end

# @label First page empty, later page has data
# @note Personal Information has data on the second page (Date of Birth) but the first page (Name) is empty. Shows "Continue" because some sub-page has data, and routes to the first incomplete page.
def first_page_empty_later_page_has_data
flow = SampleFlow.new(FactoryBot.build_stubbed(
:sample_application_form,
date_of_birth: Date.new(1990, 5, 15)
))
render Strata::Flows::TaskListComponent.new(flow:, show_step_label: true)
end

# @label First task completed
# @note Personal Information is completed, unlocking Employment Details. Leave Details still shows "Cannot start yet" because it depends on Employment Details.
def depends_on_first_task_completed
Expand Down
171 changes: 171 additions & 0 deletions spec/models/strata/flows/question_page_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@
expect(page).to be_completed(record)
end

it "is started when the field has a value" do
expect(page).not_to be_started(record)

record.first_name = "Mary"
expect(page).to be_started(record)
end

Comment thread
lamroger-nava marked this conversation as resolved.
it "returns the correct pathnames" do
expect(page.edit_pathname).to eq("edit_first_name")
expect(page.update_pathname).to eq("update_first_name")
Expand Down Expand Up @@ -63,6 +70,170 @@
end
end

describe "#started? with a boolean field" do
before do
bool_class = Class.new do
include ActiveModel::Model
include ActiveModel::Attributes

attribute :consents, :boolean
end

stub_const("BooleanModel", bool_class)
end

let(:record) { BooleanModel.new }
let(:page) { described_class.new("consents") }

it "is not started when the field is nil" do
expect(page).not_to be_started(record)
end

it "is started when the field is true" do
record.consents = true
expect(page).to be_started(record)
end

it "is started when the field is explicitly false" do
record.consents = false
expect(page).to be_started(record)
end

context "when mixed with a string field that is blank" do
before do
mixed_class = Class.new do
include ActiveModel::Model
include ActiveModel::Attributes

attribute :consents, :boolean
attribute :note, :string
end

stub_const("MixedBooleanModel", mixed_class)
end

let(:record) { MixedBooleanModel.new }
let(:page) { described_class.new("preferences", fields: [ :note, :consents ]) }

it "is started when only the boolean field is explicitly false" do
record.consents = false
expect(page).to be_started(record)
end
end
end

describe "#started? when a field is not defined on the record" do
let(:page) { described_class.new("ghost", fields: [ :nonexistent_field ]) }

it "does not raise" do
expect { page.started?(record) }.not_to raise_error
end

it "is not started" do
expect(page).not_to be_started(record)
end

context "when mixed with a defined field that has a value" do
let(:page) { described_class.new("mixed", fields: [ :nonexistent_field, :first_name ]) }

it "is started" do
record.first_name = "Mary"
expect(page).to be_started(record)
end
end
end

describe "#started? with a multi-parameter field" do
before do
test_model_class = Class.new(ActiveRecord::Base) do
self.table_name = "test_records"
include Strata::Attributes

strata_attribute :date_of_birth, :memorable_date
end

stub_const("DobModel", test_model_class)
end

let(:record) { DobModel.new }
let(:page) { described_class.new("dob", fields: [ date_of_birth: [ :month, :day, :year ] ]) }

it "is not started when the field has no value" do
expect(page).not_to be_started(record)
end

it "is started when the field has a value" do
record.date_of_birth = Date.new(1990, 5, 15)
expect(page).to be_started(record)
end
end

describe "#started? with a nested-attributes field" do
before do
child_class = Class.new(ActiveRecord::Base) do
self.table_name = "test_application_forms"
end
stub_const("NestedAttrsForm", child_class)

parent_class = Class.new(ActiveRecord::Base) do
self.table_name = "users"
has_many :forms, class_name: "NestedAttrsForm", foreign_key: :user_id
accepts_nested_attributes_for :forms
end
stub_const("NestedAttrsUser", parent_class)
end

let(:record) { NestedAttrsUser.new(first_name: "Alice", last_name: "Smith") }

context "with the hash form" do
let(:page) { described_class.new("forms", fields: [ forms_attributes: [ :test_string ] ]) }

it "is not started when no nested records have been assigned" do
expect(page).not_to be_started(record)
end

it "is started after nested attributes are assigned" do
record.forms_attributes = [ { test_string: "hello" } ]
expect(page).to be_started(record)
end
end

context "with the symbol form" do
let(:page) { described_class.new("forms", fields: [ :forms_attributes ]) }

it "is started after nested attributes are assigned" do
record.forms_attributes = [ { test_string: "hello" } ]
expect(page).to be_started(record)
end
end
end

describe "#started? with multiple fields" do
before do
multi_field_class = Class.new do
include ActiveModel::Model
include ActiveModel::Attributes

attribute :first_name, :string
attribute :last_name, :string
end

stub_const("MultiFieldModel", multi_field_class)
end

let(:record) { MultiFieldModel.new }
let(:page) { described_class.new("name", fields: [ :first_name, :last_name ]) }

it "is not started when no fields have values" do
expect(page).not_to be_started(record)
end

it "is started when any field has a value" do
record.last_name = "Smith"
expect(page).to be_started(record)
end
end

describe "#attributes" do
before do
test_model_class = Class.new(ActiveRecord::Base) do
Expand Down
83 changes: 59 additions & 24 deletions spec/models/strata/flows/task_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,55 +9,90 @@
include ActiveModel::Attributes

attribute :first_name, :string
attribute :last_name, :string
attribute :middle_name, :string
validates :first_name, presence: true, on: :first_name
validates :last_name, presence: true, on: :last_name
end

stub_const("TestModel", test_model_class)
end

let(:incomplete_page) { Strata::Flows::QuestionPage.new("first_name") }
let(:complete_page) { Strata::Flows::QuestionPage.new("last_name") }
let(:first_name_page) { Strata::Flows::QuestionPage.new("first_name") }
let(:last_name_page) { Strata::Flows::QuestionPage.new("last_name") }
let(:record) { TestModel.new }

describe "an unstarted task" do
let(:task) { described_class.new("name", pages: [ incomplete_page ]) }
describe "#started?" do
let(:task) { described_class.new("name", pages: [ first_name_page, last_name_page ]) }

it "is not started or completed" do
it "is false when no pages have data" do
expect(task).not_to be_started(record)
expect(task).not_to be_completed(record)
end

it "returns the first page path" do
allow(incomplete_page).to receive(:edit_path).and_return("edit_path")
expect(task.path(record)).to eq("edit_path")
it "is true when the first page has data" do
record.first_name = "Mary"
expect(task).to be_started(record)
end

it "is true when only a later page has data" do
record.last_name = "Smith"
expect(task).to be_started(record)
end

it "is false when a page has only an untouched optional field" do
optional_page = Strata::Flows::QuestionPage.new("middle_name")
task = described_class.new("name", pages: [ optional_page ])

expect(optional_page).to be_completed(record)
expect(task).not_to be_started(record)
end
end

describe "a started task" do
let(:task) { described_class.new("name", pages: [ complete_page, incomplete_page ]) }
describe "#completed?" do
let(:task) { described_class.new("name", pages: [ first_name_page, last_name_page ]) }

it "is started but not completed" do
expect(task).to be_started(record)
it "is false when no pages have data" do
expect(task).not_to be_completed(record)
end

it "returns the first incomplete page path" do
allow(incomplete_page).to receive(:edit_path).and_return("edit_path")
expect(task.path(record)).to eq("edit_path")
it "is false when only some pages are complete" do
record.first_name = "Mary"
expect(task).not_to be_completed(record)
end

it "is true when all pages are complete" do
record.first_name = "Mary"
record.last_name = "Smith"
expect(task).to be_completed(record)
end
end

describe "a completed task" do
let(:task) { described_class.new("name", pages: [ complete_page ]) }
describe "#path" do
let(:task) { described_class.new("name", pages: [ first_name_page, last_name_page ]) }

it "is started and completed" do
expect(task).to be_started(record)
expect(task).to be_completed(record)
before do
allow(first_name_page).to receive(:edit_path).and_return("first_name_path")
allow(last_name_page).to receive(:edit_path).and_return("last_name_path")
end

it "returns the first page path when no pages have data" do
expect(task.path(record)).to eq("first_name_path")
end

it "returns the first incomplete page path when in progress" do
record.first_name = "Mary"
expect(task.path(record)).to eq("last_name_path")
end

it "returns the first incomplete page path when only a later page has data" do
record.last_name = "Smith"
expect(task.path(record)).to eq("first_name_path")
end

it "returns the first page path" do
allow(complete_page).to receive(:edit_path).and_return("edit_path")
expect(task.path(record)).to eq("edit_path")
it "returns the first page path when all pages are complete" do
record.first_name = "Mary"
record.last_name = "Smith"
expect(task.path(record)).to eq("first_name_path")
end
end

Expand Down
Loading