Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WP creation with required hierarchy CFs #17625

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

NobodysNightmare
Copy link
Contributor

@NobodysNightmare NobodysNightmare commented Jan 16, 2025

In some cases, such as creation through the "New Child" button in the relations tab, we were not able to render a selector for the hierarchy custom fields and thus rendering the dialog failed entirely.

We don't yet have a good component for hierarchical custom fields, so as a quick fix we enable the SelectList components to also be usable for hierarchy CFs. We should later introduce proper hierarchical components for them.

Ticket

https://community.openproject.org/projects/openproject/work_packages/60590

What are you trying to accomplish?

Making it possible to create WPs from the relations tab, even if they have a required custom field of type hierarchy.

Screenshots

image

In some cases, such as creation through the "New Child" button
in the relations tab, we were not able to render a selector
for the hierarchy custom fields and thus rendering the dialog
failed entirely.

We don't yet have a good component for hierarchical custom fields,
so as a quick fix we enable the SelectList components to also
be usable for hierarchy CFs. We should later introduce proper
hierarchical components for them.
Copy link
Contributor

@mereghost mereghost left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable approach given time constraints.

Later we can work on a proper component for hierarchy field and then clean this up. =)

Nice work!

@NobodysNightmare NobodysNightmare merged commit c436989 into dev Jan 16, 2025
11 checks passed
@NobodysNightmare NobodysNightmare deleted the fix-required-hierarchy branch January 16, 2025 13:11
@cbliard
Copy link
Member

cbliard commented Jan 16, 2025

Thanks for the quick fix @NobodysNightmare.

I tested it and I noted the following issues:

  • it does a 500 if the hierarchy custom field does not have any item defined.
    undefined method `descendants' for nil (NoMethodError)
    /Users/cbliard/code/opf/openproject/app/services/custom_fields/hierarchy/hierarchical_item_service.rb:99:in `get_descendants'
    
    Sorry, this is not quite right. This is the factory defined in spec/factories/custom_field_factory.rb which is poorly defined and does not have any root item.
  • the input field steals the focus. The subject field should have focus when the "Create child" modal is displayed.

Also there seem to be some code duplication between multi_select_list.rb and single_select_list.rb files.

I can start working on a PR to try to fix these.

@NobodysNightmare
Copy link
Contributor Author

I can start working on a PR to try to fix these

Thanks for that. If you need any help, feel free to reach out.

And just to make that very clear: Using the flat lists, as we did here is intended as a patch to provide a bare minimum user experience. That's also why we accepted the code duplication, though I see that as a problem as well. There is a look-alike contest going on between SingleSelectList and MultiSelectList, but also between those and SingleVersionSelectList + MultiVersionSelectList

I guess some refactoring wouldn't hurt, but we didn't take the time for that yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants