From 8751517403580285df0a4bd8e3c1f8e6fe510eae Mon Sep 17 00:00:00 2001 From: Dario Marcelino Date: Mon, 18 Dec 2017 17:21:39 +0000 Subject: [PATCH 1/2] Adds failing test for descendant URL path update on .save --- wagtail_modeltranslation/tests/tests.py | 28 +++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/wagtail_modeltranslation/tests/tests.py b/wagtail_modeltranslation/tests/tests.py index 68185ab..d62ee01 100755 --- a/wagtail_modeltranslation/tests/tests.py +++ b/wagtail_modeltranslation/tests/tests.py @@ -488,3 +488,31 @@ class WagtailModeltranslationTest(WagtailModeltranslationTestBase): self.assertEqual(grandchild.url_path_de, '/child/grandchild1/') self.assertEqual(grandchild.url_path_en, '/child/grandchild1_en/') + + # Children url paths should update when parent changes + child.slug_en = 'child_en' + child.save() + + self.assertEqual(child.url_path_de, '/child/') + self.assertEqual(child.url_path_en, '/child_en/') + + # We should retrieve grandchild with the below command: + # grandchild_new = models.TestSlugPage1.objects.get(id=grandchild.id) + # but it's exhibiting strange behaviour during tests. See: + # https://github.com/infoportugal/wagtail-modeltranslation/issues/103#issuecomment-352006610 + grandchild_new = models.TestSlugPage1._default_manager.raw(""" + SELECT page_ptr_id, url_path_en, url_path_de FROM {} + WHERE page_ptr_id=%s LIMIT 1 + """.format(models.TestSlugPage1._meta.db_table), [grandchild.page_ptr_id])[0] + self.assertEqual(grandchild_new.url_path_en, '/child_en/grandchild1_en/') + self.assertEqual(grandchild_new.url_path_de, '/child/grandchild1/') + + + def test_page_fields_tables(self): + from wagtail_modeltranslation.patch_wagtailadmin import WagtailTranslator + + self.assertIn(models.TestSlugPage1, WagtailTranslator._patched_models) + self.assertIn('tests_testslugpage1', WagtailTranslator._page_fields_tables) + self.assertIn(models.TestSlugPage1Subclass, WagtailTranslator._patched_models) + self.assertNotIn('tests_testslugpage1subclass', WagtailTranslator._page_fields_tables) + self.assertNotIn('wagtailcore_page', WagtailTranslator._page_fields_tables) From d10d56a20281a2461d3cc8de9e50b37ee11a7d34 Mon Sep 17 00:00:00 2001 From: Dario Marcelino Date: Mon, 18 Dec 2017 17:22:48 +0000 Subject: [PATCH 2/2] Fixes descendant_url_path on .save Uses similar logic as Wagtail's .save() but adjusted for different languages --- .../patch_wagtailadmin.py | 110 ++++++++++++++---- wagtail_modeltranslation/tests/models.py | 2 + wagtail_modeltranslation/tests/translation.py | 7 +- 3 files changed, 93 insertions(+), 26 deletions(-) diff --git a/wagtail_modeltranslation/patch_wagtailadmin.py b/wagtail_modeltranslation/patch_wagtailadmin.py index 22c575c..96a9bfa 100644 --- a/wagtail_modeltranslation/patch_wagtailadmin.py +++ b/wagtail_modeltranslation/patch_wagtailadmin.py @@ -1,10 +1,11 @@ # coding: utf-8 import copy import logging +import types from django.core.exceptions import ValidationError, FieldDoesNotExist from django.core.urlresolvers import reverse -from django.db import transaction +from django.db import transaction, connection from django.http import Http404 from django.utils.translation import trans_real from django.utils.translation import ugettext_lazy as _ @@ -35,6 +36,7 @@ COMPOSED_PANEL_CLASSES = [MultiFieldPanel, FieldRowPanel] + CUSTOM_COMPOSED_PANE class WagtailTranslator(object): _patched_models = [] + _page_fields_tables = [] def __init__(self, model): # Check if this class was already patched @@ -50,6 +52,11 @@ class WagtailTranslator(object): WagtailTranslator._patched_models.append(model) + # compile all tables that are holding page translated fields (title_xx, slug_xx, url_path_xx) + options = translator.get_options_for_model(model) + if 'url_path' in options.local_fields.keys() and model._meta.db_table is not 'wagtailcore_page': + WagtailTranslator._page_fields_tables.append(model._meta.db_table) + def _patch_page_models(self, model): # PANEL PATCHING @@ -111,7 +118,10 @@ class WagtailTranslator(object): model.relative_url = _new_relative_url model.url = _new_url _patch_clean(model) - _patch_save(model) + + if not model.save.__name__.startswith('localized'): + descriptor = LocalizedSaveDescriptor(model.save) + setattr(model, 'save', descriptor) def _patch_other_models(self, model): if hasattr(model, 'edit_handler'): @@ -266,11 +276,6 @@ def _new_set_url_path(self, parent): # which always has a url_path of '/' setattr(self, localized_url_path_field, '/') - # update url_path for children pages - for child in self.get_children().specific(): - child.set_url_path(self.specific) - child.save() - return self.url_path @@ -416,35 +421,87 @@ def _patch_clean(model): model.clean = clean -def _patch_save(model): - old_save = model.save - def save(self, *args, **kwargs): +def _localized_update_descendant_url_paths(self, old_url_path, new_url_path, language): + localized_url_path = build_localized_fieldname('url_path', language) + + for db_table in WagtailTranslator._page_fields_tables: + cursor = connection.cursor() + if connection.vendor == 'sqlite': + update_statement = """ + UPDATE {db_table} + SET {localized_url_path} = %s || substr({localized_url_path}, %s) + WHERE EXISTS (SELECT * FROM wagtailcore_page AS p + WHERE p.id = {db_table}.page_ptr_id AND p.path LIKE %s) + AND page_ptr_id <> %s + """.format(db_table=db_table, localized_url_path=localized_url_path) + elif connection.vendor == 'mysql': + update_statement = """ + UPDATE {db_table} t + JOIN wagtailcore_page p ON p.id = t.page_ptr_id + SET {localized_url_path}= CONCAT(%s, substring({localized_url_path}, %s)) + WHERE p.path LIKE %s AND t.page_ptr_id <> %s + """.format(db_table=db_table, localized_url_path=localized_url_path) + elif connection.vendor in ('mssql', 'microsoft'): + update_statement = """ + UPDATE t + SET {localized_url_path}= CONCAT(%s, (SUBSTRING({localized_url_path}, 0, %s))) + FROM {db_table} t + JOIN wagtailcore_page p + ON p.id = t.page_ptr_id + WHERE p.path LIKE %s AND t.page_ptr_id <> %s + """.format(db_table=db_table, localized_url_path=localized_url_path) + else: + update_statement = """ + UPDATE {db_table} as t + SET {localized_url_path} = %s || substring({localized_url_path} from %s) + FROM wagtailcore_page AS p + WHERE p.id = t.page_ptr_id AND p.path LIKE %s AND t.page_ptr_id <> %s + """.format(db_table=db_table, localized_url_path=localized_url_path) + cursor.execute(update_statement, [new_url_path, len(old_url_path) + 1, self.path + '%', self.page_ptr_id]) + + +class LocalizedSaveDescriptor(object): + def __init__(self, f): + self.func = f + self.__name__ = 'localized_{}'.format(f.__name__) + + def __call__(self, *args, **kwargs): # when updating, save doesn't check if slug_xx has changed so it can only detect changes in slug # from current language. We need to ensure that if a given localized slug changes we call set_url_path - if not self.id: # creating a record, wagtail will call set_url_path, nothing to do. - return old_save(self, *args, **kwargs) + instance = args[0] + if not instance.id: # creating a record, wagtail will call set_url_path, nothing to do. + return self.func(*args, **kwargs) - current_language = get_language() old_record = None - call_set_url_path = False + changed_localized_slugs = [] for language in mt_settings.AVAILABLE_LANGUAGES: localized_slug = build_localized_fieldname('slug', language) # similar logic used in save if not ('update_fields' in kwargs and localized_slug not in kwargs['update_fields']): - old_record = old_record or Page.objects.get(id=self.id).specific - if getattr(old_record, localized_slug) != getattr(self, localized_slug): - if language == current_language: - # Wagtail's save will detect slug change so it will call set_url_path - call_set_url_path = False - break - call_set_url_path = True + old_record = old_record or instance.__class__.objects.get(id=instance.id) + if getattr(old_record, localized_slug) != getattr(instance, localized_slug): + changed_localized_slugs.append(language) - if call_set_url_path: - self.set_url_path(self.get_parent()) - return old_save(self, *args, **kwargs) + # if any language other than current language had it slug changed + # we'll execute set_url_path + if len(changed_localized_slugs) > 1 or \ + (len(changed_localized_slugs) == 1 and changed_localized_slugs[0] != get_language()): + instance.set_url_path(instance.get_parent()) + result = self.func(*args, **kwargs) + + # update children paths + for language in changed_localized_slugs: + localized_url_path = build_localized_fieldname('url_path', language) + old_url_path = getattr(old_record, localized_url_path) + new_url_path = getattr(instance, localized_url_path) + _localized_update_descendant_url_paths(instance, old_url_path, new_url_path, language) + + return result + + def __get__(self, instance, owner=None): + return types.MethodType(self, instance) if instance else self - model.save = save def _patch_stream_field_meaningful_value(field): old_meaningful_value = field.meaningful_value @@ -459,6 +516,7 @@ def _patch_stream_field_meaningful_value(field): field.meaningful_value = meaningful_value.__get__(field) + def _patch_pre_save(field): if not ORIGINAL_SLUG_LANGUAGE: return @@ -482,6 +540,7 @@ def _patch_pre_save(field): field.pre_save = pre_save.__get__(field) + def patch_wagtail_models(): # After all models being registered the Page or BaseSetting subclasses and snippets are patched registered_models = translator.get_registered_models() @@ -494,3 +553,4 @@ def patch_wagtail_models(): for model_class in registered_models: if issubclass(model_class, Page) or model_class in get_snippet_models() or issubclass(model_class, BaseSetting): WagtailTranslator(model_class) + diff --git a/wagtail_modeltranslation/tests/models.py b/wagtail_modeltranslation/tests/models.py index f54bef9..4e3637e 100755 --- a/wagtail_modeltranslation/tests/models.py +++ b/wagtail_modeltranslation/tests/models.py @@ -24,6 +24,8 @@ class TestSlugPage1(WagtailPage): class TestSlugPage2(WagtailPage): pass +class TestSlugPage1Subclass(TestSlugPage1): + pass class PatchTestPage(WagtailPage): description = models.CharField(max_length=50) diff --git a/wagtail_modeltranslation/tests/translation.py b/wagtail_modeltranslation/tests/translation.py index b317f60..1282a53 100755 --- a/wagtail_modeltranslation/tests/translation.py +++ b/wagtail_modeltranslation/tests/translation.py @@ -4,7 +4,7 @@ from modeltranslation.translator import translator, register, TranslationOptions from wagtail_modeltranslation.tests.models import TestRootPage, TestSlugPage1, TestSlugPage2, PatchTestPage, \ PatchTestSnippet, FieldPanelPage, ImageChooserPanelPage, FieldRowPanelPage, MultiFieldPanelPage, InlinePanelPage, \ FieldPanelSnippet, ImageChooserPanelSnippet, FieldRowPanelSnippet, MultiFieldPanelSnippet, PageInlineModel, \ - BaseInlineModel, StreamFieldPanelPage, StreamFieldPanelSnippet, SnippetInlineModel, InlinePanelSnippet + BaseInlineModel, StreamFieldPanelPage, StreamFieldPanelSnippet, SnippetInlineModel, InlinePanelSnippet, TestSlugPage1Subclass from wagtail_modeltranslation.translator import WagtailTranslationOptions @@ -25,6 +25,11 @@ class TestSlugPage2TranslationOptions(WagtailTranslationOptions): fields = () +@register(TestSlugPage1Subclass) +class TestSlugPage1SubclassTranslationOptions(WagtailTranslationOptions): + pass + + @register(PatchTestPage) class PatchTestPageTranslationOptions(WagtailTranslationOptions): fields = ('description',)