-
Notifications
You must be signed in to change notification settings - Fork 3k
[#70106] Do not force-activate custom fields with a default value #21531
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
Conversation
1b1ba92 to
badea74
Compare
6c8d53f to
fa450e8
Compare
ulferts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @EinLama , I found a bug when using the API. Sorry, custom fields are nasty.
| !cv.is_for_all? && | ||
| # Mind that a present value could just be the default value. Treat it as blank in that case: | ||
| (cv.value.blank? || cv.default?) | ||
| end.pluck(:custom_field_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach has the drawback of not activating a custom field if a client creates the project via the api sending the default value along.
In this example, the custom option 10 is the default value of custom field 518. Since it is the default value, the field will be deactivated subsequently.
This can be mitigated by checking in the params if the custom field was provided. It might even be possible to rely on params in the build_missing_project_custom_field_project_mappings step and by that completely get rid of the need to clean up here afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang, good point. I dismissed most of the API since we have another bug ticket for it. But this bug is NOT part of that ticket, so thanks for pointing it out.
As for the params idea: I tried it and this approach works. As you predicted, we do not even have to delete unwanted values anymore.
2d90938 to
5d90f6a
Compare
Without this commit, unwanted fields would still show up in the creation form, as the NewProjectService will always create the mappings. But it will only remove unwanted mappings after successfully persisting the project. This does not happen if there are errors (e.g. a required field has no value). Since the field should never have been activated in the first place, it is expected to have no value. Still, this validation errors prohibits saving - and thus the errornous field is never cleaned up.
... even if it is identical to the default value
5d90f6a to
350771d
Compare
ulferts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works like a charm, @EinLama .
Thanks for the test coverage.
Ticket
https://community.openproject.org/wp/70106
What are you trying to accomplish?
Fields that are required and have a default value will no longer be activated for new projects and will thus not show up in the
new projectwizard.This is done by teaching the NewProjectService to treat custom fields with the default value set to be treated like a custom field with no value set.
Merge checklist