-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 for bulk editting work packages with hierarchy items #17287
Conversation
2655933
to
f004989
Compare
f004989
to
38ef4a7
Compare
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.
Nice, now using the persistence layer makes it more decoupled. 👍🏽
I have one issue regarding code style and excellence. Not a blocker for a merge, but as we still have some time, lets wrap our head shortly around this case.
app/helpers/custom_fields_helper.rb
Outdated
when "hierarchy" | ||
options = [[I18n.t(:label_no_change_option), ""]] | ||
service = CustomFields::Hierarchy::HierarchicalItemService.new | ||
options += service.get_descendants(item: custom_field.hierarchy_root, include_self: false).value!.map do |item| |
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 goes into a direction, we had already on some other places:
is .value!
enough for the failure handling? it means: "Throw on failure". So, while the method currently cannot return a failure, this might change in future, the interface is a Result
.
Should we have small .either
handling here? Maybe raising (as it is now) or returning empty options together with an error log entry?
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.
I meant to do that when I refactored, but forgot. Adjusted it now.
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.
yep, makes sense now. Also this code will eventually get replaced by the specific hierarchy selector (hopefully)
38ef4a7
to
f9c9780
Compare
6e19cc4
to
1963d24
Compare
Ticket
The link to the ticket number 59734
What are you trying to accomplish?
Fix bulk edit for work packages that have custom fields with format hierarchy