Handle pagination in ModelFormSetMixin

[Real-Gecko]

It's now even able to handle filtering with django-filter:

class ManageClients(PermissionRequiredMixin, ContextTitleMixin, BaseModelFormSetView, FilterView):
    permission_required = 'clients.manage_clients'
    model = Client
    form_class = ClientForm
    filterset_class = ClientFilter
    fields = '__all__'
    template_name = 'clients/manage_clients.html'
    factory_kwargs = {'can_delete': True}
    title = _('Manage clients')
    ordering = 'date'
    paginate_by = 10

[jonashaag]

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?

[Real-Gecko]

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.

[codecov-commenter]

Codecov Report

Merging #238 (5ab1611) into master (96b8c68) will increase coverage by 0.06%. The diff coverage is 100.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.

[jonashaag]

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.

[Real-Gecko]

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.

[jonashaag]

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.

[Real-Gecko]

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.

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.

[jonashaag]

What if someone called construct_formset? If I'm not mistaken it will now miss the queryset kwarg.

[sdolemelipone]

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:

1. 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?
2. When 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?