-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[59174] Added group by for hierarchy items #17317
[59174] Added group by for hierarchy items #17317
Conversation
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.
The code looks okay, but when trying to group by on my local I get the following error
Is this an additional edge case, maybe or am I missing something on my setup?
NoMethodError (undefined method `name' for an instance of CustomField::Hierarchy::Item):
lib/api/decorators/aggregation_group.rb:84:in `map'
lib/api/decorators/aggregation_group.rb:84:in `set_links!'
lib/api/decorators/aggregation_group.rb:37:in `initialize'
lib/api/v3/work_packages/work_package_aggregation_group.rb:38:in `initialize'
app/services/api/v3/work_package_collection_from_query_service.rb:100:in `new'
app/services/api/v3/work_package_collection_from_query_service.rb:100:in `block in generate_groups'
app/services/api/v3/work_package_collection_from_query_service.rb:99:in `each'
app/services/api/v3/work_package_collection_from_query_service.rb:99:in `map'
app/services/api/v3/work_package_collection_from_query_service.rb:99:in `generate_groups'
app/services/api/v3/work_package_collection_from_query_service.rb:67:in `results_to_representer'
app/services/api/v3/work_package_collection_from_query_service.rb:47:in `call'
lib/api/v3/queries/helpers/query_representer_response.rb:38:in `block in query_representer_response'
lib/api/root_api.rb:263:in `raise_invalid_query_on_service_failure'
lib/api/v3/queries/helpers/query_representer_response.rb:35:in `query_representer_response'
lib/api/v3/queries/queries_api.rb:134:in `block (3 levels) in <class:QueriesAPI>'
@brunopagno Can you tell me when this happens? |
b6bf9ab
to
f68bdc4
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.
There is this thing with the string representation. Not major, but if we can do this maybe a bit better, I think it is worth thinking about.
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 think this looks good. 🎉
I would still try to get Eric's comment as well cause it seems small enough.
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.
The to_s fix looks good, if we can find the source for the failing test, then this is approved.
563d59f
to
5e9651d
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.
LGTM.
I just left some code suggestions that are by no means blocking (or maybe even good 🤣 )
5e9651d
to
56b32fc
Compare
56b32fc
to
151dbff
Compare
Ticket
59174
What are you trying to accomplish?
Enable
GROUP BY
for hierarchy items.Screenshots
What approach did you choose and why?
Fixed generating the API response that creates the row keys for dividing the workpackage table into groups.