Skip to content

[COMMS-916] Pin the primary target version convention to the lowest version id - #24486

Open
akabiru wants to merge 5 commits into
implementation/comms-916-adjust-cost-reports-to-read-from-target_versionsfrom
implementation/comms-916-primary-target-version-convention
Open

[COMMS-916] Pin the primary target version convention to the lowest version id#24486
akabiru wants to merge 5 commits into
implementation/comms-916-adjust-cost-reports-to-read-from-target_versionsfrom
implementation/comms-916-primary-target-version-convention

Conversation

@akabiru

@akabiru akabiru commented Jul 28, 2026

Copy link
Copy Markdown
Member

https://community.openproject.org/wp/COMMS-916

Follows #24432

This is an attempt to tighten deduction of the deprecated version_id primary version from a plain target_versions.first- which is not a consistent read to min(target_versions.id) that returns the version with the lowest ID consistently.

@akabiru akabiru self-assigned this Jul 28, 2026
@akabiru akabiru added this to the 17.8.x milestone Jul 28, 2026
@akabiru
akabiru force-pushed the implementation/comms-916-primary-target-version-convention branch from 14dc9b3 to 42a5c2f Compare July 29, 2026 10:58
@akabiru
akabiru marked this pull request as ready for review July 29, 2026 11:03
@akabiru
akabiru requested review from a team, brunopagno and thykel July 29, 2026 11:03
@github-actions

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:1:1:1]
  • rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:2:1:1]
  • rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2: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 #24486, linked for reference only):

- `rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:1:1:1]`
- `rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:2:1:1]`
- `rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]`

Treat this as a standalone task, unrelated to PR #24486. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24486 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 @akabiru 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 @akabiru, and request a review from @akabiru.
On every commit, set @akabiru 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 akabiru changed the title Implementation/COMMS-916: Pin the primary target version convention to the lowest version id [COMMS-916] Pin the primary target version convention to the lowest version id Jul 29, 2026
@akabiru
akabiru force-pushed the implementation/comms-916-primary-target-version-convention branch from 42a5c2f to 56488a4 Compare July 31, 2026 04:59
@github-actions

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./spec/features/projects/lists/columns_spec.rb[1:2:1:1]
  • rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:2]
🤖 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 #24486, linked for reference only):

- `rspec ./spec/features/projects/lists/columns_spec.rb[1:2:1:1]`
- `rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:2]`

Treat this as a standalone task, unrelated to PR #24486. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24486 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 @akabiru 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 @akabiru, and request a review from @akabiru.
On every commit, set @akabiru 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.

@brunopagno brunopagno left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the change in meaning here, using min instead of first. I have two thoughts:

  1. Are we introducing mixed behaviour? As in, are there other places in the codebase that rely on target_versions.first which we didn't adjust?

I think this is low risk, because even if we are using different approach in different places, all of them should have only a single version associated and consistently return the correct values.

  1. Introducing the concept of a "primary" target version can be misleading to whoever is maintaining this code in the future. Because we never defined it in documentation, and because essentially there is no such thing as a primary target version.

I think this is low risk as well. What we're calling primary here is more like a single value representation fallback of target versions, but less of a mouthful.

So all good to go from my side 🚢 🚢 🚢

@akabiru

akabiru commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

What we're calling primary here is more like a single value representation fallback of target versions, but less of a mouthful.

@brunopagno Indeed, "primary" is one word for it- definitely open to other better fitting options. I do think however, that's it warrants an identity as of it's been loosely inferred as the first target versions when the multiple versions feature is OFF.

akabiru added 5 commits August 5, 2026 16:09
The interim primary version is target_versions.first, which reads the
lowest version id, while the mirror column followed assignment order.
The two diverged whenever versions were assigned out of id order, so
legacy version_id readers disagreed with target_versions readers.
target_versions.first is the interim primary version. Unloaded reads
already order by id; an explicit order makes preloaded reads agree
instead of returning insertion order.
The mirror column now stores the lowest target version id, so the
contradiction check must compare against the same value; comparing
against the first assigned id rejected consistent writes and let
contradicting ones through whenever the order differed.
With leftover multi-target rows and the feature off, the version
column picked its representative row by name while the mirror column,
API and cost reports name the lowest version id; the same work package
grouped under different versions per surface. The groupable subquery
needs no versions join once the pick is by id.
@akabiru
akabiru force-pushed the implementation/comms-916-primary-target-version-convention branch from 56488a4 to 6ab962d Compare August 5, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants