-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Closed
laymonage
wants to merge
1
commit into
wagtail:main
from
laymonage:page-revisions-revert-extend-edit-view
Closed
Refactor pages revisions_revert
view to be a subclass of EditView
#12690
laymonage
wants to merge
1
commit into
wagtail:main
from
laymonage:page-revisions-revert-extend-edit-view
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
added
type:Cleanup/Optimisation
component:Page
component:Revisions
RevisionMixin and revision handling
labels
Dec 12, 2024
Manage this branch in SquashTest this branch here: https://laymonagepage-revisions-revert-0o1qr.squash.io |
12 tasks
gasman
approved these changes
Dec 18, 2024
There was a problem hiding this 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.
Merged in 6b33690 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the code that handles the POST request for reverting a revision lives in the
EditView
class, while therevisions_revert
view is a smaller view that tries to "mimic" theEditView
for rendering the view as part of aGET
request. The view injects the revision ID into the form, which has the action URL hardcoded to theEditView
. Including the revision ID in the form allows theEditView
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 therevisions_revert
view. The fact that therevisions_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 theEditView
.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 theEditView
shouldn't know about it at all. This is how it's done for snippets: all the revisions revert-related code is put inRevisionsRevertMixin
.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 theEditView
.