Repository: planetlabs/planet-client-python

Issue:
change orders python api response from Order object to json description of the order
Change orders python api response from Order object to json description of the order

This affects:
- list orders (remove as_json option)
- get order
- cancel order

Discussion:

Patch:
diff --git a/docs/guide.md b/docs/guide.md
index 757874d..d594900 100644
--- a/docs/guide.md
+++ b/docs/guide.md
@@ -183,7 +183,7 @@ the context of a `Session` with the `OrdersClient`:
 >>> async def main():
 ...     async with Session() as sess:
 ...         cl = OrdersClient(sess)
-...         order_id = await cl.create_order(request)
+...         order = await cl.create_order(request)
 ...
 >>> asyncio.run(main())
 
@@ -210,13 +210,13 @@ from planet import reporting
 ...         with reporting.StateBar(state='creating') as bar:
 ...             # create order
 ...             order = await cl.create_order(request)
-...             bar.update(state='created', order_id=order.id)
+...             bar.update(state='created', order_id=order['id'])
 ...
 ...             # poll
-...             await cl.poll(order.id, report=bar.update)
+...             await cl.poll(order['id'], report=bar.update)
 ...
 ...         # download
-...         await cl.download_order(order.id)
+...         await cl.download_order(order['id'])
 ...
 >>> asyncio.run(create_poll_and_download())
 ```
diff --git a/planet/__init__.py b/planet/__init__.py
index 504c209..8eb5800 100644
--- a/planet/__init__.py
+++ b/planet/__init__.py
@@ -12,7 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 from .http import Session
-from .models import Order
 from . import order_request, reporting
 from .__version__ import __version__  # NOQA
 from .auth import Auth
@@ -21,7 +20,6 @@ from .clients import OrdersClient
 __all__ = [
     Session,
     OrdersClient,
-    Order,
     order_request,
     reporting,
     Auth,
diff --git a/planet/cli/cli.py b/planet/cli/cli.py
index d463144..3ca0ab5 100644
--- a/planet/cli/cli.py
+++ b/planet/cli/cli.py
@@ -26,17 +26,15 @@ LOGGER = logging.getLogger(__name__)
 
 @click.group()
 @click.pass_context
-@click.option('--verbosity',
-              default="warning",
-              help=("Optional: set verbosity level to warning, info, or debug.\
-                  Defaults to warning."))
+@click.option('-v',
+              '--verbose',
+              count=True,
+              help=('Specify verbosity level of between 0 and 2 corresponding '
+                    'to log levels warning, info, and debug respectively.'))
 @click.version_option(version=planet.__version__)
-def main(ctx, verbosity):
-    """Planet API Client
-    Parameters:
-        ctx -- context object
-        verbosity -- user input for verbosity."""
-    _configure_logging(verbosity)
+def main(ctx, verbose):
+    '''Planet API Client'''
+    _configure_logging(verbose)
 
     # ensure that ctx.obj exists and is a dict (in case `cli()` is called
     # by means other than the `if` block below)
@@ -44,26 +42,9 @@ def main(ctx, verbosity):
 
 
 def _configure_logging(verbosity):
-    """configure logging via verbosity level, corresponding
-    to log levels warning, info and debug respectfully.
-
-    Parameters:
-        verbosity -- user input for verbosity.
-    Raises:
-        click.BadParameter: on unexpected parameter input """
-    # make the user input string lowercase & strip leading/trailing spaces
-    verbosity_input = verbosity.lower()
-    verbosity_input = verbosity_input.strip()
-
-    if verbosity_input == 'warning':
-        log_level = logging.WARNING
-    elif verbosity_input == 'info':
-        log_level = logging.INFO
-    elif verbosity_input == 'debug':
-        log_level = logging.DEBUG
-    else:
-        raise click.BadParameter("please set verbosity to \
-            warning, info, or debug.")
+    '''configure logging via verbosity level of between 0 and 2 corresponding
+    to log levels warning, info and debug respectfully.'''
+    log_level = max(logging.DEBUG, logging.WARNING - logging.DEBUG * verbosity)
     logging.basicConfig(
         stream=sys.stderr,
         level=log_level,
diff --git a/planet/cli/orders.py b/planet/cli/orders.py
index 01f677e..f558e2d 100644
--- a/planet/cli/orders.py
+++ b/planet/cli/orders.py
@@ -66,9 +66,10 @@ def orders(ctx, base_url):
 async def list(ctx, state, limit, pretty):
     '''List orders'''
     async with orders_client(ctx) as cl:
-        orders = await cl.list_orders(state=state, limit=limit, as_json=True)
+        orders = await cl.list_orders(state=state, limit=limit)
+        orders_list = [o async for o in orders]
 
-    echo_json(orders, pretty)
+    echo_json(orders_list, pretty)
 
 
 @orders.command()
@@ -85,7 +86,7 @@ async def get(ctx, order_id, pretty):
     async with orders_client(ctx) as cl:
         order = await cl.get_order(str(order_id))
 
-    echo_json(order.json, pretty)
+    echo_json(order, pretty)
 
 
 @orders.command()
@@ -94,15 +95,11 @@ async def get(ctx, order_id, pretty):
 @coro
 @click.argument('order_id', type=click.UUID)
 async def cancel(ctx, order_id):
-    '''Cancel order by order ID.
-
-    This command cancels a queued order and outputs the cancelled order
-    details.
-    '''
+    '''Cancel order by order ID.'''
     async with orders_client(ctx) as cl:
-        json_resp = await cl.cancel_order(str(order_id))
+        await cl.cancel_order(str(order_id))
 
-    click.echo(json_resp)
+    click.echo('Cancelled')
 
 
 def split_list_arg(ctx, param, value):
@@ -319,4 +316,4 @@ async def create(ctx,
     async with orders_client(ctx) as cl:
         order = await cl.create_order(request)
 
-    echo_json(order.json, pretty)
+    echo_json(order, pretty)
diff --git a/planet/clients/orders.py b/planet/clients/orders.py
index dfa7ffb..1d4dc8e 100644
--- a/planet/clients/orders.py
+++ b/planet/clients/orders.py
@@ -22,7 +22,7 @@ import uuid
 from .. import exceptions
 from ..constants import PLANET_BASE_URL
 from ..http import Session
-from ..models import Order, Orders, Request, Response, StreamingBody
+from ..models import Paged, Request, Response, StreamingBody
 
 BASE_URL = f'{PLANET_BASE_URL}/compute/ops'
 STATS_PATH = '/stats/orders/v2'
@@ -37,6 +37,14 @@ ORDER_STATE_SEQUENCE = \
 LOGGER = logging.getLogger(__name__)
 
 
+class Orders(Paged):
+    '''Asynchronous iterator over Orders from a paged response describing
+    orders.'''
+    LINKS_KEY = '_links'
+    NEXT_KEY = 'next'
+    ITEMS_KEY = 'orders'
+
+
 class OrderStates():
     SEQUENCE = ORDER_STATE_SEQUENCE
 
@@ -114,7 +122,7 @@ class OrdersClient():
         '''
         return await self._session.request(request)
 
-    async def create_order(self, request: dict) -> str:
+    async def create_order(self, request: dict) -> dict:
         '''Create an order request.
 
         Example:
@@ -132,17 +140,16 @@ class OrdersClient():
         ...     )
         ...     async with Session() as sess:
         ...         cl = OrdersClient(sess)
-        ...         order_id = await cl.create_order(request)
+        ...         order = await cl.create_order(request)
         ...
         >>> asyncio.run(main())
-
         ```
 
         Parameters:
             request: order request definition
 
         Returns:
