Implementation/73146 update admin flow for sematic work package identifiers against project historical identifiers model#22442
Conversation
akabiru
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors the Work Package identifier autofix preview pipeline by extracting identifier scope/classification logic into a reusable ProblematicIdentifiers service, and updates reserved identifier detection to use FriendlyId slug history (historical project identifiers).
Changes:
- Extracted identifier scope, classification (
error_reason), and exclusion building intoWorkPackages::IdentifierAutofix::ProblematicIdentifiers. - Updated
PreviewQueryto delegate scope/count/classification/exclusions toProblematicIdentifiers. - Updated i18n + specs to reflect the renamed error reason (
:not_fully_uppercased) and added dedicated unit tests for the new service.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/services/work_packages/identifier_autofix/problematic_identifiers.rb |
New shared service for problematic-project scoping, error classification, and exclusion set (incl. FriendlyId history). |
app/services/work_packages/identifier_autofix/preview_query.rb |
Simplifies preview orchestration by delegating analysis and exclusions to ProblematicIdentifiers. |
spec/services/work_packages/identifier_autofix/problematic_identifiers_spec.rb |
New unit coverage for ProblematicIdentifiers behavior (scope, count, reasons, exclusion set). |
spec/services/work_packages/identifier_autofix/preview_query_spec.rb |
Aligns preview specs with extracted analysis service and renamed error reason. |
config/locales/en.yml |
Renames translation key for the uppercase-related error reason. |
| def reserved_identifiers | ||
| @reserved_identifiers ||= FriendlyId::Slug | ||
| .where(sluggable_type: Project.name) | ||
| .where.not(slug: Project.select(:identifier)) | ||
| .pluck(:slug) | ||
| .to_set |
There was a problem hiding this comment.
reserved_identifiers is collected case-sensitively, but Projects::Identifier#identifier_not_historically_reserved checks slug history case-insensitively (LOWER comparison). This can lead to the suggestion generator proposing an identifier that later fails validation due to a differently-cased historical slug. Consider normalizing both sides (e.g., pluck lower/upper-cased slugs and compare against normalized project identifiers, and normalize the identifier passed into reserved_identifiers/include? checks accordingly).
| RSpec.describe WorkPackages::IdentifierAutofix::ProblematicIdentifiers do | ||
| subject(:analysis) { described_class.new } | ||
|
|
||
| let(:max_length) { WorkPackages::IdentifierAutofix::ProjectIdentifierSuggestionGenerator::IDENTIFIER_LENGTH[:max] } |
There was a problem hiding this comment.
let(:max_length) is defined but never used in this spec. This can trip RSpec/UnusedLet and adds noise—either remove it or use it in the expectations (e.g., build an identifier based on max_length).
| it "returns :reserved when identifier is a historical slug of another project" do | ||
| project = create_valid_project(name: "Gamma", identifier: "GAMMA") | ||
| FriendlyId::Slug.create!(slug: "OLDIE", sluggable: Project.find(project.id), sluggable_type: "Project") | ||
|
|
||
| expect(analysis.error_reason("OLDIE")).to eq(:reserved) |
There was a problem hiding this comment.
The example description says the slug is from “another project”, but the test creates the slug on the same project it just created. Please adjust the wording or create a second project so the spec matches the scenario being described.
akabiru
merged commit 6c84213
into
open-point/73149-how-do-we-handle-project-identifiers-case-insensitive-storage
akabiru
force-pushed
the
open-point/73149-how-do-we-handle-project-identifiers-case-insensitive-storage
branch
from
0738f98 to
5c7bed1
Compare
akabiru
deleted the
implementation/73146-update-administration-setting-for-sematic-work-package-identifiers-ui-against-project-historical-identifiers-model
branch
Ticket
https://community.openproject.org/work_packages/73146
What are you trying to accomplish?
Extract
ProblematicIdentifiersand wire up historical identifier exclusion.Prepare the identifier autofix pipeline for the batch migration job by extracting shared logic from
PreviewQueryinto a reusableProblematicIdentifiersclass, and wiring up reserved identifier detection against real FriendlyId slug history.What approach did you choose and why?
PreviewQuerymixed orchestration with scope-building and classification logic. Since the batch migration job will need the same scope and classification, extractingProblematicIdentifiersavoids future duplication.Reserved identifiers query
friendly_id_slugsfor historical project slugs that don't match any current active identifier -- these are "retired" identifiers locked to their original project by theidentifier_not_historically_reservedvalidation.Merge checklist
Added/updated documentation in Lookbook (patterns, previews, etc)N/A -- backend-onlyTested major browsers (Chrome, Firefox, Edge, ...)N/A -- no UI changes