Merge lp:~nataliabidart/ubuntuone-client/fix-807235 into lp:ubuntuone-client

[Natalia Bidart]

- Renamed 'get_share_path' to 'get_share_dir_name' and unified the implementation between the platforms (LP: #807235).

[Roberto Alsina (ralsina)]

+1

review: Approve (code review only)

[Manuel de la Peña (mandel)]

review: Approve

=== modified file 'tests/platform/linux/test_vm.py'
--- tests/platform/linux/test_vm.py	2011-06-15 14:46:04 +0000
+++ tests/platform/linux/test_vm.py	2011-07-08 12:29:23 +0000
@@ -26,7 +26,7 @@
     LegacyShareFileShelf, _Share, Share, Shared, Root, UDF, _UDF,
     MetadataUpgrader, VMFileShelf,
 )
-from ubuntuone.platform.linux import get_udf_path, get_share_path
+from ubuntuone.platform.linux import get_udf_path
 
 
 class VolumesTests(BaseVolumeManagerTests):
@@ -41,31 +41,6 @@
                                        suggested_path.encode('utf-8')),
                           udf_path)
 
-    def test_get_share_path(self):
-        """Test for get_share_path."""
-        share_id = uuid.uuid4()
-        name = 'The little pretty share'
-        other_name = 'Dorian Grey'
-        share = self._create_share_volume(volume_id=share_id, name=name,
-                                          other_visible_name=other_name)
-        result = get_share_path(share)
-
-        expected = '%s (%s, %s)' % (name, other_name, share_id)
-        self.assertEqual(result, expected)
-
-    def test_get_share_path_visible_name_empty(self):
-        """Test for get_share_path."""
-        share_id = uuid.uuid4()
-        name = 'The little pretty share'
-        other_name = ''
-        share = self._create_share_volume(volume_id=share_id, name=name,
-                                          other_visible_name=other_name)
-        result = get_share_path(share)
-
-        expected = '%s (%s)' % (name, share_id)
-        self.assertEqual(result, expected)
-
-
 class MetadataOldLayoutTests(MetadataTestCase):
     """Tests for 'old' layouts and metadata upgrade"""
 

=== modified file 'tests/platform/windows/test_vm_helper.py'
--- tests/platform/windows/test_vm_helper.py	2011-06-15 09:41:04 +0000
+++ tests/platform/windows/test_vm_helper.py	2011-07-08 12:29:23 +0000
@@ -18,7 +18,7 @@
 """Windows specific tests for vm_helper."""
 
 import os
-import uuid
+
 from mocker import Mocker
 
 from contrib.testing.testcase import environ
@@ -26,7 +26,8 @@
 
 from ubuntuone.platform.windows.os_helper import LONG_PATH_PREFIX
 from ubuntuone.platform.windows.vm_helper import (
-    create_shares_link, get_udf_path, get_udf_path_name, get_share_path)
+    create_shares_link, get_udf_path, get_udf_path_name,
+)
 
 
 class VMHelperTest(BaseVolumeManagerTests):
@@ -77,27 +78,3 @@
         self.assertTrue(LONG_PATH_PREFIX in udf_path)
         self.assertFalse('/' in udf_path)
         self.mocker.verify()
-
-    def test_get_share_path(self):
-        """Test for get_share_path."""
-        share_id = uuid.uuid4()
-        name = 'The little pretty share'
-        other_name = 'Dorian Grey'
-        share = self._create_share_volume(volume_id=share_id, name=name,
-                                          other_visible_name=other_name)
-        result = get_share_path(share)
-
-        expected = '%s (%s, %s)' % (name, other_name, share_id)
-        self.assertEqual(result, expected)
-
-    def test_get_share_path_visible_name_empty(self):
-        """Test for get_share_path."""
-        share_id = uuid.uuid4()
-        name = 'The little pretty share'
-        other_name = ''
-        share = self._create_share_volume(volume_id=share_id, name=name,
-                                          other_visible_name=other_name)
-        result = get_share_path(share)
-
-        expected = '%s (%s)' % (name, share_id)
-        self.assertEqual(result, expected)

=== modified file 'tests/syncdaemon/test_vm.py'
--- tests/syncdaemon/test_vm.py	2011-06-30 16:01:16 +0000
+++ tests/syncdaemon/test_vm.py	2011-07-08 12:29:23 +0000
@@ -28,19 +28,20 @@
 import uuid
 
 from mocker import Mocker, MATCH
+from twisted.internet import defer, reactor
 
