feat: Re-introduce deleting languages of a page #443
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #443 +/- ##
==========================================
- Coverage 91.09% 90.96% -0.13%
==========================================
Files 72 72
Lines 2685 2702 +17
Branches 310 314 +4
==========================================
+ Hits 2446 2458 +12
- Misses 167 169 +2
- Partials 72 75 +3 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@krosefield Can you install this branch to check if it works to your expectations? I'll be adding some tests in the meantime. |
|
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request reintroduces the functionality to delete page translations when the Sequence diagram for deleting a page translationsequenceDiagram
actor User
participant TB as Toolbar
participant LM as Language Menu
participant DM as Delete Menu
participant PC as PageContent
User->>TB: Access page toolbar
TB->>LM: Open language menu
alt DJANGOCMS_ALLOW_DELETING_VERSIONS is True && CMS version > 4.1.4
LM->>DM: Show 'Delete Translation' submenu
User->>DM: Select language to delete
DM->>PC: Delete page content for selected language
PC-->>User: Redirect to preview/refresh page
else
LM-->>User: Delete Translation menu not shown
end
Class diagram for versioning toolbar componentsclassDiagram
class VersioningToolbar {
+change_language_menu()
}
class PageContent {
+language: str
+pk: int
+delete()
}
class Page {
+get_admin_content(language)
+get_languages()
}
VersioningToolbar --> Page
Page --> PageContent
note for VersioningToolbar "Added delete translation functionality"
note for Page "Enhanced content retrieval for language operations"
Flow diagram for translation deletion decision processgraph TD
A[Check Delete Translation Request] --> B{DJANGOCMS_ALLOW_DELETING_VERSIONS?}
B -->|Yes| C{CMS version > 4.1.4?}
B -->|No| E[Hide Delete Option]
C -->|Yes| D[Show Delete Translation Menu]
C -->|No| E
D --> F{Multiple translations?}
F -->|Yes| G[Enable Delete Option]
F -->|No| H[Disable Delete Option]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@fsbraun Hey. I tested this PR together with @krosefield. First off: the original change request is implemented and works. We now see a "Delete Translations" menu entry and can successfully delete page translations. However, there is one edge case that might need further refinement. Let me explain: Assume a page with translations in DE & EN, with a versioning history and multiple existing (and previously published) version objects; let's call them versions A, B, C. They were created in that order, all have been published. Deleting the english page only removes the latest version C. We would expect that the entire versioning history of that page would get deleted as well. We do understand that this is up to debate though, so this is rather our view and can be discarded if you don't agree with us on this one. But there is a second issue which I can only describe as "inconsistent state" when following the above procedure. Since version C was deleted, we are now left with B as the latest version. Inside the pagecontent admin we also see that this page is marked as "published" with said version. Accessing the German page yields an interesting error: we can't retrieve the english version of the page! Our language switch seems to be unaware of that page from now on, almost as if the page tree is getting disrupted there. The page DOES exist, but it seems like the
This seems like a bug to me, the only viable thing you can do to resolve this as a user is to unpublish the english page, delete all versions and recreate the page from the German one. This was a rather lengthy text, sorry for that, but please ping me if you need any more information. And thank you very much for working on this feature! |
|
@filipweidemann Thanks for the quick reply. I need to think about deleting all versions. That's a great point. (And maybe the reason it was removed.) Concerning your second issue, I believe this is an instance of #441 which was solved in #440. Having a page without published or draft states could shadow other pages/languages. |
|
@fsbraun The way I see it is as follows: you have language-scoped pages, each of them with their own version history. That is fine, maybe we have diverging content for some languages, maybe they are maintained by different teams of people, ... However, when I decide to delete a whole language, I would expect that not only the current version is removed but rather the whole language including versions. I would expect this simply because in my mind, a page consists of many different languages and their contents, and each language has their own contents (and versions). This almost looks like a graph to me. When deleting a language, I would expect their leaf nodes (versions) to be gone as well. But again, think about it, take your time, this is not necessarily the correct approach for this project. I am in no position to judge this. Regarding the error: #440 was merged already, I would expect this to already be present in your feature branch, no? And what about the Also, why would #441 even trigger here? The admin view / page content view clearly tells me that the english page I just deleted is still published. I think this is the other way round, maybe it just looks like #441 because the Or am I missing something here? |
|
Oh nvm, you just merged it 3 hours ago. Any chance to update this branch to include #440 so I can test this again? Maybe it does fix it after all.. |
|
@filipweidemann I see your point. After looking into this, I believe this needs to be fixed additionally in the django CMS core:
Next step: Fix translation deleting in the core. |
|
@fsbraun thanks for the update! If you need our help testing this again when it is done, just ping us. |
|
Thank you, @filipweidemann ! django-cms/django-cms#8111 will fix the delete language problem for the upcoming django CMS 4.2. We'll need a backport to django CMS 4.1, too. Once I have that, I will ping you. |
…angocms-versioning into feat/deleting-translations
|
@filipweidemann You could test against this branch and this fix for the core: django-cms/django-cms#8112 Once I get your feedback I will push a piece of code that will deactivate the Delete Translations menu for versions of django CMS that do not correctly delete all versions the page contents. (This will make the menu disappear again until versions are bumped.) |
|
@fsbraun we did. Same errors are still present. I'd like to double-down on my hint that a cascaded deletion of the language versions' Let me walk through this with some screenshots. As you can see, nothing too fancy here. Let me also show you the initial state of the Let's try to delete the English language by creating a new version of the German one (we have to do that to enable the edit controls) and then using the new menu controls. We are greeted with this prompt, asking us to confirm: As you can see, not only are the The publishing state of our page inside the admin view now looks like this: The page is unpublished, so that's good. But the We can access the page though, either by clicking on "view" inside the admin or using the language tabs in the frontend. When we do that, the language chooser will still only display the German language, even when we are obviously on an English page! When we try to edit the pages settings through the frontend editing, we are greeted with this error, confirming that the missing All of this can only be replicated when deleting a language whose |
|
Hi @filipweidemann ! Have you installed both #443 and django-cms/django-cms#8112 ? If a page content object exists in a language, a page url object must exist. Deleting a translation implies deleting all page content objects and the page url object. So, if you delete, say, the German translation, the German page url object should go. In the page tree the page should be visible as "not-existing", (an empty circle), not unpublished. The fix in the django-cms package should make sure that (besides the page url) all page content objects are removed. I have not been able to reproduce the |
|
@fsbraun I manage my deps using This was followed by a I locked & rebuilt everything again, just to be safe. But I still get this behaviour. Any ideas? |
|
Yeah... It seems I didn't push everything 🤦♂️. The ref for django-cms should be |
|
Heh this would explain it I guess :D |
|
@fsbraun okay good news: it looks like this works now! I now get a whole bunch of This is great tbh. I think this is done! Thanks for your effort on this! |
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| """ | ||
| from djangocms_versioning import cms_toolbars | ||
|
|
||
| with patch.object(cms_toolbars, "ALLOW_DELETING_VERSIONS", True): |
There was a problem hiding this comment.
suggestion (testing): Test edge case where ALLOW_DELETING_VERSIONS is True but CMS_SUPPORTS_DELETING_TRANSLATIONS is False.
It would be beneficial to add a test case that specifically checks the behavior when ALLOW_DELETING_VERSIONS is True, but the CMS version is older than 4.1.5 (where CMS_SUPPORTS_DELETING_TRANSLATIONS would be False). This ensures that the delete translation option is not offered in unsupported CMS versions even if the setting is enabled.
Suggested implementation:
def test_change_language_menu_page_toolbar_including_delete(self):
"""Check that patched PageToolbar.change_language_menu also provides
Delete Translation links if DJANGOCMS_ALLOW_DELETING_VERSIONS is True.
"""
from djangocms_versioning import cms_toolbars
with patch.object(cms_toolbars, "ALLOW_DELETING_VERSIONS", True):
def test_change_language_menu_page_toolbar_no_delete_unsupported_cms(self):
"""Check that Delete Translation links are not shown when ALLOW_DELETING_VERSIONS
is True but CMS version does not support deleting translations (pre-4.1.5).
"""
from djangocms_versioning import cms_toolbars
with patch.object(cms_toolbars, "ALLOW_DELETING_VERSIONS", True), \
patch("cms.utils.conf.get_cms_setting") as mock_get_cms_setting:
# Simulate CMS < 4.1.5 where CMS_SUPPORTS_DELETING_TRANSLATIONS would be False
mock_get_cms_setting.return_value = False
version = PageVersionFactory(content__language="en")
PageContentWithVersionFactory(page=version.content.page, language="de")
request = self.get_page_request(version.content.page, self.superuser)
toolbar = CMSToolbar(request)
toolbar.populate()
# Get the language menu items
page_menu = toolbar.get_menu('page')
language_menu = page_menu.find_items(LanguageMenu)[0].item
# Verify no delete options are present in the menu
menu_items = language_menu.get_items()
delete_items = [item for item in menu_items if 'delete-translation' in str(item.url)]
self.assertEqual(len(delete_items), 0)You may need to:
- Import additional dependencies at the top of the file (CMSToolbar, LanguageMenu)
- Adjust the test setup based on your specific test case base class and available helper methods
- Modify the assertions based on your specific menu structure and URL patterns
Description
If
DJANGOCMS_ALLOW_DELETING_VERSIONSis set toTrue, the delete translation menu is offered.fixes #442
Related resources
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Re-introduce the ability to delete page translations if
DJANGOCMS_ALLOW_DELETING_VERSIONSis set toTrue.Bug Fixes:
DJANGOCMS_ALLOW_DELETING_VERSIONSis enabled, addressing issue [BUG] Language chooser in toolbar not showing all options #441.Enhancements:
Summary by Sourcery
Restore the "Delete Translation" menu in the language menu if
DJANGOCMS_ALLOW_DELETING_VERSIONSis set toTrue.Bug Fixes:
DJANGOCMS_ALLOW_DELETING_VERSIONSis enabled.Enhancements: