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]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.84%. Comparing base (96b8c68) to head (5ab1611).
⚠️ Report is 26 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[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?