Merge lp:~alecu/ubuntuone-client/fix-download-finished into lp:ubuntuone-client

[Alejandro J. Cura]

- DownloadFinished ipc signal is now thrown after the partial is commited. (LP: #1031197)

[Roberto Alsina (ralsina)]

+1

review: Approve

[lp:~alecu/ubuntuone-client/fix-download-finished]

1288. By Alejandro J. Cura on 2012-08-08

fixing comment

[Brian Curtin (brian.curtin)]

Approved, but with a minor nit-pick that's not enough to really hold this up:

32 + kwargs = dict(share_id=mdobj.share_id, node_id=mdobj.node_id)

Any reason not to use a dict literal there?

review: Approve

=== modified file 'tests/syncdaemon/test_fsm.py'
--- tests/syncdaemon/test_fsm.py	2012-04-09 20:07:05 +0000
+++ tests/syncdaemon/test_fsm.py	2012-08-08 18:38:19 +0000
@@ -1626,6 +1626,33 @@
         mdobj = self.fsm.get_by_mdid(mdid)
         self.assertEqual(mdobj.stat, stat_path(path))
 
+    def test_commit_partial_pushes_event(self):
+        """Test that the right event is pushed after the commit."""
+        listener = Listener()
+        self.eq.subscribe(listener)
+
+        path = os.path.join(self.share.path, "thisfile")
+        open_file(path, "w").close()
+        mdobj = self.create_node("thisfile")
+        mdid = mdobj.mdid
+        oldstat = stat_path(path)
+        self.assertEqual(mdobj.stat, oldstat)
+
+        # create a partial
+        self.fsm.create_partial(mdobj.node_id, mdobj.share_id)
+        fh = self.fsm.get_partial_for_writing(mdobj.node_id, mdobj.share_id)
+        fh.write("foobar")
+        fh.close()
+        mdobj = self.fsm.get_by_mdid(mdid)
+        self.assertEqual(mdobj.stat, oldstat)
+
+        # commit the partial
+        self.fsm.commit_partial(mdobj.node_id, mdobj.share_id, "localhash")
+        mdobj = self.fsm.get_by_mdid(mdid)
+
+        kwargs = dict(share_id=mdobj.share_id, node_id=mdobj.node_id)
+        self.assertTrue(("FSM_PARTIAL_COMMITED", kwargs) in listener.events)
+
     def test_move(self):
         """Test that move refreshes stat."""
         path1 = os.path.join(self.share.path, "thisfile1")

=== modified file 'tests/syncdaemon/test_interaction_interfaces.py'
--- tests/syncdaemon/test_interaction_interfaces.py	2012-04-09 20:07:05 +0000
+++ tests/syncdaemon/test_interaction_interfaces.py	2012-08-08 18:38:19 +0000
@@ -1226,18 +1226,19 @@
         self.addCleanup(self.main.event_q.unsubscribe, self.sd_obj)
 
 
-class DownloadTestCase(SyncdaemonEventListenerTestCase):
-    """Test the Download events in SyncdaemonEventListener."""
+class UploadTestCase(SyncdaemonEventListenerTestCase):
+    """Test the Upload events in SyncdaemonEventListener."""
 
     add_fsm_key = True
-    direction = 'Download'
-    bytes_key = 'n_bytes_read'
-    hash_kwarg = 'server_hash'
-    extra_finished_args = {}
+    direction = 'Upload'
+    bytes_key = 'n_bytes_written'
+    hash_kwarg = 'hash'
+    extra_finished_args = dict(new_generation='new_generation', hash='')
+    finished_event = 'AQ_UPLOAD_FINISHED'
 
     @defer.inlineCallbacks
     def setUp(self):
-        yield super(DownloadTestCase, self).setUp()
+        yield super(UploadTestCase, self).setUp()
         self.deferred = None
         self.signal_name = None
         if self.add_fsm_key:
@@ -1304,7 +1305,7 @@
         return self.deferred
 
     def test_handle_finished(self):
-        """Test the handle_AQ_<direction>_FINISHED method."""
+        """Test the handle_<finished_event> method."""
         self.signal_name = self.direction + 'Finished'
         self.deferred = defer.Deferred()
 
@@ -1320,10 +1321,9 @@
             self.patch(self.sd_obj.interface.status, 'SignalError',
                        self.error_handler)
 
-        kwargs = {'share_id': '', 'node_id': 'node_id', self.hash_kwarg: ''}
+        kwargs = {'share_id': '', 'node_id': 'node_id'}
         kwargs.update(self.extra_finished_args)
-        self.main.event_q.push('AQ_%s_FINISHED' % self.direction.upper(),
-                               **kwargs)
+        self.main.event_q.push(self.finished_event, **kwargs)
         return self.deferred
 
     def test_handle_event_error(self):
@@ -1350,21 +1350,29 @@
         return self.deferred
 
 
+class DownloadTestCase(UploadTestCase):
+    """Test the Download events in SyncdaemonEventListener."""
+
+    direction = 'Download'
+    bytes_key = 'n_bytes_read'
+    hash_kwarg = 'server_hash'
+    extra_finished_args = {}
+    finished_event = 'FSM_PARTIAL_COMMITED'
+
+    # The download is special, because we don't want to throw the ipc signal on
+    # AQ_DOWNLOAD_FINISHED but instead we should wait for FSM_PARTIAL_COMMITED
+
+    def test_ignore_pre_partial_commit_event(self):
+        """The AQ_DOWNLOAD_FINISHED signal is ignored."""
+        self.assertNotIn("handle_AQ_DOWNLOAD_FINISHED", vars(self.sd_class))
+
+
 class DownloadNoKeyTestCase(DownloadTestCase):
     """Test the Download events when there is a fsm KeyError."""
 
     add_fsm_key = False
 
 
-class UploadTestCase(DownloadTestCase):
-    """Test the Upload events in SyncdaemonEventListener."""
-
-    direction = 'Upload'
-    bytes_key = 'n_bytes_written'
-    hash_kwarg = 'hash'
-    extra_finished_args = dict(new_generation='new_generation')
-
-
 class UploadNoKeyTestCase(UploadTestCase):
     """Test the Upload events when there is a fsm KeyError."""
 

=== modified file 'ubuntuone/syncdaemon/event_queue.py'
--- ubuntuone/syncdaemon/event_queue.py	2012-07-31 08:26:30 +0000
+++ ubuntuone/syncdaemon/event_queue.py	2012-08-08 18:38:19 +0000
@@ -159,6 +159,7 @@
 
     'FSM_FILE_CONFLICT': ('old_name', 'new_name'),
     'FSM_DIR_CONFLICT': ('old_name', 'new_name'),
+    'FSM_PARTIAL_COMMITED': ('share_id', 'node_id'),
 
     'VM_UDF_SUBSCRIBED': ('udf',),
     'VM_UDF_SUBSCRIBE_ERROR': ('udf_id', 'error'),

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2012-06-22 11:29:10 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2012-08-08 18:38:19 +0000
@@ -1142,7 +1142,7 @@
         return fd
 
     def commit_partial(self, node_id, share_id, local_hash):
-        """Create a .partial in disk and set the flag in metadata."""
+        """Commit a file from a .partial to disk."""
         mdid = self._idx_node_id[(share_id, node_id)]
         mdobj = self.fs[mdid]
         if mdobj["is_dir"]:
@@ -1166,6 +1166,8 @@
         mdobj["info"]["is_partial"] = False
         mdobj["stat"] = get_stat(path)
         self.fs[mdid] = mdobj
+        self.eq.push("FSM_PARTIAL_COMMITED", share_id=share_id,
+                     node_id=node_id)
 
     def remove_partial(self, node_id, share_id):
         """Remove a .partial in disk and set the flag in metadata."""

=== modified file 'ubuntuone/syncdaemon/interaction_interfaces.py'
--- ubuntuone/syncdaemon/interaction_interfaces.py	2012-05-22 14:07:55 +0000
+++ ubuntuone/syncdaemon/interaction_interfaces.py	2012-08-08 18:38:19 +0000
@@ -816,8 +816,8 @@
         self._path_from_ids(share_id, node_id, 'DownloadFileProgress', info)
 
     @log_call(logger.debug)
-    def handle_AQ_DOWNLOAD_FINISHED(self, share_id, node_id, server_hash):
-        """Handle AQ_DOWNLOAD_FINISHED."""
+    def handle_FSM_PARTIAL_COMMITED(self, share_id, node_id):
+        """Handle FSM_PARTIAL_COMMITED."""
         self._path_from_ids(share_id, node_id, 'DownloadFinished', info={})
 
     @log_call(logger.debug)