-            The ID of the order
+            JSON description of the created order
 
         Raises:
             planet.exceptions.APIError: On API error.
@@ -151,18 +158,16 @@ class OrdersClient():
 
         req = self._request(url, method='POST', json=request)
         resp = await self._do_request(req)
+        return resp.json()
 
-        order = Order(resp.json())
-        return order
-
-    async def get_order(self, order_id: str) -> Order:
+    async def get_order(self, order_id: str) -> dict:
         '''Get order details by Order ID.
 
         Parameters:
             order_id: The ID of the order
 
         Returns:
-            Order information
+            JSON description of the order
 
         Raises:
             planet.exceptions.ClientError: If order_id is not a valid UUID.
@@ -173,18 +178,20 @@ class OrdersClient():
 
         req = self._request(url, method='GET')
         resp = await self._do_request(req)
+        return resp.json()
 
-        order = Order(resp.json())
-        return order
-
-    async def cancel_order(self, order_id: str) -> dict:
+    async def cancel_order(self, order_id: str) -> Response:
         '''Cancel a queued order.
 
+        **Note:** According to the API docs, cancel order should return the
+        cancelled order details. But testing reveals that an empty response is
+        returned upon success.
+
         Parameters:
             order_id: The ID of the order
 
         Returns:
-            Results of the cancel request
+            Empty response
 
         Raises:
             planet.exceptions.ClientError: If order_id is not a valid UUID.
@@ -290,14 +297,16 @@ class OrdersClient():
                 state.
         """
         order = await self.get_order(order_id)
-        if not OrderStates.is_final(order.state):
+
+        order_state = order['state']
+        if not OrderStates.is_final(order_state):
             raise exceptions.ClientError(
                 'Order cannot be downloaded because the order state '
-                f'({order.state}) is not a final state. '
+                f'({order_state}) is not a final state. '
                 'Consider using wait functionality before '
                 'attempting to download.')
 
-        locations = order.locations
+        locations = self._get_order_locations(order)
         LOGGER.info(
             f'downloading {len(locations)} assets from order {order_id}')
 
@@ -310,6 +319,12 @@ class OrdersClient():
         ]
         return filenames
 
+    @staticmethod
+    def _get_order_locations(order):
+        links = order['_links']
+        results = links.get('results', None)
+        return list(r['location'] for r in results if r)
+
     async def wait(self,
                    order_id: str,
                    state: str = None,
@@ -372,12 +387,12 @@ class OrdersClient():
             t = time.time()
 
             order = await self.get_order(order_id)
-            current_state = order.state
+            current_state = order['state']
 
-            LOGGER.debug(state)
+            LOGGER.debug(current_state)
 
             if callback:
-                callback(order.state)
+                callback(current_state)
 
             if OrderStates.is_final(current_state) or \
                     (state and OrderStates.reached(state, current_state)):
@@ -395,17 +410,12 @@ class OrdersClient():
 
         return current_state
 
-    async def list_orders(
-            self,
-            state: str = None,
-            limit: int = None,
-            as_json: bool = False) -> typing.Union[typing.List[Order], dict]:
+    async def list_orders(self, state: str = None, limit: int = None):
         """Get all order requests.
 
         Parameters:
             state: Filter orders to given state.
             limit: Limit orders to given limit.
-            as_json: Return orders as a json dict.
 
         Returns:
             User orders that match the query
@@ -425,15 +435,5 @@ class OrdersClient():
         else:
             params = None
 
-        orders = await self._get_orders(url, params=params, limit=limit)
-
-        if as_json:
-            ret = [o.json async for o in orders]
-        else:
-            ret = [o async for o in orders]
-        return ret
-
-    async def _get_orders(self, url, params=None, limit=None):
         request = self._request(url, 'GET', params=params)
-
         return Orders(request, self._do_request, limit=limit)
diff --git a/planet/models.py b/planet/models.py
index af01e7b..482126c 100644
--- a/planet/models.py
+++ b/planet/models.py
@@ -14,7 +14,6 @@
 """Manage data for requests and responses."""
 import copy
 from datetime import datetime
-import json
 import logging
 import mimetypes
 import random
@@ -347,73 +346,3 @@ class Paged():
             LOGGER.debug('end of the pages')
             next_link = False
         return next_link
-
-
-class Order():
-    '''Managing description of an order returned from Orders API.
-
-    :param data: Response json describing order
-    :type data: dict
-    '''
-    LINKS_KEY = '_links'
-    RESULTS_KEY = 'results'
-    LOCATION_KEY = 'location'
-
-    def __init__(self, data):
-        self.data = data
-
-    def __str__(self):
-        return "<Order> " + json.dumps(self.data)
-
-    @property
-    def results(self):
-        '''Results for each item in order.
-
-        :return: result for each item in order
-        :rtype: list of dict
-        '''
-        links = self.data[self.LINKS_KEY]
-        results = links.get(self.RESULTS_KEY, None)
-        return results
-
-    @property
-    def locations(self):
-        '''Download locations for order results.
-
-        :return: download locations in order
-        :rtype: list of str
-        '''
-        return list(r[self.LOCATION_KEY] for r in self.results)
-
-    @property
-    def state(self):
-        '''State of the order.
-
-        :return: state of order
-        :rtype: str
-        '''
-        return self.data['state']
-
-    @property
-    def id(self):
-        '''ID of the order.
-
-        :return: id of order
-        :rtype: str
-        '''
-        return self.data['id']
-
-    @property
-    def json(self):
-        return self.data
-
-
-class Orders(Paged):
-    '''Asynchronous iterator over Orders from a paged response describing
-    orders.'''
-    LINKS_KEY = '_links'
-    NEXT_KEY = 'next'
-    ITEMS_KEY = 'orders'
-
-    async def __anext__(self):
-        return Order(await super().__anext__())


Test patch:
diff --git a/tests/integration/test_orders_api.py b/tests/integration/test_orders_api.py
index 53f991d..5b77f6d 100644
--- a/tests/integration/test_orders_api.py
+++ b/tests/integration/test_orders_api.py
@@ -113,9 +113,7 @@ async def test_list_orders_basic(order_descriptions, session):
 
     cl = OrdersClient(session, base_url=TEST_URL)
     orders = await cl.list_orders()
-
-    oids = list(o.id for o in orders)
-    assert oids == ['oid1', 'oid2', 'oid3']
+    assert order_descriptions == [o async for o in orders]
 
 
 @respx.mock
@@ -138,9 +136,7 @@ async def test_list_orders_state(order_descriptions, session):
     # if the value of state doesn't get sent as a url parameter,
     # the mock will fail and this test will fail
     orders = await cl.list_orders(state='failed')
-
-    oids = list(o.id for o in orders)
-    assert oids == ['oid1', 'oid2']
+    assert [order1, order2] == [o async for o in orders]
 
 
 @pytest.mark.asyncio
@@ -172,23 +168,7 @@ async def test_list_orders_limit(order_descriptions, session):
     # since nono_page_url is not mocked, an error will occur if the client
     # attempts to access the next page when the limit is already reached
     orders = await cl.list_orders(limit=1)
-
-    oids = [o.id for o in orders]
-    assert oids == ['oid1']
-
-
-@respx.mock
-@pytest.mark.asyncio
-async def test_list_orders_asjson(order_descriptions, session):
-    order1, order2, order3 = order_descriptions
-
-    page1_response = {"_links": {"_self": "string"}, "orders": [order1]}
-    mock_resp1 = httpx.Response(HTTPStatus.OK, json=page1_response)
-    respx.get(TEST_ORDERS_URL).return_value = mock_resp1
-
-    cl = OrdersClient(session, base_url=TEST_URL)
-    orders = await cl.list_orders(as_json=True)
-    assert orders[0]['id'] == 'oid1'
+    assert [order1] == [o async for o in orders]
 
 
 @respx.mock
@@ -200,7 +180,7 @@ async def test_create_order(oid, order_description, order_request, session):
     cl = OrdersClient(session, base_url=TEST_URL)
     order = await cl.create_order(order_request)
 
-    assert order.json == order_description
+    assert order == order_description
 
 
 @respx.mock
@@ -259,8 +239,7 @@ async def test_get_order(oid, order_description, session):
 
     cl = OrdersClient(session, base_url=TEST_URL)
     order = await cl.get_order(oid)
-
-    assert order.state == 'queued'
+    assert order_description == order
 
 
 @pytest.mark.asyncio
@@ -291,12 +270,12 @@ async def test_cancel_order(oid, order_description, session):
     cancel_url = f'{TEST_ORDERS_URL}/{oid}'
     order_description['state'] = 'cancelled'
     mock_resp = httpx.Response(HTTPStatus.OK, json=order_description)
-    example_resp = mock_resp.json()
     respx.put(cancel_url).return_value = mock_resp
 
+    # TODO: the api says cancel order returns the order details but as
+    # far as I can test thus far, it returns nothing. follow up on this
     cl = OrdersClient(session, base_url=TEST_URL)
-    json_resp = await cl.cancel_order(oid)
-    assert json_resp == example_resp
+    await cl.cancel_order(oid)
 
 
 @pytest.mark.asyncio
diff --git a/tests/integration/test_orders_cli.py b/tests/integration/test_orders_cli.py
index 9d22d7a..506c89e 100644
--- a/tests/integration/test_orders_cli.py
+++ b/tests/integration/test_orders_cli.py
@@ -177,7 +177,7 @@ def test_cli_orders_cancel(invoke, oid, order_description):
 
     result = invoke(['cancel', oid])
     assert not result.exception
-    assert str(mock_resp.json()) + '\n' == result.output
+    assert 'Cancelled\n' == result.output
 
 
 @respx.mock
diff --git a/tests/unit/test_cli_main.py b/tests/unit/test_cli_main.py
index 9fb3715..0b5d0e2 100644
--- a/tests/unit/test_cli_main.py
+++ b/tests/unit/test_cli_main.py
@@ -13,8 +13,6 @@
 # the License.
 import logging
 
