Feature/44 show content size #46
Conversation
|
@danielmursa-dev please rebase your changes into a single commit, after that @stevenbal can do a review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
- Coverage 97.75% 96.36% -1.39%
==========================================
Files 21 22 +1
Lines 445 468 +23
Branches 57 29 -28
==========================================
+ Hits 435 451 +16
- Misses 6 13 +7
Partials 4 4 ☔ View full report in Codecov by Sentry. |
|
@stevenbal please can you do a review, thanks |
There was a problem hiding this comment.
Looks good overall, some minor comments. Make sure to also add Fixes: #<issue number> to the description of the pull request, this links the PR to the issue and closes it once the PR is merged.
It looks like the commits aren't squashed though, it would be nice if there are two commits (one for the implementation and another to add tests). You can do this with interactive rebase git rebase -i <first commit hash>^ and choosing squash for the commits that should be squashed or by resetting the commits with git reset HEAD~<number of commits to reset> and recreating the manually
| @@ -76,6 +78,7 @@ class OutgoingRequestsLogAdmin(admin.ModelAdmin): | |||
| "response_ms", | |||
| "res_headers", | |||
| "res_content_type", | |||
| "response_content_length", | |||
There was a problem hiding this comment.
I think it would be nice if it's possible to sort by this field in the admin listview, that way it's easier to find requests with unexpectedly large response bodies. Since it's a computed field, you might have to annotate the queryset to implement this: https://docs.djangoproject.com/en/4.2/ref/models/querysets/#annotate
There was a problem hiding this comment.
I'm absolutely agree with making it sortable, but I think it is not possible to annotate the queryset because this property reads the length of another computed property response_body_decoded. I think it is therefore for this reason, I could be wrong.
There was a problem hiding this comment.
Hmm that's true, though I do wonder how useful this if we cannot sort by length.
I think it's only possible to make it sortable if we use res_body instead of response_body_decoded, although I do think the lengths won't be exactly correct depending on the content_type of the response.
Example for an HTML response
>>> len(large.res_body)
233210
>>> len(large.response_body_decoded)
233181
We could do something like this in that case:
from django.db.models import Func
class LengthFunction(Func):
function = 'LENGTH'
def get_queryset(self, request):
qs = super().get_queryset(request)
return qs.annotate(
annotated_response_length=LengthFunction('response_body_decoded')
)
def response_content_length(self, obj):
return obj.annotated_response_length
response_content_length.admin_order_field = 'annotated_response_length'@alextreme what are your thought on this. Would it be okay if the response_content_length in the list view isn't exactly correct, but it would be sortable? Or would you rather have the exact length without sortability?
danielmursa-dev
force-pushed
the
feature/44-show-content-size
branch
from
cd3d202 to
23b483a
Compare
Fixes: #44