Repository: Duke-GCB/DukeDSClient

Issue:
improve communication when downloading a project
A user re-ran a failed `ddsclient download` command and was concerned about verifying the file hashes for the files downloaded in the first run.  When ddsclient downloads a project the first thing it does is create file hashes for any local files and compares them to the remote file hashes.
If the file hashes differ ddsclient will re-download those files. If the file hashes match the files will be excluded from the download process (since they are already correct). So the file hashes from the first run had already been verified, but not effectively communicated to the user.

Improve the messaging for downloading similar to what was done for uploading in PR #271

Discussion:

Patch:
diff --git a/ddsc/core/download.py b/ddsc/core/download.py
index dab4306..ca96cc6 100644
--- a/ddsc/core/download.py
+++ b/ddsc/core/download.py
@@ -4,7 +4,7 @@ import traceback
 import math
 import requests
 from ddsc.core.localstore import HashUtil
-from ddsc.core.util import ProgressPrinter
+from ddsc.core.util import ProgressPrinter, plural_fmt
 from ddsc.core.parallel import TaskExecutor, TaskRunner
 from ddsc.core.ddsapi import DataServiceAuth, DataServiceApi
 from ddsc.core.remotestore import RemoteStore, ProjectFile, RemoteFileUrl
@@ -53,20 +53,37 @@ class ProjectDownload(object):
 
         self.try_create_dir(self.dest_directory)
         num_files_to_download = len(files_to_download)
-        print("Downloading {} files.".format(num_files_to_download))
+        print("Downloading {}.".format(plural_fmt('file', num_files_to_download)))
         self.download_files(files_to_download)
-        print("Verifying contents of {} downloaded files using file hashes.".format(num_files_to_download))
+        print("Verifying contents of {} using file hashes.".format(plural_fmt('downloaded file', num_files_to_download)))
         self.check_downloaded_files(files_to_download)
 
     def get_files_to_download(self):
         files_to_download = []
-        for project_file in self.remote_store.get_project_files(self.project):
-            if self.include_project_file(project_file):
+        local_verified_files = 0
+        included_project_files = self.get_included_project_files()
+        print("Checking {}.".format(plural_fmt('file', len(included_project_files))))
+        for project_file in included_project_files:
+            if self.file_exists_with_same_hash(project_file):
+                local_verified_files += 1
+            else:
                 local_path = project_file.get_local_path(self.dest_directory)
                 file_to_download = FileToDownload(project_file.json_data, local_path)
                 files_to_download.append(file_to_download)
+        if local_verified_files:
+            if local_verified_files == 1:
+                print("Verified 1 local file is already up to date.")
+            else:
+                print("Verified {} local files are already up to date.".format(local_verified_files))
         return files_to_download
 
+    def get_included_project_files(self):
+        included_project_files = []
+        for project_file in self.remote_store.get_project_files(self.project):
+            if self.path_filter.include_path(project_file.path):
+                included_project_files.append(project_file)
+        return included_project_files
+
     def include_project_file(self, project_file):
         if not self.path_filter.include_path(project_file.path):
             return False  # the file has been filtered out


Test patch:
diff --git a/ddsc/core/tests/test_download.py b/ddsc/core/tests/test_download.py
index e43fff5..bb08107 100644
--- a/ddsc/core/tests/test_download.py
+++ b/ddsc/core/tests/test_download.py
@@ -144,8 +144,8 @@ class TestProjectDownload(TestCase):
         self.assertTrue(mock_file_downloader.return_value.run.called)
 
         mock_print.assert_has_calls([
-            call('Downloading 1 files.'),
-            call('Verifying contents of 1 downloaded files using file hashes.'),
+            call('Downloading 1 file.'),
+            call('Verifying contents of 1 downloaded file using file hashes.'),
             call('/tmp/dest/data1.txt 111 md5 FAILED'),
         ])
 
@@ -162,11 +162,15 @@ class TestProjectDownload(TestCase):
         self.assertEqual(project_download.check_warnings().strip(), 'WARNING: Path(s) not found: tmp/data.txt.')
 
     @patch('ddsc.core.download.FileToDownload')
-    def test_get_files_to_download(self, mock_file_to_download):
+    @patch('ddsc.core.download.os')
+    @patch('ddsc.core.download.print')
+    def test_get_files_to_download(self, mock_print, mock_os, mock_file_to_download):
         project_download = ProjectDownload(self.mock_remote_store, self.mock_project, '/tmp/dest',
                                            self.mock_path_filter)
         project_download.include_project_file = Mock()
         project_download.include_project_file.return_value = True
+        project_download.file_exists_with_same_hash = Mock()
+        project_download.file_exists_with_same_hash.return_value = False
         files = project_download.get_files_to_download()
         self.assertEqual(2, len(files))
         self.assertEqual(files[0], mock_file_to_download.return_value)
@@ -175,6 +179,48 @@ class TestProjectDownload(TestCase):
             call(self.mock_file1.json_data, self.mock_file1.get_local_path.return_value),
             call(self.mock_file2.json_data, self.mock_file2.get_local_path.return_value),
         ])
+        mock_print.assert_has_calls([
+            call("Checking 2 files.")
+        ])
+
+    @patch('ddsc.core.download.FileToDownload')
+    @patch('ddsc.core.download.os')
+    @patch('ddsc.core.download.print')
+    def test_get_files_to_download_one_verified_local(self, mock_print, mock_os, mock_file_to_download):
+        project_download = ProjectDownload(self.mock_remote_store, self.mock_project, '/tmp/dest',
+                                           self.mock_path_filter)
+        project_download.include_project_file = Mock()
+        project_download.include_project_file.return_value = True
+        project_download.file_exists_with_same_hash = Mock()
+        project_download.file_exists_with_same_hash.side_effect = [False, True]
+        files = project_download.get_files_to_download()
+        self.assertEqual(1, len(files))
+        self.assertEqual(files[0], mock_file_to_download.return_value)
+        mock_file_to_download.assert_has_calls([
+            call(self.mock_file1.json_data, self.mock_file1.get_local_path.return_value),
+        ])
+        mock_print.assert_has_calls([
+            call("Checking 2 files."),
+            call("Verified 1 local file is already up to date.")
+        ])
+
+    @patch('ddsc.core.download.FileToDownload')
+    @patch('ddsc.core.download.os')
+    @patch('ddsc.core.download.print')
+    def test_get_files_to_download_two_verified_local(self, mock_print, mock_os, mock_file_to_download):
+        project_download = ProjectDownload(self.mock_remote_store, self.mock_project, '/tmp/dest',
+                                           self.mock_path_filter)
+        project_download.include_project_file = Mock()
+        project_download.include_project_file.return_value = True
+        project_download.file_exists_with_same_hash = Mock()
+        project_download.file_exists_with_same_hash.return_value = True
+        files = project_download.get_files_to_download()
+        self.assertEqual(0, len(files))
+        mock_file_to_download.assert_not_called()
+        mock_print.assert_has_calls([
+            call("Checking 2 files."),
+            call("Verified 2 local files are already up to date.")
+        ])
 
     @patch('ddsc.core.download.os')
     @patch('ddsc.core.download.HashUtil')