-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
Fix grouping counts #353
Fix grouping counts #353
Conversation
8db68c4
to
918e91b
Compare
@eito-fis Please see the new format suggestion in the issue (comma separation of values could be confusing, since values can have commas in them). |
mathesar/pagination.py
Outdated
@@ -67,7 +67,8 @@ def paginate_queryset(self, queryset, request, table_id, | |||
filters=filters, order_by=order_by | |||
) | |||
# Convert the tuple keys into strings so it can be converted to JSON | |||
group_count = {','.join(k): v for k, v in group_count.items()} | |||
group_count = [{"columns": list(cols), "count": count} |
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.
@eito-fis On second thought, I think changing this key to values
instead of columns
makes more sense, these aren't column names.
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.
@eito-fis I tested this locally by going to http://localhost:8000/api/v0/tables/2/records/?group_count_by%3D[%22Status%22]
in my browser and I'm getting
"group_count": {
"group_count_by": null,
"results": null
},
It should be returning the group count for the Status
column, right? Am I accessing the API correctly?
That's strange, running 'group_count': {
'group_count_by': ['Status'],
'results': [
{'values': ['Issued'], 'count': 48},
{'values': ['Application'], 'count': 2}
]
} Are the tests passing for you? |
@eito-fis The tests are passing for me. Do you get the same result when you go the URL in your browser? |
@eito-fis I figured out the issue. When I replace the |
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.
@eito-fis Looks good overall, nice work. Please handle URL encoding correctly and also add a test for it so that we don't break it in the future.
@kgodey After looking around a bit, I think this is desired behavior? it seems like the |
@eito-fis I think you're correct. I probably ended up with that URL because I mistyped the query to begin with and my browser got confused by the |
Fixes #352
Updates the record endpoint to have proper grouping behaviors.
Technical details
Updates the
get_group_counts()
function to create a subquery with the appropriate parameters applied, before grouping on the result. Also updates the limit offset tests and adds a new filtering + grouping test.Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin