feat: implement HA-side accumulation for monthly spent sensor#54
Conversation
…for delivery time calculations.
- Replace API-based calculation with Home Assistant state accumulation - Only track orders with final prices (delivered and closed) - Use RestoreEntity to persist monthly totals across restarts - Track processed order IDs to avoid duplicates - Automatically reset at start of each month - Add state_class for proper history tracking - Fixes issue where only 15 days of history were visible due to API limit (50 orders) This approach eliminates dependency on API order limit and provides full monthly history regardless of order count.
The translation file already defines unit_of_measurement for monthly_spent, so we should not set _attr_native_unit_of_measurement in code to avoid the ValueError conflict.
There was a problem hiding this comment.
Pull request overview
This PR implements three improvements to the Rohlík CZ integration: handling concurrent orders correctly, implementing Home Assistant-side accumulation for the monthly spent sensor with state persistence, and ensuring consistent unit of measurement handling.
- Adds
get_earliest_order()utility to properly handle multiple concurrent orders by selecting the one with earliest delivery time - Implements
RestoreEntityforMonthlySpentsensor to persist monthly totals across HA restarts with order deduplication - Updates
NextOrderSince,NextOrderTill, andIsOrderedSensorto use the new utility function
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| custom_components/rohlikcz/utils.py | Adds get_earliest_order() function to find order with earliest delivery time, handling multiple datetime formats |
| custom_components/rohlikcz/sensor.py | Refactors MonthlySpent to use RestoreEntity with state persistence and order deduplication; updates NextOrderSince and NextOrderTill to use get_earliest_order() |
| custom_components/rohlikcz/binary_sensor.py | Updates IsOrderedSensor to use get_earliest_order() utility for consistent order selection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._monthly_total = 0.0 | ||
| self._processed_orders = set() | ||
| self._current_month = current_month | ||
| self._last_reset = datetime.now(ZoneInfo("Europe/Prague")) |
There was a problem hiding this comment.
Timezone inconsistency: In _check_and_reset_month(), self._last_reset is set using datetime.now(ZoneInfo("Europe/Prague")) (line 500), but in __init__() and when checking month boundaries, the code uses datetime.now() without timezone (lines 444, 494, 512). This could lead to inconsistent datetime comparisons or timezone-related issues. Consider using timezone-aware datetime consistently throughout, particularly datetime.now(ZoneInfo("Europe/Prague")) for all datetime.now() calls in this class.
| super().__init__(rohlik_account) | ||
| self._monthly_total: float = 0.0 | ||
| self._processed_orders: set[str] = set() # Store order IDs | ||
| self._current_month: str = datetime.now().strftime("%Y-%m") |
There was a problem hiding this comment.
Timezone inconsistency: Using naive datetime.now() for month calculation could cause issues around month boundaries if the system timezone differs from "Europe/Prague" (the timezone used elsewhere in this integration). Consider using datetime.now(ZoneInfo("Europe/Prague")).strftime("%Y-%m") to ensure consistent month detection with the integration's target timezone.
| self._current_month: str = datetime.now().strftime("%Y-%m") | |
| self._current_month: str = datetime.now(ZoneInfo("Europe/Prague")).strftime("%Y-%m") |
|
|
||
| def _check_and_reset_month(self) -> None: | ||
| """Reset total if month changed.""" | ||
| current_month = datetime.now().strftime("%Y-%m") |
There was a problem hiding this comment.
Timezone inconsistency: Using naive datetime.now() for month boundary checking. This should use datetime.now(ZoneInfo("Europe/Prague")) to match the timezone used in line 500 and ensure consistent month detection regardless of the system timezone.
| if not orders: | ||
| return | ||
|
|
||
| current_month_pattern = datetime.now().strftime("%Y-%m-") |
There was a problem hiding this comment.
Timezone inconsistency: Using naive datetime.now() for current month pattern. This should use datetime.now(ZoneInfo("Europe/Prague")) to ensure orders are filtered based on the correct timezone, matching the integration's target timezone.
| current_month_pattern = datetime.now().strftime("%Y-%m-") | |
| current_month_pattern = datetime.now(ZoneInfo("Europe/Prague")).strftime("%Y-%m-") |
| def native_value(self) -> float | None: | ||
| """Returns amount spend within last month.""" | ||
| return calculate_current_month_orders_total(self._rohlik_account.data.get('delivered_orders', [])) | ||
| """Returns amount spent in current month.""" | ||
| self._check_and_reset_month() | ||
| self._process_new_orders() | ||
| return self._monthly_total if self._monthly_total > 0 else 0.0 |
There was a problem hiding this comment.
Performance concern: The native_value property processes orders on every access by calling _process_new_orders(). While the deduplication check (if order_key in self._processed_orders) prevents double-counting, it still iterates through all delivered orders on every property access. Consider creating a dedicated callback method that calls these processing functions, and register it instead of async_write_ha_state. For example:
async def _handle_coordinator_update(self):
self._check_and_reset_month()
self._process_new_orders()
await self.async_write_ha_state()
async def async_added_to_hass(self) -> None:
...
self._rohlik_account.register_callback(self._handle_coordinator_update)Then native_value can simply return self._monthly_total.
| Returns: | ||
| dict: The order with the earliest delivery time, or None if no valid order found | ||
| """ | ||
| if not orders or len(orders) == 0: |
There was a problem hiding this comment.
Redundant check: len(orders) == 0 is unnecessary since not orders already covers empty lists. The condition can be simplified to just if not orders:.
| if not orders or len(orders) == 0: | |
| if not orders: |
| slot_start = datetime.strptime(earliest_order.get("deliverySlot", {}).get("since", None), | ||
| "%Y-%m-%dT%H:%M:%S.%f%z") | ||
| return slot_start | ||
| except (ValueError, TypeError): | ||
| # Try without microseconds if the format doesn't match | ||
| try: | ||
| slot_start = datetime.strptime(earliest_order.get("deliverySlot", {}).get("since", None), | ||
| "%Y-%m-%dT%H:%M:%S%z") | ||
| return slot_start | ||
| except (ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
Code duplication: The datetime parsing logic with fallback (lines 738-749) duplicates the same logic already implemented in get_earliest_order() (lines 191-199 in utils.py). Since get_earliest_order() already returns an order that has been validated to have a parseable datetime, this additional parsing with the same fallback is redundant. Consider either:
- Trusting that
get_earliest_order()returns a valid order and simplify this to a single parse attempt, or - Extract the datetime from the earliest_order that
get_earliest_order()has already validated, or - Create a helper function to extract and parse datetime strings to avoid duplication.
| slot_start = datetime.strptime(earliest_order.get("deliverySlot", {}).get("since", None), | |
| "%Y-%m-%dT%H:%M:%S.%f%z") | |
| return slot_start | |
| except (ValueError, TypeError): | |
| # Try without microseconds if the format doesn't match | |
| try: | |
| slot_start = datetime.strptime(earliest_order.get("deliverySlot", {}).get("since", None), | |
| "%Y-%m-%dT%H:%M:%S%z") | |
| return slot_start | |
| except (ValueError, TypeError): | |
| return None | |
| slot_start = datetime.strptime( | |
| earliest_order.get("deliverySlot", {}).get("since", None), | |
| "%Y-%m-%dT%H:%M:%S.%f%z" | |
| ) | |
| return slot_start | |
| except (ValueError, TypeError): | |
| return None |
| try: | ||
| slot_end = datetime.strptime(earliest_order.get("deliverySlot", {}).get("till", None), | ||
| "%Y-%m-%dT%H:%M:%S.%f%z") | ||
| return slot_start | ||
| else: | ||
| return None | ||
| return slot_end | ||
| except (ValueError, TypeError): | ||
| # Try without microseconds if the format doesn't match | ||
| try: | ||
| slot_end = datetime.strptime(earliest_order.get("deliverySlot", {}).get("till", None), | ||
| "%Y-%m-%dT%H:%M:%S%z") | ||
| return slot_end | ||
| except (ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
Code duplication: The datetime parsing logic with fallback (lines 774-785) is duplicated from NextOrderSince and also duplicates logic in get_earliest_order(). This is the same parsing pattern repeated three times in the codebase. Consider extracting this into a reusable helper function or simplifying since get_earliest_order() already validates datetime parseability.
| from .entity import BaseEntity | ||
| from .hub import RohlikAccount | ||
| from .utils import extract_delivery_datetime, calculate_current_month_orders_total | ||
| from .utils import extract_delivery_datetime, calculate_current_month_orders_total, get_earliest_order |
There was a problem hiding this comment.
Import of 'calculate_current_month_orders_total' is not used.
| from .utils import extract_delivery_datetime, calculate_current_month_orders_total, get_earliest_order | |
| from .utils import extract_delivery_datetime, get_earliest_order |
|
@eMeF1 Can you have a look on copilot comments to see if anything needs to be addressed? Thanks! |
@dvejsada Frankly I am using cursor.com for that but it says the recommendations makes sense. The below is a summary of fixes. Completed Changes
All changes pass linting with no errors. The code now addresses all Copilot AI review comments from the PR. |
- Remove unused import calculate_current_month_orders_total - Fix timezone inconsistencies (use ZoneInfo consistently) - Create parse_delivery_datetime_string helper to eliminate code duplication - Refactor NextOrderSince and NextOrderTill to use helper function - Fix performance issue: move order processing to async callback instead of native_value property - Remove redundant len(orders) == 0 check in utils.py Addresses all Copilot AI review comments from PR dvejsada#54
|
Hi @dvejsada, Given the scope of changes (HA-side accumulation, performance improvements, timezone fixes), would you consider creating a beta release (e.g., v0.3.2-beta or v0.4.0-beta) after merging? This would let users test before a stable release. |
Summary
This PR includes three related improvements:
Changes
✅ Fix: Handle concurrent orders with
get_earliest_orderutilityget_earliest_order()function inutils.pyto find the order with earliest delivery timeNextOrderSinceandNextOrderTillsensors to use this utilityIsOrderedSensorbinary sensor to use this utility✅ Feature: HA-side accumulation for monthly spent sensor
RestoreEntityto persist monthly totals across Home Assistant restartsdelivered_ordersendpoint✅ Fix: Remove conflicting
native_unit_of_measurementunit_of_measurement: "CZK"formonthly_spent_attr_native_unit_of_measurementto avoid ValueError conflictTechnical Details
Concurrent Orders Fix
get_earliest_order()parses delivery slotsincetimestamps from all ordersNoneif no valid orders foundDeliveryTime,NextOrderSince,NextOrderTillsensors andIsOrderedSensorbinary sensorMonthly Spent Accumulation
SensorStateClass.TOTALfor proper accumulation behaviorprocessed_orderssetextra_state_attributesfor restoration:monthly_total: Current month's total spendingprocessed_orders: List of processed order IDscurrent_month: Current month identifier (YYYY-MM)last_reset: Timestamp of last month resetprocessed_count: Number of processed ordersTesting