Open Real-Gecko opened 3 years ago
Thanks for the contribution!
Can you please explain what's the essence of the changes? What was missing for pagination to work? Is there a way to make those changes in a backwards-compatible way?
Well I don't think it's possible cause MultiplObjectMixin
paginates queryset in get_context_data
:
def get_context_data(self, *, object_list=None, **kwargs):
"""Get the context for this view."""
queryset = object_list if object_list is not None else self.object_list
page_size = self.get_paginate_by(queryset)
context_object_name = self.get_context_object_name(queryset)
if page_size:
paginator, page, queryset, is_paginated = self.paginate_queryset(queryset, page_size)
context = {
'paginator': paginator,
'page_obj': page,
'is_paginated': is_paginated,
'object_list': queryset
}
else:
context = {
'paginator': None,
'page_obj': None,
'is_paginated': False,
'object_list': queryset
}
if context_object_name is not None:
context[context_object_name] = queryset
context.update(kwargs)
return super().get_context_data(**context)
django-filter
's BaseFilterView does it prior to that:
def get(self, request, *args, **kwargs):
filterset_class = self.get_filterset_class()
self.filterset = self.get_filterset(filterset_class)
if not self.filterset.is_bound or self.filterset.is_valid() or not self.get_strict():
self.object_list = self.filterset.qs
else:
self.object_list = self.filterset.queryset.none()
context = self.get_context_data(filter=self.filterset,
object_list=self.object_list)
return self.render_to_response(context)
That's why filtered data can be also paginated.
So by moving construction of formset to get_context_data
of ModelFormSetMixin
we're able to kill two birds with one stone: queryset will be filtered by django-filters
and paginated by MultiplObjectMixin
. Don't know if it breaks anything tox
tests were giving strange errors about package not being found.
Merging #238 (5ab1611) into master (96b8c68) will increase coverage by
0.06%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #238 +/- ##
==========================================
+ Coverage 88.77% 88.84% +0.06%
==========================================
Files 6 6
Lines 490 493 +3
Branches 54 54
==========================================
+ Hits 435 438 +3
Misses 40 40
Partials 15 15
Impacted Files | Coverage Δ | |
---|---|---|
extra_views/formsets.py | 98.37% <100.00%> (+0.04%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 96b8c68...5ab1611. Read the comment docs.
I'm sorry, I still lack a lot of context. I have to say that it's been a long time since I've really worked on the code base so I'm not familiar with it anymore.
I'm pretty sure that the patch is breaking backwards compatibility even if the tests don't cover it.
I'm sorry, I still lack a lot of context. I have to say that it's been a long time since I've really worked on the code base so I'm not familiar with it anymore.
I'm pretty sure that the patch is breaking backwards compatibility even if the tests don't cover it.
Actually I only removed get_formset_kwargs
which was doing only kwargs["queryset"] = self.get_queryset()
so I doubt someones code rely heavily on this part of code. As for get_context_data
it executes super first and then does some other stuff, so unless someones get_context_data
is not calling super then nothing will be broken.
At the very least we'd need to have a deprecation path for the removed get_formset_kwargs
. Maybe also something for the new keys in get_context_data
.
At the very least we'd need to have a deprecation path for the removed
get_formset_kwargs
. Maybe also something for the new keys inget_context_data
.
Please review the code once more, only thing changed is that kwargs["queryset"] = self.get_queryset()
removed, which we do again in get_context_data
: formset_kwargs["queryset"] = context['object_list']
. There's nothing deprecated as get_formset_kwargs
from FormSetMixin
is still triggered and it does all the heavy job.
What if someone called construct_formset
? If I'm not mistaken it will now miss the queryset
kwarg.
Changing this package to better integrate with django-filter
and pagination seems a good idea. I see a few issues with this change to move the queryset
kwarg to be passed in as part of get_context_data
:
construct_formset()
will still be called as part of ProcessFormSetView.get()
. Doesn't this mean that formset will be constructed twice, first without the queryset
kwarg passed, then with it?ProcessFormSetView.post()
is called, if the formset is valid then get_context_data
is not called, which means the queryset
kwarg is again not passed in. Might this cause an issue?
It's now even able to handle filtering with
django-filter
: