Repository: django-cms/django-cms
Pull request: Fix: Bad Title.path in Multilanguage sites if parent slug is created or modified
## Description

Fix #6622 Bad Title.path in Multilanguage sites if parent slug is created or modified
Just a reopening of PR #6965

## Related resources

First bug:

In 'en','fr' site

    Create /en/en-1/ page
    Create from this page a subpage with slug "en-1-1"
    Create "fr" translation with slug "fr-1-1"
    Create "fr" translation of parent /en/en-1/ with slug "fr-1"

As a result, the path of subpage "fr-1-1" is not updated. Here the paths:

/en/en-1/
/en/en-1/en-1-1/
/fr/fr-1/
/fr/en-1/fr-1-1/ <-- Bad Title.path "en-1/fr-1-1" instead of "fr-1/fr-1-1"
Second bug:

Change "en" slug of /en/en-1/ page to "en-1b",
As a result the path of subpage "en-1-1" is changed to "fr-1/en-1-1" instead of "en-1b/en-1-1". Here the paths:

/en/en-1b/
/en/fr-1/en-1-1/ <-- Bad Title.path "fr-1/en-1-1" instead of "en-1b/fr-1-1"
/fr/fr-1/
/fr/en-1/fr-1-1/ <-- Bad Title.path "en-1/fr-1-1" instead of "fr-1/fr-1-1"



## Checklist

<!--
Please check the following items before submitting, otherwise,
your pull request will be closed.

Use 'x' to check each item: [x] I have ...
-->

* [x] I have opened this pull request against ``develop``
* [x] I have updated the **CHANGELOG.rst**
* [x] I have added or modified the tests when changing logic


Problem statement:
The slugs of child pages are built incorrectly and return a 404 error when a user adds a custom slug for another language of the parent page
### Summary

The slugs of child pages are breaking when a user adds a custom slug for another language of the parent page, this leads to incorrect urls being displayed and the correct urls return a 404 error.

### Actual behaviour

1. Create an empty website with 2 languages - English and German.
2. Create a Page with the slug `parent-en` in English and publish it, leave the German slug empty.
3. Create a child Page of the first page with the slug `child-en` in English and `child-de` in German
4. Confirm that the created child page has the url `parent-en/child-en` and the respective German url, all is fine for now.
5. Open the parent Page and add a custom German slug, specifically `parent-de` and publish all the unpublished pages.
6. Now open the German version of the website, and see in the menu that the link to the child page is `parent-en/child-de` instead of the correct `parent-de/child-de` url. Confirm that the expected url `parent-de/child-de` actually returns a 404 error.

### Expected behaviour

On the 6th the url in the menu should be `parent-de/child-de` , and the url `parent-de/child-de` shouldn't return a 404 error.

### Environment

* the first one
    * Python version: 3.5.3
    * Django version: 1.11.20
    * django CMS version: 3.6.0

* the second one
    * Python version: 2.7.6
    * Django version: 1.9.5
    * django CMS version: 3.5.1

The issue doesn't appear to be present in django-cms 3.4.5 though.

Hints from issue discussion:
I just experienced the same issue on a 3.5.3 installation. Glad I found this report, was quite worried for a moment ;)
A prerequisite for reproducing this is that at least one of the parent's (i.e. parent-(en|de)) ancestors (including the root page) does not have a German translation. If all pages in the chain have a German translation, it works fine.

If a translation in the chain is unpublished, all children of the page will show a tooltip "unpublished parent" in the page tree view for the publish icon of the translation.

I also have a page with a CMSApp hook and a CMSAttachMenu only in English. The CMSAttachMenu will raise NoReverseMatch errors if another language is selected, presumably because the urlpatterns have not been loaded.

So, I'd guess the fallback mechanism isn't working properly (I have configured fallbacks in CMS_LANGUAGES).
cms.models.pagemodel.mark_descendants_as_published() updates the paths of the published titles of its children:
unpublished_public.exclude(has_url_overwrite=True).update(path=new_path)

I think it should update all titles of its children:
Title.objects.filter(language=language, page__in=page_children, has_url_overwrite=False).update(path=new_path)

Facing almost same issue. If a parent page gets translated after child-pages exists, the translated slug is set incorrectly in the path of all child-pages. So there is a mixup in the url of 2 languages.

Manual fix: Move the parent page (with all children) to another place in the page tree and then move it back again. Then all paths (urls) get updated.
> Manual fix: Move the parent page (with all children) to another place in the page tree and then move it back again. Then all paths (urls) get updated.

Thanks for sharing this fix @zypro (and nice to "meet" you on GitHub :blush:)!

Additional note to the proposed manual fix:
I had pages which were nested a couple of levels; here it wasn't enough to move the top-parent page. I instead had to move every page which had children (starting with the inner-most pages).
The Problem isn't the slug itself but the `path` in the title object. This contains all slugs until the root node. These aren't updated if a parent node's slug is changed...
@zypro yes. THX. fix is a bit of a pain, when having many pages...would need investigation, when to update that path.
@benzkji probably you should update the `path` in the referenced title object if the according page got updated. Maybe triggered by an event like `post_publish`?
By the way here's a temporary fix that I'm adding to my projects for the time being:

https://gitlab.com/what-digital/djangocms-helpers/-/blob/master/djangocms_helpers/fixes/language_slug_fix.py

It's also available on pypi as `djangocms-helpers`
ha, work already done! thanks! @victor-yunenko @zypro

Commit message: Merge remote-tracking branch 'origin/fp-test-before-fix6622' into fp-fix6622

Patch to review:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 1d38f6a3006..b345e72fda4 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -2,12 +2,18 @@
 Changelog
 =========
 
+fp-6622 (2021-01-19)
+====================
+
+* Fixed 66622 bad Title.path in multilingual sites when parent slug is created or modified
+
 
 Unreleased
 ==================
 
 * Fixed builds on RTD
 
+
 3.8.0 (2020-10-28)
 ==================
 
diff --git a/cms/admin/forms.py b/cms/admin/forms.py
index 36d35fe69e1..63ed0ddc0eb 100644
--- a/cms/admin/forms.py
+++ b/cms/admin/forms.py
@@ -472,8 +472,9 @@ def save(self, commit=True):
 
         if update_count == 0:
             api.create_title(language=self._language, page=cms_page, **translation_data)
-        else:
-            cms_page._update_title_path_recursive(self._language)
+        # _update_title_path_recursive should be called if the new page is the parent
+        # of already created children in multilingual sites.
+        cms_page._update_title_path_recursive(self._language, slug=self.data['slug'])
         cms_page.clear_cache(menu=True)
         return cms_page
 
diff --git a/cms/models/pagemodel.py b/cms/models/pagemodel.py
index 539e857de1d..32653b83d4c 100644
--- a/cms/models/pagemodel.py
+++ b/cms/models/pagemodel.py
@@ -340,7 +340,7 @@ def _update_title_path(self, language):
         title_obj.path = title_obj.get_path_for_base(base)
         title_obj.save()
 
-    def _update_title_path_recursive(self, language):
+    def _update_title_path_recursive(self, language, slug=None):
         assert self.publisher_is_draft
         from cms.models import Title
 
@@ -348,7 +348,10 @@ def _update_title_path_recursive(self, language):
             return
 
         pages = self.get_child_pages()
-        base = self.get_path(language, fallback=True)
+        if slug:
+            base = self.get_path_for_slug(slug, language)
+        else:
+            base = self.get_path(language, fallback=True)
 
         if base:
             new_path = Concat(models.Value(base), models.Value('/'), models.F('slug'))
diff --git a/cms/tests/test_titlepath.py b/cms/tests/test_titlepath.py
new file mode 100644
index 00000000000..01dc8eb244b
--- /dev/null
+++ b/cms/tests/test_titlepath.py
@@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+from cms.api import create_page, create_title, publish_pages
+from cms.test_utils.testcases import (CMSTestCase)
+
+
+class TitlePathTestCase(CMSTestCase):
+
+    def test0001_translated_subpage_title_path(self):
+        """
+        Test the path when parent page is created after child translation 
+        """
+
+        p_1 = create_page('en-1', "nav_playground.html", 'en',
+                          slug = 'en-1', published=True)
+        p_1_1 = create_page('en-1-1', "nav_playground.html", 'en',
+                            slug = 'en-1-1', parent=p_1, published=True)
+
+        create_title('de', 'de-1-1', p_1_1, slug='de-1-1')
+
+        # Parent 'de' title created after de-1-1 translation
+        create_title('de', 'de-1', p_1, slug='de-1')
+
+        p_1.publish('de')
+        p_1_1.publish('de')
+
+        response = self.client.get('/de/de-1/')
+        self.assertEqual(response.status_code, 200)
+
+        response = self.client.get('/de/en-1/de-1-1/')
+        if response.status_code == 200:
+            print('\n********** Unexpected response 200 for /de/en-1/de-1-1/')
+        response = self.client.get('/de/de-1/de-1-1/')
+        self.assertEqual(response.status_code, 200)
+        
+
+    def test0002_changed_parent_slug(self):    
+        """
+        Test the child path when parent page slug is changed
+        """
+        
+        p_1 = create_page('BadFoo', "nav_playground.html", 'en',
+                          slug = 'badfoo', published=True)
+        p_1_1 = create_page('Bar', "nav_playground.html", 'en',
+                            slug = 'bar', parent=p_1, published=True)
+        t_1 = p_1.get_title_obj(language='en', fallback=False)
+        t_1.title='Foo'
+        t_1.path='foo'
+        t_1.save()
+
+        p_1.publish('en')
+        p_1_1.publish('en')
+
+        response = self.client.get('/en/foo/')
+        self.assertEqual(response.status_code, 200)
+
+        response = self.client.get('/en/badfoo/bar/')
+        if response.status_code == 200:
+            print('\n********** Unexpected response 200 for /en/badfoo/bar/')
+        response = self.client.get('/en/foo/bar/')
+        self.assertEqual(response.status_code, 200)
+
+        


Code review comments:
diff_hunk: @@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+from cms.api import create_page, create_title, publish_pages
+from cms.test_utils.testcases import (CMSTestCase)
+
+
+class TitlePathTestCase(CMSTestCase):
+
+    def test0001_translated_subpage_title_path(self):
+        """
+        Test the path when parent page is created after child translation 
+        """
+
+        p_1 = create_page('en-1', "nav_playground.html", 'en',
+                          slug = 'en-1', published=True)
+        p_1_1 = create_page('en-1-1', "nav_playground.html", 'en',
+                            slug = 'en-1-1', parent=p_1, published=True)
+
+        create_title('de', 'de-1-1', p_1_1, slug='de-1-1')
+
+        # Parent 'de' title created after de-1-1 translation
+        create_title('de', 'de-1', p_1, slug='de-1')
+
+        p_1.publish('de')
+        p_1_1.publish('de')
+
+        response = self.client.get('/de/de-1/')
+        self.assertEqual(response.status_code, 200)
+
+        response = self.client.get('/de/en-1/de-1-1/')
+        if response.status_code == 200:
+            print('\n********** Unexpected response 200 for /de/en-1/de-1-1/')
path: cms/tests/test_titlepath.py
text: @user1:
We shouldn't print in the test suite like this, if you really have to use the logger. I would advise not printing at all, only output if an error exists. 
```
# We should get a 404 not a successful 200 response, because the slug is wrong: /de/en-1/de-1-1/
self.assertEqual(response.status_code, 200)

@author:
Yes I do agree. It was just to trace the bug.

@author:
I have removed unnecessary test code lines

diff_hunk: @@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+from cms.api import create_page, create_title, publish_pages
+from cms.test_utils.testcases import (CMSTestCase)
+
+
+class TitlePathTestCase(CMSTestCase):
+
+    def test0001_translated_subpage_title_path(self):
+        """
+        Test the path when parent page is created after child translation 
+        """
+
+        p_1 = create_page('en-1', "nav_playground.html", 'en',
path: cms/tests/test_titlepath.py
text: @user1:
try to follow a similar pattern to the other tests for naming, we can see in the page tests that most tests use `page = `: https://github.com/django-cms/django-cms/blob/1df448b7593cca8d5f25b1a2ff002805355899db/cms/tests/test_page.py#L946 

You could use `page_1`, or for parent and child: `parent = `, `child = `

@author:
Done

diff_hunk: @@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+from cms.api import create_page, create_title, publish_pages
+from cms.test_utils.testcases import (CMSTestCase)
+
+
+class TitlePathTestCase(CMSTestCase):
+
+    def test0001_translated_subpage_title_path(self):
+        """
+        Test the path when parent page is created after child translation 
+        """
+
+        p_1 = create_page('en-1', "nav_playground.html", 'en',
+                          slug = 'en-1', published=True)
+        p_1_1 = create_page('en-1-1', "nav_playground.html", 'en',
+                            slug = 'en-1-1', parent=p_1, published=True)
+
+        create_title('de', 'de-1-1', p_1_1, slug='de-1-1')
+
+        # Parent 'de' title created after de-1-1 translation
+        create_title('de', 'de-1', p_1, slug='de-1')
+
+        p_1.publish('de')
+        p_1_1.publish('de')
+
+        response = self.client.get('/de/de-1/')
+        self.assertEqual(response.status_code, 200)
+
+        response = self.client.get('/de/en-1/de-1-1/')
+        if response.status_code == 200:
+            print('\n********** Unexpected response 200 for /de/en-1/de-1-1/')
+        response = self.client.get('/de/de-1/de-1-1/')
+        self.assertEqual(response.status_code, 200)
+        
+
+    def test0002_changed_parent_slug(self):    
path: cms/tests/test_titlepath.py
text: @user1:
Please update this test using the same concepts as the test above.

@author:
New name is:
"test_subpage_title_path_regeneration_after_parent_slug_change"

diff_hunk: @@ -2,12 +2,18 @@
 Changelog
 =========
 
+fp-6622 (2021-01-19)
+====================
+
+* Fixed 66622 bad Title.path in multilingual sites when parent slug is created or modified
path: CHANGELOG.rst
text: @user1:
Can you please move this item below "Unreleased" as shown below.