[#73373] Background job for migrating pre-existing data to semantic identifiers#22566
[#73373] Background job for migrating pre-existing data to semantic identifiers#22566
Conversation
…convert-to-wp-semantic-ids
judithroth
left a comment
There was a problem hiding this comment.
This is coming along nicely! I'm sorry I found another edge case that keeps me from approving 😅
app/workers/project_identifiers/convert_instance_to_semantic_ids_job.rb
Outdated
Show resolved
Hide resolved
app/workers/project_identifiers/convert_instance_to_semantic_ids_job.rb
Outdated
Show resolved
Hide resolved
| # or by GoodJob as an on_success batch callback with (batch, params). | ||
| def perform(_batch = nil, params = nil) | ||
| iteration = params.to_h.with_indifferent_access.fetch(:iteration, 0).to_i | ||
| remaining = project_ids_needing_backfill |
There was a problem hiding this comment.
Okay, I needed a moment to understand what's going on here with the initial dispatch or callback 😅
You coupled these to catch all work packages and projects that were created while the job was running. That's hard to understand for someone not so deep into the topic and I am not sure how well this will work if we really have to iterate over all projects / work packages to catch the moves in the switch back-and-forth scenario.
I would prefer to handle those separately - in case of "the work package / project was created while the conversion migration was already running" we really can check for empty identifier values, in case of "initial dispatch" we can not. An extra "check and cleanup" method after the setting was flipped should be fast enough and easy to undertstand, no?
There was a problem hiding this comment.
Fair enough -- I will run a proper batched parallel processing for the first pass, and then just invoke a quick synchronous scanner in a dedicated final batch job.
spec/controllers/admin/settings/work_packages_identifier_controller_spec.rb
Outdated
Show resolved
Hide resolved
This reverts commit 7520e92.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
app/models/work_package/semantic_identifier.rb:103
- The removal of
privatemakesalias_rows_for_sequence_numbera public instance method onWorkPackage, which looks like an internal helper used only byallocate_and_register_semantic_id. Consider restoringprivate(or marking just this method private) to avoid accidentally expanding the public API surface.
# Builds alias rows for every identifier this project has ever used at the given sequence (including the current one).
# This also includes "ghost identifiers" -- i.e. those that weren't ever actually generated, but should work
# as a historical alias (e.g. OLDPROJ-42 should work even if WP #42 was created after rename to NEWPROJ)
def alias_rows_for_sequence_number(seq)
project.slugs
.pluck(:slug)
.map { |prefix| { identifier: "#{prefix}-#{seq}", work_package_id: id } }
end
spec/workers/project_identifiers/convert_project_to_semantic_ids_job_spec.rb
Outdated
Show resolved
Hide resolved
spec/workers/project_identifiers/convert_instance_to_semantic_ids_job_spec.rb
Show resolved
Hide resolved
app/services/project_identifiers/convert_project_to_semantic_service.rb
Outdated
Show resolved
Hide resolved
app/services/project_identifiers/convert_project_to_semantic_service.rb
Outdated
Show resolved
Hide resolved
app/services/project_identifiers/convert_project_to_semantic_service.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Ticket
https://community.openproject.org/projects/communicator-stream/work_packages/71645/activity
What are you trying to accomplish?
Add a background procedure to convert project & work package identifiers from
classic(projectcool_project, WP1) tosemantic(projectCP, WPCP-1).The procedure is invoked by switching to the semantic ("alphanumeric") identifier format via the admin settings.
Screenshots
What approach did you choose and why?
The original noop background job is now replaced by
ProjectIdentifiers::ConvertInstanceToSemanticIdsJob. This is the flow:semantic.ProjectIdentifiers::ConvertInstanceToSemanticIdsJobproj=>PROJ), simply set the the identifier mode tosemanticand exit with success. Otherwise, continue further.ProjectIdentifiers::BackfillProjectJob)Additionally, tweak the interface of the admin setting controller to have some proper shape -- previously it required presence of settings hash + a separate
confirm_dangerous_actionparam, now it only needs a properly set settings hash.Merge checklist