Fix volume transfers request commands

* Fix volume transfer request accept to actually not crash when
  trying to call Volume API.
* Fix volume transfer request accept syntax to have only one
  positional argument, which is the ID of the resource in the command
* Change the output column order in volume transfer request list to
  have ID followed by Name then the remaining columns.

Closes-bug: 1633582
Change-Id: I5cc005f039d171cc70859f60e7fe649b09ead229

4 files changed, 108 insertions, 28 deletions

diff --git a/openstackclient/tests/functional/volume/v1/test_transfer_request.py b/openstackclient/tests/functional/volume/v1/test_transfer_request.py
index e03cd717..3fe11913 100644
--- a/openstackclient/tests/functional/volume/v1/test_transfer_request.py
+++ b/openstackclient/tests/functional/volume/v1/test_transfer_request.py
@@ -10,6 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import uuid
 
 from openstackclient.tests.functional.volume.v1 import common
@@ -67,11 +68,12 @@ class TransferRequestTests(common.BaseVolumeTests):
         self.assertNotEqual('', auth_key)
 
         # accept the volume transfer request
-        opts = self.get_opts(self.FIELDS)
-        raw_output = self.openstack(
-            'volume transfer request accept ' + name +
-            ' ' + auth_key + opts)
-        self.assertEqual(name + '\n', raw_output)
+        json_output = json.loads(self.openstack(
+            'volume transfer request accept -f json ' +
+            name + ' ' +
+            '--auth-key ' + auth_key
+        ))
+        self.assertEqual(name, json_output.get('name'))
 
         # the volume transfer will be removed by default after accepted
         # so just need to delete the volume here
diff --git a/openstackclient/tests/functional/volume/v2/test_transfer_request.py b/openstackclient/tests/functional/volume/v2/test_transfer_request.py
index 1791f8ac..99d91ac0 100644
--- a/openstackclient/tests/functional/volume/v2/test_transfer_request.py
+++ b/openstackclient/tests/functional/volume/v2/test_transfer_request.py
@@ -10,6 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import uuid
 
 from openstackclient.tests.functional.volume.v2 import common
@@ -67,11 +68,12 @@ class TransferRequestTests(common.BaseVolumeTests):
         self.assertNotEqual('', auth_key)
 
         # accept the volume transfer request
-        opts = self.get_opts(self.FIELDS)
-        raw_output = self.openstack(
-            'volume transfer request accept ' + name +
-            ' ' + auth_key + opts)
-        self.assertEqual(name + '\n', raw_output)
+        json_output = json.loads(self.openstack(
+            'volume transfer request accept -f json ' +
+            name + ' ' +
+            '--auth-key ' + auth_key
+        ))
+        self.assertEqual(name, json_output.get('name'))
 
         # the volume transfer will be removed by default after accepted
         # so just need to delete the volume here
diff --git a/openstackclient/tests/unit/volume/v1/test_transfer_request.py b/openstackclient/tests/unit/volume/v1/test_transfer_request.py
index b3788d6e..4c013dc0 100644
--- a/openstackclient/tests/unit/volume/v1/test_transfer_request.py
+++ b/openstackclient/tests/unit/volume/v1/test_transfer_request.py
@@ -64,24 +64,62 @@ class TestTransferAccept(TestTransfer):
 
     def test_transfer_accept(self):
         arglist = [
+            '--auth-key', 'key_value',
             self.volume_transfer.id,
-            'auth_key',
         ]
         verifylist = [
             ('transfer_request', self.volume_transfer.id),
-            ('auth_key', 'auth_key'),
+            ('auth_key', 'key_value'),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         columns, data = self.cmd.take_action(parsed_args)
 
         self.transfer_mock.get.assert_called_once_with(
-            self.volume_transfer.id)
+            self.volume_transfer.id,
+        )
         self.transfer_mock.accept.assert_called_once_with(
-            self.volume_transfer.id, 'auth_key')
+            self.volume_transfer.id,
+            'key_value',
+        )
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, data)
 
+    def test_transfer_accept_deprecated(self):
+        arglist = [
+            self.volume_transfer.id,
+            'key_value',
+        ]
+        verifylist = [
+            ('transfer_request', self.volume_transfer.id),
+            ('old_auth_key', 'key_value'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.transfer_mock.accept.assert_called_once_with(
+            self.volume_transfer.id,
+            'key_value',
+        )
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.data, data)
+
+    def test_transfer_accept_no_option(self):
+        arglist = [
+            self.volume_transfer.id,
+        ]
+        verifylist = [
+            ('transfer_request', self.volume_transfer.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args,
+        )
+
 
 class TestTransferCreate(TestTransfer):
 
@@ -219,7 +257,7 @@ class TestTransferDelete(TestTransfer):
                 self.fail('CommandError should be raised.')
             except exceptions.CommandError as e:
                 self.assertEqual('1 of 2 volume transfer requests failed '
-                                 'to delete.', str(e))
+                                 'to delete', str(e))
 
             find_mock.assert_any_call(
                 self.transfer_mock, self.volume_transfers[0].id)
@@ -256,8 +294,8 @@ class TestTransferList(TestTransfer):
 
         expected_columns = [
             'ID',
-            'Volume',
             'Name',
+            'Volume',
         ]
 
         # confirming if all expected columns are present in the result.
@@ -265,8 +303,8 @@ class TestTransferList(TestTransfer):
 
         datalist = ((
             self.volume_transfers.id,
-            self.volume_transfers.volume_id,
             self.volume_transfers.name,
+            self.volume_transfers.volume_id,
         ), )
 
         # confirming if all expected values are present in the result.
@@ -295,8 +333,8 @@ class TestTransferList(TestTransfer):
 
         expected_columns = [
             'ID',
-            'Volume',
             'Name',
+            'Volume',
         ]
 
         # confirming if all expected columns are present in the result.
@@ -304,8 +342,8 @@ class TestTransferList(TestTransfer):
 
         datalist = ((
             self.volume_transfers.id,
-            self.volume_transfers.volume_id,
             self.volume_transfers.name,
+            self.volume_transfers.volume_id,
         ), )
 
         # confirming if all expected values are present in the result.
diff --git a/openstackclient/tests/unit/volume/v2/test_transfer_request.py b/openstackclient/tests/unit/volume/v2/test_transfer_request.py
index 8cd6534b..37eed11e 100644
--- a/openstackclient/tests/unit/volume/v2/test_transfer_request.py
+++ b/openstackclient/tests/unit/volume/v2/test_transfer_request.py
@@ -64,24 +64,62 @@ class TestTransferAccept(TestTransfer):
 
     def test_transfer_accept(self):
         arglist = [
+            '--auth-key', 'key_value',
             self.volume_transfer.id,
-            'auth_key',
         ]
         verifylist = [
             ('transfer_request', self.volume_transfer.id),
-            ('auth_key', 'auth_key'),
+            ('auth_key', 'key_value'),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         columns, data = self.cmd.take_action(parsed_args)
 
         self.transfer_mock.get.assert_called_once_with(
-            self.volume_transfer.id)
+            self.volume_transfer.id,
+        )
         self.transfer_mock.accept.assert_called_once_with(
-            self.volume_transfer.id, 'auth_key')
+            self.volume_transfer.id,
+            'key_value',
+        )
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, data)
 
+    def test_transfer_accept_deprecated(self):
+        arglist = [
+            self.volume_transfer.id,
+            'key_value',
+        ]
+        verifylist = [
+            ('transfer_request', self.volume_transfer.id),
+            ('old_auth_key', 'key_value'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.transfer_mock.accept.assert_called_once_with(
+            self.volume_transfer.id,
+            'key_value',
+        )
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.data, data)
+
+    def test_transfer_accept_no_option(self):
+        arglist = [
+            self.volume_transfer.id,
+        ]
+        verifylist = [
+            ('transfer_request', self.volume_transfer.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args,
+        )
+
 
 class TestTransferCreate(TestTransfer):
 
@@ -219,7 +257,7 @@ class TestTransferDelete(TestTransfer):
                 self.fail('CommandError should be raised.')
             except exceptions.CommandError as e:
                 self.assertEqual('1 of 2 volume transfer requests failed '
-                                 'to delete.', str(e))
+                                 'to delete', str(e))
 
             find_mock.assert_any_call(
                 self.transfer_mock, self.volume_transfers[0].id)
@@ -256,8 +294,8 @@ class TestTransferList(TestTransfer):
 
         expected_columns = [
             'ID',
-            'Volume',
             'Name',
+            'Volume',
         ]
 
         # confirming if all expected columns are present in the result.
@@ -265,8 +303,8 @@ class TestTransferList(TestTransfer):
 
         datalist = ((
             self.volume_transfers.id,
-            self.volume_transfers.volume_id,
             self.volume_transfers.name,
+            self.volume_transfers.volume_id,
         ), )
 
         # confirming if all expected values are present in the result.
@@ -295,8 +333,8 @@ class TestTransferList(TestTransfer):
 
         expected_columns = [
             'ID',
-            'Volume',
             'Name',
+            'Volume',
         ]
 
         # confirming if all expected columns are present in the result.
@@ -304,8 +342,8 @@ class TestTransferList(TestTransfer):
 
         datalist = ((
             self.volume_transfers.id,
-            self.volume_transfers.volume_id,
             self.volume_transfers.name,
+            self.volume_transfers.volume_id,
         ), )
 
         # confirming if all expected values are present in the result.