Repository: SebRut/pygrocy (python)

Issue:
Return values inconsistent between API call methods
When implementing stuff in grocy-telegram-bot using the `remove_product_in_shopping_list()` and `add_product()` methods of pygrocy I stumbled over the fact that the first does return the HTTP response but the latter does not. This is a problem, especially since you do not use the `response.raise_for_status()` method, because errors happening on the API side will just get lost, even if the developer was cautious enough to check the method result.

I don't know if you have a plan on how to deal with this going forward, but imho pygrocy should never return any HTTP related data back to the developer using the library since that would negate the whole point of an abstraction layer.

In a different project I was involved with we always used the `raise_for_status()` method. Imho a little restructuring would greatly benefit the readability and simplicity of the API call related methods. Have a look at the `_do_request` method [here](https://github.com/femueller/python-n26/blob/a28b124f175e2dec62e8106a16213ad76e6d8c22/n26/api.py#L269) which allowed us to turn all of the API call methods into one-liners.

PS: I know you are currently busy, so I don't expect you to do anything. A little heads up would be great though :crossed_fingers: 

Pull request:
Rework API requests
## Description

Rework API request : GET

**Related issue (if applicable):** fixes #70 

## Checklist
  - [x] The code change is tested and works locally.
  - [x] tests pass. **Your PR won't be merged unless tests pass**
  - [x] There is no commented out code in this PR


Patch:
diff --git a/pygrocy/grocy.py b/pygrocy/grocy.py
index cb972af..fcc8101 100644
--- a/pygrocy/grocy.py
+++ b/pygrocy/grocy.py
@@ -161,15 +161,11 @@ class Grocy(object):
     def __init__(self, base_url, api_key, port: int = DEFAULT_PORT_NUMBER, verify_ssl = True):
         self._api_client = GrocyApiClient(base_url, api_key, port, verify_ssl)
 
-    def stock(self, get_details: bool = False) -> List[Product]:
+    def stock(self) -> List[Product]:
         raw_stock = self._api_client.get_stock()
-        if raw_stock is None:
-            return
+
         stock = [Product(resp) for resp in raw_stock]
 
-        if get_details:
-            for item in stock:
-                item.get_details(self._api_client)
         return stock
 
     def volatile_stock(self) -> CurrentVolatilStockResponse:
@@ -238,8 +234,6 @@ class Grocy(object):
     
     def shopping_list(self, get_details: bool = False) -> List[ShoppingListProduct]:
         raw_shoppinglist = self._api_client.get_shopping_list()
-        if raw_shoppinglist is None:
-            return
         shopping_list = [ShoppingListProduct(resp) for resp in raw_shoppinglist]
 
         if get_details:
@@ -261,8 +255,6 @@ class Grocy(object):
         
     def product_groups(self) -> List[Group]:
         raw_groups = self._api_client.get_product_groups()
-        if raw_groups is None:
-            return
         return [Group(resp) for resp in raw_groups]
         
     def add_product_pic(self, product_id: int, pic_path: str):
diff --git a/pygrocy/grocy_api_client.py b/pygrocy/grocy_api_client.py
index 0388422..cf717ad 100644
--- a/pygrocy/grocy_api_client.py
+++ b/pygrocy/grocy_api_client.py
@@ -327,39 +327,64 @@ class GrocyApiClient(object):
                 "GROCY-API-KEY": api_key
             }
 
+    def _do_get_request(self, end_url: str):
+        req_url = urljoin(self._base_url, end_url)
+        resp = requests.get(
+            req_url, verify=self._verify_ssl, headers=self._headers)
+        resp.raise_for_status()
+        if len(resp.content) > 0:
+            return resp.json()
+
+    def _do_post_request(self, end_url: str, data: dict):
+        req_url = urljoin(self._base_url, end_url)
+        resp = requests.post(
+            req_url, verify=self._verify_ssl,
+            headers=self._headers,
+            data=data)
+        resp.raise_for_status()
+        if len(resp.content) > 0:
+            return resp.json()
+
+    def _do_put_request(self, end_url: str, data):
+        req_url = urljoin(self._base_url, end_url)
+        up_header = self._headers.copy()
+        up_header['accept'] = '*/*'
+        if isinstance(data, dict):
+            up_header['Content-Type'] = 'application/json'
+            data = json.dumps(data)
+        else:
+            up_header['Content-Type'] = 'application/octet-stream'
+        resp = requests.put(
+            req_url, verify=self._verify_ssl,
+            headers=up_header,
+            data=data)
+        resp.raise_for_status()
+        if len(resp.content) > 0:
+            return resp.json()
+
     def get_stock(self) -> List[CurrentStockResponse]:
-        req_url = urljoin(self._base_url, "stock")
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        if resp.status_code != 200 or not resp.text:
-            return
-        parsed_json = resp.json()
+        parsed_json = self._do_get_request("stock")
         return [CurrentStockResponse(response) for response in parsed_json]
 
     def get_volatile_stock(self) -> CurrentVolatilStockResponse:
-        req_url = urljoin(self._base_url, "stock/volatile")
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        parsed_json = resp.json()
+        parsed_json = self._do_get_request("stock/volatile")
         return CurrentVolatilStockResponse(parsed_json)
 
     def get_product(self, product_id) -> ProductDetailsResponse:
-        req_url = urljoin(urljoin(self._base_url, "stock/products/"), str(product_id))
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        if resp.status_code != 200 or not resp.text:
-            return
-        parsed_json = resp.json()
-        return ProductDetailsResponse(parsed_json)
+        url = f"stock/products/{product_id}"
+        parsed_json = self._do_get_request(url)
+        if parsed_json:
+            return ProductDetailsResponse(parsed_json)
 
     def get_chores(self) -> List[CurrentChoreResponse]:
-        req_url = urljoin(self._base_url, "chores")
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        parsed_json = resp.json()
+        parsed_json = self._do_get_request("chores")
         return [CurrentChoreResponse(chore) for chore in parsed_json]
 
     def get_chore(self, chore_id: int) -> ChoreDetailsResponse:
-        req_url = urljoin(urljoin(self._base_url, "chores/"), str(chore_id))
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        parsed_json = resp.json()
-        return ChoreDetailsResponse(parsed_json)
+        url = f"chores/{chore_id}"
+        parsed_json = self._do_get_request(url)
+        if parsed_json:
+            return ChoreDetailsResponse(parsed_json)
 
     def execute_chore(self, chore_id: int, done_by: int = None, tracked_time: datetime = datetime.now()):
         # Grocy API expects UTC time; time returned from datetime.now() is local time without timezone
@@ -374,8 +399,7 @@ class GrocyApiClient(object):
         if done_by is not None:
             data["done_by"] = done_by
 
