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

Add Django CMS 4 support #38

Merged
merged 7 commits into from
Nov 19, 2024
Merged

Conversation

stefanw
Copy link
Contributor

@stefanw stefanw commented Jun 17, 2024

Description

This PR makes djangocms-transfer roughly work with Django CMS 4.1 installations.
We needed import/export between different installations and this works for us.

Maybe this can be a starting point for a 4.1 compatible release.

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.

Summary by Sourcery

Add support for Django CMS 4.1 in the djangocms-transfer package, enabling import/export functionality. Refactor methods to handle many-to-many relationships and related model fields more effectively, and simplify plugin import and export processes by removing unnecessary operations.

New Features:

  • Add support for Django CMS 4.1 in the djangocms-transfer package, enabling import/export functionality between different installations.

Enhancements:

  • Refactor the restore method to handle many-to-many relationships and related model fields more effectively.
  • Simplify the import_plugins function by removing unnecessary tree order handling and plugin reordering.
  • Improve the get_plugin_export_data function by removing the order_by clause for descendants.

@fsbraun
Copy link
Member

fsbraun commented Jun 17, 2024

@stefanw Thanks for this work. Looks good at first sight! I will review in more detail, but

  • Uses API to create plugins 👍
  • Respects CMS 4 toolbar object 👍 (here, we might need to look at separate code for v3 and v4)

@fsbraun
Copy link
Member

fsbraun commented Nov 19, 2024

@sourcery-ai review

Copy link
Contributor

sourcery-ai bot commented Nov 19, 2024

Reviewer's Guide by Sourcery

This PR updates djangocms-transfer to be compatible with Django CMS 4.1 by refactoring the plugin import/export functionality and updating page content handling. The main changes focus on modernizing the plugin restoration process, handling many-to-many relationships, and adapting to Django CMS 4's new page content model.

Class diagram for updated plugin restoration process

classDiagram
    class PluginRestoration {
        +restore(placeholder, language, parent)
        -m2m_data: dict
        -data: dict
    }
    class CMSPlugin {
        +add_root()
        +set_base_attr()
    }
    class Model {
        +_meta: Meta
    }
    class Meta {
        +get_fields()
    }
    PluginRestoration --> CMSPlugin : uses
    PluginRestoration --> Model : uses
    Model --> Meta : has
    note for PluginRestoration "Handles plugin restoration with m2m relationships"
Loading

Class diagram for updated page content handling

classDiagram
    class PageContent {
        +pk
        +get_slug(language)
    }
    class ExportImportForm {
        +cms_page: ModelChoiceField
        +get_filename()
    }
    ExportImportForm --> PageContent : uses
    note for ExportImportForm "Updated to use PageContent model"
Loading

File-Level Changes

Change Details Files
Refactored plugin restoration logic to use modern Django CMS plugin API
  • Replaced manual plugin tree manipulation with add_plugin() function
  • Added support for many-to-many relationships in plugin data
  • Removed manual plugin reordering and tree order management
  • Added handling for related model fields during plugin restoration
djangocms_transfer/datastructures.py
Updated toolbar integration for Django CMS 4 compatibility
  • Changed page draft handling to work with new page model
  • Updated object type checking to use PageContent
  • Modified edit mode detection logic
djangocms_transfer/cms_toolbars.py
Simplified plugin import and export process
  • Removed explicit plugin tree ordering
  • Simplified descendant plugin handling
  • Removed placeholder dirty marking
djangocms_transfer/importer.py
djangocms_transfer/exporter.py
Updated form handling for new PageContent model
  • Changed queryset to use PageContent instead of Page model
  • Updated slug retrieval to handle new page content structure
djangocms_transfer/forms.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @stefanw - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please add tests to verify the changes, especially around plugin restoration and page content handling. This will help ensure compatibility with Django CMS 4.1 without regressions.
  • Follow the conventional commits guidelines to properly document these architectural changes in the commit history.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

for field in fields:
if field.related_model is not None and m2m_data.get(field.name):
if field.many_to_many:
objs = field.related_model.objects.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding error handling for invalid many-to-many relations

The code silently ignores invalid PKs in many-to-many relations. Consider logging or raising warnings when related objects don't exist to prevent silent data loss.

                        requested_pks = m2m_data[field.name]
                        objs = field.related_model.objects.filter(pk__in=requested_pks)
                        if objs.count() != len(requested_pks):
                            logger.warning(
                                f"Some {field.name} relations could not be found: "
                                f"requested {len(requested_pks)}, found {objs.count()}"
                            )

djangocms_transfer/datastructures.py Show resolved Hide resolved
@fsbraun fsbraun merged commit 88ed994 into django-cms:master Nov 19, 2024
0 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants