-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issues #35 #36: Planned dispatch source is None #37
Fix issues #35 #36: Planned dispatch source is None #37
Conversation
9c45e77
to
12e968f
Compare
@@ -46,7 +46,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry): | |||
) | |||
|
|||
try: | |||
await hass.async_add_executor_job(octopus_system.start) | |||
await octopus_system.start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is a git cherry-pick from PR #38 because this PR depends on that one.
@@ -191,7 +191,7 @@ async def __async_cancel_boost_charge(self, session, account_id: str): | |||
result = await session.execute(query, variable_values=params, operation_name="deleteBoostCharge") | |||
return result['deleteBoostCharge'] | |||
|
|||
async def __async_get_accounts(session): | |||
async def __async_get_accounts(self, session): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is another git cherry-pick from PR #38. If that PR is merged first, this change disappears from here.
@@ -0,0 +1,93 @@ | |||
"""Persistent data storage for the integration, based on the HASS helpers.storage.Store class.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PersistentData
class introduced here is fairly generic and could be used to save any other data when/if needed. Building on the Home Assistant helpers.storage.Store
class, it will create / load / save / delete a JSON file in the HASS config/.storage/
directory. For example, in a HASS container:
edea03bac07a:~# cd /config/.storage
edea03bac07a:/config/.storage# ls -l
-rw------- 1 root root 373 Mar 3 16:52 core.config
-rw-r--r-- 1 root root 2826 Mar 5 00:39 core.config_entries
...
-rw-r--r-- 1 root root 156 Mar 5 02:21 octopus_intelligent.A-1234ABCD
edea03bac07a:/config/.storage# cat octopus_intelligent.A-1234ABCD
{
"version": 1,
"minor_version": 1,
"key": "octopus_intelligent.A-1234ABCD",
"data": {
"last_seen_planned_dispatch_source": "smart-charge"
}
Where A-1234ABCD
is the user’s Octopus account number.
I have also taken care of adding the async_remove_entry()
function to __init__.py
in order to arrange for the store file ('octopus_intelligent.A-1234ABCD'
) to be deleted if the user deletes the integration using the HASS frontend web UI.
The proposed solution has a corner-case limitation involving bump charge. During the 5-minute API polling interval, or while Home Assistant is down for any reason (upgrade, server maintenance...), it is in theory possible for the planned dispatch I believe that most users will never come across that corner case, even if they regularly use bump charge (which I suspect is also rare). This PR thus offers an effective workaround until Octopus fixes their API to ensure that the |
12e968f
to
9adfe35
Compare
# --------------------------------------------- | ||
# Start of JSON-serialisable persistent fields. | ||
# | ||
last_seen_planned_dispatch_source: str = "smart-charge" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smart-charge
default value only applies when the integration is first installed, or reinstalled after being deleted, when no previous data exists in persistent storage. Data read from persistent storage — even if it is an empty string — overrides this default value.
An alternative default value could be the empty string. If the default value was the empty string, and the Octopus API was responding with "source": None
when the integration was installed or reinstalled, then issues #35 and #36 would apply until the Octopus API responded with a non-None source
attribute. During this period, the integration sensors would not report that a smart charge was taking place, even if it was.
I think that this default value should be smart-charge
for two reasons:
-
It means that issues #35 and #36 are immediately resolved (subject to the limitations of the solution) when users upgrade the integration to a new version including the solution in this PR. Similarly, new users of this integration are more likely to find that it works straight away. (Otherwise, new users might simply decide that "it doesn't work" and uninstall it.)
-
If one was to argue that this default value should be the empty string instead of
smart-charge
, then they’d also have to argue against the persistent storage of thesource
attribute more generally, because the persisted value — whethersmart-charge
orbump-charge
— could be incorrect when the integration starts.
This goes back to the discussion of the corner-case involving bump charge in another comment. It is possible, but unlikely, that a user will first install the integration while a bump charge is taking place and while the Octopus API is responding with "source": None
. I think it is better to accept a temporary misreporting of a smart charge (when it is actually a bump charge) in this unlikely scenario than to misreport more often to more users (failing to report a smart charge while one is taking place).
9adfe35
to
56f0ab3
Compare
async def async_remove_entry(hass: HomeAssistant, entry: ConfigEntry): | ||
"""Called when the config entry is removed (the integration is deleted).""" | ||
octopus_system: OctopusIntelligentSystem = ( | ||
hass.data[DOMAIN][entry.entry_id][OCTOPUS_SYSTEM] | ||
) | ||
try: | ||
await octopus_system.async_remove_entry() | ||
except Exception as ex: # pylint: disable=broad-exception-caught | ||
_LOGGER.error(ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The async_remove_entry()
function is called when the integration’s config entries are deleted — i.e., when the user deletes the integration from Home Assistant (Settings > Integrations > Octopus Intelligent Tariff > Three vertical dots > Delete). The code added here (following the call chain) ensures that when this happens, the persistent data is deleted as well (JSON file at config/.storage/octopus_intelligent.A-1234ABCD
).
Importantly, I have tested to confirm that this does not apply to HACS updates (Home Assistant Community Store), using the custom repository installation method documented in this integration’s README file. This is because a HACS update does not involve deleting config entries (the config flow does not seem to get involved at all), so async_remove_entry()
is not called. This means that when users update the integration through HACS, the persistent data is not removed, which is good (the intended behaviour).
If the user tries to delete the integration through the HACS user interface, they are asked by HACS to first delete the integration through the HASS UI (Settings > Integrations > ...) prior to deleting the custom repository from HACS.
So everything works as intended, whether the integration is installed / deleted with or without HACS.
56f0ab3
to
6df6809
Compare
6df6809
to
a816b0c
Compare
This PR proposes a fix for issues #35 and #36. As @matt7aylor had pointed out, sometimes the
source
attribute in the Octopus API planned dispatch list gets changed from"source": "smart-charge"
to"source": None
. The proposed solution / workaround is to remember the last seen non-None source and use it whensource
is None. For example:Initially,
'source': 'smart-charge'
:Then it goes rogue,
'source': None
:The proposed solution replaces
'source': None
with'source': 'smart-charge'
(in this particular example) before passing it on to the integration sensors etc.The proposed solution also introduces a
PersistentData
class that uses the Home Assistanthelpers.storage.Store
class to persist the last seen non-Nonesource
to disk when Home Assistant is stopped / restarted, and restores it when the integration is loaded again.It is worth noting that initially I tried something else. I looked at the Octopus GraphQL schema for plannedDispatches and noticed that they document two
source
attributes, one in themeta
inner object and one in the outer scope:Octopus API query documentation:
Octopus API response documentation:
So I tried modifying the query in
graphql_client.py
__async_get_combined_state() to include bothsource
attributes, in the hope that when one is None, the other isn’t. Alas, I observed that the newly added outersource
attribute was alwaysNone
, regardless of the whether themeta.source
attribute wasNone
or not. Example: