Closed bradenmacdonald closed 9 months ago
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.
Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.
Please let us know once your PR is ready for our review and all tests are green.
@ChrisChV @yusuf-musleh Here's my proposed fix for the inconsistency when searching in the "list tags" API. Can you let me know your thoughts?
BTW @yusuf-musleh what I would recommend for the frontend is using a few different combinations of parameters like ?page_size=5&full_depth_threshold=0
, ?full_depth_threshold=20
, and ?full_depth_threshold=10000
during development to make sure that everything is working correctly: populating the children if they're included in the result, "load more" for root tags if there's more than one page, "load more" for child tags if the children are paginated, and loading sub-tags as needed.
@ChrisChV @yusuf-musleh Here's my proposed fix for the inconsistency when searching in the "list tags" API. Can you let me know your thoughts?
@bradenmacdonald Thanks for simplifying the endpoint and keeping it more consistent. I agree, its a lot more predictable and less confusing when they are more deliberate, giving the API consumer the option to pick how they want their data to appear.
I set this up locally and tried it out with the tags drawer, and it fixed things up quite well. However I noticed some inconsistencies with the search results, namely when searching without the full_depth_threshold
param.
It seems like results only show up when the search term exists at the top level. For example, when searching in the Light Cast taxonomy, that looks like so:
http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/
{
"next":null,
"previous":null,
"count":7,
"num_pages":1,
"current_page":1,
"start":0,
"results":[
{
"value":"Abilities",
"external_id":"A",
"child_count":4,
"depth":0,
"parent_value":null,
"_id":626250,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Abilities"
},
{
"value":"Interests",
"external_id":"C",
"child_count":1,
"depth":0,
"parent_value":null,
"_id":626373,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Interests"
},
{
"value":"Knowledge",
"external_id":"G",
"child_count":11,
"depth":0,
"parent_value":null,
"_id":626442,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Knowledge"
},
{
"value":"Personal Attributes",
"external_id":"B",
"child_count":5,
"depth":0,
"parent_value":null,
"_id":626326,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Personal+Attributes"
},
{
"value":"Skills",
"external_id":"F",
"child_count":5,
"depth":0,
"parent_value":null,
"_id":626382,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Skills"
},
{
"value":"Work Activities",
"external_id":"K",
"child_count":4,
"depth":0,
"parent_value":null,
"_id":626603,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Work+Activities"
},
{
"value":"Work Context",
"external_id":"J",
"child_count":4,
"depth":0,
"parent_value":null,
"_id":626511,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Work+Context"
}
]
}
If we take a look at "Abilities", it looks like so:
http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Abilities
{
"next":null,
"previous":null,
"count":4,
"num_pages":1,
"current_page":1,
"start":0,
"results":[
{
"value":"Cognitive Abilities",
"external_id":"A01",
"child_count":6,
"depth":1,
"parent_value":"Abilities",
"_id":626251,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Cognitive+Abilities"
},
{
"value":"Physical Abilities",
"external_id":"A02",
"child_count":3,
"depth":1,
"parent_value":"Abilities",
"_id":626279,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Physical+Abilities"
},
{
"value":"Psychomotor Abilities",
"external_id":"A03",
"child_count":3,
"depth":1,
"parent_value":"Abilities",
"_id":626293,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Psychomotor+Abilities"
},
{
"value":"Sensory Abilities",
"external_id":"A04",
"child_count":3,
"depth":1,
"parent_value":"Abilities",
"_id":626307,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Sensory+Abilities"
}
]
}
If I search at the top level for "Sensory", I get the following:
http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?search_term=sensory
{
"next":null,
"previous":null,
"count":0,
"num_pages":1,
"current_page":1,
"start":0,
"results":[
]
}
However if we search for "sensory" within "Abilities" we get the results:
http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Abilities&search_term=sensory
{
"next":null,
"previous":null,
"count":1,
"num_pages":1,
"current_page":1,
"start":0,
"results":[
{
"value":"Sensory Abilities",
"external_id":"A04",
"child_count":3,
"depth":1,
"parent_value":"Abilities",
"_id":626307,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Sensory+Abilities&search_term=sensory"
}
]
}
This is consistent all levels. However if I pass in the full_depth_threshold
query, I get the expected results:
http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?search_term=sensory&full_depth_threshold=1000
{
"next":null,
"previous":null,
"count":3,
"num_pages":1,
"current_page":1,
"start":0,
"results":[
{
"value":"Abilities",
"external_id":"A",
"child_count":4,
"depth":0,
"parent_value":null,
"_id":626250,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Abilities&full_depth_threshold=1000&search_term=sensory"
},
{
"value":"Sensory Abilities",
"external_id":"A04",
"child_count":3,
"depth":1,
"parent_value":"Abilities",
"_id":626307,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Sensory+Abilities&full_depth_threshold=1000&search_term=sensory"
},
{
"value":"Other Sensory Abilities",
"external_id":"A04c",
"child_count":4,
"depth":2,
"parent_value":"Sensory Abilities",
"_id":626321,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Other+Sensory+Abilities&full_depth_threshold=1000&search_term=sensory"
}
]
}
Here is another example if I search for "Other" which exists 2 levels deep with and without full_depth_threshold
.
Without full_depth_threshold
http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?search_term=other
{
"next":null,
"previous":null,
"count":0,
"num_pages":1,
"current_page":1,
"start":0,
"results":[
]
}
With full_depth_threshold
http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?search_term=other&full_depth_threshold=1000
{
"next":null,
"previous":null,
"count":5,
"num_pages":1,
"current_page":1,
"start":0,
"results":[
{
"value":"Abilities",
"external_id":"A",
"child_count":4,
"depth":0,
"parent_value":null,
"_id":626250,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Abilities&full_depth_threshold=1000&search_term=other"
},
{
"value":"Cognitive Abilities",
"external_id":"A01",
"child_count":6,
"depth":1,
"parent_value":"Abilities",
"_id":626251,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Cognitive+Abilities&full_depth_threshold=1000&search_term=other"
},
{
"value":"Other Cognitive Abilities",
"external_id":"A01f",
"child_count":3,
"depth":2,
"parent_value":"Cognitive Abilities",
"_id":626275,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Other+Cognitive+Abilities&full_depth_threshold=1000&search_term=other"
},
{
"value":"Sensory Abilities",
"external_id":"A04",
"child_count":3,
"depth":1,
"parent_value":"Abilities",
"_id":626307,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Sensory+Abilities&full_depth_threshold=1000&search_term=other"
},
{
"value":"Other Sensory Abilities",
"external_id":"A04c",
"child_count":4,
"depth":2,
"parent_value":"Sensory Abilities",
"_id":626321,
"sub_tags_url":"http://localhost:18010/api/content_tagging/v1/taxonomies/14/tags/?parent_tag=Other+Sensory+Abilities&full_depth_threshold=1000&search_term=other"
}
]
}
If I understood this correctly, when the result is not in the top level but within its subtags (and the full_depth_threshold is not included), its still should return those top level tags with subtag urls containing the search term so you can expand it further to view the search results.
BTW @yusuf-musleh what I would recommend for the frontend is using a few different combinations of parameters like
?page_size=5&full_depth_threshold=0
,?full_depth_threshold=20
, and?full_depth_threshold=10000
during development to make sure that everything is working correctly: populating the children if they're included in the result, "load more" for root tags if there's more than one page, "load more" for child tags if the children are paginated, and loading sub-tags as needed.
Will do! I still haven't implemented the "Load more" functionality/button, I have it on my todo list. This would definitely help with testing it out.
Here's my proposed fix for the inconsistency when searching in the "list tags" API. Can you let me know your thoughts?
@bradenmacdonald I took a quick look, and I like your proposal. Could you updated the ADR with this new?
@yusuf-musleh Good catch. I fixed the bug with 8c4c0dd538af7df45d97813fa60f4605b6bd59d6 and included a test case for it.
@ChrisChV Sure thing. ADR is updated.
@pomegranited If you have time, would you be able to review this as CC?
I can include a version bump when I merge.
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.
This change fixes some bugs and inconsistencies in the "get tags" REST API. Hopefully it will be much easier to understand and use now.
Bugs:
New behavior:
?page_size
).sub_tags_url
on "Eukaryota" and you'll get "Animalia" and then followsub_tags_url
again you'll get "Arthropoda". However, there's a more efficient way!?full_depth_threshold=1000
, any response that would return fewer than 1,000 tags will return a single giant page of results with all the tags in tree order, up to the maximum supported search depth (3). Unlike before, this now works with any type of query, whether you're searching or not, loading root tags or some deeper tags, or anything else. Also unlike before, this is now based on the number of results that are returned, not the size of the taxonomy.The
?root_only
parameter has been removed because it's now on by default. If you want the children included in the result (if possible), you just opt-in to that using?full_depth_threshold
.There are no changes at all to the python API.
Also, this change seems to work fine with the current implementation of the tagging drawer (i.e. despite the major changes, it's mostly backwards-compatible).