-from ubuntuone.storageprotocol.client import ListShares
-from ubuntuone.storageprotocol.sharersp import (
-    NotifyShareHolder,
-    ShareResponse,
-)
-from ubuntuone.storageprotocol import volumes, request
 from contrib.testing.testcase import (
     FakeMain,
     BaseTwistedTestCase,
     environ,
 )
 from ubuntuone.devtools.handlers import MementoHandler
+from ubuntuone.storageprotocol import volumes, request
+from ubuntuone.storageprotocol.client import ListShares
+from ubuntuone.storageprotocol.sharersp import (
+    NotifyShareHolder,
+    ShareResponse,
+)
 from ubuntuone.syncdaemon import config, event_queue, tritcask
 from ubuntuone.syncdaemon.volume_manager import (
     Share,
@@ -49,6 +50,7 @@
     Root,
     _Share,
     allow_writes,
+    get_share_dir_name,
     VolumeManager,
     LegacyShareFileShelf,
     MetadataUpgrader,
@@ -59,11 +61,10 @@
 from ubuntuone.platform import (
     abspath,
     get_udf_path,
-    get_share_path,
     make_link,
     make_dir,
-    remove_link)
-from twisted.internet import defer, reactor
+    remove_link,
+)
 
 # grab the metadata version before tests fiddle with it
 CURRENT_METADATA_VERSION = VolumeManager.METADATA_VERSION
@@ -179,7 +180,7 @@
              node_id=node_id, name=name, generation=generation,
              free_bytes=free_bytes, access_level=access_level,
              accepted=accepted, other_visible_name=other_visible_name)
-        dir_name = get_share_path(share_volume)
+        dir_name = get_share_dir_name(share_volume)
         share_path = os.path.join(self.shares_dir, dir_name)
         share = Share.from_share_volume(share_volume, share_path)
         share.subscribed = subscribed
@@ -187,6 +188,33 @@
         return share
 
 
+class GetShareDirNameTests(BaseVolumeManagerTests):
+
+    def test_get_share_dir_name(self):
+        """Test for get_share_dir_name."""
+        share_id = uuid.uuid4()
+        name = 'The little pretty share'
+        other_name = 'Dorian Grey'
+        share = self._create_share_volume(volume_id=share_id, name=name,
+                                          other_visible_name=other_name)
+        result = get_share_dir_name(share)
+
+        expected = '%s (%s, %s)' % (name, other_name, share_id)
+        self.assertEqual(result, expected)
+
+    def test_get_share_dir_name_visible_name_empty(self):
+        """Test for get_share_dir_name."""
+        share_id = uuid.uuid4()
+        name = 'The little pretty share'
+        other_name = ''
+        share = self._create_share_volume(volume_id=share_id, name=name,
+                                          other_visible_name=other_name)
+        result = get_share_dir_name(share)
+
+        expected = '%s (%s)' % (name, share_id)
+        self.assertEqual(result, expected)
+
+
 class VolumeManagerTests(BaseVolumeManagerTests):
     """ Tests for Volume Manager internal API. """
 
@@ -253,7 +281,7 @@
         share = self._create_share(access_level='View', subscribed=False)
         yield self.vm.add_share(share)
 
-        dir_name = get_share_path(share)
+        dir_name = get_share_dir_name(share)
         share_path = os.path.join(self.shares_dir, dir_name)
         self.assertEqual(share_path, share.path)
         self.assertEqual(2, len(self.vm.shares))  # root and share
@@ -292,7 +320,7 @@
         share = self._create_share(access_level='Modify', subscribed=False)
         yield self.vm.add_share(share)
 
-        dir_name = get_share_path(share)
+        dir_name = get_share_dir_name(share)
         share_path = os.path.join(self.shares_dir, dir_name)
         self.assertEqual(share_path, share.path)
         self.assertEqual(2, len(self.vm.shares))  # root and share
@@ -1263,7 +1291,7 @@
         # check
         share = self.vm.shares['share_id']
         shouldbe_dir = os.path.join(self.shares_dir,
-                                    get_share_path(share_response))
+                                    get_share_dir_name(share_response))
         self.assertEquals(abspath(shouldbe_dir), share.path)
 
     def test_handle_SHARES_visible_username(self):
@@ -1282,7 +1310,7 @@
         # check
         share = self.vm.shares['share_id']
         shouldbe_dir = os.path.join(self.shares_dir,
-                                    get_share_path(share_response))
+                                    get_share_dir_name(share_response))
         self.assertEquals(abspath(shouldbe_dir), share.path)
 
     def test_handle_SV_SHARE_CHANGED_sharename(self):
