-
Notifications
You must be signed in to change notification settings - Fork 1
Hea 651/api endpoint for dynamic search #142
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
Conversation
…iewset to support product and wealth_characteristics filters see HEA-651
apps/common/models.py
Outdated
| ) | ||
|
|
||
|
|
||
| class CountryManager(models.Manager): |
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.
Is this necessary? Can we use objects = IdentifierManager.from_queryset(ReferenceDataQuerySet)() instead?
apps/baseline/viewsets.py
Outdated
| """ | ||
| field_lookups = [ | ||
| *[(field, "icontains") for field in translation_fields("wealth_characteristic__name")], | ||
| ("wealth_characteristic__code", "istartswith"), |
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.
I don't think the codes are hierarchical, so I think this can be iexact
apps/baseline/viewsets.py
Outdated
| """ | ||
|
|
||
| renderer_classes = [JSONRenderer] | ||
| permission_classes = [DjangoModelPermissionsOrAnonReadOnly] |
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 DjangoModelPermissionsOrAnonReadOnly and not just AllowAny?
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.
Or just leave permissions classes blank and override get_permissions?
apps/baseline/viewsets.py
Outdated
| """ | ||
|
|
||
| renderer_classes = [JSONRenderer] | ||
| permission_classes = [] |
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.
I think a better approach is:
permission_classes = [AllowAny]and no get_permissions. Does that work?
apps/baseline/viewsets.py
Outdated
| ] | ||
|
|
||
|
|
||
| class LivelihoodBaselineFacetedSearchView(APIView): |
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.
Should be LivelihoodZoneBaselineFacetedSearchView
hea/urls.py
Outdated
| ] | ||
| urlpatterns += [ | ||
| path( | ||
| "api/livelihoodbaselinefacetedsearch/", |
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.
Should be /api/livelihoodzonebaselinefacetedsearch
apps/baseline/tests/test_viewsets.py
Outdated
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| data = response.data | ||
| self.assertEqual(len(data["products"]), 0) |
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.
Can we extend these unit tests so that they finish by using the filter and checking that the number of results matches the facet count?
I.e. call /api/livelihoodzonebaseline.json?product=XXX and check len(response.json)?
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 the values from data["products"][0]["filter"], etc. to create the url.
apps/baseline/viewsets.py
Outdated
| Return a faceted set of matching filters | ||
| """ | ||
| results = {} | ||
| search_term = request.query_params.get("search", "") |
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.
I think that search here should be read from settings.SEARCH_PARAM.
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.
No description provided.