Merge lp:~jamesh/bindwood/reorder-after-move into lp:bindwood

[James Henstridge]

Fix reorder_children() to handle the case where an expected child has been moved to a different folder.

This branch fixes a bug in the reorder_children() method of the synchroniser. This method takes a child list for a folder from CouchDB and tries to make the local folder match the ordering.

Prior to this branch, it assumed that if a child GUID existed locally we could assign an index for it in the parent folder. However, this would fail if the user had moved the bookmark to a different folder.

This branch fixes the function so that it only tries to reorder children if they are actually a child of the folder in question.

[Eric Casteleijn (thisfred)]

+1

review: Approve

[Manuel de la Peña (mandel)]

review: Approve

=== modified file 'modules/sync.jsm'
--- modules/sync.jsm	2011-02-21 15:04:37 +0000
+++ modules/sync.jsm	2011-03-01 02:26:54 +0000
@@ -361,13 +361,13 @@
                 if (!child_id)
                     continue;
 
-                // Set the item's index.
-                bookmarksService.setItemIndex(child_id, idx++);
-
-                // Remove from the list of existing children, if they exist.
+                // If the child exists in the folder, assign its new
+                // index.
                 var existing_idx = children.indexOf(child_id);
-                if (existing_idx >= 0)
+                if (existing_idx >= 0) {
+                    bookmarksService.setItemIndex(child_id, idx++);
                     children.splice(existing_idx, 1);
+                }
             }
             // Stick the remaining items at the end of the folder.
             for each (var child_id in children) {

=== modified file 'mozmill/tests/test_sync_from_couch.js'
--- mozmill/tests/test_sync_from_couch.js	2011-02-22 11:02:10 +0000
+++ mozmill/tests/test_sync_from_couch.js	2011-03-01 02:26:54 +0000
@@ -302,3 +302,50 @@
     jum.assertEquals(bm.bookmarksService.getIdForItemAt(parent_id, 3),
                      untracked_child);
 };
+
+var test_reorder_after_move = function() {
+    // Create parent folder and children.
+    parent_doc = {
+        _id: 'parent',
+        record_type: 'http://www.freedesktop.org/wiki/Specifications/desktopcouch/folder',
+        parent_guid: 'toolbar_profile_name',
+        title: 'Folder',
+        children: ['child1', 'child2', 'child3'],
+    };
+    synchroniser.processRecord(parent_doc);
+    var parent_id = synchroniser.guid_to_id('parent');
+
+    for each (var child_name in ['child1' ,'child2', 'child3']) {
+        synchroniser.processRecord({
+            _id: child_name,
+            record_type: 'http://www.freedesktop.org/wiki/Specifications/desktopcouch/bookmark',
+            parent_guid: 'parent',
+            title: child_name,
+            uri: LOCAL_TEST_PAGE + '#' + child_name,
+        });
+    }
+    synchroniser.reorder_children();
+
+    // Move one of the children to a different folder.
+    var child2_id = synchroniser.guid_to_id('child2');
+    bm.bookmarksService.moveItem(
+        child2_id, bm.bookmarksService.bookmarksMenuFolder,
+        bm.bookmarksService.DEFAULT_INDEX);
+
+    // Now impose a new ordering to the parent folder, including a
+    // position for the moved item.
+    parent_doc.children = ['child2', 'child3', 'child1'];
+    synchroniser.processRecord(parent_doc);
+    synchroniser.reorder_children();
+
+    // Children should now be in the requested order, with the
+    // untracked child at the end.
+    jum.assertEquals(bm.bookmarksService.getIdForItemAt(parent_id, 0),
+                     synchroniser.guid_to_id('child3'));
+    jum.assertEquals(bm.bookmarksService.getIdForItemAt(parent_id, 1),
+                     synchroniser.guid_to_id('child1'));
+
+    // The moved bookmark should still be in its new parent folder.
+    jum.assertEquals(bm.bookmarksService.getFolderIdForItem(child2_id),
+                     bm.bookmarksService.bookmarksMenuFolder);
+};