Bug/agile 306 backlogs non admin user can t sort or group by backlog bucket#24090
Draft
EinLama wants to merge 6 commits into
Draft
Conversation
b06df44 to
f2f2dae
Compare
f2f2dae to
9830dc2
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy 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. |
… names Defensive changes to avoid issues in the future.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes a regression where non-admin users could not group/sort work packages by backlog bucket when semantic work package identifiers are enabled, caused by a mismatch between Rails’ internal join aliasing and OpenProject’s alias prediction in Query::Results.
Changes:
- Pre-scan raw SQL join strings using a Rails-parity regex to align
Query::Results#include_aliaseswith Rails’AliasTrackeraliasing decisions. - Apply computed association aliases to non-association sort expressions (e.g., replacing hardcoded
projects.identifierwith the actual alias). - Remove an obsolete “explicit projects join” workaround from the project phase definitions query and add regression coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| spec/models/query/results_project_phase_filter_integration_spec.rb | Regression coverage ensuring the project phase filter join no longer needs the explicit projects join under semantic/classic identifiers. |
| spec/models/query/results_alias_tracker_parity_spec.rb | Spec that guards RAW_JOIN_TABLE_SCAN_REGEX parity against Rails’ AliasTracker.initial_count_for. |
| modules/backlogs/spec/models/query_results_group_by_bucket_spec.rb | Regression spec for AGILE-306 (non-admin grouping by backlog bucket under semantic identifiers). |
| app/models/query/results.rb | Core fix: raw-join pre-scan for alias prediction + alias substitution for non-association sortable expressions. |
| app/models/queries/project_phase_definitions/database_queries.rb | Removes the prior explicit LEFT OUTER JOIN "projects" workaround now made unnecessary by corrected alias handling. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/wp/AGILE-306
Grouping by buckets would fail for non-admin users if (and only if) semantic work package identifiers were enabled.
This happens because:
Query::Resultsalways eagerly loads the project association viaWorkPackage.includes(:project). Rails internally runs AliasTracker to decide whether to alias the resulting join. It does this by text-scanning every raw SQL string already in the query forJOIN ... "table" ONpatterns. If the table was seen at least once, Rails aliases its generated join — soprojectsbecomesprojects_work_packages. So far, no problem.The backlog bucket grouping adds a raw SQL join that embeds a permission subquery (projects_with_view_sprints.to_sql). For non-admin users, that subquery internally contains
INNER JOIN "projects" ON (member permission check). This references the projects table -> table was seen once. Admin users take a different code path that only joinsenabled_modules, so their subquery never mentions projects and aliasing never triggers -> projects table was not seen.For non-admin users, Rails aliases the outmost projects join to
projects_work_packages. As a result, the ORDER BY of the query breaks. The reason is that with semantic identifiers enabled, it will sort via a hardcoded project table name that is only defined deep down in the permission check subquery and thus unknown.Queries::Results#include_aliasestries to predict which alias will be used by Rails for each table name, in order to build the correct ORDER BY expressions. But, it misses table references inside raw SQL join strings, so our aliases and the aliases defined by Rails can diverge in some cases. Here, this diversion caused the bug.What approach did you choose and why?
Query::Results#include_aliasesnow pre-scans all raw SQL join strings (sort criteria joins, group-by join, filter joins) using the same regex pattern asAliasTracker, before computing aliases. This ensures its alias prediction matches what Rails will generate.The regex was copied from Rails (to
Query::Results::RAW_JOIN_TABLE_SCAN_REGEX). To avoid that a change to Rails silently breaks our application, there is a spec that directly compares our regex against the liveAliasTracker.initial_count_forimplementation, so any future change to the Rails implementation will cause it to go red.Query::Results#aliased_sorting_by_column_namenow also applies the computed aliases to non-association sort expressions — replacing hardcoded table names likeprojects.identifierwith their actual aliases likeprojects_work_packages.identifier.As a drive-by cleanup, the project phase definitions join previously worked around the same aliasing issue by adding an explicit
LEFT OUTER JOIN "projects"to keep the table accessible regardless of the alias. This workaround could now be removed 👏Merge checklist