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
CM-71: added support for applying queryset based on custom filters provided by frontend #10
Conversation
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.
Looks solid, I've made some comments regarding documentation.
social_protection/custom_filters.py
Outdated
""" | ||
Apply custom filters to a queryset. | ||
|
||
:param custom_filters: List of named tuples representing custom filters. |
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 information is already provided through in typehint, it's better to give some example of how this named tuple looks like.
social_protection/custom_filters.py
Outdated
Apply custom filters to a queryset. | ||
|
||
:param custom_filters: List of named tuples representing custom filters. | ||
:type custom_filters: List[namedtuple] |
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 this also is redundant, we have information about type in the hint.
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.
Same for queryset
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.
ok
social_protection/custom_filters.py
Outdated
:param custom_filters: List of named tuples representing custom filters. | ||
:type custom_filters: List[namedtuple] | ||
|
||
:param query: The original queryset with filters. |
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.
Any specific type of QuerySet is required? Like Queryset[BenefitPlan] ?
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.
yes, sth like this. Mostly related to Queryset[Beneficiary] in that case.
@dborowiecki docsstring fixed |
part of ticket: https://openimis.atlassian.net/browse/CM-71