Repository: bimmerconnected/bimmer_connected
Pull request #167 (2020-03-28 19:10:05)

Refactor send_message and send_poi for HomeAssistant
This PR provides the necessary refactoring to enable #165 in Home Assistant.

__WARNING!__ This is a breaking change as we're moving the `Message` and `PointOfInterest` classes from `vehicle.py` to `remote_services.py`, where they actually belong. 
The _cli_ is not affected by this change as it has has been adapted already.

See the implementation in `cli.py` on how to call `trigger_send_message` and `trigger_send_poi` correctly. Both require a `dict` as parameter with different keys.

rikroe: Maybe the docs are failing due to :param... : in remote_services.py...
Thise were the only tests I haven't run locally 
gerard33: They are in `trigger_send_message` as well. Looks like docs are not liking the indentation. Removed an additional space, but that hasn't fixed it and still gives the unexpected indendation error.
rikroe: Sphinx does work with indentions, but one needs to add a blank line before `:params data:` (cf. [here](https://stackoverflow.com/questions/45880348/how-to-remove-the-cause-of-an-unexpected-indentation-warning-when-generating-cod)) 🙉 

Patch:
diff --git a/bimmer_connected/cli.py b/bimmer_connected/cli.py
index 85fb78b..4a1359f 100644
--- a/bimmer_connected/cli.py
+++ b/bimmer_connected/cli.py
@@ -12,7 +12,7 @@ import requests
 
 from bimmer_connected.account import ConnectedDriveAccount
 from bimmer_connected.country_selector import get_region_from_name, valid_regions
-from bimmer_connected.vehicle import VehicleViewDirection, PointOfInterest, Message
+from bimmer_connected.vehicle import VehicleViewDirection
 
 FINGERPRINT_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)),
                                'vehicle_fingerprint')
@@ -147,10 +147,16 @@ def send_poi(args) -> None:
     """Send Point Of Interest to car."""
     account = ConnectedDriveAccount(args.username, args.password, get_region_from_name(args.region))
     vehicle = account.get_vehicle(args.vin)
-    poi = PointOfInterest(args.latitude, args.longitude, name=args.name,
-                          street=args.street, city=args.city, postalCode=args.postalcode, country=args.country)
-    msg = Message.from_poi(poi)
-    vehicle.send_message(msg)
+    poi_data = dict(
+        lat=args.latitude,
+        lon=args.longitude,
+        name=args.name,
+        street=args.street,
+        city=args.city,
+        postalCode=args.postalcode,
+        country=args.country
+    )
+    vehicle.remote_services.trigger_send_poi(poi_data)
 
 
 def send_poi_from_address(args) -> None:
@@ -169,29 +175,31 @@ def send_poi_from_address(args) -> None:
     except IndexError:
         print('\nAddress not found')
         sys.exit(1)
-    lat = response["lat"]
-    long = response["lon"]
-    name = args.name
     address = response.get("address", {})
-    street = address.get("road")
     city = address.get("city")
     town = address.get("town")
-    city = town if city is None and town is not None else None
-    postcode = address.get("postcode")
-    country = address.get("country")
 
-    print("\nSending '" + lat, long, name, street, city, postcode, country + "' to your car\n")
-    poi = PointOfInterest(lat, long, name=args.name, street=street, city=city, postalCode=postcode, country=country)
-    msg = Message.from_poi(poi)
-    vehicle.send_message(msg)
+    poi_data = dict(
+        lat=response["lat"],
+        lon=response["lon"],
+        name=args.name,
+        street=address.get("road"),
+        city=town if city is None and town is not None else None,
+        postalCode=address.get("postcode"),
+        country=address.get("country")
+    )
+    vehicle.remote_services.trigger_send_poi(poi_data)
 
 
 def send_message(args) -> None:
-    """Send Point Of Interest to car."""
+    """Send a message to car."""
     account = ConnectedDriveAccount(args.username, args.password, get_region_from_name(args.region))
     vehicle = account.get_vehicle(args.vin)
