Conversation
…rs' into feature/72855-new-project-with-semantic-identifiers
…rs' into feature/72855-new-project-with-semantic-identifiers
|
I see there's still a useless vertical scrollbar in the modal alert & some missing whitespace in the main form -- will fix that soon. The rest should be good to review. |
app/components/projects/settings/general/show_component.html.erb
Outdated
Show resolved
Hide resolved
Deploying openproject with ⚡ PullPreview
|
akabiru
left a comment
There was a problem hiding this comment.
Nice one- just a few remarks to consider
app/components/projects/settings/change_identifier_dialog_component.html.erb
Outdated
Show resolved
Hide resolved
| module Settings | ||
| class EditableIdentifierForm < ApplicationForm | ||
| form do |f| | ||
| if Project.semantic_alphanumeric_identifier? |
There was a problem hiding this comment.
🍏 nitpick: I would have expected to see the check Setting::WorkPackageIdentifier.alphanumeric? here- but I see this check Project.semantic_alphanumeric_identifier? includes a feature flag check as well. Still, I wonder if should overload the Project class or keep all (semantic) settings encapsulated within Setting::WorkPackageIdentifier esp as the feature flag should soon be removed
There was a problem hiding this comment.
nitpick: I would have expected to see the check Setting::WorkPackageIdentifier.alphanumeric? here- but I see this check Project.semantic_alphanumeric_identifier? includes a feature flag check as well.
My original line of thinking was that we don't want someone do this:
- Toggle feature flag on
- Switch to alphanumeric
- Toggle feature flag off
- Encounter alphanumeric-specific behaviour in the form
But it's true that the sequence of 2. + 3. is gonna really mess up the app, and this guard is not really going to offer that much help.
Should we maybe prevent users from toggling the feature flag off while alphanumeric setting is still in effect? It's not like the blame would be on us (as it's an experimental feature), but it could make any future testing a bit more safe to execute.
spec/components/projects/settings/general/show_component_spec.rb
Outdated
Show resolved
Hide resolved
|
@thykel one UX issue to consider: with semantic identifiers enabled, and user attempts to "change identifier" without the Autofix job having ran/completed- they are not able to type unless they clear all input and start typing. In this scenario- I think the form should be rendered in an invalid state (with a validation message) so the user is made aware. I understand this might be a moot point once we have the background job- that should already autofix things- but I reckon it's a good failsafe to have from a UX perspective. |
…ion-setting-for-sematic-work-package-identifiers-ui' into feature/71896-change-identifier-with-semantic-identifiers
…dentifier-with-semantic-identifiers
…rs' into feature/72855-new-project-with-semantic-identifiers
akabiru
left a comment
There was a problem hiding this comment.
Nice one 💯. I only found a few issues that are worth clarifying before merge.
config/routes.rb
Outdated
| resource :filters, only: %i[show] | ||
| end | ||
|
|
||
| get "projects/identifier_suggestion", to: "projects/identifier_suggestions#show", as: :projects_identifier_suggestion |
There was a problem hiding this comment.
🍏 if we can avoid overloading the /project/{id} finder route- I think that would be nice. To keep things simple, and not have to worry about clashes when user sets routing names as identifiers. (noting that we already have "reserved identifiers")
Ticket
https://community.openproject.org/projects/communicator-stream/work_packages/71896
What are you trying to accomplish?
We are currently adding support for "semantic work package identifiers".
project-zeus/ work package ID1PZEUS/ work packagePZEUS-1.This PR rewrites the "Change Project Identifier" functionality to support semantic identifiers.
Screenshots
What approach did you choose and why?
This PR also incorporates content of #22330 due to a faulty merge.
Merge checklist