@@ -1294,7 +1322,7 @@
         self.vm._got_root('root_uuid')
         self.vm.handle_SV_SHARE_CHANGED(info=share_holder)
         shouldbe_dir = os.path.join(self.shares_dir,
-                                    get_share_path(share_holder))
+                                    get_share_dir_name(share_holder))
         self.assertEquals(abspath(shouldbe_dir),
                           abspath(self.vm.shares['share_id'].path))
 
@@ -1307,7 +1335,7 @@
         self.vm._got_root('root_uuid')
         self.vm.handle_SV_SHARE_CHANGED(info=share_holder)
         shouldbe_dir = os.path.join(self.shares_dir,
-                                    get_share_path(share_holder))
+                                    get_share_dir_name(share_holder))
         self.assertEquals(abspath(shouldbe_dir),
                           abspath(self.vm.shares['share_id'].path))
 
@@ -3908,4 +3936,3 @@
         self.assertEqual(self.shelf._root_key,
                          self.shelf._get_key(request.ROOT))
         self.assertEqual('foo', self.shelf._get_key('foo'))
-

=== modified file 'ubuntuone/platform/linux/__init__.py'
--- ubuntuone/platform/linux/__init__.py	2011-06-29 17:09:32 +0000
+++ ubuntuone/platform/linux/__init__.py	2011-07-08 12:29:23 +0000
@@ -57,7 +57,6 @@
 )
 from ubuntuone.platform.linux.vm_helper import (
     create_shares_link,
-    get_share_path,
     get_udf_path_name,
     get_udf_path,
 )

=== modified file 'ubuntuone/platform/linux/vm_helper.py'
--- ubuntuone/platform/linux/vm_helper.py	2011-06-28 21:23:19 +0000
+++ ubuntuone/platform/linux/vm_helper.py	2011-07-08 12:29:23 +0000
@@ -69,35 +69,3 @@
     # Unicode boundary! the name is Unicode in protocol and server,
     # but here we use bytes for paths
     return os.path.expanduser(suggested_path).encode("utf8")
-
-
-def get_share_path(share):
-    """Builds the root path of a share using the share information."""
-    if hasattr(share, 'volume_id'):
-        share_id = share.volume_id
-    elif hasattr(share, 'share_id'):
-        share_id = share.share_id
-    else:
-        share_id = share.id
-
-    if hasattr(share, 'name'):
-        share_name = share.name
-    else:
-        share_name = share.share_name
-
-    if hasattr(share, 'other_visible_name'):
-        visible_name = share.other_visible_name
-    else:
-        visible_name = share.from_visible_name
-
-    if visible_name:
-        dir_name = '%s (%s, %s)' % (share_name, visible_name, share_id)
-    else:
-        dir_name = '%s (%s)' % (share_name, share_id)
-
-    # Unicode boundary! the name is Unicode in protocol and server,
-    # but here we use bytes for paths
-    dir_name = dir_name.encode("utf8")
-
-    return dir_name
-

=== modified file 'ubuntuone/platform/windows/__init__.py'
--- ubuntuone/platform/windows/__init__.py	2011-06-30 20:57:41 +0000
+++ ubuntuone/platform/windows/__init__.py	2011-07-08 12:29:23 +0000
@@ -27,7 +27,6 @@
 from ubuntuone.platform.windows.vm_helper import (
     get_udf_path_name,
     get_udf_path,
-    get_share_path,
     create_shares_link
 )
 

=== modified file 'ubuntuone/platform/windows/vm_helper.py'
--- ubuntuone/platform/windows/vm_helper.py	2011-06-17 16:59:53 +0000
+++ ubuntuone/platform/windows/vm_helper.py	2011-07-08 12:29:23 +0000
@@ -21,7 +21,7 @@
 from ubuntuone.platform.windows.os_helper import (
     LONG_PATH_PREFIX,
     make_link,
-    abspath)
+)
 
 def create_shares_link(source, dest):
     """Create the shares symlink."""
@@ -69,34 +69,3 @@
     """Build the udf path using the suggested_path."""
     path = suggested_path.replace('/', '\\')
     return LONG_PATH_PREFIX + os.path.expanduser(path).encode("utf8")
