The :only_changes filter re-seeked each journal's predecessor in every EXISTS
branch (~7 LATERAL lookups per row). A CTE now shadows the journals table,
exposing predecessor_id/predecessor_data_id once per row, and each branch reads
those columns instead. On a 703-journal work package this cuts the COUNT from
~1.13M to ~35K shared buffers.
The LATERAL subquery is aliased `predecessor` where it is joined rather
than inside the helper, so the relation each EXISTS clause references is
visible without reading the helper.
The :only_changes activity filter identified each journal's predecessor
with `version = (SELECT MAX(version) WHERE version < current)`. That
predicate cannot use the (journable_type, journable_id, version) index,
so Postgres scanned every journal of the journable and filtered by
version — turning a per-page filter into an O(history) sweep run twice
(pagy's count plus the page query). A LATERAL `ORDER BY version DESC
LIMIT 1` seeks the predecessor through that index in a single row,
preserving gap-tolerant matching on `< version`.
https://community.openproject.org/wp/STC-462
Two readability passes over the work package activity tab, no behaviour change. The paginator's private methods are reordered to follow their call order so the file reads top-down from `#call`, and the three activity filter modes (`:all`, `:only_comments`, `:only_changes`) — until now bare symbols duplicated across the controller, paginator, journal components and the hidden form — move into a single `WorkPackages::ActivitiesTab::Filters` module so the modes have one source of truth and can't drift apart. The diff reaches beyond the paginator into the controller, several components and a form, since that's where the symbols were scattered.
The Paginator.paginate class method bypassed the instance, discarding the
resolved_anchor state the controller reads after .call; it had no callers, so
drop it and keep the single new(...).call entry point. Extract the activity
anchor side effect into record_resolved_anchor so the intent is explicit at the
call site, and pin the server contract with a request spec asserting an
unresolvable activity anchor omits the resolved-comment value.
The work package activity tab computed a per-journal sequence_version on
every render — a ROW_NUMBER() window function over a LATERAL join — only to
stamp the legacy data-anchor-activity-id that #activity-N deep links rely on.
Nothing mints those links anymore; copy and share links use
#comment-<journal id>, which needs no extra query.
The activity number is now resolved on demand. Only a request carrying
?anchor=activity-N runs the window function, mapping the number to a journal
id the paginator exposes as resolved_anchor. The view hands that to the
client, which rewrites #activity-N to the canonical #comment-<id> and scrolls
using the comment anchor already present in the DOM. Default renders no longer
touch the window function.
References WP #68063.
The earlier write-time canonicalization stored the rendered
display_id ("#PROJ-7") in the journal note, which would rot under
project-identifier renames and leave dangling semantic strings if
semantic mode were rolled back. Restore the PK shape ("#42") and
let the formatter pipeline turn it into the user-facing identifier
at render time, where the resolver already handles both modes.
Both spec contexts now assert the same PK shape; the mode-specific
rendering of "#N" lives in the formatter specs.
The macro preload was visibility-scoped — references to work packages the
recipient cannot see would fall through to the literal `#43` shape, even
when the same reference rendered as `DCP-1` for an author with full view
permission. Notification recipients saw misleading numeric ids for cross-
project references in journal notes.
Splits label resolution from link gating:
- `ResourceLinksMatcher.build_lookup` now does an unscoped fetch for the
primary identifier and a separate visibility-scoped id pluck. The link
handler reads `visible_to_current_user?` to decide between a navigable
anchor and a plain-text label.
- `UpdateAncestorsService#set_journal_note` writes `#display_id` so new
notes carry the semantic shape at the source; render-time resolution
heals legacy `#N` content for users with view permission.
Tradeoff: a recipient without view permission now sees the WP's semantic
identifier (e.g. `DCP-1`) as plain text rather than `#43`. The reference's
existence was already disclosed by the stored journal text; the project
identifier is the only new piece of information surfaced, and is not
treated as a secret elsewhere in the system (URLs, exports, API).
Pagy 43.4.3 deprecated `:max_pages` and recommends capping records
before pagination instead. Caps the combined array in `base_journals`.
https://community.openproject.org/wp/75314
clear_changes_information would also clobber any unrelated dirty
attributes the work package happened to be carrying at this point.
The intent is narrower: only the two attributes we just synced from
the raw-SQL UPDATE should be marked clean. Use clear_attribute_changes
with the explicit attribute list instead.
clear_changes_information is the established pattern in this codebase
for syncing in-memory dirty state after a side-channel persist (queries,
historic-attribute eager loading, progress/date-picker controllers).
changes_applied has the same effect but no other call site, so prefer
the local convention.
Previous fix reloaded each moved work package after the bulk SQL
UPDATE so HAL representers and the move-and-follow redirect saw the
freshly allocated semantic id. That cost one round trip per WP, which
matters when a parent moves with descendants. Now that
reserve_semantic_id_block! returns the {wp_id => identifier}
assignments directly, apply them with assign_attributes +
changes_applied — same observable result, zero extra queries
regardless of bulk size.
Project#reserve_semantic_id_block! rewrites work_packages.identifier
and sequence_number via a raw SQL UPDATE, leaving the in-memory
records with the nil identifier set by SetAttributesService when the
project changed. Callers that read the WP straight out of the
ServiceResult (HAL action links, the move-and-follow redirect in
WorkPackages::BulkJob#redirect_path) then saw an empty display_id and
fell back to numeric URLs. Reload after the bulk allocation so the
in-memory state matches the database.
The class method .reserved_identifiers returns all slugs (current +
historical) for use as an exclusion set. The private instance method
only returns historical slugs no longer active. Having both share
the same name with different semantics is a maintenance trap.
Rename the private method to historical_identifiers so exclusion_set
reads naturally: historical_identifiers | in_use_identifiers.
The friendly_id_slugs table already contains both current and historical
project identifiers (backfilled by InitializeHistoricIdentifiers migration,
maintained by FriendlyId history module on create/update). Query it
directly via ProblematicIdentifiers.reserved_identifiers instead of
merging Project.pluck with a filtered slug query.
Remove SemanticIdentifierGenerator wrapper class and move collision-aware
logic directly into Projects::Identifier.suggest_identifier. This gives
both the model callback and the frontend suggestion controller automatic
collision detection.
Promote reserved_identifiers to a class method on ProblematicIdentifiers
since it has no instance state dependency.
This error is intended for cases when a method is
intentionally not implemented, because the module/class defining
it expects a subclass (or class including the module) to implement
the method.
This is intended to distinguish it from other cases, such as:
* feature not implemented yet
* edge case of a method call not yet supported
Notably it avoids the misuse of the Ruby-defined NotImplementedError,
which is only intended for much more specific scenarios:
> Raised when a feature is not implemented on the current platform. For example, methods depending on the fsync or fork system calls may raise this exception [...]
Also see https://docs.ruby-lang.org/en/master/NotImplementedError.html
The suggestion generator produces uppercase candidates and checks
exclusions with case-sensitive `include?`. Historical slugs stored in
lowercase (e.g. "proj") would not block the generator from suggesting
"PROJ", even though `identifier_not_historically_reserved` would reject
it on save via LOWER(). Upcasing the exclusion set at the consumption
point keeps ProblematicIdentifiers#exclusion_set casing-neutral for
other consumers while ensuring the preview never proposes identifiers
that collide with historical slugs.
Unify test helpers into a single create_project_with_raw_identifier
method that better communicates intent (bypasses validations to set
an exact identifier). Add project.reload after update_all so the
in-memory object stays consistent with the database.
Also documents that the migration rollback is intentionally lossy —
deduplicated identifiers keep their suffixes under the restored
case-sensitive index.
After the background autofix job converts identifiers from lowercase
to uppercase, historical slugs in friendly_id_slugs will differ in
case from current project identifiers. Without LOWER() in the NOT IN
subquery, those slugs are incorrectly included in the reserved set,
causing the suggestion generator to over-reserve.
Example: a project created in numeric mode gets identifier "proj" and
FriendlyId records slug "proj". The autofix job later uppercases it to
"PROJ". Now the case-sensitive `slug NOT IN (SELECT identifier ...)`
sees "proj" != "PROJ" and incorrectly treats "proj" as reserved,
preventing the suggestion generator from offering it.
The underscore fix (allowing _ in identifiers per spec) needs to target
ProblematicIdentifiers instead of PreviewQuery after the extraction
refactor on the target branch.
Drop the DB-backed ExclusionSet in favour of a simple Set from pluck.
This is a one-off admin migration — the brief memory cost of loading
all non-problematic identifiers is not worth the added complexity of
a custom duck-typed wrapper.
Also adds performance notes documenting index considerations for the
regex scope conditions and the eager-load trade-off.
Replace the TODO stub in ProblematicIdentifiers#reserved_identifiers
with a real query against the friendly_id_slugs table. Historical slugs
(identifiers a project used in the past but has since changed) are now
excluded from suggestion generation and classified as :reserved in
error_reason output.
The query excludes slugs that match a current active project identifier
(those are already covered by the in_use exclusion path).
Separate scope-building, identifier classification, and exclusion set
logic into a reusable class that both PreviewQuery (admin UI) and the
future ApplyHandlesJob (batch migration) can compose from.
Key changes:
- ProblematicIdentifiers owns FORMAT_RULES, problematic scope, error
classification, and a dual-mode exclusion set (DB-backed ExclusionSet
for preview, preloaded Set for batch)
- PreviewQuery becomes a thin orchestrator delegating to
ProblematicIdentifiers
- Add deterministic ordering (.order(:id)) to preview results
- Rename :not_uppercase → :not_fully_uppercased to match scope method
naming, update corresponding en.yml translation key
- ExclusionSet uses raw SQL to bypass Rails normalizes on :identifier
which would transform query parameters based on current setting mode
Expand PreviewQuery to detect additional alphanumeric identifier
format violations: case-mismatch, underscores, leading digits, and
purely numerical identifiers. Restructure error classification with
FORMAT_RULES for clarity and add corresponding locale strings.
Update MCP tool descriptions for search_projects, search_programs,
and search_portfolios to reflect case-insensitive identifier
matching, with updated test expectations.
Replace the separate `reserved_identifiers` and `in_use_identifiers` keyword arguments with a single `exclude` parameter.
The generator immediately merged these two sets on every call path -- the distinction only matters to `PreviewQuery` for error classification, which it already handles independently. Collapsing them into one parameter removes unnecessary coupling.
- Replace scan+join with gsub for stripping non-alphanumeric chars
- Use string slicing instead of .chars arrays throughout multi-word
pipeline, eliminating intermediate array allocations per word
- Merge two-pass filter_map+select into single filter_map
- Replace index.with_index enumerator with each_index.find
- Simplify combined_identifiers from splat+reduce to set union
- Flatten multi-word candidate generation from 5 methods to 4 by
removing unnecessary indirection layers
- Apply ensure_starts_with_letter in numeric_suffix_fallback so
fallback identifiers also satisfy the starts-with-letter constraint
- Add test verifying batch mode assigns identifiers in array order
- Replace numeric suffix collision strategy with progressive acronym
widening ("SC" → "STC" → "STCO" instead of "SC" → "SC2" → "SC3")
- Allow underscores in identifiers (fix regex in PreviewQuery)
- Enforce identifiers must start with a letter (strip leading digits)
- Use DEFAULT_IDENTIFIER_BASE_LENGTH (5) for initial generation with
MAX_IDENTIFIER_LENGTH (10) as expansion ceiling for collisions
- Enforce MIN_IDENTIFIER_LENGTH (2) for generated identifiers
- Expand single-word identifiers on collision ("BAN" → "BANA" → "BANAN")
Aligns the class name and all internal terminology with the domain
language: "handle" → "identifier" throughout. Renames the file, class,
constants (HANDLE_MAX_LENGTH → IDENTIFIER_MAX_LENGTH, FALLBACK_HANDLE →
FALLBACK_IDENTIFIER), public API (suggest_handle → suggest_identifier,
suggested_handle hash key → suggested_identifier), keyword arguments
(in_use_handles → in_use_identifiers, reserved_handles →
reserved_identifiers), and private helper methods accordingly. All
call-sites and specs updated to match.
Kabiru Mwenja