-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add get_content_filter_hash endpoint #858
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to reverse the URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call-out! Added error handling for invalid json input.
cbf0c61
to
9128cc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. Handful of nits/suggestions then it should be good to go!
""" | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you should be able to write this as self.client.get(url, data=test_query)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and if you use request.data
, you won't explicitly have to json.loads()
here)
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
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