-
-
-def get_share_path(share):
-    """Builds the root path of a share using the share information."""
-    if hasattr(share, 'volume_id'):
-        share_id = share.volume_id
-    elif hasattr(share, 'share_id'):
-        share_id = share.share_id
-    else:
-        share_id = share.id
-
-    if hasattr(share, 'name'):
-        share_name = share.name
-    else:
-        share_name = share.share_name
-
-    if hasattr(share, 'other_visible_name'):
-        visible_name = share.other_visible_name
-    else:
-        visible_name = share.from_visible_name
-
-    if visible_name:
-        dir_name = '%s (%s, %s)' % (share_name, visible_name, share_id)
-    else:
-        dir_name = '%s (%s)' % (share_name, share_id)
-
-    # Unicode boundary! the name is Unicode in protocol and server,
-    # but here we use bytes for paths
-    dir_name = dir_name.encode("utf8")
-
-    return abspath(dir_name)

=== modified file 'ubuntuone/syncdaemon/volume_manager.py'
--- ubuntuone/syncdaemon/volume_manager.py	2011-06-30 19:26:53 +0000
+++ ubuntuone/syncdaemon/volume_manager.py	2011-07-08 12:29:23 +0000
@@ -46,7 +46,6 @@
     expanduser,
     get_path_list,
     get_udf_path,
-    get_share_path,
     get_udf_path_name,
     is_link,
     listdir,
@@ -69,6 +68,41 @@
 UDF_ROW_TYPE = 5
 
 
+def get_share_dir_name(share):
+    """Builds the directory name of a share using the share information.
+
+    This method is not platform dependent, so do not override in platform.
+
+    """
+    if hasattr(share, 'volume_id'):
+        share_id = share.volume_id
+    elif hasattr(share, 'share_id'):
+        share_id = share.share_id
+    else:
+        share_id = share.id
+
+    if hasattr(share, 'name'):
+        share_name = share.name
+    else:
+        share_name = share.share_name
+
+    if hasattr(share, 'other_visible_name'):
+        visible_name = share.other_visible_name
+    else:
+        visible_name = share.from_visible_name
+
+    if visible_name:
+        dir_name = '%s (%s, %s)' % (share_name, visible_name, share_id)
+    else:
+        dir_name = '%s (%s)' % (share_name, share_id)
+
+    # Unicode boundary! the name is Unicode in protocol and server,
+    # but here we use bytes for paths
+    dir_name = dir_name.encode("utf8")
+
+    return dir_name
+
+
 class _Share(object):
     """Represents a share or mount point"""
 
@@ -632,7 +666,7 @@
         """
         self.log.debug('_handle_new_volume: volume %r.', volume)
         if isinstance(volume, ShareVolume):
-            dir_name = get_share_path(volume)
+            dir_name = get_share_dir_name(volume)
             path = os.path.join(self.m.shares_dir, dir_name)
             share = Share.from_share_volume(volume, path)
             autosubscribe = config.get_user_config().get_share_autosubscribe()
@@ -668,7 +702,7 @@
             self.log.debug('share %r: id=%s, name=%r', a_share.direction,
                            share_id, a_share.name)
             if a_share.direction == "to_me":
-                dir_name = get_share_path(a_share)
+                dir_name = get_share_dir_name(a_share)
                 path = os.path.join(self.m.shares_dir, dir_name)
                 share = Share.from_response(a_share, path)
                 shares.append(share.volume_id)
@@ -750,7 +784,7 @@
             self.log.debug("New share notification, share_id: %s",
                            info.share_id)
             # XXX: request a refresh of the shares list(?)
-            dir_name = get_share_path(info)
+            dir_name = get_share_dir_name(info)
             path = os.path.join(self.m.shares_dir, dir_name)
             share = Share.from_notify_holder(info, path)
             self.add_share(share)
@@ -1797,9 +1831,11 @@
         shutil.rmtree(self._shared_md_dir)
         shutil.rmtree(self._udfs_md_dir)
 
+
 class VMFileShelf(file_shelf.CachedFileShelf):
-    """Custom file shelf that allow request.ROOT as key, it's replaced
-    by the string: root_node_id.
+    """Custom file shelf.
+
+    Allow request.ROOT as key, it's replaced by the string: root_node_id.
 
     """
 
@@ -1846,8 +1882,7 @@
 
 
 class LegacyShareFileShelf(VMFileShelf):
-    """A FileShelf capable of replacing pickled classes
-    with a different class.
+    """A FileShelf capable of replacing pickled classes with a different class.
 
     upgrade_map attribute is a dict of (module, name):class