-    msg = Message.from_text(args.text, args.subject)
-    vehicle.send_message(msg)
+    msg_data = dict(
+        message=args.text,
+        subject=args.subject
+    )
+    vehicle.remote_services.trigger_send_message(msg_data)
 
 
 def _add_default_arguments(parser: argparse.ArgumentParser):
diff --git a/bimmer_connected/remote_services.py b/bimmer_connected/remote_services.py
index 43a6061..377668d 100644
--- a/bimmer_connected/remote_services.py
+++ b/bimmer_connected/remote_services.py
@@ -1,12 +1,17 @@
 """Trigger remote services on a vehicle."""
 
-from enum import Enum
 import datetime
+import json
 import logging
 import time
+from enum import Enum
+from urllib.parse import urlencode
+
 import requests
-from bimmer_connected.const import REMOTE_SERVICE_URL, REMOTE_SERVICE_STATUS_URL
 
+from bimmer_connected.const import (REMOTE_SERVICE_STATUS_URL,
+                                    REMOTE_SERVICE_URL,
+                                    VEHICLE_POI_URL)
 
 TIME_FORMAT = '%Y-%m-%dT%H:%M:%S.%f'
 
@@ -39,6 +44,71 @@ class _Services(Enum):
     REMOTE_AIR_CONDITIONING = 'CLIMATE_NOW'
 
 
+# pylint: disable=too-many-instance-attributes, too-few-public-methods
+class PointOfInterest:
+    """Point of interest to be sent to the vehicle.
+
+    The latitude/longitude of a POI are mandatory, all other attributes are optional. CamelCase attribute names are
+    used here so that we do not have to convert the names between the attributes and the keys as expected on the server.
+    """
+
+    # pylint: disable=too-many-arguments
+    def __init__(self, lat: float, lon: float, name: str = None,
+                 additionalInfo: str = None, street: str = None, city: str = None,
+                 postalCode: str = None, country: str = None, website: str = None,
+                 phoneNumbers: [str] = None):
+        """Constructor.
+
+        :arg latitude: latitude of the POI
+        :arg longitude: longitude of the POI
+        :arg name: name of the POI (Optional)
+        :arg additionalInfo: additional text shown below the address (Optional)
+        :arg street: street with house number of the POI (Optional)
+        :arg city: city of the POI (Optional)
+        :arg postalCode: zip code of the POI (Optional)
+        :arg country: country of the POI (Optional)
+        :arg website: website of the POI (Optional)
+        :arg phoneNumbers: List of phone numbers of the POI (Optional)
+        """
+        # pylint: disable=invalid-name
+        self.lat = lat  # type: float
+        self.lon = lon  # type: float
+        self.name = name  # type: str
+        self.additionalInfo = additionalInfo if additionalInfo is not None \
+            else 'Sent with ♥ by bimmer_connected'  # type: str
+        self.street = street  # type: str
+        self.city = city  # type: str
+        self.postalCode = postalCode  # type: str
+        self.country = country  # type: str
+        self.website = website  # type: str
+        self.phoneNumbers = phoneNumbers  # type: list[str]
+
+
+class Message:
+    """Text message or PointOfInterst to be sent to the vehicle."""
+
+    @classmethod
+    def from_poi(cls, poi: PointOfInterest):
+        """Create a message from a PointOfInterest"""
+        return cls(poi.__dict__)
+
+    @classmethod
+    def from_text(cls, text: str, subject: str = None):
+        """Create a text message"""
+        return cls({"name": subject, "additionalInfo": text[:255]})
+
+    def __init__(self, data: dict):
+        self.data = data
+
+    @property
+    def as_server_request(self) -> str:
+        """Convert to a dictionary so that it can be sent to the server."""
+        result = {
+            'poi': {k: v for k, v in self.data.items() if v is not None}
+        }
+        return urlencode({'data': json.dumps(result)})
+
+
 class RemoteServiceStatus:  # pylint: disable=too-few-public-methods
     """Wraps the status of the execution of a remote service."""
 
@@ -172,3 +242,51 @@ class RemoteServices:
     def _trigger_state_update(self) -> None:
         time.sleep(_UPDATE_AFTER_REMOTE_SERVICE_DELAY)
         self._account.update_vehicle_states()
