[59069] Implemented filtering for hierarchies #17247
Conversation
|
@apfohl I opened a PR in the current branch. We can for sure create a separate branch and reapply the changes here if you prefer. But alternatively we could keep it simple and just do a |
|
It's good. Let's just squash it. |
apfohl
force-pushed
the
explore/hierarchy-filters
branch
from
5285822
to
75b1b5b
Compare
apfohl
changed the title
There was a problem hiding this comment.
This was a quick fly-over glance and not a proper review, i.e. I haven't tried out the functionality and didn't spent a long time to think this through. Especially the actual SQL used for filtering would be important to look at in this case.
To that end it would be helpful to know the expected filter behaviour. E.g. is filtering on a category (encompassing multiple items below) intended to be possible and if so, what is the intended outcome.
An integration spec on the filter would serve to guarantee the quality of the changes as well as explain the expected filter results. https://github.com/opf/openproject/blob/dev/spec/models/query/results_custom_field_filter_integration_spec.rb could be a good place for this. And of course a feature spec to safeguard the whole functionality.
But the structure of the change seems to be going into the right direction.
| private | ||
|
|
||
| attr_accessor :item | ||
| end |
There was a problem hiding this comment.
❓ What is the purpose of this class here?
There was a problem hiding this comment.
@ulferts it's because of the expected values on the query_filter_instance_representer.rb. It seems to be designed to operate on a type of object and we needed the adapter so that it would render correctly.
Same for the path we added to the path_helper.rb
There was a problem hiding this comment.
Specifically we are talking about the filter representer being very specific about needing a .name method on entities. the hierarchy items do not have a name. To not pollute either the record or the representer, an adapter was created, wrapping the hierarchy item into a readable model for the filter representer.
| private | ||
|
|
||
| attr_accessor :item | ||
| end |
There was a problem hiding this comment.
Specifically we are talking about the filter representer being very specific about needing a .name method on entities. the hierarchy items do not have a name. To not pollute either the record or the representer, an adapter was created, wrapping the hierarchy item into a readable model for the filter representer.
spec/features/work_packages/table/queries/hierarchy_cf_filter_spec.rb
Outdated
Show resolved
Hide resolved
|
|
||
| # Filtering by hierarchy (=) | ||
|
|
||
| filters.add_filter_by(hierarchy_cf.name, "is", [luke.label], hierarchy_cf.attribute_name(:camel_case)) |
There was a problem hiding this comment.
❓ ooc: how much effort is it to have a clone of the is filter operator which is called is any, being more precise in what it is doing?
question opts for maybe suggesting within our current epic scope a renaming (in code) from is to is any, to easily distinguish between is any and is all in future.
There was a problem hiding this comment.
We initially had implemented a custom operator for hierarchies, but after some investigation it turned out the behaviour would be exactly the same as the Equals operator.
And after looking into other operators, I found that there is already a EqualsAll operator.
Which leads me to think that using the default operators here is better because it is more aligned with the rest of the application. Meaning I don't think it is better to explicitly duplicate it for the is any label
|
@brunopagno you already added a UI test as Jens suggested. For the correct wire up of schemas, representers and all the stuff, this is a nice and valuable thing. But it is not proper (IMHO) for a full functionality test of the filter. For the current operators (is any and is not) this is no big deal, but my take would be, that we write requests specs (API integration) once we have the requests specs are much faster then UI tests, and hence can be used to cover all relevant functionality of filtering the wp collection. |
@Kharonus that's a very valid point. I think I caught a glimpse of previous discussions around the topic in the past. To be honest at this point I don't have strong opinions, so I would be fine if we want to ditch these feature specs for API tests. |
- Added filter implementation for a custom field of type hierarchy - Added "=" operator for filtering on any of the selected items - Added "!" operator for filtering on none of the selected items - Added specs for FilterDependecyRepresenterFactory and the FilterDependecyRepresenter
brunopagno
force-pushed
the
explore/hierarchy-filters
branch
from
b6fb7ed
to
8d81339
Compare
Ticket
https://community.openproject.org/wp/59069 & https://community.openproject.org/wp/59070
What are you trying to accomplish?
Include filtering for hierarchy items
Screenshots
Screencast.From.2024-11-21.15-59-49.mp4
What approach did you choose and why?
We introduced a new filter strategy that uses the already existing equals and notequals operators
Merge checklist