"Revert" feature in admin reverts entire revision, not specific version

[simonw]

Just got caught out by this one. This button here, on /admin/app/model/recover/ID/:

Calls this code here:

django-reversion/reversion/admin.py

Lines 164 to 172 in df3bcf7

	def _reversion_revisionform_view(self, request, version, template_name, extra_context=None):
	# Check that database transactions are supported.
	if not connection.features.uses_savepoints:
	raise ImproperlyConfigured("Cannot use VersionAdmin with a database that does not support savepoints.")
	# Run the view.
	try:
	with transaction.atomic(using=version.db):
	# Revert the revision.
	version.revision.revert(delete=True)

Note that it's calling version.revision.revert(delete=True) here.

The problem is, django-reversion supports bundling multiple changes to different objects up into a single revision. In our application we use this to bundle up a bunch of bulk-edits made by an API endpoint, using code that looks like this:

    with reversion.create_revision():
        for location in locations_to_resolve:
            location.county = resolve_county(location)
            location.save()
        reversion.set_comment("/api/resolveMissingCounties")

We used this to update twenty location objects at once. But then... someone clicked the "Save" button to recover an older version of one of those twenty objects, and all twenty were invisibly reverted!

Some potential fixes:

• Fix the implementation of _reversion_revisionform_view to only affect the specific object, not touching any of the other objects that were bundled into the same revision. That's my preferred fix!
• Make it clear in the documentation that bundling multiple changes together across multiple objects in the same revision is not recommended, and will result in this behaviour

[simonw]

Looks like this is the same root problem as #798

[etianen]

This is an interesting problem. I think it should be possible to introspect the admin class and only revert the specific object. There's a few variations of this approach that might be worth supporting.

1. Revert entire revision (current approach)
2. Revert current model and all inlines
3. Revert current model, but no inlines (as requested in Admin: Issues when updating records through list_editable fields #798)

I'd be reluctant to change the defaut behaviour, as it could break existing deployments. Adding in the option to enable (2) or (3) on a per-admin basic would make sense though.

[zyv]

We were recently bit by this issue as well... our use case was as follows:

1. Users primarily edit objects via Django admin
2. At some point we wrote a migration, which bundled updates to many objects in one revision
3. User clicked "delete" in Django admin, but reversion didn't record a new revision upon deletion for the model to be deleted
4. User clicked "restore" in Django admin and the complete last revision was reverted, causing many unrelated objects to be updated to the old state

Our naive understanding is that this wouldn't have happened, if when deleting a model via Django admin a new revision had been created for this model. This way, when restoring the deleted object, this revision created immediately before deletion would have been reverted, and not the last one, which happened to contain edits to many unrelated objects.

Could @etianen please explain why no revision is created upon deletion of the objects via Django admin? It seems to me that if this was done, then the use case of @simonw would have been covered as well, and it doesn't have the downsides of the two mentioned alternative approaches, so maybe it can be made to the default behaviour...

[etianen]

Unfortunately, saving deletions as revisions was removed as a mis-feature a long while ago. See #164 for an in-depth discussion.