Skip to content

[#71645] Improve interface of WP identifier setting controller#22764

Merged
thykel merged 1 commit intodevfrom
feature/71654-semantic-converter-1
Apr 16, 2026
Merged

[#71645] Improve interface of WP identifier setting controller#22764
thykel merged 1 commit intodevfrom
feature/71654-semantic-converter-1

Conversation

@thykel
Copy link
Copy Markdown
Contributor

@thykel thykel commented Apr 14, 2026

Ticket

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

What are you trying to accomplish?

Replace the single update action (with boolean confirm_dangerous_action guard) with an explicit case dispatch to switch_to_semantic / switch_to_classic. Each branch enqueues the matching instance-level job and redirects; an unrecognised setting parameter yields 400.

The two new job classes are introduced as no-op stubs here; their real bodies land in the next two commits.

WorkPackages::IdentifierAutofix.job_in_progress? is updated to check GoodJob for the new job class names instead of the old ApplyHandlesJob (which is deleted here).

Screenshots

What approach did you choose and why?

Merge checklist

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the admin controller responsible for switching the Work Package identifier mode to use explicit dispatch (switch_to_semantic / switch_to_classic) and updates the background-job wiring accordingly (including adding new GoodJob-backed migration job stubs and updating the “job in progress” query).

Changes:

  • Replace the previous update flow/guard with an explicit case dispatch in Admin::Settings::WorkPackagesIdentifierController.
  • Introduce new instance-level migration job stubs (ConvertInstanceToSemanticIdsJob, RevertInstanceToClassicIdsJob) and update WorkPackages::IdentifierAutofix.job_in_progress? to check for them.
  • Add a controller spec covering the new update behavior (enqueue/redirect/400 scenarios).

Reviewed changes

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

Show a summary per file
File Description
spec/controllers/admin/settings/work_packages_identifier_controller_spec.rb New controller spec for the updated dispatch/enqueue behavior.
app/workers/project_identifiers/convert_instance_to_semantic_ids_job.rb New job stub for switching instance to semantic identifiers (with GoodJob concurrency config).
app/workers/project_identifiers/revert_instance_to_classic_ids_job.rb New job stub for reverting instance back to classic identifiers (with GoodJob concurrency config).
app/services/work_packages/identifier_autofix.rb Update job_in_progress? to look for the new job class names in GoodJob.
app/controllers/admin/settings/work_packages_identifier_controller.rb Refactor update into explicit branches that enqueue the corresponding jobs and handle invalid params as 400.
Comments suppressed due to low confidence (1)

app/workers/project_identifiers/convert_instance_to_semantic_ids_job.rb:36

  • The GoodJob concurrency limit is currently per job class (total_limit: 1 with the default key). That still allows ConvertInstanceToSemanticIdsJob and RevertInstanceToClassicIdsJob to run at the same time (one of each), which seems undesirable for mutually exclusive migrations and can still happen under concurrent requests. Consider setting an explicit shared key for both jobs (and using perform_limit: 1 if you only want one running at a time) so they are globally serialized.

Comment thread spec/controllers/admin/settings/work_packages_identifier_controller_spec.rb Outdated
Comment thread app/controllers/admin/settings/work_packages_identifier_controller.rb Outdated
@thykel thykel marked this pull request as ready for review April 14, 2026 21:27
@thykel thykel marked this pull request as draft April 14, 2026 21:28
@thykel
Copy link
Copy Markdown
Contributor Author

thykel commented Apr 15, 2026

@copilot Fill in the PR description again, respecting the template

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 15, 2026

@copilot Fill in the PR description again, respecting the template

Done — the PR description now fills in all template sections (Ticket, What are you trying to accomplish?, approach rationale including why switch_to_semantic intentionally defers the setting update, Screenshots, and Merge checklist).

@thykel thykel marked this pull request as ready for review April 15, 2026 20:57
@thykel thykel requested review from akabiru and judithroth April 15, 2026 21:33
Copy link
Copy Markdown
Member

@akabiru akabiru left a comment

Choose a reason for hiding this comment

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

:shipit:

Comment thread app/controllers/admin/settings/work_packages_identifier_controller.rb Outdated
@thykel thykel force-pushed the feature/71654-semantic-converter-1 branch from 6ead97a to cc475e6 Compare April 16, 2026 18:35
@thykel thykel merged commit 8d79fe1 into dev Apr 16, 2026
16 of 17 checks passed
@thykel thykel deleted the feature/71654-semantic-converter-1 branch April 16, 2026 19:13
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants