Expand on tag filters
I noticed the tags filter was a bit rigid so I have some proposed changes:
- Adds
useTagIdPathas 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
hasAllTagsas a query variable. Currently the tag filter matches any of the tags provided in the query, setting this variable totruewill require results match all the tags provided.
@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:
@abrenoch friendly reminder :alarm_clock:. I would love to see this PR merged :tada:.
@abrenoch :alarm_clock: please have another look at this PR :lollipop:
@Corepex Hey must have missed your pings - I'll try to take a look at this in the next couple days
@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
useTagIdPathflag to filter based on the tags path (instead of just the name). In that example, theidPathwas/3/4/(Quality/high-res) - but that returned no result. After checking the SQL I noticed that you can only filter for anidPath"group" (in my case/3/) since theidPath` 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, 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 Whoops how silly of me - was too focused on the other problem to notice I lost it in the merge 😅 . Will fix that shortly!
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅
I have read the CLA Document and I hereby sign the CLA
@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:
Query:
query
{
getEventListing(tags: "2, 3, /3/4/", hasAllTags: true, useTagIdPath: true)
{
edges {
node {
id
}
}
}
}
Quality Gate passed
Issues
3 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
4.8% Duplication on New Code
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.
@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.