feat: add get_content_filter_hash endpoint #858
Conversation
marlonkeating
force-pushed
the
mkeating/ENT-8661
branch
3 times, most recently
from
3ccb613
to
cbf0c61
Compare
| """ | ||
| Test that get content filter hash returns md5 hash of query | ||
| """ | ||
| url = reverse('api:v1:get-content-filter-hash') |
There was a problem hiding this comment.
why do we need to reverse the URL?
There was a problem hiding this comment.
Not strictly needed, but if we hypothetically need to change the url scheme in the future, we can more easily track references this way rather than having to hunt down url snippets.
There was a problem hiding this comment.
Using reverse() to turn a view name into a full URL is a pretty standard practice for django unit tests.
https://stackoverflow.com/a/11241936 has a good overview of what/why reverse is.
| """ | ||
| Get md5 hash of a catalog query | ||
| """ | ||
| content_filter_hash = get_content_filter_hash(json.loads(request.body)) |
There was a problem hiding this comment.
Are we guaranteed to get a valid response? What about failure scenarios where the response is invalid or the API call times out?
There was a problem hiding this comment.
Good call-out! Added error handling for invalid json input.
marlonkeating
force-pushed
the
mkeating/ENT-8661
branch
from
cbf0c61
to
9128cc3
Compare
| """ | ||
| Test that get content filter hash returns md5 hash of query | ||
| """ | ||
| url = reverse('api:v1:get-content-filter-hash') |
There was a problem hiding this comment.
Using reverse() to turn a view name into a full URL is a pretty standard practice for django unit tests.
https://stackoverflow.com/a/11241936 has a good overview of what/why reverse is.
| """ | ||
| url = reverse('api:v1:get-content-filter-hash') | ||
| test_query = '{ "content_type": [ "political", "unit", "market" ] }' | ||
| response = self.client.generic('GET', url, data=test_query) |
There was a problem hiding this comment.
nit: you should be able to write this as self.client.get(url, data=test_query)
There was a problem hiding this comment.
You would think, but it looks like client.get doesn't support the parameter, throwing the error message ValueError: not enough values to unpack (expected 2, got 1)
| """ | ||
| Get md5 hash of a catalog query | ||
| """ | ||
| catalog_query = request.body.decode() |
There was a problem hiding this comment.
You should be able to use request.data to get this as de-serialized JSON: https://www.django-rest-framework.org/api-guide/requests/#request-parsing
There was a problem hiding this comment.
Also, is this really a catalog_query, or is it the content_filter value of a catalog query?
| """ | ||
| catalog_query = request.body.decode() | ||
| try: | ||
| content_filter_hash = get_content_filter_hash(json.loads(catalog_query)) |
There was a problem hiding this comment.
(and if you use request.data, you won't explicitly have to json.loads() here)
marlonkeating
force-pushed
the
mkeating/ENT-8661
branch
from
9128cc3
to
a2707dc
Compare
style: fix pycodestyle issues chore: pylint disable=no-member chore: pycodestyle fix chore: add bad json error handling for get_content_filter_hash chore: address review comments chore: pycodestyle space issue
marlonkeating
force-pushed
the
mkeating/ENT-8661
branch
from
a2707dc
to
0aba243
Compare
feat: add get_content_filter_hash endpoint
Jira Ticket
Following up on the changes in #837, this adds a method for retrieving content query hashes for query text, which can then be passed to
get_query_by_hash.Post-review