-        req_url = urljoin(urljoin(urljoin(self._base_url, "chores/"), str(chore_id) + "/"), "execute")
-        requests.post(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
+        self._do_post_request(f"chores/{chore_id}/execute", data)
 
     def add_product(self, product_id, amount: float, price: float, best_before_date: datetime = None,
                     transaction_type: TransactionType = TransactionType.PURCHASE):
@@ -388,8 +412,7 @@ class GrocyApiClient(object):
         if best_before_date is not None:
             data["best_before_date"] = best_before_date.strftime('%Y-%m-%d')
 
-        req_url = urljoin(urljoin(urljoin(self._base_url, "stock/products/"), str(product_id) + "/"), "add")
-        requests.post(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
+        self._do_post_request(f"stock/products/{product_id}/add", data)
 
     def consume_product(self, product_id: int, amount: float = 1, spoiled: bool = False,
                         transaction_type: TransactionType = TransactionType.CONSUME):
@@ -399,16 +422,10 @@ class GrocyApiClient(object):
             "transaction_type": transaction_type.value
         }
 
-        req_url = urljoin(urljoin(urljoin(self._base_url, "stock/products/"), str(product_id) + "/"), "consume")
-        requests.post(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
-
+        self._do_post_request(f"stock/products/{product_id}/consume", data)
         
     def get_shopping_list(self) -> List[ShoppingListItem]:
-        req_url = urljoin(self._base_url, "objects/shopping_list")
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        if resp.status_code != 200:
-            return
-        parsed_json = resp.json()
+        parsed_json = self._do_get_request("objects/shopping_list")
         return [ShoppingListItem(response) for response in parsed_json]
 
     def add_missing_product_to_shopping_list(self, shopping_list_id: int = 1):
@@ -416,9 +433,7 @@ class GrocyApiClient(object):
             "list_id": shopping_list_id
         }
 
-        req_url = urljoin(self._base_url, "stock/shoppinglist/add-missing-products")
-        resp = requests.post(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
-        return resp
+        self._do_post_request("stock/shoppinglist/add-missing-products", data)
     
     def add_product_to_shopping_list(self, product_id: int, shopping_list_id: int = 1, amount: int = 1):
         data = {
@@ -426,19 +441,14 @@ class GrocyApiClient(object):
             "list_id": shopping_list_id,
             "product_amount": amount
         }
-
-        req_url = urljoin(self._base_url, "stock/shoppinglist/add-product")
-        resp = requests.post(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
-        return resp
+        self._do_post_request("stock/shoppinglist/add-product", data)
     
     def clear_shopping_list(self, shopping_list_id: int = 1):
         data = {
             "list_id": shopping_list_id
         }
 
-        req_url = urljoin(self._base_url, "stock/shoppinglist/clear")
-        resp = requests.post(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
-        return resp
+        self._do_post_request("stock/shoppinglist/clear", data)
             
     def remove_product_in_shopping_list(self, product_id: int, shopping_list_id: int = 1, amount: int = 1):
         data = {
@@ -446,57 +456,36 @@ class GrocyApiClient(object):
             "list_id": shopping_list_id,
             "product_amount": amount
         }
-        req_url = urljoin(self._base_url, "stock/shoppinglist/remove-product")
-        resp = requests.post(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
-        return resp
+        self._do_post_request("stock/shoppinglist/remove-product", data)
         
     def get_product_groups(self) -> List[LocationData]:
-        req_url = urljoin(self._base_url, "objects/product_groups")
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        if resp.status_code != 200 or not resp.text:
-            return
-        parsed_json = resp.json()
+        parsed_json = self._do_get_request("objects/product_groups")
         return [LocationData(response) for response in parsed_json]
-        
+
     def upload_product_picture(self, product_id: int, pic_path: str):
         if not os.path.exists(pic_path):
             return
-        up_header = self._headers.copy()
-        up_header['accept'] = '*/*'
-        up_header['Content-Type'] = 'application/octet-stream'
         b64fn = base64.b64encode('{}.jpg'.format(product_id).encode('ascii'))
-        req_url = urljoin(urljoin(self._base_url, "files/productpictures/" ), str(b64fn, "utf-8"))
+        req_url = "files/productpictures/" + str(b64fn, "utf-8")
         with open(pic_path,'rb') as pic:
-            resp =requests.put(req_url, verify=self._verify_ssl, headers=up_header , data=pic)
-            return resp
+            self._do_put_request(req_url, pic)
             
     def update_product_pic(self, product_id: int):
-        pic_name = '{}.jpg'.format(product_id)
+        pic_name = f"{product_id}.jpg"
         data = { "picture_file_name":  pic_name }
-        up_header = self._headers.copy()
-        up_header['accept'] = '*/*'
-        up_header['Content-Type'] = 'application/json'
-        req_url = urljoin(urljoin(self._base_url, "objects/products/"), str(product_id))
-        resp = requests.put(req_url, verify=self._verify_ssl, headers=up_header , data=json.dumps(data))
-        return resp
+        self._do_put_request(f"objects/products/{product_id}", data)
             
     def get_userfields(self, entity: str, object_id: int):
-        req_url = urljoin(urljoin(urljoin(self._base_url, "userfields/"), entity + "/"), str(object_id))
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        if not resp:
-            return resp
-        return resp.json()
+        url = f"userfields/{entity}/{object_id}"
+        return self._do_get_request(url)
         
     def set_userfields(self, entity: str, object_id: int, key: str, value):
         data = {
             key: value
         }
-        req_url = urljoin(urljoin(urljoin(self._base_url, "userfields/"), entity + "/"), str(object_id))
-        resp = requests.put(req_url, verify=self._verify_ssl, headers=self._headers, data=data)
-        return resp
+        self._do_put_request(f"userfields/{entity}/{object_id}", data)
 
     def get_last_db_changed(self):
-        req_url = urljoin(self._base_url, "system/db-changed-time")
-        resp = requests.get(req_url, verify=self._verify_ssl, headers=self._headers)
-        last_change_timestamp = parse_date(resp.json().get('changed_time'))
+        resp = self._do_get_request("system/db-changed-time")
+        last_change_timestamp = parse_date(resp.get('changed_time'))
         return last_change_timestamp


Test patch:
diff --git a/test/test_grocy.py b/test/test_grocy.py
index 8ffa7a1..8251b48 100644
--- a/test/test_grocy.py
+++ b/test/test_grocy.py
@@ -2,6 +2,7 @@ from unittest import TestCase
 from unittest.mock import patch, mock_open
 
 from datetime import datetime
+from requests.exceptions import HTTPError
 import responses
 from pygrocy import Grocy
 from pygrocy.grocy import Product
@@ -24,7 +25,7 @@ class TestGrocy(TestCase):
         chores = self.grocy.chores(get_details=True)
         
         assert isinstance(chores, list)
-        assert len(chores) == 6
+        self.assertEqual(len(chores), 6)
         assert chores[0].id == 1
         assert chores[1].id == 2
         assert chores[2].id == 3
@@ -50,6 +51,7 @@ class TestGrocy(TestCase):
             "amount": "0.33",
             "best_before_date": "2019-05-02"
         })
+
         product = Product(current_stock_response)
 
         api_client = GrocyApiClient(CONST_BASE_URL, "demo_mode", port = CONST_PORT, verify_ssl = CONST_SSL)
@@ -68,19 +70,11 @@ class TestGrocy(TestCase):
         for prod in stock:
             assert isinstance(prod, Product)
 
-    @responses.activate
-    def test_get_stock_invalid_no_data(self):
-        responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock", status=200)
-
-        assert self.grocy.stock() is None
-
     @responses.activate
     def test_get_stock_invalid_missing_data(self):
-        resp = [
-            {
-            }
-        ]
+        resp = []
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock", json=resp, status=200)
+        self.assertEqual(len(self.grocy.stock()) ,0)
         
     def test_get_shopping_list_valid(self):
         shopping_list = self.grocy.shopping_list(True)
@@ -93,49 +87,47 @@ class TestGrocy(TestCase):
     @responses.activate
     def test_get_shopping_list_invalid_no_data(self):
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/objects/shopping_list", status=400)
-        assert self.grocy.shopping_list() is None
+        self.assertRaises(HTTPError, self.grocy.shopping_list)
         
     @responses.activate
     def test_get_shopping_list_invalid_missing_data(self):
-        resp = [
-            {
-            }
-        ]
+        resp = []
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/objects/shopping_list", json=resp, status=200)
+        self.assertEqual(len(self.grocy.shopping_list()), 0)
         
     def test_add_missing_product_to_shopping_list_valid(self):
-        assert self.grocy.add_missing_product_to_shopping_list().status_code == 204
+         assert self.grocy.add_missing_product_to_shopping_list() is None
         
     @responses.activate
     def test_add_missing_product_to_shopping_list_error(self):
         responses.add(responses.POST, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/shoppinglist/add-missing-products", status=400)
-        assert self.grocy.add_missing_product_to_shopping_list().status_code != 204
+        self.assertRaises(HTTPError, self.grocy.add_missing_product_to_shopping_list)
         
     def test_add_product_to_shopping_list_valid(self):
-        assert self.grocy.add_product_to_shopping_list(22).status_code == 204
+        self.grocy.add_product_to_shopping_list(22)
         
     def test_add_product_to_shopping_list_error(self):
-        assert self.grocy.add_product_to_shopping_list(3000).status_code != 204
+        self.assertRaises(HTTPError, self.grocy.add_product_to_shopping_list, 3000)
         
     @responses.activate
     def test_clear_shopping_list_valid(self):
         responses.add(responses.POST, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/shoppinglist/clear", status=204)
-        assert self.grocy.clear_shopping_list().status_code == 204
+        self.grocy.clear_shopping_list()
         
     @responses.activate
     def test_clear_shopping_list_error(self):
         responses.add(responses.POST, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/shoppinglist/clear", status=400)
-        assert self.grocy.clear_shopping_list().status_code != 204
+        self.assertRaises(HTTPError, self.grocy.clear_shopping_list)
         
     @responses.activate
     def test_remove_product_in_shopping_list_valid(self):
         responses.add(responses.POST, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/shoppinglist/remove-product", status=204)
-        assert self.grocy.remove_product_in_shopping_list(1).status_code == 204
+        self.grocy.remove_product_in_shopping_list(1)
         
     @responses.activate
     def test_remove_product_in_shopping_list_error(self):
         responses.add(responses.POST, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/shoppinglist/remove-product", status=400)
-        assert self.grocy.remove_product_in_shopping_list(1).status_code != 204
+        self.assertRaises(HTTPError, self.grocy.remove_product_in_shopping_list, 1)
         
     def test_get_product_groups_valid(self):
         product_groups_list = self.grocy.product_groups()
@@ -148,15 +140,13 @@ class TestGrocy(TestCase):
     @responses.activate
     def test_get_product_groups_invalid_no_data(self):
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/objects/product_groups", status=400)
-        assert self.grocy.product_groups() is None
+        self.assertRaises(HTTPError, self.grocy.product_groups)
         
     @responses.activate
     def test_get_product_groups_invalid_missing_data(self):
-        resp = [
-            {
-            }
-        ]
+        resp = []
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/objects/product_groups", json=resp, status=200)
+        self.assertEqual(len(self.grocy.product_groups()), 0)
         
     @responses.activate
     def test_upload_product_picture_valid(self):
@@ -165,7 +155,7 @@ class TestGrocy(TestCase):
                 m_exist.return_value = True
                 api_client = GrocyApiClient(CONST_BASE_URL, "demo_mode", port = CONST_PORT, verify_ssl = CONST_SSL)
                 responses.add(responses.PUT, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/files/productpictures/MS5qcGc=", status=204)
-                assert api_client.upload_product_picture(1,"/somepath/pic.jpg").status_code == 204
+                api_client.upload_product_picture(1,"/somepath/pic.jpg")
             
     @responses.activate
     def test_upload_product_picture_invalid_missing_data(self):
@@ -182,20 +172,19 @@ class TestGrocy(TestCase):
                 m_exist.return_value = True
                 api_client = GrocyApiClient(CONST_BASE_URL, "demo_mode", port = CONST_PORT, verify_ssl = CONST_SSL)
                 responses.add(responses.PUT, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/files/productpictures/MS5qcGc=", status=400)
-                assert api_client.upload_product_picture(1,"/somepath/pic.jpg").status_code != 204
+                self.assertRaises(HTTPError, api_client.upload_product_picture, 1,"/somepath/pic.jpg")
                 
     @responses.activate
     def test_update_product_pic_valid(self):
         api_client = GrocyApiClient(CONST_BASE_URL, "demo_mode", port = CONST_PORT, verify_ssl = CONST_SSL)
         responses.add(responses.PUT, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/objects/products/1", status=204)
-        assert api_client.update_product_pic(1).status_code == 204
+        api_client.update_product_pic(1)
         
     @responses.activate
     def test_update_product_pic_error(self):
         api_client = GrocyApiClient(CONST_BASE_URL, "demo_mode", port = CONST_PORT, verify_ssl = CONST_SSL)
         responses.add(responses.PUT, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/objects/products/1", status=400)
-        assert api_client.update_product_pic(1).status_code != 204
-        
+        self.assertRaises(HTTPError, api_client.update_product_pic, 1)        
     
     def test_get_expiring_products_valid(self):
         
@@ -215,7 +204,7 @@ class TestGrocy(TestCase):
         }
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/volatile", json=resp, status=200)
 
-        assert not self.grocy.expiring_products(True)
+        self.grocy.expiring_products(True)
 
     @responses.activate
     def test_get_expiring_invalid_missing_data(self):
@@ -240,7 +229,7 @@ class TestGrocy(TestCase):
         }
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/volatile", json=resp, status=200)
 
-        assert not self.grocy.expired_products(True)
+        self.grocy.expired_products(True)
 
     @responses.activate
     def test_get_expired_invalid_missing_data(self):
@@ -265,7 +254,7 @@ class TestGrocy(TestCase):
         }
         responses.add(responses.GET, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/stock/volatile", json=resp, status=200)
 
-        assert not self.grocy.missing_products(True)
+        self.grocy.missing_products(True)
 
     @responses.activate
     def test_get_missing_invalid_missing_data(self):
@@ -286,18 +275,17 @@ class TestGrocy(TestCase):
         assert a_chore_uf['uf1'] == 0
 
     def test_get_userfields_invalid_no_data(self):
-        assert not self.grocy.get_userfields("chores",1) 
+        self.grocy.get_userfields("chores",1)
 
     @responses.activate
     def test_set_userfields_valid(self):
         responses.add(responses.PUT, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/userfields/chores/1", status=204)
-        assert self.grocy.set_userfields("chores",1,"auserfield","value").status_code == 204
+        self.grocy.set_userfields("chores",1,"auserfield", "value")
         
     @responses.activate
     def test_set_userfields_error(self):
         responses.add(responses.PUT, '{}:{}'.format(CONST_BASE_URL,CONST_PORT) + "/api/userfields/chores/1", status=400)
-        assert self.grocy.set_userfields("chores",1,"auserfield","value").status_code != 204
-        
+        self.assertRaises(HTTPError, self.grocy.set_userfields, "chores",1,"auserfield","value")
 
     def test_get_last_db_changed_valid(self):