Handle url conflicts in move_page and copy_page.

Actually the latter doesn't need any special handling because pages are copied in unpublished state.
In move_page checks are performed in check_title_slugs so that new slugs can be generated on the fly in case of conflicts.
Related javascript admin functions has been updated to accept jsonified response

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, is_valid_overwrite_url
+from cms.utils.page_resolver import get_page_from_path, is_valid_url
 from cms.utils.permissions import (get_current_user, get_subordinate_users, 
     get_subordinate_groups)
 from cms.utils.urlutils import any_path_re
@@ -126,7 +126,7 @@ def clean(self):
                 title.slug = self.cleaned_data['slug']
                 title.save()
                 try:
-                    is_valid_overwrite_url(title.path,page)
+                    is_valid_url(title.path,page)
                 except ValidationError,e:
                     title.slug = oldslug
                     title.save()
@@ -189,7 +189,7 @@ def clean(self):
     def clean_overwrite_url(self):
         if 'overwrite_url' in self.fields:
             url = self.cleaned_data['overwrite_url']
-            is_valid_overwrite_url(url,self.instance)
+            is_valid_url(url,self.instance)
             # TODO: Check what happens if 'overwrite_url' is NOT in self.fields
             return url
 

cms/admin/pageadmin.py

@@ -17,6 +17,7 @@
 from cms.utils import (copy_plugins, helpers, moderator, permissions, plugins, 
     get_template_from_request, get_language_from_request, 
     placeholder as placeholder_utils, admin as admin_utils, cms_static_url)
+from cms.utils.page_resolver import is_valid_url
 from cms.utils.admin import jsonify_request
 from cms.utils.permissions import has_plugin_permission
 from copy import deepcopy
@@ -46,7 +47,6 @@
 
 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
@@ -771,20 +771,20 @@ def move_page(self, request, page_id, extra_context=None):
             page = self.model.objects.get(pk=page_id)
             target = self.model.objects.get(pk=target)
         except self.model.DoesNotExist:
-            return HttpResponseBadRequest("error")
+            return jsonify_request(HttpResponseBadRequest("error"))
 
         # does he haves permissions to do this...?
         if not page.has_move_page_permission(request) or \
             not target.has_add_permission(request):
-                return HttpResponseForbidden("Denied")
+            return jsonify_request(HttpResponseForbidden(_("Error! You don't have permissions to move this page. Please reload the page")))
 
         # move page
         page.move_page(target, position)
 
         if "reversion" in settings.INSTALLED_APPS:
             helpers.make_revision_with_plugins(page)
-            
-        return admin_utils.render_admin_menu_item(request, page)
+
+        return jsonify_request(HttpResponse(admin_utils.render_admin_menu_item(request, page).content))
 
     def get_permissions(self, request, page_id):
         page = get_object_or_404(Page, id=page_id)
@@ -839,12 +839,15 @@ def copy_page(self, request, page_id, extra_context=None):
                 return HttpResponse("error")
                 #context.update({'error': _('Page could not been moved.')})
             else:
-                kwargs = {
-                    'copy_permissions': request.REQUEST.get('copy_permissions', False),
-                    'copy_moderation': request.REQUEST.get('copy_moderation', False),
-                }
-                page.copy_page(target, site, position, **kwargs)
-                return HttpResponse("ok")
+                try:
+                    kwargs = {
+                        'copy_permissions': request.REQUEST.get('copy_permissions', False),
+                        'copy_moderation': request.REQUEST.get('copy_moderation', False),
+                    }
+                    page.copy_page(target, site, position, **kwargs)
+                    return jsonify_request(HttpResponse("ok"))
+                except ValidationError,e:
+                    return jsonify_request(HttpResponseBadRequest(e.messages))
         context.update(extra_context or {})
         return HttpResponseRedirect('../../')
 
@@ -1061,7 +1064,7 @@ def change_status(self, request, page_id):
         page = get_object_or_404(Page, pk=page_id)
         if page.has_publish_permission(request):
             try:
-                if page.published or is_valid_overwrite_url(page.get_absolute_url(),page,False):
+                if page.published or is_valid_url(page.get_absolute_url(),page,False):
                     page.published = not page.published
                     page.save()
                 return jsonify_request(HttpResponse(admin_utils.render_admin_menu_item(request, page).content))

cms/models/pagemodel.py

@@ -124,11 +124,15 @@ def get_absolute_url(self, language=None, fallback=True):
         return urlutils.urljoin(reverse('pages-root'), path)
 
     def move_page(self, target, position='first-child'):
-        """Called from admin interface when page is moved. Should be used on
+        """
+        Called from admin interface when page is moved. Should be used on
         all the places which are changing page position. Used like an interface
         to mptt, but after move is done page_moved signal is fired.