+
+    def trigger_send_message(self, data: dict) -> RemoteServiceStatus:
+        """Send a message to the vehicle.
+
+        :param data: A dictonary containing a 'text' and an optional 'subject'
+        :type data: dict
+
+        A state update is NOT triggered after this, as the vehicle state is unchanged.
+        """
+        _LOGGER.debug('Sending message to car')
+        if "text" not in data:
+            raise TypeError("from_text() missing 1 required positional argument: 'text'")
+        self._send_message(Message.from_text(data['text'], data.get('subject')))
+        # _send_message has no separate ExecutionStates
+        return RemoteServiceStatus({'executionStatus': {'status': 'EXECUTED', 'eventId': -1}})
+
+    def trigger_send_poi(self, data: dict) -> RemoteServiceStatus:
+        """Send a PointOfInterest to the vehicle.
+
+        :param data: A dictonary containing at least 'lat' and 'lon' and optionally
+            'name', 'additionalInfo', 'street', 'city', 'postalCode', 'country',
+            'website' or 'phoneNumbers'
+        :type data: dict
+
+        A state update is NOT triggered after this, as the vehicle state is unchanged.
+        """
+        _LOGGER.debug('Sending PointOfInterest to car')
+        if "lat" not in data or "lon" not in data:
+            raise TypeError("__init__() missing 2 required positional arguments: 'lat' and 'lon'")
+        poi = PointOfInterest(**data)
+        self._send_message(Message.from_poi(poi))
+        # _send_message has no separate ExecutionStates
+        return RemoteServiceStatus({'executionStatus': {'status': 'EXECUTED', 'eventId': -1}})
+
+    def _send_message(self, msg: Message) -> None:
+        """Send a message/point of interest to the vehicle."""
+        url = VEHICLE_POI_URL.format(
+            vin=self._vehicle.vin,
+            server=self._account.server_url
+        )
+        header = self._account.request_header
+        # the accept field of the header needs to be updated not the usual JSON
+        header['Content-Type'] = 'application/x-www-form-urlencoded'
+        return self._account.send_request(url,
+                                          headers=header,
+                                          data=msg.as_server_request,
+                                          post=True,
+                                          expected_response=204)
diff --git a/bimmer_connected/vehicle.py b/bimmer_connected/vehicle.py
index ab3f2af..da50ea8 100644
--- a/bimmer_connected/vehicle.py
+++ b/bimmer_connected/vehicle.py
@@ -2,12 +2,10 @@
 from enum import Enum
 import logging
 from typing import List
-import json
-from urllib.parse import urlencode
 
 from bimmer_connected.state import VehicleState, WINDOWS, LIDS
 from bimmer_connected.remote_services import RemoteServices
-from bimmer_connected.const import VEHICLE_IMAGE_URL, VEHICLE_POI_URL
+from bimmer_connected.const import VEHICLE_IMAGE_URL
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -54,71 +52,6 @@ class LscType(Enum):
     LSC_PHEV = 'LSC_PHEV'
 
 
