refactor: consolidate run and work order state definitions#4600
refactor: consolidate run and work order state definitions#4600officialasishkumar wants to merge 3 commits intoOpenFn:mainfrom
Conversation
Hardcoded run and work order state lists were scattered across multiple files (dashboard_stats.ex, query.ex, impeded_project_helper.ex, dashboard_components.ex, search_params.ex, and tests), creating a maintenance burden where any state change required manual updates in multiple locations that the compiler cannot catch. Add Run.active_states/0 for [:available, :claimed, :started], WorkOrder.states/0 for all valid states, and WorkOrder.active_states/0 for [:pending, :running]. Replace all hardcoded state lists across the codebase with references to these canonical functions. Derive SearchParams.@statuses from WorkOrder.states/0 to keep the filter UI in sync automatically. Closes OpenFn#4589 Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes canonical “active” and “all” state groupings for runs and work orders by adding public helper functions on the schema modules, then replacing repeated inline state lists across backend code and tests to reduce duplication and maintenance risk.
Changes:
- Added
Lightning.Run.active_states/0,Lightning.WorkOrder.states/0, andLightning.WorkOrder.active_states/0. - Replaced hardcoded run/work order state lists in queries, dashboard stats aggregation, PromEx helper logic, LiveView rendering, search param validation, and tests.
- Documented the refactor in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/lightning/janitor_test.exs | Uses Run.active_states/0 instead of duplicating in-progress run states in test setup. |
| test/lightning/digest_email_worker_test.exs | Uses WorkOrder.active_states/0 instead of duplicating active work order states in test setup. |
| lib/lightning/workorders/workorder.ex | Introduces canonical states/0 and active_states/0 for work orders and reuses them via module attributes. |
| lib/lightning/workorders/search_params.ex | Derives valid status strings from WorkOrder.states/0 instead of a hardcoded list. |
| lib/lightning/runs/run.ex | Adds active_states/0 for runs. |
| lib/lightning/runs/query.ex | Replaces inline active run state list with Run.active_states/0 in query filtering. |
| lib/lightning/runs/prom_ex_plugin/impeded_project_helper.ex | Derives “in progress consuming concurrency” states from Run.active_states/0 -- [:available]. |
| lib/lightning/dashboard_stats.ex | Uses centralized active state lists for aggregation and for excluded-states filtering in batched last-failed lookups. |
| lib/lightning_web/live/workflow_live/dashboard_components.ex | Uses WorkOrder.active_states/0 for status-dot “ping” animation conditions. |
| CHANGELOG.md | Notes the internal refactor and links the issue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| last_failed_workorders = | ||
| batch_get_last_workorders(workflow_ids, [:pending, :running, :success]) | ||
| batch_get_last_workorders( | ||
| workflow_ids, | ||
| WorkOrder.active_states() ++ [:success] | ||
| ) |
There was a problem hiding this comment.
get_last_failed_workorder/2 later in this module still hardcodes [:pending, :running, :success] as excluded_states, which conflicts with the PR goal of removing hardcoded work order state lists from lib/. Consider deriving this list from WorkOrder.active_states() (or @wo_active) to keep a single source of truth.
There was a problem hiding this comment.
Fixed. Now deriving excluded_states from @wo_active ++ [:success], which matches the pattern already used in batch_get_last_workorders on line 65.
| @doc """ | ||
| Returns the list of active (in-progress) states for a run. | ||
|
|
||
| These are all non-final states: available, claimed, and started. | ||
| """ | ||
| def active_states, do: [:available, :claimed, :started] | ||
|
|
There was a problem hiding this comment.
Run.active_states/0 duplicates the same non-final state list that is also embedded in the field :state, Ecto.Enum, values: ... definition below. To avoid these drifting apart (and to better support the “single source of truth” goal), consider introducing an @active_states module attribute and using it both in active_states/0 and in the enum values definition.
There was a problem hiding this comment.
Addressed. Introduced @active_states module attribute and wired it into both active_states/0 and the Ecto.Enum values definition, so the list only lives in one place now.
Align pipe operator to satisfy mix format --check-formatted. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Derived excluded_states in dashboard_stats from WorkOrder.active_states instead of hardcoding. Introduced @active_states module attribute in Run to share between the function and Ecto.Enum definition. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
theroinaochieng
added
the
Community Contribution 🏅
Description
This PR establishes a single source of truth for run and work order state groupings by adding named public functions on the schema modules and replacing all hardcoded state lists throughout the codebase.
New functions:
Run.active_states/0→[:available, :claimed, :started]WorkOrder.states/0→ all valid work order statesWorkOrder.active_states/0→[:pending, :running]Replaced hardcoded lists in:
lib/lightning/dashboard_stats.ex(4 occurrences acrosscount_workorders,count_runs,batch_count_workorders,batch_count_runs, and thebatch_get_last_workordersexcluded-states argument)lib/lightning/runs/query.ex(workflow_limited_runs/1)lib/lightning/runs/prom_ex_plugin/impeded_project_helper.ex(derived fromRun.active_states/0 -- [:available])lib/lightning_web/live/workflow_live/dashboard_components.ex(status dot animation)lib/lightning/workorders/search_params.ex(@statusesnow derived fromWorkOrder.states/0)test/lightning/janitor_test.exsandtest/lightning/digest_email_worker_test.exsCloses #4589
Validation steps
mix test— all existing tests should pass without assertion changes.[:available, :claimed, :started]or[:pending, :running]lists inlib/(outside schema modules):grep -rn "\[:available, :claimed, :started\]\|\[:pending, :running\]" lib/Additional notes for the reviewer
dashboard_stats.exuse module attributes (@wo_active,@run_active) which are evaluated at compile time from the function calls — this is the standard Elixir pattern for using dynamic values in guards.impeded_project_helper.exintentionally used[:claimed, :started](not[:available, :claimed, :started]) because it counts runs actively consuming concurrency slots. This is preserved asRun.active_states() -- [:available].types/history.tsanduseRunRetry.tsare excluded from this change per the issue's explicit scoping.AI Usage
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)