+
+        Note for issue #1166: url conflicts are handled by updated
+        check_title_slugs, overwrite_url on the moved page don't need any check
+        as it remains the same regardless of the page position in the tree
         """
-        # 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,8 +159,10 @@ 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.
+
+        Note for issue #1166: when copying pages there is no need to check for conflicting
+        URLs as pages as copied unplublished.
         """
-        # 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
@@ -270,6 +276,8 @@ def copy_page(self, target, site, position='first-child',
 
                 # create slug-copy for standard copy
                 if not public_copy:
+                    title.save() # We need to save the title in order to
+                                 # retrieve it in get_available_slug
                     title.slug = page_utils.get_available_slug(title)
                 title.save()
 

cms/models/titlemodels.py

@@ -32,6 +32,11 @@ def __unicode__(self):
         return "%s (%s)" % (self.title, self.slug)
 
     def save(self, *args, **kwargs):
+        # Update the path attribute before saving
+        self.update_path()
+        super(Title, self).save(*args, **kwargs)
+
+    def update_path(self):
         # Build path from parent page's path and slug
         current_path = self.path
         parent_page = self.page.parent
@@ -43,7 +48,6 @@ def save(self, *args, **kwargs):
                 parent_title = Title.objects.get_title(parent_page, language=self.language, language_fallback=True)
                 if parent_title:
                     self.path = u'%s/%s' % (parent_title.path, slug)
-        super(Title, self).save(*args, **kwargs)
 
     @property
     def overwrite_url(self):

cms/static/cms/js/change_list.js

@@ -223,37 +223,37 @@
 			if(jtarget.hasClass("publish-checkbox")) {
 				pageId = jtarget.attr("name").split("status-")[1];
 				// if I don't put data in the post, django doesn't get it
-                reloadItem(
-                        jtarget, admin_base_url + "cms/page/" + pageId + "/change-status/",
-                        { 1:1 },
-                        // on success
-                        function(decoded,textStatus){
-                            response = decoded.content;
-                            status = decoded.status;
-                            if(status==200) {
-                                if (/page_\d+/.test($(jtarget).attr('id'))) {
-                                    // one level higher
-                                    target = $(jtarget).find('div.cont:first');
-                                } else {
-                                    target = $(jtarget).parents('div.cont:first');
-                                }
+				reloadItem(
+						jtarget, admin_base_url + "cms/page/" + pageId + "/change-status/",
+						{ 1:1 },
+						// on success
+						function(decoded,textStatus){
+							response = decoded.content;
+							status = decoded.status;
+							if(status==200) {
+								if (/page_\d+/.test($(jtarget).attr('id'))) {
+									// one level higher
+									target = $(jtarget).find('div.cont:first');
+								} else {
+									target = $(jtarget).parents('div.cont:first');
+								}
 
-                                var parent = target.parent();
-                                if (response == "NotFound") {
-                                    return parent.remove();
-                                }
-                                target.replace(response);
-                                parent.find('div.cont:first').yft();
+								var parent = target.parent();
+								if (response == "NotFound") {
+									return parent.remove();
+								}
+								target.replace(response);
+								parent.find('div.cont:first').yft();
 
-                                return true;
-                            }
-                            else {
-                                $(jtarget).attr("checked",false)
-                                alert(response);
-                                return false;
-                            }
-                        }
-                );
+								return false;
+							}
+							else {
+								$(jtarget).attr("checked",false)
+								alert(response);
+								return false;
+							}
+						}
+				);
 				e.stopPropagation();
 				return true;
 			}
@@ -429,12 +429,15 @@
 			};
 			data = $.extend(data, options);
 
-			$.post("./" + item_id + "/copy-page/", data, function(html) {
-				if(html=="ok"){
+			$.post("./" + item_id + "/copy-page/", data, function(decoded) {
+				response = decoded.content;
+				status = decoded.status;
+				if(status==200) {
 					// reload tree
 					window.location = window.location.href;
 				}else{
-					moveError($('#page_'+item_id + " div.col1:eq(0)"));  
+					alert(response);
+					moveError($('#page_'+item_id + " div.col1:eq(0)"),response);
 				}
 			});
 		}
@@ -468,24 +471,24 @@
 		}
 
 		function onSuccess(response, textStatus) {
-            status = true;
+			status = true;
 			if (callback) status = callback(response, textStatus);
 
-            if(status==true) {
-                if (/page_\d+/.test($(el).attr('id'))) {
-                    // one level higher
-                    target = $(el).find('div.cont:first');
-                } else {
-                    target = $(el).parents('div.cont:first');
-                }
+			if(status==true) {
+				if (/page_\d+/.test($(el).attr('id'))) {
+					// one level higher
+					target = $(el).find('div.cont:first');
+				} else {
+					target = $(el).parents('div.cont:first');
+				}
 
-                var parent = target.parent();
-                if (response == "NotFound") {
-                    return parent.remove();
-                }
-                target.replace(response);
-                parent.find('div.cont:first').yft();
-            }
+				var parent = target.parent();
+				if (response == "NotFound") {
+					return parent.remove();
+				}
+				target.replace(response);
+				parent.find('div.cont:first').yft();
+			}
 
 			return true;
 		}
@@ -509,23 +512,25 @@
 		reloadItem(
 			jtarget, "./" + item_id + "/move-page/", 
 
-			{ position: position, target: target_id }, 
-			
+			{ position: position, target: target_id },
+
 			// on success
-			function(){
-				if (tree) {
-					var tree_pos = {'left': 'before', 'right': 'after'}[position] || 'inside';
-					tree.moved("#page_" + item_id, $("#page_" + target_id + " a.title")[0], tree_pos, false, false);
-				} else {
-					moveSuccess($('#page_'+item_id + " div.col1:eq(0)"));
+			function(decoded,textStatus){
+				response = decoded.content;
+				status = decoded.status;
+				if(status==200) {
+					if (tree) {
+						var tree_pos = {'left': 'before', 'right': 'after'}[position] || 'inside';
+						tree.moved("#page_" + item_id, $("#page_" + target_id + " a.title")[0], tree_pos, false, false);
+					} else {
+						moveSuccess($('#page_'+item_id + " div.col1:eq(0)"));
+					}
+					return false;
+				}
+				else {
+					moveError($('#page_'+item_id + " div.col1:eq(0)"),response);
+					return false;
 				}
-                return true;
-			},
-			
-			// on error
-			function(){
-				moveError($('#page_'+item_id + " div.col1:eq(0)"));
-                return false;
 			}
 		);
 	}

cms/templates/admin/cms/page/change_list.html

@@ -60,8 +60,13 @@
 		node.append(msg);
 	    msg.fadeOut(3000);
 	}
-	moveError = function(node){
-		var msg = $({% javascript_string %}<span class="success">{% trans "An error occured. Please reload the page" %}</span>{% end_javascript_string %});
+	moveError = function(node,message){
+        if(message && message!="error") {
+            var msg = $({% javascript_string %}<span class="success">{% end_javascript_string %}+message+{% javascript_string %}</span>{% end_javascript_string %});
+        }
+        else {
+            var msg = $({% javascript_string %}<span class="success">{% trans "An error occured. Please reload the page" %}</span>{% end_javascript_string %});
+        }
 		node.append(msg);
 	}
 	// some settings used by javascript functions

cms/test_utils/testcases.py

@@ -166,8 +166,9 @@ def copy_page(self, page, target_page):
 
         response = self.client.post(URL_CMS_PAGE + "%d/copy-page/" % page.pk, data)
         self.assertEquals(response.status_code, 200)
-        self.assertEquals(response.content, "ok")
-
+        # Altered to reflect the new django-js jsonified response messages
+        self.assertEquals(response.content, '{"status": 200, "content": "ok"}')
+        
         title = page.title_set.all()[0]
         copied_slug = get_available_slug(title)
 

cms/tests/page.py

@@ -19,7 +19,7 @@
     URL_CMS_PAGE_ADD)
 from cms.test_utils.util.context_managers import (LanguageOverride, 
     SettingsOverride)
-from cms.utils.page_resolver import get_page_from_request, is_valid_overwrite_url
+from cms.utils.page_resolver import get_page_from_request, is_valid_url
 from django.conf import settings
 from django.contrib.sites.models import Site
 from django.core.exceptions import ValidationError
@@ -675,17 +675,17 @@ def test_slug_url_overwrite_clash(self):
             home = create_page('home', 'nav_playground.html', 'en', published=True)
             bar = create_page('bar', 'nav_playground.html', 'en', published=False)
             foo = create_page('foo', 'nav_playground.html', 'en', published=True)
-            # Tests to assure is_valid_overwrite_url is ok on plain pages
-            self.assertTrue(is_valid_overwrite_url(bar.get_absolute_url('en'),bar))
-            self.assertTrue(is_valid_overwrite_url(foo.get_absolute_url('en'),foo))
+            # Tests to assure is_valid_url is ok on plain pages
+            self.assertTrue(is_valid_url(bar.get_absolute_url('en'),bar))
+            self.assertTrue(is_valid_url(foo.get_absolute_url('en'),foo))
 
             # Set url_overwrite for page foo
             title = foo.get_title_obj(language='en')
             title.has_url_overwrite = True
             title.path = '/bar/'
             title.save()
             try:
-                url = is_valid_overwrite_url(bar.get_absolute_url('en'),bar)
+                url = is_valid_url(bar.get_absolute_url('en'),bar)
             except ValidationError:
                 url = False
             if url: