[#73389] Clean up controllers and routing constraints#22478
[#73389] Clean up controllers and routing constraints#22478
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Backlogs “scrum_projects” feature-flagged endpoints by moving the new Scrum functionality into dedicated Agile::* controllers and routing it via a route constraint, while simplifying the legacy Rb* controllers by removing feature-flag branches. It also updates permissions and specs accordingly.
Changes:
- Introduce
Agile::*controllers (sprint planning, sprints, stories, taskboards) and route them only whenscrum_projectsis active. - Simplify legacy
Rb*controllers by removing feature-flagged code paths and shifting specs to the new controllers. - Extend Backlogs permission mappings and update form routing for sprint update.
Reviewed changes
Copilot reviewed 25 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/spec/routing/rb_stories_routing_spec.rb | Removes feature-flag routing assertions for the old stories move endpoint. |
| modules/backlogs/spec/routing/rb_sprints_routing_spec.rb | Adjusts legacy sprint routing expectations to the flag-inactive case. |
| modules/backlogs/spec/routing/agile/taskboards_routing_spec.rb | Adds routing coverage for the new agile taskboard endpoint under the flag. |
| modules/backlogs/spec/routing/agile/stories_routing_spec.rb | Adds routing coverage for the new agile stories move endpoint. |
| modules/backlogs/spec/routing/agile/sprints_routing_spec.rb | Adds routing coverage for the new agile sprints endpoints (incl. named routes). |
| modules/backlogs/spec/routing/agile/sprint_planning_routing_spec.rb | Adds routing coverage for the new sprint planning controller and legacy fallback. |
| modules/backlogs/spec/controllers/rb_taskboards_controller_spec.rb | Removes feature-flag-active behavior tests from the legacy controller spec. |
| modules/backlogs/spec/controllers/rb_stories_controller_spec.rb | Removes feature-flag-active move tests from the legacy controller spec. |
| modules/backlogs/spec/controllers/rb_sprints_controller_spec.rb | Removes feature-flag-active sprint CRUD/lifecycle tests from the legacy controller spec. |
| modules/backlogs/spec/controllers/agile/taskboards_controller_spec.rb | Adds controller specs for the new agile taskboards controller. |
| modules/backlogs/spec/controllers/agile/sprints_controller_spec.rb | Adds controller specs for the new agile sprints controller. |
| modules/backlogs/lib/open_project/backlogs/engine.rb | Extends permission mappings to include the new agile/* controllers/actions. |
| modules/backlogs/config/routes.rb | Adds constrained routes for agile controllers; keeps legacy routes outside constraint. |
| modules/backlogs/app/views/agile/sprint_planning/show.html.erb | Adds new sprint planning page wrapper rendering a turbo-frame list. |
| modules/backlogs/app/views/agile/sprint_planning/_sprint_planning_list.html.erb | Adds the new sprint planning list partial (frame content). |
| modules/backlogs/app/views/agile/sprint_planning/_agile_list.html.erb | Adds an additional list partial (currently appears unused). |
| modules/backlogs/app/controllers/rb_taskboards_controller.rb | Removes feature-flag branch; legacy controller now only serves legacy sprint taskboard. |
| modules/backlogs/app/controllers/rb_stories_controller.rb | Removes agile move action; keeps legacy move/reorder and simplifies turbo stream updates. |
| modules/backlogs/app/controllers/rb_sprints_controller.rb | Removes feature-flagged agile sprint CRUD/lifecycle endpoints from legacy controller. |
| modules/backlogs/app/controllers/rb_master_backlogs_controller.rb | Removes sprint planning behavior and feature-flag redirects from legacy controller. |
| modules/backlogs/app/controllers/rb_application_controller.rb | Removes feature-flag branching from sprint loading and plugin configuration check. |
| modules/backlogs/app/controllers/agile/taskboards_controller.rb | Adds new taskboards controller for flagged behavior (board redirect). |
| modules/backlogs/app/controllers/agile/stories_controller.rb | Adds new stories controller for flagged move behavior (turbo streams). |
| modules/backlogs/app/controllers/agile/sprints_controller.rb | Adds new sprints controller for flagged CRUD/lifecycle (dialogs, start/finish). |
| modules/backlogs/app/controllers/agile/sprint_planning_controller.rb | Adds new sprint planning controller handling list + split view details. |
| modules/backlogs/app/controllers/agile/base_controller.rb | Adds shared base controller for agile backlogs endpoints (project/sprint loading + authorize). |
| modules/backlogs/app/components/backlogs/new_sprint_form_component.rb | Updates sprint update URL to use the new standard project_sprint_path. |
modules/backlogs/app/controllers/rb_master_backlogs_controller.rb
Outdated
Show resolved
Hide resolved
modules/backlogs/app/controllers/agile/sprint_planning_controller.rb
Outdated
Show resolved
Hide resolved
81900e6 to
b12fd3b
Compare
Introduces dedicated `Agile::` controllers for scrum-project functionality, gated behind `Constraints::FeatureDecision`. New controllers: - `Agile::BaseController` — shared project/sprint loading - `Agile::SprintPlanningController` — sprint planning + details - `Agile::SprintsController` — sprint CRUD and lifecycle - `Agile::StoriesController` — work-package move between sprints - `Agile::TaskboardsController` — taskboard redirect shim Routes use `scope module: "agile"` to keep URL paths unchanged. Sprint planning mirrors the legacy backlogs nesting to preserve `sprint_planning_backlogs_project_backlogs_path`. The legacy `sprint_planning` route is removed since the feature-gated route now owns that URL. `Agile::SprintsController#load_sprint` scopes to `@project.sprint_source` (not `for_project`) to prevent mutation of sprints only visible via work-package references. Views moved from `rb_master_backlogs/` to `agile/sprint_planning/` with no content changes. Menu items updated to point to the new controller. Permission registrations extended with the new controller paths. https://community.openproject.org/wp/73389
Removes feature-flag branches and new-path code from legacy controllers, leaving them single-purpose for the legacy flow. `RbSprintsController` is reduced to inline name editing (`edit_name`, `show_name`, `update`). Sprint loading now scopes to `Sprint.visible.apply_to(@project)` to prevent cross-project mutation via known IDs. `RbStoriesController` renames `move_legacy` back to `move` and drops `Agile::Sprint` handling — the legacy controller only works with Version-backed sprints. `RbMasterBacklogsController` drops the `sprint_planning` stub; the index redirects to the agile route when the flag is active. `RbTaskboardsController` drops its feature-flag branch. Cleans up `engine.rb` permissions: removes stale `rb_sprints` actions (`new_dialog`, `refresh_form`, `start`, `finish`, etc.) and renames `move_legacy` to `move`. https://community.openproject.org/wp/73389
Align sprint menu permissions with controller mutability rules. This hides sprint mutation actions when a sprint is only visible through work package references so the UI no longer advertises operations the controllers will reject. https://community.openproject.org/wp/73389
b12fd3b to
94511f1
Compare
| end | ||
|
|
||
| def mutable_sprint_in_project? | ||
| project.sprint_source.exists?(id: sprint.project_id) |
There was a problem hiding this comment.
| replace_backlog_component_via_turbo_stream(sprint: new_sprint) | ||
| end | ||
|
|
||
| def infer_attributes_from_target |
There was a problem hiding this comment.
I think this can be removed?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 37 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
modules/backlogs/app/controllers/rb_stories_controller.rb:121
RbStoriesController#moveis now described/implemented as moving between version-backed sprint containers, and it only updates the target UI for version moves. However,infer_attributes_from_targetstill acceptstarget_type == "sprint"and will setsprint_id, which can move a story into an Agile sprint (e.g., whentarget_idcomes fromBacklogs::SprintComponent'ssprint:target ids). In that case the action won’t render a success flash/target component update, and it also reintroduces a path to mutate Agile sprint assignments through the legacy controller. If this controller is intended to be version-only, reject/raise ontarget_type == "sprint"(or otherwise fully handle sprint targets here).
def infer_attributes_from_target
target_type, target_id = move_params[:target_id].split(":")
case target_type
when "version"
{ version_id: target_id, sprint_id: nil }
when "sprint"
# If the story is assigned to a version, we will only nullify the version
# if it is used as a backlog. We will keep a "regular" version reference.
# Otherwise, moving a story to a sprint would delete it from any version it is
# assigned to.
if @story.version&.used_as_backlog?
{ version_id: nil, sprint_id: target_id }
else
{ sprint_id: target_id }
end
else
raise ArgumentError, "target_type must include one of: version, sprint."
end
| before_action :load_backlogs, only: :index | ||
|
|
||
| def index | ||
| return redirect_to action: :sprint_planning if OpenProject::FeatureDecisions.scrum_projects_active? | ||
| return redirect_to sprint_planning_backlogs_project_backlogs_path(@project) if redirect_to_agile_sprint_planning? | ||
|
|
There was a problem hiding this comment.
load_backlogs runs as a before_action for #index, but #index immediately redirects when scrum_projects_active? is true. This means every visit to /projects/:id/backlogs in the new mode still executes the legacy backlog queries unnecessarily. Consider moving the redirect ahead of loading backlogs (e.g., perform the redirect in a separate before_action and skip load_backlogs when redirecting, or call load_backlogs inside #index only after the redirect guard).
| def check_if_plugin_is_configured | ||
| return if OpenProject::FeatureDecisions.scrum_projects_active? | ||
|
|
||
| settings = Setting.plugin_openproject_backlogs | ||
| if settings["story_types"].blank? || settings["task_type"].blank? | ||
| respond_to do |format| | ||
| format.html { render template: "shared/not_configured" } | ||
| end | ||
| end |
There was a problem hiding this comment.
check_if_plugin_is_configured no longer skips execution when scrum_projects is active. This changes behavior compared to the previous guard and can interfere with the legacy controllers that now redirect in scrum mode (e.g., /projects/:id/backlogs), potentially rendering shared/not_configured (or causing a double-render/redirect situation) instead of performing the redirect. If the intent is still to bypass this check when scrum_projects is active, reintroduce the feature-flag guard; if the intent is to enforce the check, the equivalent check likely needs to live in Agile::BaseController as well so behavior is consistent across the new controllers.
|
Hi @myabc I didn't review this yet since it is too big to review shortly before the feature freeze with us still having to finish a number of features. I suggest we merge this after the feature freeze, when we have time for this. Then, we can merge it and remove the old controllers in one go, together with the feature flag. |
…-sprint-controller-routing-cleanup # Conflicts: # modules/backlogs/app/components/backlogs/finish_sprint_dialog_component.html.erb # modules/backlogs/app/controllers/rb_master_backlogs_controller.rb # modules/backlogs/app/controllers/rb_sprints_controller.rb # modules/backlogs/app/controllers/rb_stories_controller.rb # modules/backlogs/app/views/agile/sprint_planning/_agile_list.html.erb # modules/backlogs/app/views/rb_master_backlogs/_agile_list.html.erb # modules/backlogs/lib/open_project/backlogs/engine.rb # modules/backlogs/spec/controllers/rb_sprints_controller_spec.rb # modules/backlogs/spec/controllers/rb_stories_controller_spec.rb
Ticket
https://community.openproject.org/wp/73389
What are you trying to accomplish?
Splits dual-mode backlogs controllers that mix legacy and new scrum-project code (behind the
scrum_projectsfeature flag) into clean, separate controllers:Rb*Controller) — stripped of all feature-flag branches, only serve legacy pathsAgile::*Controller) — dedicated controllers for sprint planning, sprints, stories, and taskboardsThis eliminates flag-conditional logic scattered across controllers, making each controller single-purpose and easier to reason about.
New controllers created
Agile::BaseControllerAgile::SprintPlanningControllerAgile::SprintsControllerAgile::StoriesControllerAgile::TaskboardsControllerScreenshots
N/A — no UI changes. All existing URLs and behavior are preserved.
What approach did you choose and why?
URL-preserving routing with
scope module:— the newAgile::controllers are routed behind the existingConstraints::FeatureDecisionrouting constraint, usingscope module: "agile"so URL paths remain unchanged. Legacy route nesting is mirrored to preserve named route helpers (e.g.sprint_planning_backlogs_project_backlogs_path). Only one URL helper needed updating (NewSprintFormComponent).Views were moved via
git mvfromrb_master_backlogs/toagile/sprint_planning/with no content changes.Permission registrations in
engine.rbwere extended to include the new controller paths alongside the existingrb_*entries.Merge checklist