STC-847 Include work package associated versions model by brunopagno · Pull Request #23841 · opf/openproject

brunopagno · 2026-06-22T12:28:02Z

Ticket

https://community.openproject.org/projects/STC/work_packages/STC-847/activity

What are you trying to accomplish?

Establish a new database model to enable building multiple versions fields on work packages

What approach did you choose and why?

Create db table
Define association methods on WorkPackage and Version models
Include tests
Include a translation because somehow the test was expecting it

Merge checklist

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

brunopagno · 2026-06-22T12:29:08Z

@opf/communicator how do you feel about merging a database change that has no impact on the application? How do you feel about merging this to dev vs developing the whole feature on a feature branch?

thykel · 2026-06-22T12:36:31Z

@opf/communicator how do you feel about merging a database change that has no impact on the application? How do you feel about merging this to dev vs developing the whole feature on a feature branch?

+1 for merging on my part.

We're still far away from a code freeze
The change is pretty innocent, since it's just a DDL for a new table. So, even if we follow up with more migrations, they aren't likely to significantly slow down the migration process (as we're not touching any pre-existing & potentially large table).

thykel · 2026-06-22T13:53:50Z

+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+class WorkPackageAssociatedVersion < ApplicationRecord


How about making this a tad lighter?

Suggested change

class WorkPackageAssociatedVersion < ApplicationRecord

class WorkPackageVersion < ApplicationRecord

Either that, or lean in the opposite direction and make the naming more descriptive -- since the table stores associations, not versions.

Suggested change

class WorkPackageAssociatedVersion < ApplicationRecord

class WorkPackageVersionAssociation < ApplicationRecord

Good point. I prefer WorkPackageVersion, so I'll check that option 👍

thykel · 2026-06-22T13:58:13Z

+  has_many :work_package_associated_versions, dependent: :delete_all
+  has_many :work_packages_target_versions,
+           -> { where(work_package_associated_versions: { kind: "target" }) },
+           through: :work_package_associated_versions, source: :work_package
+  has_many :work_packages_observed_in_versions,
+           -> { where(work_package_associated_versions: { kind: "observed_in" }) },
+           through: :work_package_associated_versions, source: :work_package


These should be inverted, right? Something like:

Suggested change

has_many :work_package_associated_versions, dependent: :delete_all

has_many :work_packages_target_versions,

-> { where(work_package_associated_versions: { kind: "target" }) },

through: :work_package_associated_versions, source: :work_package

has_many :work_packages_observed_in_versions,

-> { where(work_package_associated_versions: { kind: "observed_in" }) },

through: :work_package_associated_versions, source: :work_package

has_many :associated_work_packages, dependent: :delete_all

has_many :targeted_work_packages,

-> { where(work_package_associated_versions: { kind: "target" }) },

through: :associated_work_packages, source: :work_package

has_many :observed_in_work_packages,

-> { where(work_package_associated_versions: { kind: "observed_in" }) },

through: :associated_work_packages, source: :work_package

thykel · 2026-06-22T14:03:36Z

+  belongs_to :work_package
+  belongs_to :version
+
+  validates :kind, presence: true, inclusion: { in: VERSION_KINDS }


Shall we throw in the uniqueness validation, since it's already there at the index level?

You mean validating the combination of kind + version_id + work_package_id?

I think it is okay for clarity when looking at the model, but I assume rails will do slower shenanigans to validate such constraint (as opposed to the index validation directly on the DB). I don't think there's a lot of benefit of doing both

In the end I did not include this one. If you feel strongly let me know and I'll check again.

thykel · 2026-06-22T14:04:21Z

+#++
+
+class WorkPackageAssociatedVersion < ApplicationRecord
+  VERSION_KINDS = %w[target observed_in].freeze


I'd consider a modern enum here. 👀

It would mean changing the DB field to an integer. I don't feel too strongly about the options here. We get rails goodies at the expense of clarity on the DB itself.

class WorkPackageVersion < ApplicationRecord # VERSION_KINDS = %w[target observed_in].freeze enum :kind, { target: 0, observed_in: 1 }, validate: true belongs_to :work_package belongs_to :version # validates :kind, presence: true, inclusion: { in: VERSION_KINDS } end

I preferred enum, so switched to it. Thanks for the suggestion 🙇

It would mean changing the DB field to an integer. I don't feel too strongly about the options here. We get rails goodies at the expense of clarity on the DB itself.

That's the neat part, the "modern" Postgres enums are also enums in the database! [related guide]

The usual downside is that adding a new enum value requires a DB migration -- but in this case, this is not likely to get touched very often.

🍏 nit: Apparently (IIRC from Wieland) "string" enums are preffered to integers for their speaking nature in db exports, whenever those happen. However, doing a quick search across enum definitions does not support this 🤷🏾

@akabiru that's also a good point. If we have previously made such decisions, then I need to rollback to a text field.

I'll poke around a bit further following the guide that @thykel linked 👀

github-actions · 2026-06-23T08:27:03Z

Warning

Flaky specs

rspec ./spec/features/projects/lists/filters_spec.rb[1:17:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23841, linked for reference only):

- `rspec ./spec/features/projects/lists/filters_spec.rb[1:17:1:1]`

Treat this as a standalone task, unrelated to PR #23841. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23841 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @brunopagno to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @brunopagno, and request a review from @brunopagno.
On every commit, set @brunopagno as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

akabiru · 2026-06-23T08:35:07Z

+    it "is valid with kind 'target'" do
+      record.kind = "target"
+      expect(record).to be_valid
+    end
+
+    it "is valid with kind 'observed_in'" do
+      record.kind = "observed_in"
+      expect(record).to be_valid
+    end


🍏 can be simplified- assumes string backed enum (no strong preference)

Suggested change

it "is valid with kind 'target'" do

record.kind = "target"

expect(record).to be_valid

end

it "is valid with kind 'observed_in'" do

record.kind = "observed_in"

expect(record).to be_valid

end

it do

expect(subject).to define_enum_for(:kind)

.with_values(target: "target", observed_in: "observed_in")

.backed_by_column_of_type(:string)

end

judithroth · 2026-06-23T09:43:54Z

-  def permitted_params
-    RequestStore.fetch(:permitted_params) { PermittedParams.new(params, User.current) }
-  end
+class WorkPackageVersion < ApplicationRecord


You could name this "WorkPackage::Version". (Then the file goes to app/models/work_package/version). This helps to keep the global namespace overseeable.

I like the idea of scoping models, but I don't like having two models called version.rb. That and the fact that WorkPackage & Version are both core models of OpenProject. We could discuss for example if the model should be scoped the other way around Version::WorKPackage.

So at this moment I'd prefer to keep it work_package_version.

Open to being convinced otherwise, though.

Since WorkPackageVersion is a join table, I don't think the namespacing semantics apply here, and we're better off keeping it as-is.

With that said, it would be nice to pin down a single specific convention in place for these join tables -- AFAIK this is one of the two that are currently in use (= just concatenating the names of both models).

github-actions · 2026-06-23T10:39:09Z

Warning

Flaky specs

rspec ./modules/my_page/spec/features/my/work_package_table_spec.rb[1:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23841, linked for reference only):

- `rspec ./modules/my_page/spec/features/my/work_package_table_spec.rb[1:1:1]`

Treat this as a standalone task, unrelated to PR #23841. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23841 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @brunopagno to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @brunopagno, and request a review from @brunopagno.
On every commit, set @brunopagno as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

thykel

🚀 🚀

brunopagno · 2026-06-23T11:47:50Z

@judithroth @akabiru since this is not easilly reversible if I press merge, I'll wait for your follow up before pressing the button.

akabiru

akabiru · 2026-06-23T13:47:58Z

-    end
+class CreateWorkPackageVersions < ActiveRecord::Migration[8.1]
+  def change
+    create_enum :work_package_version_kind, ["target", "observed_in"]


TIL! Nice one, cheers Tom! #23841 (comment)

brunopagno self-assigned this Jun 22, 2026

brunopagno requested a review from a team June 22, 2026 13:07

thykel reviewed Jun 22, 2026

View reviewed changes

include work package associated versions model

4826e52

brunopagno force-pushed the impl/STC-847-multiple-versions-data branch from 2922d47 to 4826e52 Compare June 23, 2026 08:08

brunopagno requested a review from thykel June 23, 2026 08:09

akabiru reviewed Jun 23, 2026

View reviewed changes

use text field to persist enum data

d375c36

brunopagno force-pushed the impl/STC-847-multiple-versions-data branch from e90ea69 to d375c36 Compare June 23, 2026 09:35

judithroth reviewed Jun 23, 2026

View reviewed changes

brunopagno requested review from akabiru and judithroth June 23, 2026 10:42

thykel approved these changes Jun 23, 2026

View reviewed changes

akabiru approved these changes Jun 23, 2026

View reviewed changes

brunopagno merged commit ebf6d9d into dev Jun 23, 2026
15 of 16 checks passed

brunopagno deleted the impl/STC-847-multiple-versions-data branch June 23, 2026 14:01

github-actions Bot locked and limited conversation to collaborators Jun 23, 2026

	class WorkPackageAssociatedVersion < ApplicationRecord
	class WorkPackageVersion < ApplicationRecord

	class WorkPackageAssociatedVersion < ApplicationRecord
	class WorkPackageVersionAssociation < ApplicationRecord

Conversation

brunopagno commented Jun 22, 2026

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Merge checklist

Uh oh!

brunopagno commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thykel commented Jun 22, 2026

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!

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!

brunopagno Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

akabiru Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

github-actions Bot commented Jun 23, 2026

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!

thykel Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 23, 2026

Uh oh!

thykel left a comment

Choose a reason for hiding this comment

Uh oh!

brunopagno commented Jun 23, 2026

Uh oh!

akabiru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

brunopagno commented Jun 22, 2026 •

edited

Loading

brunopagno Jun 23, 2026 •

edited

Loading

akabiru Jun 23, 2026 •

edited

Loading

thykel Jun 23, 2026 •

edited

Loading