etianen / django-reversion

django-reversion is an extension to the Django web framework that provides version control for model instances.
https://django-reversion.readthedocs.io
BSD 3-Clause "New" or "Revised" License
3.03k stars 489 forks source link

Admin integration: Issue when reverting #948

Closed julianklotz closed 11 months ago

julianklotz commented 1 year ago

When trying to revert an object (a proxy model) through the admin, I get a “version not found” error, raised in the else block of the following code in admin.py.

The request.method is – in my case - "GET", because I redirect the changeform_view method of the concrete model to the changeform view if the proxy model. This causes the revision admin to fail, even though the object has been restored successfully.

I was wondering if the request.method == 'POST' check is required here, since the “302” redirect should be enough to indicate that it’s fine.

What do you think?

with self.create_revision(request):
                    response = self.changeform_view(request, quote(version.object_id), request.path, extra_context)
                    # Decide on whether the keep the changes.
                    if request.method == "POST" and response.status_code == 302:
                        set_comment(_("Reverted to previous version, saved on %(datetime)s") % {
                            "datetime": localize(template_localtime(version.revision.date_created)),
                        })
                    elif response.status_code == 200:
                        response.template_name = template_name  # Set the template name to the correct template.
                        response.render()  # Eagerly render the response, so it's using the latest version.
                        raise _RollBackRevisionView(response)  # Raise exception to undo the transaction and revision.
                    else:

                        raise RevertError(_("Could not load %(object_repr)s version - not found") % {
                            "object_repr": self.object_repr,
                        })
etianen commented 1 year ago

I don't mind tweaking this so long as we don't break existing functionality.

Here's the lifecycle:

  1. User views the preview: GET -> 200
  2. Use performs the rollback POST -> 302

At what point is your 302 redirect given? Is is before the preview, or after the rollback?

julianklotz commented 11 months ago

Sorry, my bad. Fixed it a few days ago. But here’s some context and the solution. No need for changes from your side.


# models.py
class Media:

    class TypeChoices:
        PHOTO = 'PH', 'photo'
        VIDEO = 'VI', 'video'

    proxy_map = {
        TypeChoices.PHOTO: "Photo",
        TypeChoices.VIDEO: "Video",
    }

    type = fields.CharField(choices=TypeChoices.choices, max_length=20, db_index=True)
    file = fields.FileField(...)

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self._cast_to_proxy()
    def get_edit_url(self):
        # Build a dynamic edit url here
        return "the-url"

    def _cast_to_proxy(self):
        if not self.type:
            return

        proxy_model_name = self.proxy_map.get(self.type, None)

        if not proxy_model_name:
            return

        proxy_model = apps.get_model(self._meta.app_label, proxy_model_name)
        self.__class__ = proxy_model

class Photo(Media):
    class Meta:
        proxy = True

class Video(Media):
    class Meta:
        proxy = True

# admin.py

class MediaAdmin:
    def render_change_form(
        self, request, context, add=False, change=False, form_url="", obj=None
    ):
        """
        In some cases, an admin is only registered e.g. to provide auto complete,
        but editing is done through proxy models (and their admin classes).
        """

        # Revert is set when restoring object through the model admin.
        # When the revert flag is set, don't mess around with the redirects.
        revert = context.get("revert", False) # => This is how I fixed it.

        if obj is not None and revert is False:
            # Without the revert check, this would cause an exception, because reversion expects a 20x, not a redirect (30x)
            edit_url = obj.get_edit_url()

            if not request.path.startswith(edit_url):
                return HttpResponseRedirect(
                    f"{obj.get_edit_url()}?{request.GET.urlencode()}"
                )

        return super().render_change_form(request, context, add, change, form_url, obj)
etianen commented 11 months ago

Thanks for the context! Glad you sorted it yourself!