Consistent queryset creation in ModelAdmin export mixin

[19greg96]

Problem

#1864

Solution

The new flow of queryset acquisition is:

• export_action calls get_export_queryset
• get_export_queryset gets the queryset from the ModelAdmin through the ModelAdmin's ChangeList, which uses the ModelAdmin's get_queryset
• This queryset is passed to _do_file_export when is_skip_export_form_enabled() is True
• Otherwise it is conditionally filtered if "export_items" in form.changed_data
• Finally the queryset is passed to _do_file_export

Acceptance Criteria

• Added three tests to check all paths related to export queryset from the admin mixins
• Backend change, no UI updates were done, so no screenshots
• Documentation does not detail the queryset usage of the ModelAdmin mixins

[coveralls]

coverage: 100.0%. remained the same
when pulling 9e4a91a on 19greg96:consistent-queryset-creation-1864
into 06557c3 on django-import-export:main.

[19greg96]

Thank you!

[coveralls]

coverage: 100.0%. remained the same
when pulling b101ea2 on 19greg96:consistent-queryset-creation-1864
into 06557c3 on django-import-export:main.

[matthewhegarty]

Thanks for looking into this. It looks ok to me.

I added release notes and changelog updates.

I'll release this in v4.2 because it is a minor change to existing behaviour.

I did look to see whether we should remove get_valid_export_item_pks(). It makes sense to provide a method which can be overridden by the user, but that can now be done by overriding the ModelAdmin:

class CategoryAdmin(ExportActionModelAdmin):
    def get_queryset(self, request):
        return Category.objects.all()

I can't see any issue with doing this if I document the change correctly.

Any thoughts?

[19greg96]

Thanks for looking into this. It looks ok to me.

Thank you for reviewing it, and sorry for sitting on it for so long, I had other matters to tend to.

I added release notes and changelog updates.

I'll release this in v4.2 because it is a minor change to existing behaviour.

Great! Thanks a lot!

I did look to see whether we should remove get_valid_export_item_pks(). It makes sense to provide a method which can be overridden by the user, but that can now be done by overriding the ModelAdmin:

class CategoryAdmin(ExportActionModelAdmin):
    def get_queryset(self, request):
        return Category.objects.all()

I can't see any issue with doing this if I document the change correctly.

Any thoughts?

I was thinking about this, but decided not to add it to this PR, as it would make the breaking change block the non-breaking change from being merged.

The idea is that if the ModelAdmin's get_queryset is used, that restricts the is_skip_export_form_enabled() == True path as well as the other one. This would make the behaviour more reliable, as currently the get_valid_export_item_pks is only used when the export form is not skipped.

django-import-export/import_export/admin.py

Lines 732 to 739 in b101ea2

	if request.POST and "export_items" in request.POST:
	# this field is instantiated if the export is POSTed from the
	# 'action' drop down
	form.fields["export_items"] = MultipleChoiceField(
	widget=MultipleHiddenInput,
	required=False,
	choices=[(pk, pk) for pk in self.get_valid_export_item_pks(request)],
	)

To:

        if request.POST and "export_items" in request.POST:
            # this field is instantiated if the export is POSTed from the
            # 'action' drop down
            form.fields["export_items"] = MultipleChoiceField(
                widget=MultipleHiddenInput,
                required=False,
                choices=[(pk, pk) for pk in queryset.values_list("pk", flat=True)],
            )

How shall we move forward with this? Should I add the deprecation to this PR and open a new PR for the actual change? Or should I open a new PR for both?

[matthewhegarty]

Should I add the deprecation to this PR and open a new PR for the actual change?

Yes, I think that is best if that's ok with you.

[19greg96]

Should I add the deprecation to this PR and open a new PR for the actual change?

Yes, I think that is best if that's ok with you.

Done. I created a PR for the removal, but that should really be based on main, after this is merged.

[matthewhegarty]

Added a comment about where to put the deprecation warning.

[coveralls]

coverage: 100.0%. remained the same
when pulling 3aee3b1 on 19greg96:consistent-queryset-creation-1864
into 06557c3 on django-import-export:main.

[coveralls]

coverage: 100.0%. remained the same
when pulling 4047ef6 on 19greg96:consistent-queryset-creation-1864
into 06557c3 on django-import-export:main.

[matthewhegarty]

Thanks for all your efforts on this. I added a response re the deprecation warning.

[coveralls]

coverage: 100.0%. remained the same
when pulling 63c38ba on 19greg96:consistent-queryset-creation-1864
into 06557c3 on django-import-export:main.

[coveralls]

coverage: 100.0%. remained the same
when pulling 6e0128f on 19greg96:consistent-queryset-creation-1864
into a29711c on django-import-export:main.