fix group by for EmbeddedModel field #225
Conversation
WaVEV
force-pushed
the
EMF-support-group-by
branch
from
8c0e138 to
d7d40c3
Compare
WaVEV
changed the title
| @@ -104,6 +113,46 @@ def test_nested(self): | |||
| ) | |||
| self.assertCountEqual(Book.objects.filter(author__address__city="NYC"), [obj]) | |||
|
|
|||
| def truncate_ms(self, value): | |||
There was a problem hiding this comment.
Helper methods like this should typically be organized at the top of a class so they don't lost among test methods, however, I suppose we should make it a function in this file so we don't have to repeat the method on every class (the move can be a separate commit in this PR that precedes the fix).
| (Holder.objects.create(data=Data(integer=x)) for x in range(6)), | ||
| key=lambda x: x.data.integer, | ||
| ) | ||
| query = ( |
There was a problem hiding this comment.
I would rather use qs = since "query" usually refers to SQL or, in our case, MQL.
| """Truncate microsends to millisecond precision as supported by MongoDB.""" | ||
| return value.replace(microsecond=(value.microsecond // 1000) * 1000) | ||
|
|
||
| def test_ordering_by_embedded_field(self): |
There was a problem hiding this comment.
This is good to have, but it passes without the fix, so it should be a separate commit. (I think the fix is mostly about group by, not ordering without grouping.) I would sort these tests above test_nested since they also use the Holder model.
There was a problem hiding this comment.
ok, and this test will be in this PR but in a separate commit, right?
| .annotate(max_auto_now=Max("data__auto_now")) | ||
| .order_by("data__integer") | ||
| ) | ||
| query_response = [{**e, "max_auto_now": self.truncate_ms(e["max_auto_now"])} for e in query] |
There was a problem hiding this comment.
Is truncate needed here? I'd expect MongoDB to return an already truncated result.
There was a problem hiding this comment.
you are right, removed
| self.assertSequenceEqual(query, expected) | ||
|
|
||
| def test_ordering_grouping_by_embedded_field(self): | ||
| expected = sorted( |
There was a problem hiding this comment.
I'd say expected_objs since there is still some modification in calculating the expected result.
| ], | ||
| ) | ||
|
|
||
| def test_ordering_grouping_by_sum(self): |
There was a problem hiding this comment.
Again, it's hard for me to reason about the expected result of query, so some explanatory comment would be useful. For example, the "cheat sheet" at https://docs.djangoproject.com/en/dev/topics/db/aggregation/ is easy to follow.
timgraham
changed the title
timgraham
force-pushed
the
EMF-support-group-by
branch
from
1ad815c to
3512586
Compare
timgraham
force-pushed
the
EMF-support-group-by
branch
from
3512586 to
5156577
Compare
fixes #221