[#73446] Sum of work packages and their story points in sprint header are incorrect#22633
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect sprint header totals (work package count + story points) by ensuring calculations only consider work packages from the currently viewed project, even when sprints are included via cross-project work package references.
Changes:
- Add
Agile::Sprint.for_project_with_work_packagesto preload sprint work packages filtered to the current project. - Introduce
Agile::Sprint#work_packages_for(project)and update sprint components to use it for consistent per-project work package retrieval. - Add/extend model + feature specs covering the new scope, ordering expectations, and sprint planning flows.
Reviewed changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/app/controllers/rb_master_backlogs_controller.rb | Switch sprint loading to the new eager-loading scope when scrum feature flag is active. |
| modules/backlogs/app/models/agile/sprint.rb | Add work_packages_for(project), include new scope, and tweak work_packages association options. |
| modules/backlogs/app/models/agile/sprints/scopes/for_project_with_work_packages.rb | New scope that preloads work packages for the given project (ordered by position). |
| modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb | Add WorkPackage.order_by_position helper used by the new scope/method. |
| modules/backlogs/app/components/backlogs/sprint_header_component.rb | Use work_packages_for(project) for story points + story count in header. |
| modules/backlogs/app/components/backlogs/sprint_component.rb | Use work_packages_for(project) for sprint story listing. |
| modules/backlogs/spec/models/agile/sprints/scopes/for_project_with_work_packages_spec.rb | New specs validating eager load behavior, filtering, and bounded queries. |
| modules/backlogs/spec/models/agile/sprint_spec.rb | Add specs around work package ordering and work_packages_for. |
| modules/backlogs/spec/support/pages/sprint_planning.rb | Update page object helpers to assert sprint header story points + count. |
| modules/backlogs/spec/features/sprint_planning/sprint_list_spec.rb | New feature spec verifying header totals only count current-project work packages. |
| modules/backlogs/spec/features/sprint_planning/create_spec.rb | New feature spec coverage for creating sprints in sprint planning. |
| modules/backlogs/spec/features/sprint_planning/edit_spec.rb | New feature spec coverage for editing sprints and permission-dependent menu entries. |
| modules/backlogs/spec/features/sprint_planning/start_finish_spec.rb | New feature spec coverage for starting/finishing sprints and board redirection. |
modules/backlogs/app/models/agile/sprints/scopes/for_project_with_work_packages.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@dombesz thanks for walking me through the code earlier!
Even though I understand the code a bit better now, this solution still feels over-engineered – a premature optimisation.
For this PR, I would favour going with a simple scope fix, i.e. .where(project:) (see inline comments).
We could go with includes ofc:
# Controller
@sprints = Agile::Sprint.for_project(@project).not_completed.order_by_date
.includes(:work_packages) but, as you mentioned this will over-fetch, so we would still have to filter in Ruby-land:
def stories
@stories ||= sprint.work_packages
.select { |wp| wp.project_id == project.id }
.sort_by { |wp| wp.position || Float::INFINITY }
endPreloader is a private API, and it's not something most other devs (including me) will be able to understand without some extra work. I'd vote to drop this solution for now and save it for if/when performance issues come to light.
modules/backlogs/app/components/backlogs/sprint_header_component.rb
Outdated
Show resolved
Hide resolved
modules/backlogs/app/components/backlogs/sprint_header_component.rb
Outdated
Show resolved
Hide resolved
modules/backlogs/app/controllers/rb_master_backlogs_controller.rb
Outdated
Show resolved
Hide resolved
|
Thanks @myabc for the review, I addressed your concerns. The approach I chose, is to load the stories directly in the controller, instead of using the preload internal api. |
44de7f4 to
ecb2507
Compare
… does not respect project boundaries https://un5kw2gk1a5ewenr6bvyaezm1xctjhkthr.julianrbryant.com/work_packages/73446
…by_position scope for work packages
…he controller instead.
ecb2507 to
3aeb706
Compare
Ticket
https://un5kw2gk1a5ewenr6bvyaezm1xctjhkthr.julianrbryant.com/work_packages/73446
What are you trying to accomplish?
What approach did you choose and why?
for_project_with_work_packages.work_packages_formethod in theAgile::Sprintmodel to ensure there is a single way of retrieving work packages for a project in a sprint.for_project_with_work_packagesbenefits from the eager loaded work package association.Merge checklist