Repository: bimmerconnected/bimmer_connected
Pull request #592 (2024-01-21 13:25:59)

Refactor vehicle init
## Proposed change
<!--
  Describe the big picture of your changes here to communicate to the
  maintainers why we should accept this pull request. If it fixes a bug
  or resolves a feature request, be sure to link to that issue in the
  additional information section.
-->
Refactor the way we get vehicle data a bit, hopefully to make it clearer and less of an effort to add additional endpoints.
- Only get base vehicle information in account.py
- Get vehicle state and further information in vehicle.py
- Adjust methods to initialize MyBMWVehicle

Based on #591 so that should be merged first.

## Type of change
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (which adds functionality to this library)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [x] Code quality improvements to existing code or addition of tests

## Additional information
<!--
  Details are important, and help maintainers processing your PR.
  Please be sure to fill out additional details, if applicable.
-->

- This PR fixes or closes issue: fixes #
- This PR is related to issue: 

## Checklist
<!--
  Put an `x` in the boxes that apply. You can also fill these out after
  creating the PR. If you're unsure about any of them, don't hesitate to ask.
  We're here to help! This is simply a reminder of what we are going to look
  for before merging your code.
-->

- [x] The code change is tested and works locally.
- [x] Tests have been added to verify that the new code works.


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`9d3bd67`)](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/commit/9d3bd673f28f7e997d08faa0cc40464eff7af063?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) 100.00% compared to head [(`dce6a81`)](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) 100.00%.
> Report is 1 commits behind head on master.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff            @@
##            master      #592   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         1412      1428   +16     
=========================================
+ Hits          1412      1428   +16     
```

| [Flag](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | Coverage Δ | |
|---|---|---|
| [3.10](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |
| [3.11](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |
| [3.12](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |
| [3.8](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `?` | |
| [3.9](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/592?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected).

demel42: A hint  (from my fast test)

in the function "get_vehicle_image", _headers_ must now also include th entry _"bmw-app-vehicle-type"_ with the value _"connected"_


return values from the server was

```
"statusCode":400,"error":"Bad Request","message":["bmw-app-vehicle-type should not be null or undefined","bmw-app-vehicle-type must be one of: connected, demo, non-connected, predelivery"]
```

i din't use python, so i can't create an pull-request
rikroe: Many thanks for the quick feedback!

Patch:
diff --git a/bimmer_connected/__init__.py b/bimmer_connected/__init__.py
index ee838c2..4c924ea 100644
--- a/bimmer_connected/__init__.py
+++ b/bimmer_connected/__init__.py
@@ -8,7 +8,6 @@ Disclaimer:
 This library is not affiliated with or endorsed by BMW Group.
 """
 
-
 from importlib.metadata import version
 
 __version__ = version("bimmer_connected")
diff --git a/bimmer_connected/account.py b/bimmer_connected/account.py
index dc1f466..4a1c5b7 100644
--- a/bimmer_connected/account.py
+++ b/bimmer_connected/account.py
@@ -11,10 +11,7 @@ from bimmer_connected.api.client import RESPONSE_STORE, MyBMWClient, MyBMWClient
 from bimmer_connected.api.regions import Regions
 from bimmer_connected.const import (
     ATTR_ATTRIBUTES,
-    ATTR_CAPABILITIES,
-    VEHICLE_CHARGING_DETAILS_URL,
     VEHICLE_PROFILE_URL,
-    VEHICLE_STATE_URL,
     VEHICLES_URL,
     CarBrands,
 )
@@ -94,77 +91,38 @@ class MyBMWAccount:
                         **{"vin": vehicle_profile["vin"]}
                     )
 
-                    self.add_vehicle(vehicle_base, None, None, fetched_at)
+                    await self.add_vehicle(vehicle_base, fetched_at)
 
     async def get_vehicles(self, force_init: bool = False) -> None:
         """Retrieve vehicle data from BMW servers."""
         _LOGGER.debug("Getting vehicle list")
 
-        fetched_at = datetime.datetime.now(datetime.timezone.utc)
-
         if len(self.vehicles) == 0 or force_init:
             await self._init_vehicles()
 
-        async with MyBMWClient(self.config) as client:
-            error_count = 0
-            for vehicle in self.vehicles:
-                # Get the detailed vehicle state
-                try:
-                    state_response = await client.get(
-                        VEHICLE_STATE_URL,
-                        params={
-                            "apptimezone": self.utcdiff,
-                            "appDateTime": int(fetched_at.timestamp() * 1000),
-                        },
-                        headers={
-                            **client.generate_default_header(vehicle.brand),
-                            "bmw-vin": vehicle.vin,
-                        },
-                    )
-                    vehicle_state = state_response.json()
-
-                    # Get detailed charging settings if supported by vehicle
-                    charging_settings = None
-                    if vehicle_state[ATTR_CAPABILITIES].get("isChargingPlanSupported", False) or vehicle_state[
-                        ATTR_CAPABILITIES
-                    ].get("isChargingSettingsEnabled", False):
-                        charging_settings_response = await client.get(
-                            VEHICLE_CHARGING_DETAILS_URL,
-                            params={
-                                "fields": "charging-profile",
-                                "has_charging_settings_capabilities": vehicle_state[ATTR_CAPABILITIES][
-                                    "isChargingSettingsEnabled"
-                                ],
-                            },
-                            headers={
-                                **client.generate_default_header(vehicle.brand),
-                                "bmw-current-date": fetched_at.isoformat(),
-                                "bmw-vin": vehicle.vin,
-                            },
-                        )
-                        charging_settings = charging_settings_response.json()
-
-                    self.add_vehicle(vehicle.data, vehicle_state, charging_settings, fetched_at)
-                except (MyBMWAPIError, json.JSONDecodeError) as ex:
-                    # We don't want to fail completely if one vehicle fails, but we want to know about it
-                    error_count += 1
-
-                    # If it's a MyBMWQuotaError or MyBMWAuthError, we want to raise it
-                    if isinstance(ex, (MyBMWQuotaError, MyBMWAuthError)):
-                        raise ex
-
-                    # Always log the error
-                    _LOGGER.error("Unable to get details for vehicle %s - (%s) %s", vehicle.vin, type(ex).__name__, ex)
-
-                    # If all vehicles fail, we want to raise an exception
-                    if error_count == len(self.vehicles):
-                        raise ex
-
-    def add_vehicle(
+        error_count = 0
+        for vehicle in self.vehicles:
+            # Get the detailed vehicle state
+            try:
+                await vehicle.get_vehicle_state()
+            except (MyBMWAPIError, json.JSONDecodeError) as ex:
+                # We don't want to fail completely if one vehicle fails, but we want to know about it
+                error_count += 1
+
+                # If it's a MyBMWQuotaError or MyBMWAuthError, we want to raise it
+                if isinstance(ex, (MyBMWQuotaError, MyBMWAuthError)):
+                    raise ex
+
+                # Always log the error
+                _LOGGER.error("Unable to get details for vehicle %s - (%s) %s", vehicle.vin, type(ex).__name__, ex)
+
+                # If all vehicles fail, we want to raise an exception
+                if error_count == len(self.vehicles):
+                    raise ex
+
+    async def add_vehicle(
         self,
         vehicle_base: dict,
-        vehicle_state: Optional[dict],
-        charging_settings: Optional[dict],
         fetched_at: Optional[datetime.datetime] = None,
     ) -> None:
         """Add or update a vehicle from the API responses."""
@@ -173,9 +131,9 @@ class MyBMWAccount:
 
         # If vehicle already exists, just update it's state
         if existing_vehicle:
-            existing_vehicle.update_state(vehicle_base, vehicle_state, charging_settings, fetched_at)
+            await existing_vehicle.get_vehicle_state()
         else:
-            self.vehicles.append(MyBMWVehicle(self, vehicle_base, vehicle_state, charging_settings, fetched_at))
+            self.vehicles.append(MyBMWVehicle(self, vehicle_base, fetched_at))
 
     def get_vehicle(self, vin: str) -> Optional[MyBMWVehicle]:
         """Get vehicle with given VIN.
diff --git a/bimmer_connected/api/regions.py b/bimmer_connected/api/regions.py
index 4fb6c6c..ce5ef29 100644
--- a/bimmer_connected/api/regions.py
+++ b/bimmer_connected/api/regions.py
@@ -1,4 +1,5 @@
 """Get the right url for the different countries."""
+
 from base64 import b64decode
 from typing import List
 
diff --git a/bimmer_connected/const.py b/bimmer_connected/const.py
index 3dde449..367aae9 100644
--- a/bimmer_connected/const.py
+++ b/bimmer_connected/const.py
@@ -1,4 +1,5 @@
 """URLs for different services and error code mapping."""
+
 from enum import Enum
 
 
diff --git a/bimmer_connected/utils.py b/bimmer_connected/utils.py
index b55f66d..454c72a 100644
--- a/bimmer_connected/utils.py
+++ b/bimmer_connected/utils.py
@@ -1,6 +1,5 @@
 """General utils and base classes used in the library."""
 
-
 import datetime
 import inspect
 import json
diff --git a/bimmer_connected/vehicle/fuel_and_battery.py b/bimmer_connected/vehicle/fuel_and_battery.py
index 7ecbced..9927899 100644
--- a/bimmer_connected/vehicle/fuel_and_battery.py
+++ b/bimmer_connected/vehicle/fuel_and_battery.py
@@ -1,6 +1,5 @@
 """Generals models used for bimmer_connected."""
 
-
 import datetime
 import logging
 from dataclasses import dataclass
diff --git a/bimmer_connected/vehicle/vehicle.py b/bimmer_connected/vehicle/vehicle.py
index be2bb6a..524fb9b 100644
--- a/bimmer_connected/vehicle/vehicle.py
+++ b/bimmer_connected/vehicle/vehicle.py
@@ -1,7 +1,8 @@
 """Models state and remote services of one vehicle."""
+
 import datetime
 import logging
-from typing import TYPE_CHECKING, Dict, List, Optional, Tuple, Type
+from typing import TYPE_CHECKING, Dict, List, Optional, Tuple, Type, Union
 
 from bimmer_connected.api.client import MyBMWClient
 from bimmer_connected.const import (
@@ -9,7 +10,9 @@ from bimmer_connected.const import (
     ATTR_CAPABILITIES,
     ATTR_CHARGING_SETTINGS,
     ATTR_STATE,
+    VEHICLE_CHARGING_DETAILS_URL,
     VEHICLE_IMAGE_URL,
+    VEHICLE_STATE_URL,
     CarBrands,
 )
 from bimmer_connected.models import StrEnum, ValueWithUnit
@@ -72,13 +75,11 @@ class MyBMWVehicle:
         self,
         account: "MyBMWAccount",
         vehicle_base: dict,
-        vehicle_state: Optional[dict] = None,
-        charging_settings: Optional[dict] = None,
         fetched_at: Optional[datetime.datetime] = None,
     ) -> None:
         """Initialize a MyBMWVehicle."""
         self.account = account
-        self.data = self.combine_data(account, vehicle_base, vehicle_state, charging_settings, fetched_at)
+        self.data = {}
         self.remote_services = RemoteServices(self)
         self.fuel_and_battery: FuelAndBattery = FuelAndBattery(account_timezone=account.timezone)
         self.vehicle_location: VehicleLocation = VehicleLocation(account_region=account.region)
@@ -90,17 +91,59 @@ class MyBMWVehicle:
         self.charging_profile: Optional[ChargingProfile] = None
         self.tires: Optional[Tires] = None
 
-        self.update_state(vehicle_base, vehicle_state, charging_settings, fetched_at)
+        self.data = self.combine_data(vehicle_base, fetched_at=fetched_at)
+
+    async def get_vehicle_state(self) -> None:
+        """Retrieve vehicle data from BMW servers."""
+        _LOGGER.debug("Getting vehicle list")
+
+        fetched_at = datetime.datetime.now(datetime.timezone.utc)
+
+        async with MyBMWClient(self.account.config) as client:
+            # Get state details
+            state_response = await client.get(
+                VEHICLE_STATE_URL,
+                params={
+                    "apptimezone": self.account.utcdiff,
+                    "appDateTime": int(fetched_at.timestamp() * 1000),
+                },
+                headers={
+                    **client.generate_default_header(self.brand),
+                    "bmw-vin": self.vin,
+                },
+            )
+            vehicle_state: Dict = state_response.json()
+
+            # If vehicle has not been initialized with capabilities from state, do it once
+            if not self.data.get(ATTR_CAPABILITIES):
+                self.data = self.combine_data(vehicle_state, fetched_at=fetched_at)
+
+            # Get detailed charging settings if supported by vehicle
+            charging_settings = {ATTR_CHARGING_SETTINGS: None}
+            if self.is_charging_plan_supported or self.is_charging_settings_supported:
+                charging_settings_response = await client.get(
+                    VEHICLE_CHARGING_DETAILS_URL,
+                    params={
+                        "fields": "charging-profile",
+                        "has_charging_settings_capabilities": self.is_charging_settings_supported,
+                    },
+                    headers={
+                        **client.generate_default_header(self.brand),
+                        "bmw-current-date": fetched_at.isoformat(),
+                        "bmw-vin": self.vin,
+                    },
+                )
+                charging_settings = {ATTR_CHARGING_SETTINGS: charging_settings_response.json()}
+
+            self.update_state([vehicle_state, charging_settings], fetched_at)
 
     def update_state(
         self,
-        vehicle_base: dict,
-        vehicle_state: Optional[dict] = None,
-        charging_settings: Optional[dict] = None,
+        data: Union[Dict, List[Dict]],
         fetched_at: Optional[datetime.datetime] = None,
     ) -> None:
         """Update the state of a vehicle."""
-        vehicle_data = self.combine_data(self.account, vehicle_base, vehicle_state, charging_settings, fetched_at)
+        vehicle_data = self.combine_data(data, fetched_at)
         self.data = vehicle_data
 
         update_entities: List[Tuple[Type["VehicleDataBase"], str]] = [
@@ -124,22 +167,26 @@ class MyBMWVehicle:
             except (KeyError, TypeError) as ex:
                 _LOGGER.warning("Unable to update %s - (%s) %s", vehicle_attribute, type(ex).__name__, ex)
 
-    @staticmethod
-    def combine_data(
-        account: "MyBMWAccount",
-        vehicle_base: dict,
-        vehicle_state: Optional[dict],
-        charging_settings: Optional[dict],
-        fetched_at: Optional[datetime.datetime] = None,
-    ) -> Dict:
+    def combine_data(self, data: Union[Dict, List[Dict]], fetched_at: Optional[datetime.datetime] = None) -> Dict:
         """Combine API responses and additional information to a single dictionary."""
-        return {
-            **vehicle_base,
-            **(vehicle_state or {}),
-            ATTR_CHARGING_SETTINGS: charging_settings or {},
-            "fetched_at": fetched_at or datetime.datetime.now(datetime.timezone.utc),
+        if isinstance(data, dict):
+            data = [data]
+
+        vehicle_data = {
+            ATTR_ATTRIBUTES: {},
+            ATTR_CAPABILITIES: {},
+            ATTR_STATE: {},
+            ATTR_CHARGING_SETTINGS: {},
+            **self.data,
         }
 
+        for entry in data:
+            vehicle_data.update(entry)
+
+        vehicle_data["fetched_at"] = fetched_at or datetime.datetime.now(datetime.timezone.utc)
+
+        return vehicle_data
+
     # # # # # # # # # # # # # # #
     # Generic attributes
     # # # # # # # # # # # # # # #
@@ -209,6 +256,11 @@ class MyBMWVehicle:
         """Return True if charging profile is available and can be set via API."""
         return self.data[ATTR_CAPABILITIES].get("isChargingPlanSupported", False)
 
+    @property
+    def is_charging_settings_supported(self) -> bool:
+        """Return True if charging settings can be set via API."""
+        return self.data[ATTR_CAPABILITIES].get("isChargingSettingsEnabled", False)
+
     @property
     def is_vehicle_tracking_enabled(self) -> bool:
         """Return True if vehicle finder is enabled in vehicle."""


Test patch:
diff --git a/bimmer_connected/tests/conftest.py b/bimmer_connected/tests/conftest.py
index ca9532b..f160469 100644
--- a/bimmer_connected/tests/conftest.py
+++ b/bimmer_connected/tests/conftest.py
@@ -1,4 +1,5 @@
 """Fixtures for BMW tests."""
+
 from collections import deque
 from typing import Deque, Generator, Optional
 
diff --git a/bimmer_connected/tests/test_api.py b/bimmer_connected/tests/test_api.py
index c910278..beb074d 100644
--- a/bimmer_connected/tests/test_api.py
+++ b/bimmer_connected/tests/test_api.py
@@ -1,4 +1,5 @@
 """Tests for API that are not covered by other tests."""
+
 import json
 
 import httpx
diff --git a/bimmer_connected/tests/test_remote_services.py b/bimmer_connected/tests/test_remote_services.py
index a8bd9cc..6567621 100644
--- a/bimmer_connected/tests/test_remote_services.py
+++ b/bimmer_connected/tests/test_remote_services.py
@@ -1,4 +1,5 @@
 """Test for remote_services."""
+
 from unittest import mock
 from uuid import uuid4
 
@@ -247,7 +248,7 @@ async def test_vehicles_without_enabled_services(bmw_fixture: respx.Router):
     # Errors on combustion engines
     vehicle = account.get_vehicle(VIN_F31)
 
-    vehicle.update_state(vehicle.data, {"capabilities": {}})
+    vehicle.update_state({"capabilities": {}})
 
     for service in ALL_SERVICES.values():
         with pytest.raises(ValueError):
@@ -268,7 +269,7 @@ async def test_trigger_charge_start_stop_warnings(caplog, bmw_fixture: respx.Rou
         "chargingStatus": "INVALID",
         "isChargerConnected": False,
     }
-    vehicle.update_state(vehicle.data, {"state": {"electricChargingState": fixture_not_connected}})
+    vehicle.update_state({"state": {"electricChargingState": fixture_not_connected}})
 
     result = await vehicle.remote_services.trigger_charge_start()
     assert result.state == ExecutionState.IGNORED
@@ -285,7 +286,7 @@ async def test_trigger_charge_start_stop_warnings(caplog, bmw_fixture: respx.Rou
         "chargingStatus": "WAITING_FOR_CHARGING",
         "isChargerConnected": True,
     }
-    vehicle.update_state(vehicle.data, {"state": {"electricChargingState": fixture_connected_not_charging}})
+    vehicle.update_state({"state": {"electricChargingState": fixture_connected_not_charging}})
 
     result = await vehicle.remote_services.trigger_charge_stop()
     assert result.state == ExecutionState.IGNORED
diff --git a/bimmer_connected/tests/test_utils.py b/bimmer_connected/tests/test_utils.py
index 4aa2f32..cc84d62 100644
--- a/bimmer_connected/tests/test_utils.py
+++ b/bimmer_connected/tests/test_utils.py
@@ -1,4 +1,5 @@
 """Tests for utils."""
+
 import datetime
 import json
 
@@ -31,6 +32,7 @@ async def test_drive_train(bmw_fixture: respx.Router):
         "has_combustion_drivetrain",
         "has_electric_drivetrain",
         "is_charging_plan_supported",
+        "is_charging_settings_supported",
         "is_lsc_enabled",
         "is_remote_charge_start_enabled",
         "is_remote_charge_stop_enabled",
diff --git a/bimmer_connected/tests/test_vehicle.py b/bimmer_connected/tests/test_vehicle.py
index fc18340..53af19b 100644
--- a/bimmer_connected/tests/test_vehicle.py
+++ b/bimmer_connected/tests/test_vehicle.py
@@ -1,4 +1,5 @@
 """Tests for MyBMWVehicle."""
+
 import pytest
 import respx