ezsystems / eztags

GNU General Public License v2.0
40 stars 40 forks source link

Added a new extended attribute filter that allows tag path filtering #101

Closed thiagocamposviana closed 8 years ago

thiagocamposviana commented 8 years ago

I have created a new extended attribute filter, so it should be possible to filter by tag path string. Example:

    {def $hash_params=hash(
                            'parent_node_id', 2
                            , 'extended_attribute_filter', hash(
                                'id', 'TagsTreeAttributeFilter',
                                'params', hash(
                                    'parent_tag_id', $tag_id_array,
                                    'exclude_parent', true(),

                                )
                            )
                            , 'sort_by', array( 'published', false() )
                      )
         $children_extra=fetch( 'content', 'tree', $hash_params )}

path_string: a single path string or an array of path strings. Optional params: exclude_parent: boolean. language: example: eng-US type: 'and' or 'or', defaults to 'or'. path_string is an array, or a single tag path_string, you can build like:

The parent_tag_id is an array of tag ids.

emodric commented 8 years ago

Hi @thiagocamposviana. I'm not really sure I understand the purpose of the extended filter. Can you clarify a bit?

peterkeung commented 8 years ago

Needs documentation in doc/USAGE.md

emodric commented 8 years ago

@peterkeung Sure, but I'm having a hard time understanding the usecases for the extended attribute filter. What can this extended attribute filter achieve that the already existing ones can't.

I mean, filtering by tag path string is exactly the same as filtering by tag ID, is it not?

peterkeung commented 8 years ago

(Technical documentation was a separate note I forgot to make yesterday; sorry for the confusion)

We have a few use cases, but they're all variations on the same example.

Suppose your tag tree is by country, organized in continents:

Europe -- France -- Germany Africa -- Eritrea -- Angola

Your articles are tagged by country (not continent / other grouping -- there are a few reasons for that, including when someone decides that a country should be in a different continent), but on some pages you want to show content tagged with any country in a continent. Or you have a search that has continent or country filtering. Without this filter, you have you loop through all of the continent's countries and send those specific tags as part of the fetch.

So, it's about being able to filter by articles tagged with anything in an entire tag tree.

emodric commented 8 years ago

Why not just tag the content with continent tags?

peterkeung commented 8 years ago

Two main reasons:

joaoinacio commented 8 years ago

On a side note, both $tagPath and $suffix should probbly be escaped before usage as part of SQL query string.

emodric commented 8 years ago

What @joaoinacio said :)

I'll take a closer look soon at the rest.

peterkeung commented 8 years ago

Any other feedback?

emodric commented 8 years ago

Hi @peterkeung

Sorry, I haven't had the time to take a deeper look. I hope I'll have some time soon.

thiagocamposviana commented 8 years ago

Updated the code so it is now escaping $tagPath and $suffix.

emodric commented 8 years ago

Hi @peterkeung @thiagocamposviana Is there a specific reason why path string is transfered instead of only the parent tag ID?

Because to select anything that is tagged with tags below a specific tag ID, you don't need the entire path string, only the tag ID itself (with SQL simiar to WHERE id LIKE '%/42/%')

peterkeung commented 8 years ago

@emodric Path string is simply already available as a parameter without having to build the string, and we're used to using the concept for some core eZ fetches such as excluding a subtree.

emodric commented 8 years ago

My issue with using it is that it was always meant to be purely internal element, not to be used outside extension itself.

It is also more intutive to use the ID, as you would most likely to say: "I want all content tagged with tags below tag ID 42", and not " I want all content tagged with tag with path string ..." Besides, developers would need to know how path strings work to be able to use it this way.

So if you agree, and to keep it consistent with other filters too, I would like for tag ID to be used here.

peterkeung commented 8 years ago

OK, then we could change the parameter to be called something like "parent_tag_id" -- any objects to the name?

emodric commented 8 years ago

That's good :)

thiagocamposviana commented 8 years ago

Fixed!

thiagocamposviana commented 8 years ago

All fixed

thiagocamposviana commented 8 years ago

Hi, is it ready for merge? I think I made the necessary changes, in order to test the '/%/%' wildcard match I have created a pgsql table with a string column and did some tests with some entries just like a tree path ( /1/2/3/4 ), everything seems right.

emodric commented 8 years ago

Hi, yes, I think so. I havenn't had time to have a final look, so please give me couple of more days to check it out.

emodric commented 8 years ago

Thanks everyone !