Repository: Duke-GCB/datadelivery-cli

Issue:
Improve error messages with bad or missing token
When an invalid token is in `.datadelivery.yml`:

```
$ datadelivery deliver -b handover-demo-no-symlinks --email john.bradley@duke.edu
Traceback (most recent call last):
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/s3.py", line 41, in _check_response
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/requests-2.18.4-py3.6.egg/requests/models.py", line 935, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: http://127.0.0.1:8000/api/v2/s3-endpoints/?name=default

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/bin/datadelivery", line 11, in <module>
    load_entry_point('datadelivery==0.0.1', 'console_scripts', 'datadelivery')()
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/__main__.py", line 12, in main
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/argparser.py", line 26, in parse_and_run_commands
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/argparser.py", line 73, in _run_deliver
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/commands.py", line 24, in deliver
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/commands.py", line 14, in _create_s3
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/s3.py", line 11, in __init__
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/s3.py", line 51, in _get_current_endpoint
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/s3.py", line 28, in _get_request
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/s3.py", line 43, in _check_response
datadelivery.s3.S3Exception: {"detail":"Invalid token."}
```

When no token is in `.datadelivery.yml`:

```
$ datadelivery deliver -b handover-demo-no-symlinks --email john.bradley@duke.edu
Traceback (most recent call last):
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/bin/datadelivery", line 11, in <module>
    load_entry_point('datadelivery==0.0.1', 'console_scripts', 'datadelivery')()
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/__main__.py", line 12, in main
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/argparser.py", line 26, in parse_and_run_commands
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/argparser.py", line 73, in _run_deliver
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/commands.py", line 24, in deliver
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/commands.py", line 13, in _create_s3
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/config.py", line 23, in read_or_create_config
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/config.py", line 48, in read_config
  File "/Users/dcl9/Code/python/datadelivery-cli/venv/lib/python3.6/site-packages/datadelivery-0.0.1-py3.6.egg/datadelivery/config.py", line 59, in __init__
KeyError: 'token'
```

Discussion:

Patch:
diff --git a/.circleci/config.yml b/.circleci/config.yml
new file mode 100644
index 0000000..3f54311
--- /dev/null
+++ b/.circleci/config.yml
@@ -0,0 +1,42 @@
+version: 2
+jobs:
+  build:
+    docker:
+      - image: circleci/python:2.7
+      
+    working_directory: ~/repo
+
+    steps:
+      - checkout
+
+      # Download and cache dependencies
+      - restore_cache:
+          keys:
+          - v1-dependencies-{{ checksum "setup.py" }}-{{ checksum "devRequirements.txt" }}
+          # fallback to using the latest cache if no exact match is found
+          - v1-dependencies-
+
+      - run:
+          name: install dependencies
+          command: |
+            pip install virtualenv
+            virtualenv venv
+            . venv/bin/activate
+            python setup.py install
+            pip install -r devRequirements.txt
+
+      - save_cache:
+          paths:
+            - ./venv
+          key: v1-dependencies-{{ checksum "setup.py" }}-{{ checksum "devRequirements.txt" }}
+        
+      - run:
+          name: run tests
+          command: |
+            . venv/bin/activate
+            python setup.py test
+
+      - store_artifacts:
+          path: test-reports
+          destination: test-reports
+          
diff --git a/datadelivery/__main__.py b/datadelivery/__main__.py
index ca7ccaa..4235b04 100644
--- a/datadelivery/__main__.py
+++ b/datadelivery/__main__.py
@@ -1,7 +1,10 @@
 #!/usr/bin/env python
+from __future__ import absolute_import
+import sys
 from datadelivery.commands import Commands, APP_NAME
 from datadelivery.argparser import ArgParser
 from datadelivery.config import ConfigSetupAbandoned
+from datadelivery.s3 import S3Exception
 import pkg_resources
 
 
@@ -12,6 +15,9 @@ def main():
         arg_parser.parse_and_run_commands()
     except ConfigSetupAbandoned:
         pass
+    except S3Exception as e:
+        print("Error: {}".format(e))
+        sys.exit(1)
 
 
 if __name__ == '__main__':
diff --git a/datadelivery/commands.py b/datadelivery/commands.py
index 0bf4654..902bfaa 100644
--- a/datadelivery/commands.py
+++ b/datadelivery/commands.py
@@ -1,4 +1,4 @@
-from __future__ import print_function
+from __future__ import print_function, absolute_import
 from datadelivery.config import ConfigFile
 from datadelivery.s3 import S3, NotFoundException
 
diff --git a/datadelivery/config.py b/datadelivery/config.py
index 7d838b1..1e33583 100644
--- a/datadelivery/config.py
+++ b/datadelivery/config.py
@@ -1,4 +1,4 @@
-from __future__ import print_function
+from __future__ import print_function, absolute_import
 import os
 import yaml
 from six.moves import input
@@ -19,13 +19,14 @@ class ConfigFile(object):
         self.filename = os.path.expanduser(filename)
 
     def read_or_create_config(self):
+        config = Config({})
         if os.path.exists(self.filename):
-            return self.read_config()
-        else:
-            token = self._prompt_user_for_token()
-            print("Writing new config file at {}".format(self.filename))
-            self.write_new_config(token)
-            return self.read_config()
+            config = self.read_config()
+        if not config.token:
+            config.token = self._prompt_user_for_token()
+            print("Writing config file at {}".format(self.filename))
+            self.write_config(config)
+        return config
 
     def _prompt_user_for_token(self):
         token = self.prompt_user(ENTER_DATA_DELIVERY_TOKEN_PROMPT)
@@ -47,18 +48,38 @@ class ConfigFile(object):
         with open(self.filename, 'r') as stream:
             return Config(yaml.safe_load(stream))
 
-    def write_new_config(self, token):
+    def write_config(self, config):
         with open(self.filename, 'w+') as stream:
-            yaml.safe_dump({
-                'token': token
-            }, stream)
+            yaml.safe_dump(config.to_dict(), stream)
 
 
 class Config(object):
     def __init__(self, data):
-        self.token = data['token']
-        self.url = data.get('url', DEFAULT_DATA_DELIVERY_URL)
-        self.endpoint_name = data.get('endpoint_name', DEFAULT_ENDPOINT_NAME)
+        self.token = data.get('token')
+        self._url = data.get('url')
+        self._endpoint_name = data.get('endpoint_name')
+
+    @property
+    def url(self):
+        if not self._url:
+            return DEFAULT_DATA_DELIVERY_URL
+        return self._url
+
+    @property
+    def endpoint_name(self):
+        if not self._endpoint_name:
+            return DEFAULT_ENDPOINT_NAME
+        return self._endpoint_name
+
+    def to_dict(self):
+        data = {}
+        if self.token:
+            data['token'] = self.token
+        if self._url:
+            data['url'] = self._url
+        if self._endpoint_name:
+            data['endpoint_name'] = self._endpoint_name
+        return data
 
 
 class ConfigSetupAbandoned(Exception):
diff --git a/datadelivery/s3.py b/datadelivery/s3.py
index d6822ed..50ca962 100644
--- a/datadelivery/s3.py
+++ b/datadelivery/s3.py
@@ -24,7 +24,10 @@ class S3(object):
     def _get_request(self, url_suffix):
         url = self._build_url(url_suffix)
         headers = self._build_headers()
-        response = requests.get(url, headers=headers)
+        try:
+            response = requests.get(url, headers=headers)
+        except requests.exceptions.ConnectionError as ex:
+            raise S3Exception("Failed to connect to {}\n{}".format(self.config.url, ex))
         self._check_response(response)
         return response.json()
 
@@ -40,7 +43,18 @@ class S3(object):
         try:
             response.raise_for_status()
         except requests.HTTPError:
-            raise S3Exception(response.text)
+            raise S3Exception(S3.make_message_for_http_error(response))
+
+    @staticmethod
+    def make_message_for_http_error(response):
+        message = response.text
+        try:
+            data = response.json()
+            if 'detail' in data:
+                message = data['detail']
+        except ValueError:
+            pass  # response was not JSON
+        return message
 
     def _get_current_endpoint(self):
         """
@@ -50,7 +64,7 @@ class S3(object):
         url_suffix = 's3-endpoints/?name={}'.format(self.config.endpoint_name)
         for endpoint_response in self._get_request(url_suffix):
             return S3Endpoint(endpoint_response)
-        raise NotFoundException("No endpoint found for s3 url: {}".format(self.config.s3_url))
+        raise NotFoundException("No endpoint found for s3 url: {}".format(self.config.url))
 
     def get_current_user(self):
         """


Test patch:
diff --git a/datadelivery/test_argparser.py b/datadelivery/test_argparser.py
index 658e748..054f76a 100644
--- a/datadelivery/test_argparser.py
+++ b/datadelivery/test_argparser.py
@@ -1,3 +1,4 @@
+from __future__ import absolute_import
 from unittest import TestCase
 from mock import MagicMock, patch, call
 from datadelivery.argparser import ArgParser
diff --git a/datadelivery/test_commands.py b/datadelivery/test_commands.py
index da4612e..10c5d81 100644
--- a/datadelivery/test_commands.py
+++ b/datadelivery/test_commands.py
@@ -1,3 +1,4 @@
+from __future__ import absolute_import
 from unittest import TestCase
 from mock import MagicMock, patch, call
 from datadelivery.commands import Commands
diff --git a/datadelivery/test_config.py b/datadelivery/test_config.py
index c333e05..0275908 100644
--- a/datadelivery/test_config.py
+++ b/datadelivery/test_config.py
@@ -1,3 +1,4 @@
+from __future__ import absolute_import
 from unittest import TestCase
 from mock import MagicMock, patch, call, mock_open
 from datadelivery.config import ConfigFile, Config, ConfigSetupAbandoned, \
@@ -14,10 +15,10 @@ class ConfigFileTestCase(TestCase):
             mock_config.assert_called_with("data")
 
     @patch('datadelivery.config.Config')
-    def test_write_new_config(self, mock_config):
+    def test_write_config(self, mock_config):
         config_file = ConfigFile()
         with patch("__builtin__.open", mock_open()) as mock_file:
-            config_file.write_new_config(token='secret')
+            config_file.write_config(Config({'token': 'secret'}))
             write_call_args_list = mock_file.return_value.write.call_args_list
             written = ''.join([write_call_args[0][0] for write_call_args in write_call_args_list])
             self.assertEqual(written.strip(), '{token: secret}')
@@ -45,10 +46,37 @@ class ConfigFileTestCase(TestCase):
         config_file.read_config = mock_read_config
         config_file.write_new_config = mock_write_new_config
 
-        config = config_file.read_or_create_config()
+        with patch("__builtin__.open", mock_open()) as mock_file:
+            config = config_file.read_or_create_config()
         mock_prompt_user.assert_called_with(ENTER_DATA_DELIVERY_TOKEN_PROMPT)
-        mock_write_new_config.assert_called_with('secretToken')
-        self.assertEqual(config, mock_read_config.return_value)
+        written_text = ''.join([call_args[0][0] for call_args in mock_file.return_value.write.call_args_list])
+        self.assertEqual(written_text.strip(), '{token: secretToken}')
+        self.assertEqual(config.token, 'secretToken')
+        self.assertEqual(config.url, DEFAULT_DATA_DELIVERY_URL)
+        self.assertEqual(config.endpoint_name, DEFAULT_ENDPOINT_NAME)
+
+    @patch('datadelivery.config.os')
+    def test_read_or_create_config_no_token_in_file(self, mock_os):
+        mock_os.path.exists.return_value = True
+        mock_prompt_user = MagicMock()
+        mock_prompt_user.return_value = 'secretToken'
+        mock_read_config = MagicMock()
+        mock_read_config.return_value = Config({})
+        mock_write_new_config = MagicMock()
+
+        config_file = ConfigFile()
+        config_file.prompt_user = mock_prompt_user
+        config_file.read_config = mock_read_config
+        config_file.write_new_config = mock_write_new_config
+
+        with patch("__builtin__.open", mock_open()) as mock_file:
+            config = config_file.read_or_create_config()
+        mock_prompt_user.assert_called_with(ENTER_DATA_DELIVERY_TOKEN_PROMPT)
+        written_text = ''.join([call_args[0][0] for call_args in mock_file.return_value.write.call_args_list])
+        self.assertEqual(written_text.strip(), '{token: secretToken}')
+        self.assertEqual(config.token, 'secretToken')
+        self.assertEqual(config.url, DEFAULT_DATA_DELIVERY_URL)
+        self.assertEqual(config.endpoint_name, DEFAULT_ENDPOINT_NAME)
 
     @patch('datadelivery.config.os')
     def test_read_or_create_config_when_user_doesnt_enter_token(self, mock_os):
diff --git a/datadelivery/test_s3.py b/datadelivery/test_s3.py
index d9d8792..d6fdc30 100644
--- a/datadelivery/test_s3.py
+++ b/datadelivery/test_s3.py
@@ -1,3 +1,4 @@
+from __future__ import absolute_import
 from unittest import TestCase
 from mock import MagicMock, patch, call
 from datadelivery.s3 import S3, NotFoundException
@@ -285,3 +286,27 @@ class S3TestCase(TestCase):
         mock_requests.post.assert_has_calls([
             call('someurl/s3-deliveries/888/send/?force=true', headers=self.expected_headers, json={}),
         ])
+
+    @patch('datadelivery.s3.requests')
+    def test_make_message_for_http_error(self, mock_requests):
+        response = MagicMock()
+        response.text = None
+        response.json.return_value = {'detail': 'Invalid bucket name'}
+        msg = S3.make_message_for_http_error(response)
+        self.assertEqual('Invalid bucket name', msg)
+
+    @patch('datadelivery.s3.requests')
+    def test_make_message_for_http_error_no_detail(self, mock_requests):
+        response = MagicMock()
+        response.text = '{ unexpected: json }'
+        response.json.return_value = {}
+        msg = S3.make_message_for_http_error(response)
+        self.assertEqual('{ unexpected: json }', msg)
+
+    @patch('datadelivery.s3.requests')
+    def test_make_message_for_http_error_no_json(self, mock_requests):
+        response = MagicMock()
+        response.text = 'Invalid bucket name'
+        response.json.side_effect = ValueError("No JSON object could be decoded")
+        msg = S3.make_message_for_http_error(response)
+        self.assertEqual('Invalid bucket name', msg)