-import pytest
-
 import click
 from click.testing import CliRunner
 
@@ -23,11 +21,7 @@ from planet.cli import cli
 LOGGER = logging.getLogger(__name__)
 
 
-@pytest.fixture
-def debug_input():
-    return ['debug', ' debug ', 'debu', 45]
-
-
+# TODO: when testing multiple values for verbosity, use test parameterization
 def test_cli_info_verbosity(monkeypatch):
     log_level = None
 
@@ -48,17 +42,3 @@ def test_cli_info_verbosity(monkeypatch):
     result = runner.invoke(cli.main, args=['test'])
     assert result.exit_code == 0
     assert log_level == logging.WARNING
-
-    def paramaterized_tests(debug_input):
-        runner = CliRunner()
-        result = runner.invoke(cli.main,
-                               args=['--verbosity', debug_input, 'orders'])
-        # Testing to ensure command was run succesfully and
-        # \log_level set to debug
-        if debug_input not in ['debug', ' debug ']:
-            assert result.exit_code == 2
-        else:
-            assert result.exit_code == 0
-            assert log_level == logging.DEBUG
-
-    paramaterized_tests(debug_input)
diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py
index fea2549..38b6f70 100644
--- a/tests/unit/test_models.py
+++ b/tests/unit/test_models.py
@@ -11,7 +11,6 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-import copy
 import logging
 import math
 from unittest.mock import MagicMock
@@ -179,59 +178,7 @@ def get_pages():
     return do_get
 
 
-@pytest.mark.asyncio
-async def test_Paged_iterator(get_pages):
-    req = MagicMock()
-    paged = models.Paged(req, get_pages)
-    assert [1, 2, 3, 4] == [i async for i in paged]
-
-
-@pytest.mark.asyncio
-async def test_Paged_limit(get_pages):
-    req = MagicMock()
-    paged = models.Paged(req, get_pages, limit=3)
-    assert [1, 2, 3] == [i async for i in paged]
-
-
-@pytest.fixture
-def get_orders_pages(orders_page):
-    page2 = copy.deepcopy(orders_page)
-    del page2['_links']['next']
-    responses = [
-        mock_http_response(json=orders_page), mock_http_response(json=page2)
-    ]
-
-    async def do_get(req):
-        return responses.pop(0)
-
-    return do_get
-
-
-@pytest.mark.asyncio
-async def test_Orders(get_orders_pages):
-    req = MagicMock()
-    orders = models.Orders(req, get_orders_pages)
-    expected_ids = [
-        'f05b1ed7-11f0-43da-960c-a624f7c355c8',
-        '8d4799c4-5291-40c0-a7f5-adb9a974455d',
-        'f05b1ed7-11f0-43da-960c-a624f7c355c8',
-        '8d4799c4-5291-40c0-a7f5-adb9a974455d'
-    ]
-    assert expected_ids == [o.id async for o in orders]
-
-
-def test_Order_results(order_description):
-    order = models.Order(order_description)
-    assert len(order.results) == 3
-
-
-def test_Order_locations(order_description):
-    order = models.Order(order_description)
-    expected_locations = ['location1', 'location2', 'location3']
-    assert order.locations == expected_locations
-
-
-def test_last_modified_emptyheader():
+def test_StreamingBody_last_modified_emptyheader():
     '''This function tests the last_modified function for an empty header, by
     seeing if the last_modified is None.
     '''
@@ -252,7 +199,7 @@ def test_last_modified_emptyheader():
     assert output == expected
 
 
-def test_last_modified_completeheader():
+def test_StreamingBody_last_modified_completeheader():
     '''This function tests the last_modified function for an existing header,
     by comparing the last_modified date to
     an expected output.
@@ -274,3 +221,17 @@ def test_last_modified_completeheader():
     expected = datetime.strptime(hr.headers['last-modified'],
                                  '%a, %d %b %Y %H:%M:%S GMT')
     assert output == expected
+
+
+@pytest.mark.asyncio
+async def test_Paged_iterator(get_pages):
+    req = MagicMock()
+    paged = models.Paged(req, get_pages)
+    assert [1, 2, 3, 4] == [i async for i in paged]
+
+
+@pytest.mark.asyncio
+async def test_Paged_limit(get_pages):
+    req = MagicMock()
+    paged = models.Paged(req, get_pages, limit=3)
+    assert [1, 2, 3] == [i async for i in paged]