[#71645] Revert instance to classic identifiers

[thykel]

Ticket

https://community.openproject.org/projects/communicator-stream/work_packages/71645/activity

What are you trying to accomplish?

Implement a procedure for converting OP instance from classic identifier mode to semantic identifier mode.

Screenshots

What approach did you choose and why?

Similar to the previous PR, except much simpler:

1. Flip the instance setting
2. For all projects, revert the project identifier back to the previous classic state, OR generate a new one if no previous state present.

Merge checklist

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

[Copilot]

Pull request overview

Adds the missing “revert from semantic back to classic identifiers” flow for work package identifiers, complementing the existing semantic conversion jobs and exposing progress messaging in the admin settings UI.

Changes:

• Introduces instance-level and per-project GoodJob workers to revert semantic identifiers back to classic mode, with a batch success callback.
• Adds a RevertProjectToClassicService to clear semantic identifier data (WP identifiers/sequence numbers, aliases) and restore the project identifier from FriendlyId history.
• Updates admin UI in-progress banner messaging and adds new i18n strings; adds RSpec coverage for the new jobs/service.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
app/workers/project_identifiers/revert_project_to_classic_ids_job.rb	Adds per-project job that delegates to the revert service and discards on missing project
app/workers/project_identifiers/revert_instance_to_classic_ids_job.rb	Adds instance-level batch enqueueing of per-project revert jobs
app/workers/project_identifiers/finish_reverting_instance_to_classic_ids_job.rb	Adds GoodJob batch on_success callback to finalize classic mode
app/services/project_identifiers/revert_project_to_classic_service.rb	Implements the actual per-project revert logic (clear WPs, aliases, restore identifier, reset counter)
app/models/setting/work_package_identifier.rb	Adds enable_classic! helper to set the setting back to classic
app/components/work_packages/admin/settings/identifier_settings_form_component.rb	Adds helper to choose an in-progress banner message
app/components/work_packages/admin/settings/identifier_settings_form_component.html.erb	Switches banner rendering to use the new helper
config/locales/en.yml	Adds new i18n strings for converting/reverting in-progress banners
spec/workers/project_identifiers/revert_project_to_classic_ids_job_spec.rb	Adds job spec coverage for per-project revert job
spec/workers/project_identifiers/revert_instance_to_classic_ids_job_spec.rb	Adds job spec coverage for instance revert batching and callback
spec/services/project_identifiers/revert_project_to_classic_service_spec.rb	Adds service spec coverage for data cleanup and identifier restoration behavior

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

[thykel]

This step might be actually optional -- as in, if anyone does any further rename or move on a semantic project while in classic mode, the next conversion to semantic should pick this up correctly.

But to me this way currently seems a bit safer and more consistent, as we don't have to deal with potential validation issues when saving projects. For example, if we were to leave the identifiers untouched, someone editing a random field of a Project would currently trigger a validation error, as the identifier field would still be semantic, which doesn't pass the current validation rules.

It is easy to adjust anyway.

[judithroth]

I value what you want to do with this PR. It's good to have clean and "consistent" data in the database.
However

For example, if we were to leave the identifiers untouched, someone editing a random field of a Project would currently trigger a validation error

As far as I can see the validations for the identifier format all only trigger if the identifier was changed and are omitted if not. E.g.

    validate :identifier_numeric_format,
             if: ->(p) { p.identifier_changed? && p.identifier.present? && Setting::WorkPackageIdentifier.classic? }

[thykel]

True, that one is pretty easy to solve 🙂 I had multiple reasons to choose this road, but the main advantage here is that when we switch to classic with identifier conversion, we can completely "freeze" the complex alias management subsystem and then pick it up again when we switch to semantic.

If we were to support "leftover" semantic identifiers in the classic mode, we'd also have to correctly reflect any rename operations and do additional conversions when switching to semantic again.

So, I like that we get to "fence" most of this semantic logic within the opt-in feature.

[thykel]

😫 Too much nested PR management lately. Just accidentally merged #22804 into this one instead of main...

Lemme know if you want me to pluck it out again, otherwise you can just just carry on reviewing and skip the parts related to semantic validation rules.

[thykel]

Side note: The conversion to classic is currently missing the alert modal that pops up when switching to semantic, and instead of that, you just have the simple Save button. Let's leave that concern to a different PR.

[judithroth]

Thanks for adding this. As you said, it's not too much work and data consistency matters!
Do you want to address the issue with potential collisions (through using to_url instead of acts_as_url) directly or in a separate work package?

[judithroth]

I value what you want to do with this PR. It's good to have clean and "consistent" data in the database.
However

For example, if we were to leave the identifiers untouched, someone editing a random field of a Project would currently trigger a validation error

As far as I can see the validations for the identifier format all only trigger if the identifier was changed and are omitted if not. E.g.

    validate :identifier_numeric_format,
             if: ->(p) { p.identifier_changed? && p.identifier.present? && Setting::WorkPackageIdentifier.classic? }

[judithroth]

We allow projects with the same name. acts_as_url was able to handle this and generate unique names for them. This code here could lead to collisions though. I am aware that you just improved that by avoiding collisions when to_url would not work correctly... But still.

Is there no way to just trigger acts_as_url doing what it usually does to generate a url? As far as I understand it, acts_as_url will trigger when the project is saved without an identifier. And acts_as_url already cares about collisions and will still provide useful slugs:

test_1 = Project.create!(name: "Test", workspace_type: "project")
=> ...
test_2 = Project.create!(name: "Test", workspace_type: "project")
=> ...
test_1.identifier
=> "test"
test_2.identifier
=> "test-1"

(This is not blocking and can be addressed as a bug ticket as well if you prefer that 🙂
As a bug ticket, so we can still fix for the current release it in the feature stabilization phase)

[thykel]

Back then I couldn't figure out a way to call the acts_as_url logic directly, so I went for a reasonable simulation.

I like the option of actually falling back to it by un-setting the identifier, since that truly keeps the identifier generation logic in a single place. Will explore that via a follow-up PR.

[thykel]

Unfortunately acts_as_url (as well as our own wrappers) are hard-wired to only run this stuff as a before_create hook. Additionally, we do need to respect the historical allocations as well. Building a solution now.

[thykel]

=> #22857

[akabiru]

🍎 This update! runs with Setting already flipped to classic, so all the identifier validations kick in — uniqueness, identifier_numeric_format, and especially identifier_not_historically_reserved. Two realistic cases will bomb the whole revert job:

1. Name collision. When there's no classic slug to fall back to, Project.suggest_identifier(project.name) returns name.to_url.first(100) with no uniqueness suffix. Two "My Project" projects both try my-project and the second update! raises, aborting the find_each.
2. Historical-reservation collision. identifier_not_historically_reserved rejects any identifier that appears in some other project's friendly_id_slugs. If project A once held my-app and released it, restoring my-app onto project B (because it's B's most-recent classic slug too) raises.

Could we either dedupe against current + historical slugs in here (mirroring Projects::SemanticIdentifier#previous_semantic_identifier, which already excludes taken identifiers), or at least wrap the per-project call in a rescue so one bad project doesn't take down the whole instance revert?

[thykel]

Re-building this method in a follow-up PR.

[thykel]

=> #22857