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.

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

Refactor pages revisions_revert view to be a subclass of EditView #12690

Conversation

laymonage
Copy link
Member

Currently, the code that handles the POST request for reverting a revision lives in the EditView class, while the revisions_revert view is a smaller view that tries to "mimic" the EditView for rendering the view as part of a GET request. The view injects the revision ID into the form, which has the action URL hardcoded to the EditView. Including the revision ID in the form allows the EditView to tell whether it's in a "reverting" mode or not, and adjust the POST logic accordingly.

However, this results in possible inconsistencies in both views. Whenever we want to change EditView code or template, we need to make sure to also update the revisions_revert view. The fact that the revisions_revert view is a function-based view doesn't help.

Instead of copying the view code and reusing the template with the addition of injecting the revision ID in the form, turn it into a proper subclass of the EditView, and make use of Django's URL patterns to retrieve the revision ID in the EditView.

This approach is similar to how reverting revisions is handled for snippets.

Ideally, all the code for handling revisions revert should live in the RevisionsRevertView, and the EditView shouldn't know about it at all. This is how it's done for snippets: all the revisions revert-related code is put in RevisionsRevertMixin.

However, this is currently not possible for pages without more significant refactoring, so this commit does the minimal change needed to achieve the goal of keeping the revisions_revert view in sync with the EditView.

Currently, the code that handles the POST request for reverting a
revision lives in the EditView class, while the revisions_revert view is
a smaller view that tries to "mimic" the EditView for rendering the view
as part of a GET request. The view injects the revision ID into the
form, which has the action URL hardcoded to the EditView. Including the
revision ID in the form allows the EditView to tell whether it's in a
"reverting" mode or not, and adjust the POST logic accordingly.

However, this results in possible inconsistencies in both views.
Whenever we want to change EditView code or template, we need to make
sure to also update the revisions_revert view. The fact that the
revisions_revert view is a function-based view doesn't help.

Instead of copying the view code and reusing the template with the
addition of injecting the revision ID in the form, turn it into a proper
subclass of the EditView, and make use of Django's URL patterns to
retrieve the revision ID in the EditView.

This approach is similar to how reverting revisions is handled for
snippets.

Ideally, all the code for handling revisions revert should live in the
RevisionsRevertView, and the EditView shouldn't know about it at all.
This is how it's done for snippets: all the revisions revert-related
code is put in RevisionsRevertMixin.

However, this is currently not possible for pages without more
significant refactoring, so this commit does the minimal change needed
to achieve the goal of keeping the revisions_revert view in sync with
the EditView.
@laymonage laymonage added this to the 6.4* milestone Dec 12, 2024
Copy link

squash-labs bot commented Dec 12, 2024

Manage this branch in Squash

Test this branch here: https://laymonagepage-revisions-revert-0o1qr.squash.io

Copy link
Collaborator

@gasman gasman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice bit of cleanup here.

gasman pushed a commit that referenced this pull request Dec 18, 2024
…2690)

Currently, the code that handles the POST request for reverting a
revision lives in the EditView class, while the revisions_revert view is
a smaller view that tries to "mimic" the EditView for rendering the view
as part of a GET request. The view injects the revision ID into the
form, which has the action URL hardcoded to the EditView. Including the
revision ID in the form allows the EditView to tell whether it's in a
"reverting" mode or not, and adjust the POST logic accordingly.

However, this results in possible inconsistencies in both views.
Whenever we want to change EditView code or template, we need to make
sure to also update the revisions_revert view. The fact that the
revisions_revert view is a function-based view doesn't help.

Instead of copying the view code and reusing the template with the
addition of injecting the revision ID in the form, turn it into a proper
subclass of the EditView, and make use of Django's URL patterns to
retrieve the revision ID in the EditView.

This approach is similar to how reverting revisions is handled for
snippets.

Ideally, all the code for handling revisions revert should live in the
RevisionsRevertView, and the EditView shouldn't know about it at all.
This is how it's done for snippets: all the revisions revert-related
code is put in RevisionsRevertMixin.

However, this is currently not possible for pages without more
significant refactoring, so this commit does the minimal change needed
to achieve the goal of keeping the revisions_revert view in sync with
the EditView.
@gasman
Copy link
Collaborator

gasman commented Dec 18, 2024

Merged in 6b33690

@gasman gasman closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants