Skip to content

fix: Added the new delete confirmation for pages also to delete translation #8111

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

Merged
merged 13 commits into from
Jan 30, 2025

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Jan 15, 2025

Description

#8070 added an improved delete confirmation message for pages. The confirmation message for "delete translation" was kept unchanged. This PR adds the same message for delete translation:

image

  • The format is identical to avoid confusion (delete translation will never delete a page)
  • Implementation collects the code for both the PageAdmin and PageContentAdmin in a common mixin
  • It respects the need to delete more than one PageContent objects when using versioning.

Backport needed for the last point to the release/4.1.x branch.

Related resources

Checklist

  • I have opened this pull request against develop-4
  • 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 the channel #pr-reviews on our Discord Server to find a “pr review buddy” who is going to review my pull request.

Summary by Sourcery

Standardize the delete confirmation message for pages and translations.

Enhancements:

  • Use a common mixin to handle delete confirmations for both PageAdmin and PageContentAdmin.
  • Improve the delete confirmation message for page translations to match the format of the improved message for pages introduced in feat: Improved delete page confirmation message #8070.

Copy link
Contributor

sourcery-ai bot commented Jan 15, 2025

Reviewer's Guide by Sourcery

This pull request improves the delete confirmation message for page translations by using the same message as for deleting pages. It also refactors the code to use a common mixin for both PageAdmin and PageContentAdmin, and handles the deletion of multiple PageContent objects when versioning is enabled.

Sequence diagram for the improved page translation deletion process

sequenceDiagram
    actor Admin
    participant PA as PageContentAdmin
    participant PDM as PageDeleteMessageMixin
    participant PC as PageContent
    participant P as Page
    participant PL as Plugins

    Admin->>PA: Request delete translation
    PA->>PDM: get_deleted_objects()
    PDM-->>PA: Filtered objects to delete

    PA->>PC: Delete page content
    PA->>P: Update page cache
    PA->>PL: Delete associated plugins

    Note over PA,PL: Now handles multiple PageContent objects<br/>when versioning is enabled

    PA-->>Admin: Show success message
Loading

Class diagram showing the new PageDeleteMessageMixin inheritance

classDiagram
    class PageDeleteMessageMixin {
      +delete_confirmation_template: str
      +get_deleted_objects(objs, request)
    }
    class PageAdmin {
    }
    class PageContentAdmin {
    }
    class ModelAdmin {
    }
    ModelAdmin <|-- PageAdmin
    ModelAdmin <|-- PageContentAdmin
    PageDeleteMessageMixin <|-- PageAdmin
    PageDeleteMessageMixin <|-- PageContentAdmin
    note for PageDeleteMessageMixin "New mixin for shared delete confirmation logic"
Loading

File-Level Changes

Change Details Files
Added a common mixin for the delete confirmation message.
  • Created PageDeleteMessageMixin to handle the improved delete confirmation message.
  • Used PageDeleteMessageMixin in both PageAdmin and PageContentAdmin.
cms/admin/pageadmin.py
Improved the delete confirmation message for translations.
  • Standardized the delete confirmation message for both pages and translations.
  • Updated tests to reflect the changes in the confirmation message.
cms/admin/pageadmin.py
cms/tests/test_admin.py
Updated database migration for menus.
  • Changed the id field in the CacheKey model to BigAutoField.
menus/migrations/0002_alter_cachekey_id.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 @fsbraun - I've reviewed your changes - here's some feedback:

Overall Comments:

  • There are commented out test assertions in test_delete_translation() - these should either be fixed to match the new behavior or an explanation provided for why they were disabled
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good
  • 🟢 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.

Copy link
Contributor

@jrief jrief left a comment

Choose a reason for hiding this comment

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

my team tested all the functionally

fsbraun added a commit that referenced this pull request Jan 22, 2025
….1 (#8112)

* backport: Delete translation deletes all page content objects with the page/language combination

* fix typo

* Fix message action

* Update cms/admin/pageadmin.py
@fsbraun fsbraun added the 5.0 label Jan 27, 2025
@fsbraun fsbraun merged commit df40666 into django-cms:develop-4 Jan 30, 2025
51 checks passed
@fsbraun fsbraun deleted the fix/translation-delete branch January 30, 2025 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants