Fixes Strata Task to return correct value for started? if any pages are started#361
Fixes Strata Task to return correct value for started? if any pages are started#361lamroger-nava wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Strata flow task “started” semantics so a task is considered started if any page has been interacted with, rather than inferring “started” from page completion. This better handles optional fields/pages that can be “completed” without user input.
Changes:
- Switch
Strata::Flows::Task#started?to rely onQuestionPage#started?instead ofQuestionPage#completed?. - Introduce
Strata::Flows::QuestionPage#started?based on presence of configured field values (including nested-attributes and multi-parameter field shapes). - Expand RSpec coverage for task/page started/completed/path behavior and add a Lookbook preview demonstrating “later page has data”.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/strata/flows/task.rb |
Changes task started detection to use per-page started state. |
app/models/strata/flows/question_page.rb |
Adds started? implementation that inspects configured fields for “has data”. |
spec/models/strata/flows/task_spec.rb |
Reworks task specs around #started?, #completed?, and #path with multi-page scenarios. |
spec/models/strata/flows/question_page_spec.rb |
Adds specs covering QuestionPage#started? across single field, multi-field, multi-parameter, and nested-attributes cases. |
spec/dummy/app/previews/sample_task_list_preview.rb |
Adds Lookbook preview for “first page empty, later page has data” scenario. |
| record.public_send(attribute_name).present? | ||
| end |
There was a problem hiding this comment.
I dont think we want invalid dates to be considered saved in-progress work
started? calls record.public_send(attribute_name) unconditionally. Flows can define fields that don't exist on the record (e.g., :flow_only_field, :resume, or :income_records_attributes in spec/dummy/app/flows/test_application_form_flow.rb), which will raise NoMethodError and break task list rendering. Consider guarding with respond_to? (and only stripping _attributes when needed), treating unknown fields as "not started" instead of crashing. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| value = record.public_send(attribute_name) if attribute_name | ||
| # 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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Closing for now - don't think this is the right solution. We might want to store metadata about started or started_at |
Changes
Fixes Tasks to return the correct value for
started?by seeing if any pages are started vs completed.Context
This was an issue when a field was optional and considered completed when no one interacted with it.
Testing
Added lookbook examples: http://localhost:3000/lookbook/inspect/sample_task_list/first_page_empty_later_page_has_data