-# pylint: disable=too-many-instance-attributes, too-few-public-methods
-class PointOfInterest:
-    """Point of interest to be sent to the vehicle.
-
-    The latitude/longitude of a POI are mandatory, all other attributes are optional. CamelCase attribute names are
-    used here so that we do not have to convert the names between the attributes and the keys as expected on the server.
-    """
-
-    # pylint: disable=too-many-arguments
-    def __init__(self, latitude: float, longitude: float, name: str = None,
-                 additionalInfo: str = None, street: str = None, city: str = None,
-                 postalCode: str = None, country: str = None, website: str = None,
-                 phoneNumbers: [str] = None):
-        """Constructor.
-
-        :arg latitude: latitude of the POI
-        :arg longitude: longitude of the POI
-        :arg name: name of the POI (Optional)
-        :arg additionalInfo: additional text shown below the address (Optional)
-        :arg street: street with house number of the POI (Optional)
-        :arg city: city of the POI (Optional)
-        :arg postalCode: zip code of the POI (Optional)
-        :arg country: country of the POI (Optional)
-        :arg website: website of the POI (Optional)
-        :arg phoneNumbers: List of phone numbers of the POI (Optional)
-        """
-        # pylint: disable=invalid-name
-        self.lat = latitude  # type: float
-        self.lon = longitude  # type: float
-        self.name = name  # type: str
-        self.additionalInfo = additionalInfo if additionalInfo is not None \
-            else 'Sent with ♥ by bimmer_connected'  # type: str
-        self.street = street  # type: str
-        self.city = city  # type: str
-        self.postalCode = postalCode  # type: str
-        self.country = country  # type: str
-        self.website = website  # type: str
-        self.phoneNumbers = phoneNumbers  # type: List[str]
-
-
-class Message:
-    """Text message or PointOfInterst to be sent to the vehicle."""
-
-    @classmethod
-    def from_poi(cls, poi: PointOfInterest):
-        """Create a message from a PointOfInterest"""
-        return cls(poi.__dict__)
-
-    @classmethod
-    def from_text(cls, text: str, subject: str = None):
-        """Create a text message"""
-        return cls({"name": subject, "additionalInfo": text[:255]})
-
-    def __init__(self, data: dict):
-        self.data = data
-
-    @property
-    def as_server_request(self) -> str:
-        """Convert to a dictionary so that it can be sent to the server."""
-        result = {
-            'poi': {k: v for k, v in self.data.items() if v is not None}
-        }
-        return urlencode({'data': json.dumps(result)})
-
-
 class ConnectedDriveVehicle:
     """Models state and remote services of one vehicle.
 
@@ -249,14 +182,3 @@ class ConnectedDriveVehicle:
             raise ValueError('Either latitude AND longitude are set or none of them. You cannot set only one of them!')
         self.observer_latitude = latitude
         self.observer_longitude = longitude
-
-    def send_message(self, msg: Message) -> None:
-        """Send a message/point of interest to the vehicle."""
-        url = VEHICLE_POI_URL.format(
-            vin=self.vin,
-            server=self._account.server_url
-        )
-        header = self._account.request_header
-        # the accept field of the header needs to be updated as we want a png not the usual JSON
-        header['Content-Type'] = 'application/x-www-form-urlencoded'
-        self._account.send_request(url, headers=header, data=msg.as_server_request, post=True, expected_response=204)


Test patch:
diff --git a/test/__init__.py b/test/__init__.py
index 95c7f69..61b7868 100644
--- a/test/__init__.py
+++ b/test/__init__.py
@@ -108,16 +108,12 @@ POI_REQUEST = {
             "%2C+%22phoneNumbers%22%3A+%5B%22%2B1+408-562-4949%22%5D%7D%7D")
 }
 
-MESSAGE_DATA = {
-    "min": {"text": "Message without subject!"},
-    "all": {"subject": "This is a subject...", "text": "... with some message!"}
-}
+MESSAGE_DATA = {"subject": "This is a subject...", "text": "This is a message!"}
 
 MESSAGE_REQUEST = {
-    "min": ("data=%7B%22poi%22%3A+%7B%22additionalInfo%22%3A+%22Message+without+subject%21%22%7D%7D"),
+    "min": ("data=%7B%22poi%22%3A+%7B%22additionalInfo%22%3A+%22This+is+a+message%21%22%7D%7D"),
     "all": ("data=%7B%22poi%22%3A+%7B%22name%22%3A+%22This+is+a+subject...%22%2C+%22additionalInfo"
-            "%22%3A+%22...+with+some+message%21%22%7D%7D")
-
+            "%22%3A+%22This+is+a+message%21%22%7D%7D")
 }
 
 
diff --git a/test/responses/G31_NBTevo/msg_executed.json b/test/responses/G31_NBTevo/msg_executed.json
new file mode 100644
index 0000000..43cf9a2
--- /dev/null
+++ b/test/responses/G31_NBTevo/msg_executed.json
@@ -0,0 +1,6 @@
+{
+  "executionStatus" : {
+    "eventId" : "-1",
+    "status" : "EXECUTED"
+  }
+}
\ No newline at end of file
diff --git a/test/test_remote_services.py b/test/test_remote_services.py
index 5580d75..61dbc2d 100644
--- a/test/test_remote_services.py
+++ b/test/test_remote_services.py
@@ -2,16 +2,18 @@
 import unittest
 from unittest import mock
 import datetime
-from test import BackendMock, TEST_USERNAME, TEST_PASSWORD, TEST_REGION, G31_VIN, load_response_json
+from test import BackendMock, TEST_USERNAME, TEST_PASSWORD, TEST_REGION, G31_VIN, load_response_json, \
+    POI_DATA, POI_REQUEST, MESSAGE_DATA, MESSAGE_REQUEST
 
 from bimmer_connected.account import ConnectedDriveAccount
-from bimmer_connected.remote_services import RemoteServiceStatus, ExecutionState
+from bimmer_connected.remote_services import RemoteServiceStatus, ExecutionState, PointOfInterest, Message
 from bimmer_connected import remote_services
 
 _RESPONSE_INITIATED = 'G31_NBTevo/flash_initiated.json'
 _RESPONSE_PENDING = 'G31_NBTevo/flash_pending.json'
 _RESPONSE_DELIVERED = 'G31_NBTevo/flash_delivered.json'
 _RESPONSE_EXECUTED = 'G31_NBTevo/flash_executed.json'
+_MSG_EXECUTED = 'G31_NBTevo/msg_executed.json'
 
 
 class TestRemoteServices(unittest.TestCase):
@@ -49,7 +51,9 @@ class TestRemoteServices(unittest.TestCase):
             ('DOOR_LOCK', 'trigger_remote_door_lock', True),
             ('DOOR_UNLOCK', 'trigger_remote_door_unlock', True),
             ('CLIMATE_NOW', 'trigger_remote_air_conditioning', True),
-            ('HORN_BLOW', 'trigger_remote_horn', False)
+            ('HORN_BLOW', 'trigger_remote_horn', False),
+            ('SEND_MESSAGE', 'trigger_send_message', False),
+            ('SEND_POI', 'trigger_send_poi', False),
         ]
 
         for service, call, triggers_update in services:
@@ -76,13 +80,24 @@ class TestRemoteServices(unittest.TestCase):
                     vin=G31_VIN),
                 data_files=[_RESPONSE_EXECUTED])
 
+            backend_mock.add_response(
+                r'https://.+/webapi/v1/user/vehicles/{vin}/sendpoi'.format(
+                    vin=G31_VIN),
+                data_files=[_MSG_EXECUTED],
+                status_code=204)
+
             with mock.patch('bimmer_connected.account.requests', new=backend_mock):
                 account = ConnectedDriveAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
                 mock_listener = mock.Mock(return_value=None)
                 account.add_update_listener(mock_listener)
                 vehicle = account.get_vehicle(G31_VIN)
 
-                response = getattr(vehicle.remote_services, call)()
+                if service == 'SEND_MESSAGE':
+                    response = getattr(vehicle.remote_services, call)(MESSAGE_DATA)
+                elif service == 'SEND_POI':
+                    response = getattr(vehicle.remote_services, call)(POI_DATA)
+                else:
+                    response = getattr(vehicle.remote_services, call)()
                 self.assertEqual(ExecutionState.EXECUTED, response.state)
 
                 if triggers_update:
@@ -105,3 +120,29 @@ class TestRemoteServices(unittest.TestCase):
                 data_files=['G31_NBTevo/flash_executed.json'])
             status = vehicle.remote_services._get_remote_service_status(remote_services._Services.REMOTE_LIGHT_FLASH)
             self.assertEqual(ExecutionState.EXECUTED, status.state)
+
+    def test_parsing_of_poi_min_attributes(self):
+        """Check that a PointOfInterest can be constructed using only latitude & longitude."""
+        poi = PointOfInterest(POI_DATA["lat"], POI_DATA["lon"])
+        msg = Message.from_poi(poi)
+        self.assertEqual(msg.as_server_request, POI_REQUEST["min"])
+
+    def test_parsing_of_poi_all_attributes(self):
+        """Check that a PointOfInterest can be constructed using all attributes."""
+        poi = PointOfInterest(POI_DATA["lat"], POI_DATA["lon"], name=POI_DATA["name"],
+                              additionalInfo=POI_DATA["additionalInfo"], street=POI_DATA["street"],
+                              city=POI_DATA["city"], postalCode=POI_DATA["postalCode"],
+                              country=POI_DATA["country"], website=POI_DATA["website"],
+                              phoneNumbers=POI_DATA["phoneNumbers"])
+        msg = Message.from_poi(poi)
+        self.assertEqual(msg.as_server_request, POI_REQUEST["all"])
+
+    def test_parsing_of_message_min_attributes(self):
+        """Check that a Message can be constructed using text."""
+        msg = Message.from_text(MESSAGE_DATA["text"])
+        self.assertEqual(msg.as_server_request, MESSAGE_REQUEST["min"])
+
+    def test_parsing_of_message_all_attributes(self):
+        """Check that a Message can be constructed using text."""
+        msg = Message.from_text(MESSAGE_DATA["text"], MESSAGE_DATA["subject"])
+        self.assertEqual(msg.as_server_request, MESSAGE_REQUEST["all"])
diff --git a/test/test_vehicle.py b/test/test_vehicle.py
index 6b0b259..6ff6a8e 100644
--- a/test/test_vehicle.py
+++ b/test/test_vehicle.py
@@ -3,10 +3,9 @@ import unittest
 from unittest import mock
 from test import load_response_json, BackendMock, TEST_USERNAME, TEST_PASSWORD, TEST_REGION, \
     G31_VIN, F48_VIN, I01_VIN, I01_NOREX_VIN, F15_VIN, F45_VIN, F31_VIN, TEST_VEHICLE_DATA, \
-    ATTRIBUTE_MAPPING, MISSING_ATTRIBUTES, ADDITIONAL_ATTRIBUTES, POI_DATA, POI_REQUEST, \
-    MESSAGE_DATA, MESSAGE_REQUEST
+    ATTRIBUTE_MAPPING, MISSING_ATTRIBUTES, ADDITIONAL_ATTRIBUTES
 
-from bimmer_connected.vehicle import ConnectedDriveVehicle, DriveTrainType, PointOfInterest, Message
+from bimmer_connected.vehicle import ConnectedDriveVehicle, DriveTrainType
 from bimmer_connected.account import ConnectedDriveAccount
 
 
@@ -72,29 +71,3 @@ class TestVehicle(unittest.TestCase):
                                           if a not in MISSING_ATTRIBUTES])
             expected_attributes = sorted([a for a in vehicle.available_attributes if a not in ADDITIONAL_ATTRIBUTES])
             self.assertListEqual(existing_attributes, expected_attributes)
-
-    def test_parsing_of_poi_min_attributes(self):
-        """Check that a PointOfInterest can be constructed using only latitude & longitude."""
-        poi = PointOfInterest(POI_DATA["lat"], POI_DATA["lon"])
-        msg = Message.from_poi(poi)
-        self.assertEqual(msg.as_server_request, POI_REQUEST["min"])
-
-    def test_parsing_of_poi_all_attributes(self):
-        """Check that a PointOfInterest can be constructed using all attributes."""
-        poi = PointOfInterest(POI_DATA["lat"], POI_DATA["lon"], name=POI_DATA["name"],
-                              additionalInfo=POI_DATA["additionalInfo"], street=POI_DATA["street"],
-                              city=POI_DATA["city"], postalCode=POI_DATA["postalCode"],
-                              country=POI_DATA["country"], website=POI_DATA["website"],
-                              phoneNumbers=POI_DATA["phoneNumbers"])
-        msg = Message.from_poi(poi)
-        self.assertEqual(msg.as_server_request, POI_REQUEST["all"])
-
-    def test_parsing_of_message_min_attributes(self):
-        """Check that a Message can be constructed using text."""
-        msg = Message.from_text(MESSAGE_DATA["min"]["text"])
-        self.assertEqual(msg.as_server_request, MESSAGE_REQUEST["min"])
-
-    def test_parsing_of_message_all_attributes(self):
-        """Check that a Message can be constructed using text."""
-        msg = Message.from_text(MESSAGE_DATA["all"]["text"], MESSAGE_DATA["all"]["subject"])
-        self.assertEqual(msg.as_server_request, MESSAGE_REQUEST["all"])