Implementation/71248 create sprint model#21854
Conversation
5792c37 to
8089530
Compare
| "completed" => "completed" | ||
| }, default: "in_planning", validate: true | ||
|
|
||
| SPRINT_SHARINGS = %w(none descendants system).freeze |
There was a problem hiding this comment.
Because that would auto-generate a none method which causes a conflict with ActiveRecord. I also don't want a prefix here, so I chose to go with the same approach that Version takes by manually managing the valid strings.
There was a problem hiding this comment.
@EinLama it's not well documented, but you can pass a scopes: false option to enum.
There was a problem hiding this comment.
shared_with would be an OK name or prefix - and make the setting slightly more obvious.
There was a problem hiding this comment.
(anyway, please feel free to ignore 😄 )
myabc
left a comment
There was a problem hiding this comment.
Hi @EinLama ,
Nice start!
I've made a few initial comments - mostly related to using built-in Rails APIs wherever possible and data integrity. Let me know if anything is unclear.
I've seen you are using Should matchers, but it might be worth perusing the docs as there are a bunch of options that can be passed to DRY up specs even further.
Thanks!
Alex
|
@EinLama it might also be worth running ActiveRecord doctor, which can be helpful in finding mismatches between database schema constraints and model validations, etc. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new namespaced Agile::Sprint ActiveRecord model (backlogs module) and wires WorkPackage to reference it via sprint_id, including journaling support so sprint assignments/removals show up in work package journals.
Changes:
- Add
Agile::Sprintmodel and backingsprintstable, plus basic validations. - Add
sprint_idtowork_packagesandwork_package_journals, and register sprint changes for journal formatting. - Add/extend specs and factories to cover sprint validations and work package sprint journaling.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
modules/backlogs/app/models/agile/sprint.rb |
Adds new Agile::Sprint model with validations and status enum. |
modules/backlogs/db/migrate/20260202093215_create_sprints.rb |
Creates sprints table. |
modules/backlogs/db/migrate/20260202150252_add_sprint_id_to_work_packages.rb |
Adds work_packages.sprint_id + index. |
modules/backlogs/db/migrate/20260202150253_add_sprint_id_to_work_package_journals.rb |
Adds work_package_journals.sprint_id. |
modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb |
Adds belongs_to :sprint association to WorkPackage. |
modules/backlogs/lib/open_project/backlogs/patches/project_patch.rb |
Adds has_many :sprints association to Project. |
app/models/work_package/journalized.rb |
Registers sprint_id for named-association journal formatting. |
app/contracts/work_packages/base_contract.rb |
Makes sprint_id writable via the base work package contract. |
config/locales/en.yml |
Adds error message + attribute labels for sprint/end date. |
modules/backlogs/spec/factories/sprint_factory.rb |
Adds :agile_sprint factory. |
modules/backlogs/spec/models/agile/sprint_spec.rb |
Adds model specs for Agile::Sprint. |
modules/backlogs/spec/models/project_spec.rb |
Adds association spec for Project#sprints. |
spec/models/work_package/sprint_journaling_spec.rb |
Adds journaling specs for assigning/changing/removing sprint on work packages. |
|
Thank you for the valuable feedback, @myabc . I took and applied most of it 👍 |
myabc
left a comment
There was a problem hiding this comment.
@EinLama thanks for the response to my first round of feedback!
Marking as "request changes" as it looks like we're using INTEGER rather than BIGINT. Using foreign keys is also good practice, but you might want a second opinion as we're not using them consistently acrosss the codebase.
|
|
||
| class AddSprintIdToWorkPackageJournals < ActiveRecord::Migration[8.0] | ||
| def change | ||
| add_column :work_package_journals, :sprint_id, :integer, default: nil, null: true |
There was a problem hiding this comment.
| add_column :work_package_journals, :sprint_id, :integer, default: nil, null: true | |
| add_column :work_package_journals, :sprint_id, :bigint, default: nil, null: true |
(I'm not sure if we want to use foreign key via add_reference)
There was a problem hiding this comment.
I wondered a bit about this 🤔
What should happen if a sprint gets deleted? For work packages, I figured the work package will live on and the sprint field is nullified. But for journals, I was wondering what will happen.
So I did some testing with versions. We allow deleting a version on database and model level, but the DeleteContract of versions does not allow you to delete a version that still has work packages assigned.
If you remove all work packages and then delete the version, the journal states: "Version deleted ()". So the specific version name is gone. But the information that there once was a version is preserved.
For the first iteration of sprints, I would expect them to behave similar to versions. With that, a nullable foreign key for the work package journal seems like the right choice 👍
cc @ulferts
|
Great point, thank you @myabc I have adjusted the migrations to use proper foreign keys. I initially went by the conventions that (EDIT: some) other modules established. But now I think it pays off to properly set up the relation in the database. It expresses what the de-facto relationship is, and improves performance, which should come in handy for the many work packages we will be querying during sprint planning. |
4f7e739 to
a949a6e
Compare
a949a6e to
8169725
Compare
|
Rebased target branch and this branch onto the latest |
8169725 to
d37bfaa
Compare
myabc
left a comment
There was a problem hiding this comment.
This looks good! - a bit more minor feedback, but good to merge when you're ready.
6054248
into
feature/70496-introduce-sprints-and-replace-versions-in-current-backlogs
Ticket
https://community.openproject.org/wp/71248
Please note that the target branch is not
dev. We are merging into the feature branch (that is based ondev) until we have a usable feature.What are you trying to accomplish?
Merge checklist