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

fix: Remove patching the django CMS core #300

Merged
merged 25 commits into from
Dec 29, 2022

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Nov 25, 2022

Description

Patching easily introduces side-effects, especially if patches happen at diverse places in the code base. This PR removes all patches to the django CMS core.

Mixin for CMSToolbar using django CMS new config option to provide a mixin for the cms.toolbar.toolbar.CMSToolbar class.

See this PR.

Creating Version instances when a versioned model is created

So far, djangocms-versioning puts the burden of creating Version objects for versioned models on the app owning the versioned models. To keep the core agnostic of versioning capabilities. This PR lets djangocms-versioning automatically create a draft Version object as soon as a versioned object is created. This is achieved by modifying the versioned model's manager's create method.

Since the Version model requires a User object the following syntax has been introduced:

MyVersionedModel.with_user(request.user).create(...)

The with_user() statement tells versioning which user the first version of that object will be assigned to and also automatically creates a Version object.

If the with_user() statement is missing, djangocms-verisoning will issue a warning and not create a Version object. It will not raise an exception to stay backwards compatible.

Patches to PageContentAdmin

These patches have been moved to the already existing VersioningCMSPageAdminMixin where they belong.

Patches to PageExtension and PageContentExtension

These can be handled by allowing to already used signals to forward changes to the extension objects to the extended objects.

Performance/feature patches

Patches which are not related to versioning functionality have been moved to the core with this PR.

Changes to Django's ORM

Versioning introduces changes to Django's handling of versioned models. These changes are, of course, retained:

  • Replacing the manager for versioned models
  • Removing the unique together constraint for PageContent model

Compatibility

This PR will not work with versions 4.0.x of django cms since they do not offer the CMSToolbar configuration.

Related resources

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #300 (7b60aca) into master (5fe601b) will increase coverage by 1.06%.
The diff coverage is 86.88%.

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
+ Coverage   88.68%   89.75%   +1.06%     
==========================================
  Files          76       67       -9     
  Lines        2334     2196     -138     
  Branches      298      293       -5     
==========================================
- Hits         2070     1971      -99     
+ Misses        203      170      -33     
+ Partials       61       55       -6     
Impacted Files Coverage Δ
djangocms_versioning/cms_config.py 80.29% <79.54%> (-0.92%) ⬇️
djangocms_versioning/managers.py 82.35% <83.72%> (+7.35%) ⬆️
djangocms_versioning/datastructures.py 95.55% <100.00%> (ø)
djangocms_versioning/handlers.py 97.14% <100.00%> (+0.26%) ⬆️
djangocms_versioning/helpers.py 91.11% <100.00%> (+0.20%) ⬆️
djangocms_versioning/plugin_rendering.py 54.16% <100.00%> (+12.06%) ⬆️
djangocms_versioning/test_utils/factories.py 94.50% <100.00%> (+0.21%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 25, 2022

This pull request introduces 1 alert when merging 02ceb4b into 31f14df - view on LGTM.com

new alerts:

  • 1 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

# Conflicts:
#	djangocms_versioning/monkeypatch/extensions.py
#	djangocms_versioning/monkeypatch/page.py
#	tests/test_integration_with_core.py
add tests for current_content_iterator
…ve-patching

# Conflicts:
#	djangocms_versioning/cms_config.py
#	djangocms_versioning/monkeypatch/templatetags.py
#	tests/requirements/requirements_base.txt
#	tests/test_integration_with_core.py
@fsbraun fsbraun merged commit 4567b34 into django-cms:master Dec 29, 2022
@fsbraun fsbraun deleted the fix/remove-patching branch January 10, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants