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

Extract revision and draft state logic from generic views into mixins #9709

Merged
merged 4 commits into from Dec 6, 2022

Conversation

laymonage
Copy link
Member

@laymonage laymonage commented Nov 23, 2022

Fixes #9710.

An alternate approach to #9190. I initially tried to create a separate mixin for each corresponding model mixin, and dynamically applying the view mixins in SnippetViewSet based on the model class. However, that seems to add more complexity as we'll need to have one view mixin for each model, for each view (e.g. CreateViewDraftStateMixin, EditViewDraftStateMixin). It will be very hard to make sure all the combinations work correctly, especially with the upcoming LockableMixin (and soon WorkflowMixin) integrations.

After tinkering around, I think it's worth to just combine all the model mixins logic into a single mixin for each view (XViewOptionalFeaturesMixin). This allows us to handle the model mixin combinations in each method in a simpler way (using if checks), while still removing the model mixin logic from the generic views.

I'd suggest reviewing each individual commit. The last commit combines the mixins for CreateView and EditView into a single mixin as some methods are exactly the same (publish_action(), form_valid(), etc.). Not sure if the reusability vs readability tradeoff is worth it, though. I'd be happy to remove that commit if desired.

I also noticed some method overrides in snippets views are no longer necessary as of #9221, so I removed them.

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.

Please describe additional details for testing this change.

Footnotes

  1. Development Testing

@squash-labs
Copy link

squash-labs bot commented Nov 23, 2022

Manage this branch in Squash

Test this branch here: https://laymonagerefactor-generic-view-kxfpy.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.

Thanks - I'm happy with all of this! Good to have these special cases separated out from the rest of the admin.views.generic.models logic.

@gasman gasman merged commit 2f5d419 into wagtail:main Dec 6, 2022
gasman added a commit that referenced this pull request Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract revisions and draft state logic from generic views
2 participants