Conversation
f85b12b to
ff84a0b
Compare
Adds the admin settings page for choosing between numeric and project-based alphanumeric work package identifiers (#72461): - ProjectHandleSuggestionGenerator service that scans projects for identifiers that don't meet alphanumeric handle requirements and generates uppercase acronym suggestions (e.g. "PROJ" from "My Project") - IdentifierAutofixPreviewComponent: Primer BorderBox table showing the first 5 problematic projects with their suggestions and a "... N more" footer row - work-packages-identifier Stimulus controller that shows/hides the autofix section and updates the submit button label on radio change - Updated IdentifierSettingsFormComponent wired to the Stimulus controller, showing the warning banner and preview table when "Project-based alphanumerical identifiers" is selected
- ProjectHandleSuggestionGenerator: replace Project.all.to_a + Ruby-side filter with a SQL-filtered, column-minimal query (select :id/:name/:identifier, WHERE length > 10 OR non-alphanumeric). Adds FIXME(project_handles) markers showing the exact ProjectHandle query to swap in once the data model exists. Adds inline docs explaining the unique_handle collision-resolution algorithm. - IdentifierSettingsFormComponent template: restructure for full-width banner and table. The form (radio buttons only) stays inside the 680px settings_primer_form_with wrapper and gets id="wp-identifier-settings-form". The autofix section (banner + table) is a sibling div outside the wrapper. The submit button lives in its own 680px wrapper and links back to the form via the HTML5 form="wp-identifier-settings-form" attribute.
- handle_from_name: replace /[a-zA-Z0-9]+/ with /[[:alpha:][:digit:]]+/ so accented letters (é, ñ, ü…) are kept inside their word rather than treated as separators. "Cécile Martin" now produces "CM" instead of "CCM". Transliterate each word's first character via I18n.transliterate (consistent with app/models/exports/exporter.rb) before uppercasing, so non-ASCII initials map to their ASCII equivalent (é→E, ñ→N). filter_map silently drops any initial that produces no usable character after transliteration. - Service spec: remove the receive_message_chain stub entirely. All fixtures are now real create(:project, ...) records so the SQL query is exercised against the test DB. shared_let used where the same record backs multiple it-blocks; inline create for single-assertion tests. Two new Unicode examples: "Cécile Martin" → "CM" and "étude de cas" → "EDC".
Replace the hardcoded '-1' suffix with a deterministic sample number derived from the handle string (range 1–500, zero-padded for single digits). Each handle consistently produces the same example ID on every render, but IDs look naturally varied across different projects.
Adds FIXME(project_handles) markers in the suggestion generator, preview component, and i18n file to document the two error cases that require the project_handles model before they can be implemented: - :handle_reserved — identifier already stored in project_handles for another project (current or historical); all handle values route permanently to their owning project and cannot be reassigned - :identifier_taken — identifier is a valid format and will be auto-adopted as another project's handle during migration, making it unavailable for this project FIXME comments cover: - Class-level doc and @return YARD tag in the generator - #call: explains the future query naturally catches both new cases - #generate_suggestions: shows DB pre-seed needed for used_handles - #error_reason: stubs the two new branches with priority order - IdentifierAutofixPreviewComponent#error_label: stubs two new whens - en.yml: placeholder copy for error_handle_reserved / error_identifier_taken
78afe9f to
3baf3cc
Compare
IdentifierAutofixPreviewComponent → IdentifierAutofixSectionComponent. The banner (existing_identifiers_notice) and the preview table always appear together, so they are now co-located in a single component. The parent template renders one component instead of a banner + separate component. Changes: - git mv preview → section for all three files (rb, erb, spec) - Add @total_count to the initializer for the banner's project_count - Template: banner rendered above the border-box table - Parent template: remove inline banner render, call SectionComponent - Spec: update described_class; add banner count assertion
Three issues corrected: 1. Bug fix: ALLOWED_VALUES is an Array; ALLOWED_VALUES[:alphanumeric] raises TypeError at runtime (symbols are not valid Array indices). Extract named string constants NUMERIC and ALPHANUMERIC so comparisons are explicit. 2. Lazy-load guard: ProjectHandleSuggestionGenerator ran a DB query on every component render, even in numeric mode where the result is never used. Now the query only runs when alphanumeric? is true; numeric mode gets []. 3. show_autofix_section? simplified: the alphanumeric? guard moved into the initializer, so the private method is now just projects_data.any?. Also: wrap definition.rb's `allowed:` in a lambda to defer constant resolution past Rails autoload (fixes a load-order error in specs), and add a spec for Setting::WorkPackageIdentifier covering the bug scenario.
The autofix section must be pre-rendered with data on every page load, even when the current setting is 'numeric'. The Stimulus controller reveals the section as soon as the user selects 'alphanumeric' — before the form is saved — so the table and banner data must already be in the DOM. show_autofix_section? is restored to guard on alphanumeric? so the section is hidden initially in numeric mode but still populated.
ce8b5a5 to
20b7a28
Compare
Documents and pins the behaviour for scripts without transliteration
entries (Japanese, Chinese, Arabic, …):
- Fully non-Latin name (e.g. "日本語プロジェクト"): every initial
maps to "?" via I18n.transliterate, filter_map drops them all,
empty acronym falls back to FALLBACK_HANDLE ("PROJ").
- Mixed name (e.g. "Plan 日本"): Latin initials survive, non-Latin
ones are silently dropped, result is the Latin-only acronym ("P").
Addresses code review: the loop in unique_handle had no upper bound. Adds SUFFIX_LIMIT = 10_000 and raises if the counter exceeds it. 10 000 projects sharing one acronym is not reachable in practice; hitting the limit would indicate a bug in used_handles pre-seeding. The other three review points were false positives: - Module name WorkPackages:: is correct for this feature's domain - SQL ? binding sanitises the regex pattern (no injection surface) - upcase[0] nil case is already handled by ch&.match? on the next line
8f170f3 to
5f83f29
Compare
Replace the two-component approach (IdentifierSettingsFormComponent + IdentifierChangeInProgressComponent) with a single component cycling through three lifecycle states: :edit, :change_in_progress, :completed.
- Swap wrapper_data_attrs condition: poll-for-changes must only be active in :change_in_progress state, not :edit/:completed - Replace update_to_alphanumeric? with autofix_requested? keyed on confirm_dangerous_action param (DangerDialog checkbox signal) - Use ActiveRecord::Type::Boolean cast for truthy check - Fix spec: radio group label renders as <legend>, not <h2>; add visible: :all for hidden element assertion
The status polling UI assumes at most one active instance of this job at a time. Add a FIXME to enforce this with GoodJob concurrency control once the real migration body is implemented.
Only append the reference query param when a non-empty reference value is available. Callers that use the reference mechanism (e.g. meetings, which always supply a changed_hash) are unaffected.
Single-word names previously produced a 1-char handle ("Banana" → "B").
Add a SINGLE_WORD_LENGTH = 3 constant and branch handle_from_name so
single-word names return the first 3 transliterated, uppercased chars
("Banana" → "BAN", "Kiwi" → "KIW"). Multi-word names continue to use
the initials/acronym path unchanged.
2de1c78 to
ebc0e4d
Compare
Deploying openproject with ⚡ PullPreview
|
- Move DISPLAY_COUNT constant from IdentifierAutofixSectionComponent to PreviewQuery, eliminating a service-layer dependency on a view component. The component now forwards to PreviewQuery::DISPLAY_COUNT. - Guard PreviewQuery.new.call to only run in the :edit state. Previously it executed on every render, hitting the DB twice per Hotwire status-poll during the :change_in_progress phase. - Replace nil guard in error_label with I18n.t default: "" to cover any unrecognised error reason, not just nil. - Add component spec for IdentifierSettingsFormComponent covering all three states (:change_in_progress, :completed, :edit) including the autofix-section visibility branch. - Update preview_query_spec to reference PreviewQuery::DISPLAY_COUNT directly instead of the UI component constant.
Replace CSS role selectors with capybara_accessible_selectors helpers and substitute raw English strings for I18n.t() calls where the value serves as a contract on the exact locale text.
…or-sematic-work-package-identifiers-ui
Replace the imperative Stimulus openConfirmDialog action (which pre-rendered the dialog in the page and called showModal() via JS) with the idiomatic Hotwire approach: a GET endpoint that streams the dialog on demand. - Add confirm_dialog route and controller action using respond_with_dialog - Include OpTurbo::Streamable in ChangeIdentifiersDialogComponent - Convert autofix button to a link with data-turbo-stream pointing to the new route; remove static dialog render from the form template - Remove openConfirmDialog method from the Stimulus controller - Update specs to use click_on and raw English strings instead of I18n.t() Relates to #72461
Previously only the first radio button was checked by label. Now both "Instance-wide numerical sequence" and "Project-based alphanumerical identifiers" are asserted for completeness.
The autofix button is now a link (<a> tag), so update the component spec to use have_link/have_no_link instead of have_button. Also replace all I18n.t() calls with raw English strings throughout the spec.
judithroth
left a comment
There was a problem hiding this comment.
This is really nice!
I've got some smaller remarks and one big question that we probably have to discuss with Wieland 😅 (but that's not about your code rather about if we can do exactly the same as Jira since we have existing customers...)
config/locales/en.yml
Outdated
| box_header: | ||
| label_project: Project | ||
| label_previous_identifier: Previous identifier | ||
| label_autofixed_suggestion: Autofixed suggestion |
There was a problem hiding this comment.
There had been some discussions on the mockup lately: https://www.figma.com/design/NHUuezDJWmeG8LsH2OgVMh?node-id=1-15777#1659303178
And apparently now this should be called "Future identifier".
| def handle_from_name(name) | ||
| # Use POSIX [[:alpha:]] so accented letters (é, ñ, ü…) are kept inside | ||
| # their word rather than treated as separators by the ASCII-only [a-zA-Z]. | ||
| words = name.to_s.scan(/[[:alpha:][:digit:]]+/) |
There was a problem hiding this comment.
Nice solution! That seems to work really good for german umlauts (äöü) 👍
Ok, on second thought: Maybe it's too restrictive if we just accept ASCII characters. I know that's what we talked about all the time and we even wanted to have our validations that way (A-Z, 0-9 and only underscores allowed). But how does it work for Chinese, Japanese or cyrillic alphabet? 😅
I mean Jira probably enforced this all from the beginning but we didn't! So we might have customers where this will not work. I think we should discuss this with Wieland and Parimal...
There was a problem hiding this comment.
Ok, I thought about this again. So if people use a non-ASCII-alphabet, they will get the fallback, which means "PROJ", "PROJ2" etc. Since it should be an edge case and we can still improve if someone complains, lets not make this more complex as necessary and just go on with it.
The only thing I am not sure of is if 10.000 is enough. Do you know how big our biggest instances are / how many projects they have? Is this number enough for that case?
There was a problem hiding this comment.
With transliteration, users can provide the translations for non-latin languages- in which case they will not get the fallback version.
E.g.
# # config/locales/de.yml
# de:
# i18n:
# transliterate:
# rule:
# ü: "ue"
# ö: "oe"
# etc...There was a problem hiding this comment.
The only thing I am not sure of is if 10.000 is enough. Do you know how big our biggest instances are / how many projects they have? Is this number enough for that case?
This is a good point, I can ask for this info- abeit a far off edge case?
app/services/work_packages/identifier_autofix/project_handle_suggestion_generator.rb
Outdated
Show resolved
Hide resolved
app/services/work_packages/identifier_autofix/project_handle_suggestion_generator.rb
Outdated
Show resolved
Hide resolved
frontend/src/stimulus/controllers/dynamic/admin/work-packages-identifier.controller.ts
Show resolved
Hide resolved
spec/services/work_packages/identifier_autofix/project_handle_suggestion_generator_spec.rb
Outdated
Show resolved
Hide resolved
| end | ||
| end | ||
|
|
||
| def error_reason(identifier, reserved_handles:, in_use_handles:) |
There was a problem hiding this comment.
Hey Kabiru, to echo my point from the Figma designs -- shouldn't we be also detecting identifiers that are not uppercase?
Case in point on my instance: empty will have to be converted to EMPTY by the background job, but the admin page omits it from the listing entirely, which makes it seem like it's already fine and won't be touched.
There was a problem hiding this comment.
Hey Tom, I think we need to discuss this: currently, we store all identifiers in lowercase (at the database level). We should then enforce case insensitivity during writes and stores, and only upcase them in presentation. I’m not fully aware of the pros and cons of this approach. Shall we talk about it during our session tomorrow at @thykel and @judithroth?
There was a problem hiding this comment.
Oh, I wasn't aware of case insensitivity -- is the aim to reduce the scope of data migrations that our users have to do?
A bit skeptical about this one -- let's discuss for sure.
| def error_reason(identifier, reserved_handles:, in_use_handles:) | ||
| if identifier.length > HANDLE_MAX_LENGTH | ||
| :too_long | ||
| elsif identifier.match?(/[^a-zA-Z0-9]/) |
There was a problem hiding this comment.
Based on the identifier requirements, isn't it more like this?
| elsif identifier.match?(/[^a-zA-Z0-9]/) | |
| elsif identifier.match?(/[^A-Z]/) |
app/services/work_packages/identifier_autofix/project_handle_suggestion_generator.rb
Outdated
Show resolved
Hide resolved
| work_packages_bulk_request_limit: { | ||
| default: 10 | ||
| }, | ||
| work_packages_identifier: { |
There was a problem hiding this comment.
Mega nitpick: I wonder if we should switch to a more general term such as project_identifier_scheme, since the choice between numeric and alphanumeric also affects project identifiers, not just WPs.
There was a problem hiding this comment.
I do not fully agree.
So, you're right, depending on the setting the project identifier has different validations (at least for now). But if the project identifier is used or not is not determined by this setting. So it's not as much about projects as it is about work packages.
On the other hand it would be really nice if we also would use the - schema also for attachments in the (far) future. Or maybe even for documents at some point. So a more generic name would not need changing then. But on the other hand, changing a setting name should not be too hard and at the moment it's really just work packages.
There was a problem hiding this comment.
Good points both- I'll add this topic to our meeting tomorrow 😄
…nGenerator 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.
6dca879 to
e9900df
Compare
…extract error_reason
- 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")
Group the four related length constants (min, max, base, single_word) into a single frozen hash for better locality and fewer top-level names.
- 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
Ticket
https://community.openproject.org/wp/72461
What are you trying to accomplish?
Introduces the admin UI and supporting backend for semantic work package identifiers -- a new setting that lets administrators switch work package IDs from the legacy numeric format to short alphanumeric project handles.
The feature is gated behind the
semantic_work_package_idsfeature flag.Screenshots
What approach did you choose and why?
This PR only includes the UI flows- the background jobs and models will be implemented separately.
Merge checklist