Actual fix for Issue 1166. Reworked PageAdmin.change_status and PageForm.clean to check for clashing urls

Author: yakky

cms/admin/forms.py

@@ -5,7 +5,7 @@
     PageUserGroup)
 from cms.utils.mail import mail_page_user_change
 from cms.utils.page import is_valid_page_slug
-from cms.utils.page_resolver import get_page_from_path
+from cms.utils.page_resolver import get_page_from_path, is_valid_overwrite_url
 from cms.utils.permissions import (get_current_user, get_subordinate_users, 
     get_subordinate_groups)
 from cms.utils.urlutils import any_path_re
@@ -116,6 +116,22 @@ def clean(self):
         if site and not is_valid_page_slug(page, parent, lang, slug, site):
             self._errors['slug'] = ErrorList([_('Another page with this slug already exists')])
             del cleaned_data['slug']
+        if self.cleaned_data['published'] and page.title_set.count():
+            #Check for titles attached to the page makes sense only because
+            #AdminFormsTests.test_clean_overwrite_url validates the form with when no page instance available
+            #Looks like just a theoretical corner case
+            t = page.get_title_obj(lang)
+            if t:
+                oldslug = t.slug
+                t.slug = self.cleaned_data['slug']
+                t.save()
+                try:
+                    is_valid_overwrite_url(t.path,page)
+                except ValidationError,e:
+                    t.slug = oldslug
+                    t.save()
+                    del cleaned_data['published']
+                    self._errors['published'] = ErrorList(e.messages)
         return cleaned_data
 
     def clean_slug(self):
@@ -173,15 +189,9 @@ def clean(self):
     def clean_overwrite_url(self):
         if 'overwrite_url' in self.fields:
             url = self.cleaned_data['overwrite_url']
-            if url:
-                if not any_path_re.match(url):
-                    raise forms.ValidationError(_('Invalid URL, use /my/url format.'))
-                page = get_page_from_path(url.strip('/'))
-                if (page and
-                    page.pk not in [self.instance.pk,
-                                    self.instance.publisher_public_id]):
-                    raise forms.ValidationError(_('Page with redirect url %r already exist') % url)
-        return url
+            is_valid_overwrite_url(url,self.instance)
+            # TODO: Check what happens if 'overwrite_url' is NOT in self.fields
+            return url
 
 class PagePermissionInlineAdminForm(forms.ModelForm):
     """

cms/admin/pageadmin.py

@@ -25,12 +25,12 @@
 from django.contrib.admin.util import get_deleted_objects
 from urllib2 import unquote
 from django.contrib.sites.models import Site
-from django.core.exceptions import PermissionDenied, ObjectDoesNotExist
+from django.core.exceptions import PermissionDenied, ObjectDoesNotExist, ValidationError
 from django.core.urlresolvers import reverse
 from django.db import router, transaction, models
 from django.forms import CharField
 from django.http import (HttpResponseRedirect, HttpResponse, Http404, 
-    HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotAllowed)
+    HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotAllowed, HttpResponseServerError)
 from django.shortcuts import render_to_response, get_object_or_404
 from django.template.context import RequestContext
 from django.template.defaultfilters import (title, escape, force_escape, 
@@ -43,6 +43,8 @@
 
 DJANGO_1_3 = LooseVersion(django.get_version()) < LooseVersion('1.4')
 
+from cms.utils.page_resolver import is_valid_overwrite_url
+
 if 'reversion' in settings.INSTALLED_APPS:
     import reversion
     from reversion.admin import VersionAdmin as ModelAdmin
@@ -1055,9 +1057,13 @@ def change_status(self, request, page_id):
             return HttpResponseNotAllowed(['POST'])
         page = get_object_or_404(Page, pk=page_id)
         if page.has_publish_permission(request):
-            page.published = not page.published
-            page.save()
-            return admin_utils.render_admin_menu_item(request, page)
+            try:
+                if page.published or is_valid_overwrite_url(page.get_absolute_url(),page):
+                    page.published = not page.published
+                    page.save()
+                return admin_utils.render_admin_menu_item(request, page)
+            except ValidationError,e:
+                return HttpResponseServerError(unicode(e))
         else:
             return HttpResponseForbidden(unicode(_("You do not have permission to publish this page")))
 

cms/models/pagemodel.py

@@ -128,6 +128,7 @@ def move_page(self, target, position='first-child'):
         all the places which are changing page position. Used like an interface
         to mptt, but after move is done page_moved signal is fired.
         """
+        # TODO: Check for slug/overwrite_url clashes upon copying page (in the current page and all children pages)
         # make sure move_page does not break when using INHERIT template
         if (position in ('left', 'right')
             and not target.parent
@@ -155,6 +156,7 @@ def copy_page(self, target, site, position='first-child',
         Note: public_copy was added in order to enable the creation of a copy for creating the public page during
         the publish operation as it sets the publisher_is_draft=False.
         """
+        # TODO: Check for slug/overwrite_url clashes upon copying page (in the current page and all children pages)
         from cms.utils.moderator import update_moderation_message
 
         page_copy = None