[OP-20396] States for project lifecycle phases by planktonic · Pull Request #26164 · opf/openproject

planktonic · 2026-10-20T12:49:33Z

Ticket

https://community.openproject.org/wp/OP-20396

What are you trying to accomplish?

Add a status field to project lifecycle phases. Allow that status to be changed (currently fixed to the statuses open, in_progress, and complete).

When the status is changed, update the status of other the phases, following the rules on the ticket.

Screenshots

Before	After

With a menu open:

Firefox_Screenshot_2026-10-21T09-24-10 445Z

What approach did you choose and why?

Adds the required fields to the Project Phase model, and adds some constants for the new phase statuses, with a migration for that.

Registers a new highlighter.

Adds a new Project::PhaseStatusButtom component, mirroring the existing Project::StatusButton component. Overrides the color that is set on IconButton, so the phase status color is used instead. Adds lookbook previews to show these changes.

Adds tests.

AI involvement

Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

joelpaulkoch · 2026-10-20T13:05:36Z

-  it "renders label" do
-    expect(rendered_component).to have_element :label, text: "Subject", class: "advanced-filters--filter-name"
+    def self.find(code)
+      AVAILABLE.find { it.code.to_s == code.to_s }


why do we need the to_s here? aren't these both the same symbol?

This is how it’s done in the existing project status too. It seems to be necessary (or clearer) because rails stores the enums as strings.

joelpaulkoch · 2026-10-20T13:09:42Z

+module Projects
+  module PhaseStatuses
+    Status = Data.define(:code, :hexcode, :icon) do
+      def id = code&.to_s || :none


A small thing but this is a string when it's there and a :none when it's nil.
Is there a reason why it must be a string? I think the helper in the component turns it into a string anyways?

joelpaulkoch · 2026-10-20T13:19:58Z

