-
Notifications
You must be signed in to change notification settings - Fork 8
Add support for the tree parameter on field and concept resources #258
Conversation
for app_name, model_name in tree._models: | ||
q |= Q(app_name=app_name, model_name=model_name) | ||
|
||
queryset = self.model.objects.filter(q) |
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 reason you create the Q
conditions by ORing and filtering here but ANDing and excluding in the Concept get_queryset
method above? If there is no reason for the difference then it might be good to construct and use the Q
conditions the same way in both Field and Concept cases(nit). If there is a reason for the difference, might be worth documenting somewhere.
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.
Doing an OR on the DataConcept
filtering requires only one field to match the condition. A concept could be created that includes a field from an excluded or unrelated model, so the requirement is to exclude concepts if any fields do not match.
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.
Right, my bad. Didn't read that line clearly enough the first time. These
changes look good. Should be really useful.
-Don
On Jan 18, 2015 6:58 PM, "Byron Ruth" notifications@github.com wrote:
In serrano/resources/field/base.py
#258 (diff):@@ -82,7 +85,14 @@ def get_link_templates(self, request):
}def get_queryset(self, request, params):
queryset = self.model.objects.all()
tree = trees[params['tree']]
q = Q()
# No public method for accessing the local models on the tree
for app_name, model_name in tree._models:
q |= Q(app_name=app_name, model_name=model_name)
queryset = self.model.objects.filter(q)
Doing an OR on the DataConcept filtering requires only one field to match
the condition. A concept could be created that includes a field from an
excluded or unrelated model, so the requirement is to exclude concepts if
any fields do not match.—
Reply to this email directly or view it on GitHub
https://github.com/chop-dbhi/serrano/pull/258/files#r23140993.
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 added an extra test case for a concept that contains two unrelated fields (this should never happen in practice) and it failed. Also, I noticed that the SQL being produced awful, excessive joins and what not. So I changed the concept filtering to use a subquery on the unrelated fields which is much more straightforward and the test passes.
This change now properly filters out fields and concepts unrelated to the requested tree. Clients can use this parameter for these resources to drive the context of the application. Fix #214 Signed-off-by: Byron Ruth <b@devel.io>
Add support for the tree parameter on field and concept resources
This change now properly filters out fields and concepts unrelated to
the requested tree. Clients can use this parameter for these resources
to drive the context of the application.
Fix #214
Signed-off-by: Byron Ruth b@devel.io