data-hub icon indicating copy to clipboard operation
data-hub copied to clipboard

Expand on tag filters

Open abrenoch opened this issue 2 years ago • 12 comments

I noticed the tags filter was a bit rigid so I have some proposed changes:

  • Adds useTagIdPath as a query variable. This will allow searching for tags by their nested location (such as /7/11/15/) rather than just the name. If 2 tags with the same name existed within 2 different parents, there was no way to differentiate which one was returned. This will allow targeting specific tags within the scope of the parents.
  • Adds hasAllTags as a query variable. Currently the tag filter matches any of the tags provided in the query, setting this variable to true will require results match all the tags provided.

abrenoch avatar Sep 21 '23 19:09 abrenoch

@abrenoch, thanks for submitting a PR to data-hub :partying_face:

I tested your implementation, and I still have some questions. I've tested everything based on the demo data (see https://demo.pimcore.fun/admin)

You correctly pointed out that filtering tags based on their name could be improved since there can be tags with an identical name.

I checked out your code and tried the useTagIdPath flag to filter based on the tag`s path (instead of just the name). In that example, the idPath was /3/4/ (Quality/high-res) - but that returned no result. After checking the SQL I noticed that you can only filter for an idPath "group" (in my case /3/) since the idPath column doesn't hold the whole path of the tag.

Please have another look at this PR and fix this issue :+1:

I'm looking forward to your improved PR :100: :1st_place_medal:

Corepex avatar Feb 05 '24 14:02 Corepex

@abrenoch friendly reminder :alarm_clock:. I would love to see this PR merged :tada:.

Corepex avatar Mar 22 '24 07:03 Corepex

@abrenoch :alarm_clock: please have another look at this PR :lollipop:

Corepex avatar Mar 29 '24 07:03 Corepex

@Corepex Hey must have missed your pings - I'll try to take a look at this in the next couple days

abrenoch avatar Apr 02 '24 21:04 abrenoch

@abrenoch, thanks for submitting a PR to data-hub 🥳

I tested your implementation, and I still have some questions. I've tested everything based on the demo data (see https://demo.pimcore.fun/admin)

You correctly pointed out that filtering tags based on their name could be improved since there can be tags with an identical name.

I checked out your code and tried the useTagIdPath flag to filter based on the tags path (instead of just the name). In that example, the idPathwas/3/4/ (Quality/high-res) - but that returned no result. After checking the SQL I noticed that you can only filter for an idPath"group" (in my case/3/) since the idPath` column doesn't hold the whole path of the tag.

Please have another look at this PR and fix this issue 👍

I'm looking forward to your improved PR 💯 🥇

Hey updated so this path search should work now - not sure how I overlooked that. Using a pattern like /3/4/ like you described above should work now (extrapolating /3/ and then using 4 as an additional search condition). Just let me know if you run into any more trouble.

abrenoch avatar Apr 03 '24 19:04 abrenoch

@abrenoch, thanks for updating your PR.

The problem with the idPath seems to be solved. At least the filtering works now, as expected. I noticed another thing dought: It seems that you deleted the part with hasAllTags. At least I can't find any reference to that argument. Please bring that function back.

I guess the hasAllTags param should work in both scenarios (useTagIdPath: true and useTagIdPath: false).

Besides that, it looks quite promising :)

Corepex avatar Apr 05 '24 09:04 Corepex

@Corepex Whoops how silly of me - was too focused on the other problem to notice I lost it in the merge 😅 . Will fix that shortly!

abrenoch avatar Apr 05 '24 12:04 abrenoch

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

github-actions[bot] avatar Apr 05 '24 16:04 github-actions[bot]

I have read the CLA Document and I hereby sign the CLA

abrenoch avatar Apr 05 '24 16:04 abrenoch

@abrenoch, somehow, it still doesn't work. I created a dummy event and assigned three according tags to it 2, 3, /3/4. I also added hasAllTags: true and useTagIdPath: true. At least with that settings I don't get any results.

Please check your setup and try to recreate this usecase.

DataObject: image

Query:

query
{
  getEventListing(tags: "2, 3, /3/4/", hasAllTags: true, useTagIdPath: true)
  {
    edges {
      node {
        id
      }
    }
  }
}

Corepex avatar Apr 19 '24 07:04 Corepex

Gotcha @Corepex . Sorry but it may take me a while to get back to this; we have moved past pimcore since I originally opened this PR and I'm not sure how much time I would have to continue contributing (or, er, trying to I guess). But if someone else wanted to whip it into shape in the meantime I would certainly welcome it.

abrenoch avatar Apr 29 '24 03:04 abrenoch

@Corepex @abrenoch I will close this PR now due to inactivity and as it seems that @Corepex could not reproduce it.

It can be reopened anytime, if you want to work on it again.

mattamon avatar Aug 26 '24 11:08 mattamon