Skip to content

Fix missing context when running script from template entity#113523

Merged
bdraco merged 2 commits into
devfrom
template_script_fix_context
Mar 16, 2024
Merged

Fix missing context when running script from template entity#113523
bdraco merged 2 commits into
devfrom
template_script_fix_context

Conversation

@emontnemery
Copy link
Copy Markdown
Contributor

Proposed change

Fix missing context when running script from template entity

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @PhracturedBlue, @tetienne, @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (template) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of template can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign template Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

Copy link
Copy Markdown
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @emontnemery 👍

../Frenck

Copy link
Copy Markdown
Contributor

@allenporter allenporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix

@akloeckner
Copy link
Copy Markdown
Contributor

Question: does this new context need to produce an event in order for it to be traceable? As is done for automations? See #21028

@FlorianOosterhof
Copy link
Copy Markdown
Contributor

Question: does this new context need to produce an event in order for it to be traceable? As is done for automations? See #21028

Sounds like exactly what was done in this PR which was never reviewed and eventually marked stale: #102975

@akloeckner
Copy link
Copy Markdown
Contributor

Sounds like exactly what was done in this PR which was never reviewed and eventually marked stale: #102975

Yes. That's why I'm asking. (Plus, I'd like to understand more about the way context is supposed to be used.) Here's why the PR's author stopped developing it, in case this helps: #99914 (comment) . Maybe now, with core developers involved, the problem has a better chance to be solved. And be it simply by defining "no event needed". ;-)

@emontnemery emontnemery reopened this Mar 16, 2024
@emontnemery
Copy link
Copy Markdown
Contributor Author

emontnemery commented Mar 16, 2024

I think this PR can be merged as is, the behavior is not changed but the warning is gone.

The point of the context is to make it clear in the logbook why something happened, firing an event as done in the linked PR seems a bit roundabout but it is indeed the only way to make that work.

It does however need additional changes, the template integration needs a logbook platform, like this from the automation integration:

@callback
def async_describe_events(hass: HomeAssistant, async_describe_event): # type: ignore[no-untyped-def]
"""Describe logbook events."""
@callback
def async_describe_logbook_event(event: LazyEventPartialState): # type: ignore[no-untyped-def]
"""Describe a logbook event."""
data = event.data
message = "triggered"
if ATTR_SOURCE in data:
message = f"{message} by {data[ATTR_SOURCE]}"
return {
LOGBOOK_ENTRY_NAME: data.get(ATTR_NAME),
LOGBOOK_ENTRY_MESSAGE: message,
LOGBOOK_ENTRY_SOURCE: data.get(ATTR_SOURCE),
LOGBOOK_ENTRY_ENTITY_ID: data.get(ATTR_ENTITY_ID),
LOGBOOK_ENTRY_CONTEXT_ID: event.context_id,
}
async_describe_event(
DOMAIN, EVENT_AUTOMATION_TRIGGERED, async_describe_logbook_event
)

The event data also needs a reference to the trigger:

event_data = {
ATTR_NAME: self.name,
ATTR_ENTITY_ID: self.entity_id,
}
if "trigger" in variables and "description" in variables["trigger"]:
event_data[ATTR_SOURCE] = variables["trigger"]["description"]

I think we can add that in a follow-up PR, targeting the 2024.4 release.

@akloeckner
Copy link
Copy Markdown
Contributor

Thanks for clarifying! 👍

@doug-hoffman
Copy link
Copy Markdown
Contributor

doug-hoffman commented Mar 16, 2024

I think this PR can be merged as is, the behavior is not changed but the warning is gone.

Creating a new context without firing an event does change the current behavior, as I mentioned in #99914 (comment).

When we create a new context in the template component, it happens much earlier than when it is done in association with the warning. As a result, it becomes possible that an event may fire and be associated with that context. If the action section is used within the template block, it is very likely an event will fire. (In my testing, I observed this with call_service events being blamed for subsequent state changes.)

@akloeckner
Copy link
Copy Markdown
Contributor

In my testing, I observed this with call_service events being blamed for subsequent state changes.

TL;DR I was curious and I think this can already happen with the current implementation.

Example

A template with a time trigger and actions will simply update (giving the known warnings) and not blame its change on anything in the logbook:

Screenshot_20240316_200731_Home Assistant

template:
- trigger:
  - platform: time_pattern
    seconds: "/15"
  action:
  - service: rest_command.signal_receive
    response_variable: response
  sensor:
  - name: Signal message received
    device_class: timestamp
    state: >
      {% set content = response['content'] | from_json %}
      {% if response['status'] == 200 and content is sequence and content|length > 0 %}
        {{ (content[0]['envelope']['timestamp']/1000) | as_datetime }}
      {% else %}
        {{ this.state if this else None }}
      {% endif %}

If I substitute the trigger with a template trigger, (where the sensor.now essentially is now()) the change will be blamed on the service call in the actions (with no warnings):

Screenshot_20240316_191723_Home Assistant

template:
- trigger:
  - platform: template
    value_template: "{{ as_datetime(states('sensor.now'), now()).second % 15 == 0 }}"
  action:
  - service: rest_command.signal_receive
    response_variable: response
  sensor:
  - name: Signal message received
    device_class: timestamp
    state: >
      {% set content = response['content'] | from_json %}
      {% if response['status'] == 200 and content is sequence and content|length > 0 %}
        {{ (content[0]['envelope']['timestamp']/1000) | as_datetime }}
      {% else %}
        {{ this.state if this else None }}
      {% endif %}

So, in my opinion, the fix here will make it better and a subsequent PR, maybe preceded by some discussion as you suggested, may then clean up the greater context.

@bdraco bdraco merged commit d0352ed into dev Mar 16, 2024
@bdraco bdraco deleted the template_script_fix_context branch March 16, 2024 22:37
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 17, 2024
@home-assistant home-assistant unlocked this conversation Mar 18, 2024
@emontnemery
Copy link
Copy Markdown
Contributor Author

emontnemery commented Mar 18, 2024

I think this PR can be merged as is, the behavior is not changed but the warning is gone.

Creating a new context without firing an event does change the current behavior, as I mentioned in #99914 (comment).

When we create a new context in the template component, it happens much earlier than when it is done in association with the warning. As a result, it becomes possible that an event may fire and be associated with that context. If the action section is used within the template block, it is very likely an event will fire. (In my testing, I observed this with call_service events being blamed for subsequent state changes.)

OK, this is an interesting point. Could you share an example of a template entity configuration which causes confusing logbook entries with the changes in this PR?

@github-actions github-actions Bot locked and limited conversation to collaborators Mar 19, 2024
@home-assistant home-assistant unlocked this conversation Mar 21, 2024
balloob pushed a commit that referenced this pull request Mar 22, 2024
@balloob balloob mentioned this pull request Mar 22, 2024
balloob added a commit that referenced this pull request Mar 22, 2024
* Streamline Notion config entry updates (refresh token and user ID) (#112832)

* Bump aioautomower to 2024.3.2 (#113162)

* Bump aioautomower to 2024.3.3 (#113430)

* Check for EA release channel for UniFi Protect (#113432)

Co-authored-by: J. Nick Koston <nick@koston.org>

* Bump `pysnmp-lextudio` to version `6.0.11` (#113463)

* Tado fix water heater (#113464)

Co-authored-by: Joostlek <joostlek@outlook.com>

* Bump aiodhcpwatcher to 0.8.2 (#113466)

* Bump axis to v55 (#113479)

* Bump croniter to 2.0.2 (#113494)

* Revert setting communication delay in Risco init (#113497)

* Bump pyrisco to 0.5.10 (#113505)

* Fix missing context when running script from template entity (#113523)

Co-authored-by: J. Nick Koston <nick@koston.org>

* Bump ical to 7.0.3 to fix local-todo persisted with invalid DTSTART values (#113526)

* Fix Airthings BLE illuminance sensor name (#113560)

* Ignore Shelly block update with cfgChanged None (#113587)

* Catch `TimeoutError` in `Brother` config flow (#113593)

* Catch TimeoutError in Brother config flow

* Update tests

* Remove unnecessary parentheses

---------

Co-authored-by: Maciej Bieniek <478555+bieniu@users.noreply.github.com>

* Bump axis to v56 (#113608)

* Bump pyunifiprotect to 5.0.1 (#113630)

* Bump pyunifiprotect to 5.0.2 (#113651)

* Add removal condition to Shelly battery sensor (#113703)

Co-authored-by: Maciej Bieniek <478555+bieniu@users.noreply.github.com>

* Bump aioraven to 0.5.2 (#113714)

* Fix unknown values in onewire (#113731)

* Fix unknown values in onewire

* Update tests

* Bump pymodbus v3.6.6 (#113796)

* Catch API errors in cast media_player service handlers (#113839)

* Catch API errors in cast media_player service handlers

* Remove left over debug code

* Fix wrapping of coroutine function with api_error

* Bump pychromecast to 14.0.1 (#113841)

* Fix startup race in cast (#113843)

* Redact the area of traccar server geofences (#113861)

* Bump pytedee_async to 0.2.17 (#113933)

* Bump axis to v57 (#113952)

* Bump version to 2024.3.2

---------

Co-authored-by: Aaron Bach <bachya1208@gmail.com>
Co-authored-by: Thomas55555 <59625598+Thomas55555@users.noreply.github.com>
Co-authored-by: Christopher Bailey <cbailey@mort.is>
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Lex Li <425130+lextm@users.noreply.github.com>
Co-authored-by: Erwin Douna <e.douna@gmail.com>
Co-authored-by: Joostlek <joostlek@outlook.com>
Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>
Co-authored-by: Diogo Gomes <diogogomes@gmail.com>
Co-authored-by: On Freund <onfreund@gmail.com>
Co-authored-by: Erik Montnemery <erik@montnemery.com>
Co-authored-by: Allen Porter <allen@thebends.org>
Co-authored-by: Shay Levy <levyshay1@gmail.com>
Co-authored-by: Maciej Bieniek <bieniu@users.noreply.github.com>
Co-authored-by: Maciej Bieniek <478555+bieniu@users.noreply.github.com>
Co-authored-by: Scott K Logan <logans@cottsay.net>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: jan iversen <jancasacondor@gmail.com>
Co-authored-by: Joakim Sørensen <ludeeus@ludeeus.dev>
Co-authored-by: Josef Zweck <24647999+zweckj@users.noreply.github.com>
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trigger Update Coordinator: Running script requires passing in a context

9 participants