Repository: Duke-GCB/DukeDSClient

Issue:
Should --skip-copy be the default?
I want to revisit this choice. When delivering data, it's certainly unexpected that ddsclient will download the entire project and upload a copy. I'll reach out to the users but it may be a better default to skip the copy, and require a `--copy` argument to make a copy before delivering.

Discussion:

Patch:
diff --git a/ddsc/cmdparser.py b/ddsc/cmdparser.py
index f2f6ec5..456f8db 100644
--- a/ddsc/cmdparser.py
+++ b/ddsc/cmdparser.py
@@ -222,11 +222,11 @@ def _add_copy_project_arg(arg_parser):
     Adds optional copy_project parameter to a parser.
     :param arg_parser: ArgumentParser parser to add this argument to.
     """
-    arg_parser.add_argument("--skip-copy",
-                            help="Should we just send the deliver email and skip copying the project.",
+    arg_parser.add_argument("--copy",
+                            help="Instead of delivering the specified project, deliver a copy of the project.",
                             action='store_true',
                             default=False,
-                            dest='skip_copy_project')
+                            dest='copy_project')
 
 
 def _add_resend_arg(arg_parser, resend_help):
@@ -424,8 +424,8 @@ class CommandParser(object):
         :param deliver_func: function to run when user choses this option
         """
         description = "Initiate delivery of a project to another user. Removes other user's current permissions. " \
-                      "Makes a copy of the project. Send message to D4S2 service to send email and allow " \
-                      "access to the copy of the project once user acknowledges receiving the data."
+                      "Send message to D4S2 service to send email and allow access to the project once user " \
+                      "acknowledges receiving the data."
         deliver_parser = self.subparsers.add_parser('deliver', description=description)
         add_project_name_or_id_arg(deliver_parser)
         user_or_email = deliver_parser.add_mutually_exclusive_group(required=True)
diff --git a/ddsc/core/download.py b/ddsc/core/download.py
index 914f348..f9bf81b 100644
--- a/ddsc/core/download.py
+++ b/ddsc/core/download.py
@@ -84,7 +84,7 @@ class ProjectDownload(object):
         :param files_to_download: [ProjectFile]: files that will be downloaded
         """
         for project_file in files_to_download:
-            self.file_download_pre_processor.run(project_file)
+            self.file_download_pre_processor.run(self.remote_store.data_service, project_file)
 
     def download_files(self, files_to_download, watcher):
         settings = DownloadSettings(self.remote_store, self.dest_directory, watcher)
diff --git a/ddsc/ddsclient.py b/ddsc/ddsclient.py
index ccd7307..2b2eb4e 100644
--- a/ddsc/ddsclient.py
+++ b/ddsc/ddsclient.py
@@ -309,7 +309,7 @@ class DeliverCommand(BaseCommand):
         """
         email = args.email                  # email of person to deliver to, will be None if username is specified
         username = args.username            # username of person to deliver to, will be None if email is specified
-        skip_copy_project = args.skip_copy_project  # should we skip the copy step
+        copy_project = args.copy_project    # should we deliver a copy of the project
         force_send = args.resend            # is this a resend so we should force sending
         msg_file = args.msg_file            # message file who's contents will be sent with the delivery
         share_usernames = args.share_usernames  # usernames who will have this project shared once it is accepted
@@ -319,7 +319,7 @@ class DeliverCommand(BaseCommand):
         share_users = self.make_user_list(share_emails, share_usernames)
         print("Delivering project.")
         new_project_name = None
-        if not skip_copy_project:
+        if copy_project:
             new_project_name = self.get_new_project_name(project.name)
         to_user = self.remote_store.lookup_or_register_user_by_email_or_username(email, username)
         try:


Test patch:
diff --git a/ddsc/core/tests/test_download.py b/ddsc/core/tests/test_download.py
index 4bb4a0d..4fa9275 100644
--- a/ddsc/core/tests/test_download.py
+++ b/ddsc/core/tests/test_download.py
@@ -101,8 +101,8 @@ class TestProjectDownload(TestCase):
         mock_file2 = Mock(path='file2.txt')
         project_download.run_preprocessor([mock_file1, mock_file2])
         mock_preprocessor.run.assert_has_calls([
-            call(mock_file1),
-            call(mock_file2)
+            call(self.mock_remote_store.data_service, mock_file1),
+            call(self.mock_remote_store.data_service, mock_file2)
         ])
 
 