+    def refresh_buttons
+      @project.available_phases.each do |phase|
+        update_via_turbo_stream(
+          component: Projects::PhaseStatusButtonComponent.new(phase: phase.reload)


mh the phase.reload is reloading in a loop, no? Why do we need to reload? Maybe we can reload all at once before the loop?

joelpaulkoch · 2026-10-20T13:25:22Z

+    def cascade_to_other_phases
+      project.available_phases.each do |other|
+        next if other == model

-  subject(:rendered_component) do
-    with_controller_class(Reporting::CostReportsController) do
-      with_request_url("/reporting/cost_reports") do
-        render_component(cost_query, **options)
+        target = cascaded_status(earlier: other.position < model.position)
+        next if target.nil? || other.status == target
+
+        other.update!(status: target)
      end
    end
-  end

-  context "when there are no items" do
-    it "renders nothing" do
-      expect(rendered_component).to have_css ".generic-table--no-results-title", text: "There is currently nothing to display."
+    def cascaded_status(earlier:)
+      case model.status
+      when "open"        then earlier ? nil : "open"
+      when "in_progress" then earlier ? "completed" : "open"
+      when "completed"   then earlier ? "completed" : nil
+      end
    end


here I am wondering too if we couldn't do this all in one step instead of in a loop 🤔

Same. I guess you coud get all earlier/later phases and to a bulk edit on them

https://community.openproject.org/wp/OP-20396

Adds the required fields to the Project Phase model, and adds some constants for the new phase statuses Adds a migration for that

Adds a new Project::PhaseStatusButtom component, mirroring the existing Project::StatusButton component. Registers a new highlighter (so far unused) Adds translations. Adds the option to render only an IconButton in the OpPrimer::StatusButton component.

Updates controllers & add routes to allow changing phase states. Colors of the buttons in the dropdown are now correctly set.

Adds some space between button & text, so when hovered it doesn’t look broken. Adds a story for the new status button component Cleans up some unnecessary code Linting

Adds tests for the new project-phase status button, and the phase-status service. The phase-status-button is now disabled when the user doesn’t have edit rights.

Follows the project status approach of passing in the user model, making lookbook preview work following the gating to admin users.

Adds an override to the icon color in the phase-state-button

Updates tests to reflect the user model being passed in. Reverts the unrelated changes made to the Sass file (auto-linting applied by my editor on save) Linting.

HDinger

Works already quite nice. 👍 I left some comments.

HDinger · 2026-10-23T07:00:06Z

+    end
+
+    def refresh_buttons
+      @project.available_phases.each do |phase|


I guess in the future, we might want to make that smarter than just refreshing them all. But for now this is fine for me.

Counterproposal: make it dumber. This already knows that it is called from the project overview page which is why all the project's phases are on the page. Just render the complete turbo frame project-life-cycle-sidebar

Or, throw an event and have the frontend figure out whether it then wants to reload or not. Costs a round trip but offers a way simpler architecture. And the user will not know the difference.

HDinger · 2026-10-23T08:57:17Z

+        super()
+
+        @phase = phase
+        @user = user


What is the sense of passing the user through to all the components instead of using User.current like before?

ulferts

@planktonic , please consider this a sloppy review. I know nothing about the context of this PR and didn't spent a lot of time on it. That also means that maybe one or the other comment is off.

What I would definitely require though is the specs on the contract and another one on feature level.

ulferts · 2026-10-23T10:37:32Z

+    add_column :project_phases, :status, :integer, null: false, default: 0
+    add_column :project_phase_journals, :status, :integer, null: false, default: 0
+
+    add_index :project_phases, :status


👍 for the backfilling of existing records on the journals as well.

With this column being journaled, did you check how that looks in the activity?

E.g. when changing the phases' dates this looks like this:

ulferts · 2026-10-23T12:36:33Z

+
+module ProjectPhases
+  class UpdateStatusContract < BaseContract
+    delegate :project, to: :model


Why is this required? Only the BaseContract has a reference to project and it should have solved that. Although admittedly, just from looking at it I don't understand how. But if it is necessary, it should find its way into the superclass.

Edit: I see now that e.g. the UpdateContract does the same thing. Suggestion, move the delegation into the superclass, clean up the classes inheriting from it.

ulferts · 2026-10-23T12:39:04Z

+
+    def writable_attributes = %w[status]
+  end
+end


As small as this class looks, contracts are the main line of defence for the models. Controllers protect against access to the controller but contracts control which action is allowed to a user on a model. The controller check is usually coarse, the controller's check can be way more granular.

I am writing all this to justify why I think a unit spec on this class is a must. It should check that the permission is necessary and that only the status can be changed.

ulferts · 2026-10-23T12:51:12Z

+    end
+
+    def refresh_buttons
+      @project.available_phases.each do |phase|


Counterproposal: make it dumber. This already knows that it is called from the project overview page which is why all the project's phases are on the page. Just render the complete turbo frame project-life-cycle-sidebar

Or, throw an event and have the frontend figure out whether it then wants to reload or not. Costs a round trip but offers a way simpler architecture. And the user will not know the difference.

…-phases

- Removes unnecessarily-nested flex layout in rows - Uses test-selector in tests instead of `.op-` classname - StatusButton is not rendered if the user doesn’t have edit access, instead of simply being disabled. Adds a preview for that, and updates tests.

Removes the global helper, moves the helper functions into the component Moves the new translation strings in `config/locales/en.yml`, out of the JS version.

The phase status enum now uses string instead of integers. Updated the migration.

Fixes the StatuButton component clobbering the generated classname attribute (that has the highlight class) when you also pass in a classname. Removes the extra code in the PhaseStatucButton that was getting around that.

joelpaulkoch reviewed Oct 20, 2026

View reviewed changes

planktonic force-pushed the feature/op-20396-states-for-project-lifecycle-phases branch from 8bf3249 to a5be46b Compare October 20, 2026 13:51

planktonic added 9 commits October 21, 2026 10:59

[OP-20396] States for project lifecycle phases

6dca1a4

https://community.openproject.org/wp/OP-20396

[OP-20396] Adds status to the project phase model

dbba8e6

Adds the required fields to the Project Phase model, and adds some constants for the new phase statuses Adds a migration for that

[OP-20396] Building the UI

b00afef

Adds a new Project::PhaseStatusButtom component, mirroring the existing Project::StatusButton component. Registers a new highlighter (so far unused) Adds translations. Adds the option to render only an IconButton in the OpPrimer::StatusButton component.

[OP-20396] Functionality of switching states

e7c1ba3

Updates controllers & add routes to allow changing phase states. Colors of the buttons in the dropdown are now correctly set.

[OP-20396] Layout adjustments, lookbook preview, & linting

f5afec1

Adds some space between button & text, so when hovered it doesn’t look broken. Adds a story for the new status button component Cleans up some unnecessary code Linting

[OP-20396] Add lookbook preview for OPPrimer status button component

3756b2d

[OP-20396] Adding tests

9b193b2

Adds tests for the new project-phase status button, and the phase-status service. The phase-status-button is now disabled when the user doesn’t have edit rights.

[OP-20396] Pass in user

1716736

Follows the project status approach of passing in the user model, making lookbook preview work following the gating to admin users.

[OP-20396] Set color of button icon

468d6c2

Adds an override to the icon color in the phase-state-button

planktonic force-pushed the feature/op-20396-states-for-project-lifecycle-phases branch from 2d5d6a3 to 468d6c2 Compare October 21, 2026 08:59

[OP-20396] Cleanup

f239466

Updates tests to reflect the user model being passed in. Reverts the unrelated changes made to the Sass file (auto-linting applied by my editor on save) Linting.

planktonic force-pushed the feature/op-20396-states-for-project-lifecycle-phases branch from 50dd5b2 to f239466 Compare October 21, 2026 09:32

github-actions Bot added the ai: Collaborative 💻 AI generated a substantial part of the code; A human reviewed and understands every line. label Oct 21, 2026

planktonic marked this pull request as ready for review October 21, 2026 09:38

HDinger requested changes Oct 23, 2026

View reviewed changes

ulferts requested changes Oct 23, 2026

View reviewed changes

planktonic added 5 commits October 26, 2026 14:53

Merge branch 'dev' into feature/op-20396-states-for-project-lifecycle…

65f7cb2

…-phases

[OP-20396] PR feedback

4484e8e

- Removes unnecessarily-nested flex layout in rows - Uses test-selector in tests instead of `.op-` classname - StatusButton is not rendered if the user doesn’t have edit access, instead of simply being disabled. Adds a preview for that, and updates tests.

[OP-20396] PR feedback

93794a3

Removes the global helper, moves the helper functions into the component Moves the new translation strings in `config/locales/en.yml`, out of the JS version.

[OP-20396] PR feedback

ed765a8

The phase status enum now uses string instead of integers. Updated the migration.

[OP-20396] PR feedback

ad1e6a8

Fixes the StatuButton component clobbering the generated classname attribute (that has the highlight class) when you also pass in a classname. Removes the extra code in the PhaseStatucButton that was getting around that.

Conversation

planktonic commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

AI involvement

Merge checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HDinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ulferts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

planktonic commented Oct 20, 2026 •

edited

Loading