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

m2m and fk fields of Version objects are same as present values (not updating m2m and fk fields) #966

Closed josidridolfo closed 4 months ago

josidridolfo commented 5 months ago

First off, thank you for your developing and maintaining this app. It's extremely useful!

I'm trying to implement version control in a web app, and I'm facing difficulties retrieving historic m2m and fk fields. Here's the problem in a nutshell:

When a user updates a model, that model's state will change from 'APPROVED' to 'PENDING'. While in PENDING, all users should see the most recent APPROVED state; the PENDING state will be visible only for a privileged user who can either APPROVE or REJECT the changes.

All relevant models are registered with the @reversion.register(), and follow=['m2m_field_1', 'm2m_field_2', 'fk_field_etc'] is included as a param as needed. These models also inherit an abstract Stateful class that is used for checking and updating the model's state.

I've written some code that retrieves the most recent APPROVED version of any object; however, the m2m and FK fields aren't present as expected:

    def get_most_recent_approved(self, under_review=False):
        """
        This method returns the most recent version of the object with an APPROVED state,
        or None if no such record exists.
        """
        original = type(self).objects.get(pk=self.pk)
        # under_review is used for privileged users who can see PENDING objects
        if original.state == APPROVED or under_review:
            return original  # Current object is approved or new, return it directly.
        # Retrieve all version records for this instance
        versions = Version.objects.get_for_object(original) # get the VersionQuerySet
        most_recent_approved = None
        for version in versions: # Iterate over VersionQuerySet until an APPROVED Version is found
            version_object = version._object_version.object
            if getattr(version_object, 'state', None) == APPROVED:
                most_recent_approved = self.get_object_version_m2m(version, version_object)
                return most_recent_approved
        return original

The challenge here is now updating the m2m and fk fields on the object that will be returned to the view. This is attempted in the following method:

    def get_object_version_m2m(self, version, version_object):
        """
        Extracts and reconstruct the m2m fields for a given version Object.
        """
        data = version.serialized_data # Why do I have this? Is this necessary?
        version_values = version.field_dict
        related_objects = version.revision.version_set.all()
        for key, value in version_values.items():
            print(key, value) # For seeing what's happening under the hood
            if key != 'state':
                version_object.key = value # This doesn't seem to be working....
        if version_object.state == APPROVED:
            version_object.set_as_pending() # Set state for view, but do not save
        return version_object

Is there a more elegant way of retrieving the past m2m and fk fields and correctly modifying the returned object so that those historic fields are provided?

etianen commented 5 months ago

Thanks for taking the time to explain your problem!

django-reversion might not be the best tool for what you're trying to do. It's made a few design decisions that go against your goals:

  1. django-reversion stores all past versions using the django serialization framework. This means it works with any model and any model field, but the downside is that it becomes very slow to go through past versions of a model looking for a particular property (e.g. state).

  2. django-reversion assumes only the "latest" version of an object is interesting, and all other versions exist for auditing and rollback. This decision is again optimized for ease of integration with any model, but it does narrow the use-cases.

In your case, you want to easily be able to access two versions of a model:

I think you'll have a better long-term experience, and much better performance, by building this logic into your database table itself.

josidridolfo commented 4 months ago

Hi! Sorry for the delay and thanks for your response!

Re: design decisions - if anyone else is considering using django-reversion for a use case similar to my own, please heed @etianen's advice and build the logic required into the database itself. It's probably a good idea to have - for any Object that requires a state or other attributes similar to my use case as described above - a separate ObjectHistory table.

This blog post here was extremely helpful for me; it might be for you, too: https://kaustavdm.in/versioning-content-postgresql/

@etianen - once again, thank you for building and maintaining this, and thank you for sharing your wisdom on how to get better performance and guidance re: design!

etianen commented 3 months ago

Sorry I neglected this! I'm super busy with a new baby at the moment, and my open source projects have suffered!

I'm glad you figured out a good solution, and thanks for posting an update and a good link!