diff --git a/ddsc/tests/test_cmdparser.py b/ddsc/tests/test_cmdparser.py
index 243b7f4..e521d0d 100644
--- a/ddsc/tests/test_cmdparser.py
+++ b/ddsc/tests/test_cmdparser.py
@@ -57,6 +57,7 @@ class TestCommandParser(TestCase):
         self.assertEqual(None, self.parsed_args.project_id)
         self.assertEqual(None, self.parsed_args.share_usernames)
         self.assertEqual(None, self.parsed_args.share_emails)
+        self.assertEqual(False, self.parsed_args.copy_project)
 
     def test_deliver_with_msg(self):
         command_parser = CommandParser(version_str='1.0')
@@ -66,6 +67,7 @@ class TestCommandParser(TestCase):
         self.assertIn('setup(', self.parsed_args.msg_file.read())
         self.assertEqual(None, self.parsed_args.project_name)
         self.assertEqual('123', self.parsed_args.project_id)
+        self.assertEqual(False, self.parsed_args.copy_project)
 
     def test_deliver_with_share_users(self):
         command_parser = CommandParser(version_str='1.0')
@@ -79,6 +81,19 @@ class TestCommandParser(TestCase):
         self.assertEqual('123', self.parsed_args.project_id)
         self.assertEqual(['bob555', 'tom666'], self.parsed_args.share_usernames)
         self.assertEqual(['bob@bob.bob'], self.parsed_args.share_emails)
+        self.assertEqual(False, self.parsed_args.copy_project)
+
+    def test_deliver_with_copy(self):
+        command_parser = CommandParser(version_str='1.0')
+        command_parser.register_deliver_command(self.set_parsed_args)
+        self.assertEqual(['deliver'], list(command_parser.subparsers.choices.keys()))
+        command_parser.run_command(['deliver', '-p', 'someproject', '--user', 'joe123', '--copy'])
+        self.assertEqual(None, self.parsed_args.msg_file)
+        self.assertEqual('someproject', self.parsed_args.project_name)
+        self.assertEqual(None, self.parsed_args.project_id)
+        self.assertEqual(None, self.parsed_args.share_usernames)
+        self.assertEqual(None, self.parsed_args.share_emails)
+        self.assertEqual(True, self.parsed_args.copy_project)
 
     def test_share_no_msg(self):
         command_parser = CommandParser(version_str='1.0')
diff --git a/ddsc/tests/test_ddsclient.py b/ddsc/tests/test_ddsclient.py
index b6fe028..7a33cf5 100644
--- a/ddsc/tests/test_ddsclient.py
+++ b/ddsc/tests/test_ddsclient.py
@@ -177,8 +177,10 @@ class TestShareCommand(TestCase):
 class TestDeliverCommand(TestCase):
     @patch('ddsc.ddsclient.RemoteStore')
     @patch('ddsc.ddsclient.D4S2Project')
-    def test_run_no_message(self, mock_d4s2_project, mock_remote_store):
+    def test_run_no_message_and_copy(self, mock_d4s2_project, mock_remote_store):
         cmd = DeliverCommand(MagicMock())
+        cmd.get_new_project_name = Mock()
+        cmd.get_new_project_name.return_value = 'NewProjectName'
         myargs = Mock(project_name='mouse',
                       project_id=None,
                       email=None,
@@ -186,7 +188,7 @@ class TestDeliverCommand(TestCase):
                       username='joe123',
                       share_usernames=[],
                       share_emails=[],
-                      skip_copy_project=True,
+                      copy_project=True,
                       include_paths=None,
                       exclude_paths=None,
                       msg_file=None)
@@ -196,6 +198,7 @@ class TestDeliverCommand(TestCase):
         self.assertEqual(project, mock_remote_store.return_value.fetch_remote_project.return_value)
         self.assertEqual(False, force_send)
         self.assertEqual('', message)
+        self.assertEqual('NewProjectName', new_project_name)
         args, kwargs = mock_remote_store.return_value.fetch_remote_project.call_args
         self.assertEqual('mouse', args[0].get_name_or_raise())
 
@@ -211,7 +214,7 @@ class TestDeliverCommand(TestCase):
                           username='joe123',
                           share_emails=[],
                           share_usernames=[],
-                          skip_copy_project=True,
+                          copy_project=False,
                           include_paths=None,
                           exclude_paths=None,
                           msg_file=message_infile)
@@ -221,6 +224,7 @@ class TestDeliverCommand(TestCase):
             self.assertEqual(project, mock_remote_store.return_value.fetch_remote_project.return_value)
             self.assertEqual(False, force_send)
             self.assertIn('setup(', message)
+            self.assertEqual(new_project_name, None)
             args, kwargs = mock_remote_store.return_value.fetch_remote_project.call_args
             self.assertEqual('456', args[0].get_id_or_raise())