[24972] Feature/hierarchy mode query #5330
Conversation
oliverguenther
changed the title
oliverguenther
force-pushed
the
feature/hierarchy-mode-query
branch
2 times, most recently
from
58be6e8
to
21a99e9
Compare
oliverguenther
force-pushed
the
feature/hierarchy-mode-query
branch
from
21a99e9
to
dc5dade
Compare
| query.show_hierarchies = false | ||
| else | ||
| query.show_hierarchies = params[:show_hierarchies] if params.key?(:show_hierarchies) | ||
| end |
There was a problem hiding this comment.
I am fine with the side effect as I can see why it is necessary.
However, I'd then rather narrow down the cases when that side effect is applied. In addition, this method right now depends on the correct order of execution (only if group_by is set before calling the method, will show_hierarchies be ignored). In addition, this will prevent clients from getting an error response in a scenario where they only send show_hierarchies=true to a grouped query. The backend will currently simply discard the change.
I'd therefore rather have one method which covers both params at the same time and which only takes the params as input.
There was a problem hiding this comment.
I don't think we'll need this restriction anymore and instead, we can let the user receive the full error message.
| @@ -185,6 +185,8 @@ def initialize(model, | |||
|
|
|||
| property :timeline_visible | |||
|
|
|||
| property :show_hierarchies | |||
There was a problem hiding this comment.
can't we simply use hierarchies? It is probably just my rails infected preference but I really dislike those two word properties for booleans. Same applies for timeline_visible for that matter.
There was a problem hiding this comment.
I don't find hierarchies appropriate. I could settle with hierarchyMode, since we'll likely have other modes in the future.
oliverguenther
force-pushed
the
feature/hierarchy-mode-query
branch
2 times, most recently
from
aade38f
to
edd17b7
Compare
Includes #5318 for the time being.
https://community.openproject.com/projects/openproject/work_packages/24972