Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Snippets with RevisionMixin but not also DraftStateMixin crash. #857

Open
alexalligator opened this issue Feb 28, 2025 · 4 comments · May be fixed by #859
Open

Snippets with RevisionMixin but not also DraftStateMixin crash. #857

alexalligator opened this issue Feb 28, 2025 · 4 comments · May be fixed by #859

Comments

@alexalligator
Copy link

Hi there and thanks for this great library.

I would like to draw your attention to this code here:

wagtail-localize/wagtail_localize/models.py

Lines 807 to 816 in 7797bd3

elif isinstance(translation, RevisionMixin):
# We copied another instance which may be live, so we make sure this one matches the desired state
translation.live = publish
translation.save()
# Create a new revision of the Snippet
new_revision = translation.save_revision(user=user)
if publish:
transaction.on_commit(new_revision.publish)

Taken from TranslationSource.create_or_update_translation.

Here a new translation is being saved. The code checks to see if the translated object is a RevisionMixin and if so it creates a new_revision and then publishes it.

Revision from wagtail.models defines a publish() method:

    # wagtail/models/__init__.py
    def publish(
        self,
        user=None,
        changed=True,
        log_action=True,
        previous_revision=None,
        skip_permission_checks=False,
    ):
        return self.content_object.publish(
            self,
            user=user,
            changed=changed,
            log_action=log_action,
            previous_revision=previous_revision,
            skip_permission_checks=skip_permission_checks,
        )

Note that it in turn calls self.content_object.publish, where self.content_object is essentially the translated object we started with.

The problem is that adding RevisionMixin to, say, a snippet does not give it a publish() method, so you get an AttributeError on self.content_object.

DraftStateMixin does provide a publish() method so snippets that have both drafts and revisions are fine but those with just revisions crash.

Something along the lines of the following might fix this:

elif isinstance(translation, RevisionMixin):
    # We copied another instance which may be live, so we make sure this one matches the desired state
     translation.live = publish
     translation.save()

    # Create a new revision of the Snippet
    new_revision = translation.save_revision(user=user)

     if publish and isinstance(translation, DraftStateMixin): # <--- extra check
         transaction.on_commit(new_revision.publish)
@zerolab
Copy link
Collaborator

zerolab commented Feb 28, 2025

@alexalligator thank you for this. That is a great catch. The suggested fix sounds about right. Would be good to double check what a snippet with only RevisionMixin does. i.e. if you save a new version, is that live? I suspect that is the case, but would appreciate confirmation

@alexalligator
Copy link
Author

This is my understanding:

There is not concept of live or unpublished_changes with RevisionMixin. That stuff comes from DraftStateMixin.

You can think of it as just a vanilla snippet instance that keeps a log of timestamped, serialized versions of itself (Revisions). These can be compared and restored in the admin HistoryView.

You interact with the model itself as you would any vanilla Django model. There's no need to work out which 'revision' is live.

So it's just a sort of audit log feature for the model. That's what we use it for.

@zerolab
Copy link
Collaborator

zerolab commented Feb 28, 2025 •
edited
Loading

Thanks Alex, that is my understanding of it as well. It is always good to get a sanity check ;) and I am glad we're on the same page.

Do you have time to submit a PR?
This may need either new snippet model or adapting an existing one to add RevisionMixin to it and use it in a test or two. The original PR for supporting revisionable snippets was #751

@alexalligator
Copy link
Author

Yep, will do. Glad to finally be able to give a little something back.

alexalligator added a commit to alexalligator/wagtail-localize that referenced this issue Mar 3, 2025
@alexalligator
Snippets with RevisionMixin but not also DraftStateMixin crash.
6f50989 
Fixes wagtail#857
@alexalligator alexalligator linked a pull request Mar 3, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@zerolab @alexalligator