Skip to content

Add context to event trigger#40932

Merged
balloob merged 1 commit intohome-assistant:devfrom
OnFreund:event_trigger_context
Oct 1, 2020
Merged

Add context to event trigger#40932
balloob merged 1 commit intohome-assistant:devfrom
OnFreund:event_trigger_context

Conversation

@OnFreund
Copy link
Copy Markdown
Contributor

@OnFreund OnFreund commented Oct 1, 2020

Proposed change

As proposed in home-assistant/architecture#436, this adds backend support for event triggers to filter by context.

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
trigger:
  - platform: event
    event_type: test_event_context
    event_data:
    context:
      user_id: my-user-id

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
  • The code has been formatted using Black (black --fast 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with an integration (homeassistant) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Nice!

@balloob balloob merged commit 04f87ee into home-assistant:dev Oct 1, 2020
extra=vol.ALLOW_EXTRA,
)
event_data_schema = _populate_schema(config, CONF_EVENT_DATA)
event_context_schema = _populate_schema(config, CONF_EVENT_CONTEXT)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just realized that matching straight up on context like this is going to be annoying if one wants to match multiple users.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. How about something like:

if isinstance(value, list):
    vol.Required(key): vol.In(value)
else:
    vol.Required(key): value

when building the schema? This can also be used for event_data, not just context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Downside is that this could be a breaking change if the user intended to match a list.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed it here:
#41036

To ensure backwards compatibility I only added this for context, not data, even though I think it could be a nice touch for data as well.



def _populate_schema(config, config_parameter):
if config_parameter not in config:
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Oct 1, 2020

Choose a reason for hiding this comment

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

The existing behavior was changed slightly with this check. The old check only set an event data schema if the config had a populated event data dict. The current check sets a schema as long as the event data key is in the config. It doesn't check for a truthy event data dict.

Copy link
Copy Markdown
Contributor Author

@OnFreund OnFreund Oct 1, 2020

Choose a reason for hiding this comment

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

Good catch. I'm not sure this makes any practical difference (since the schema will be empty), but I'll fix this together with the multiple user selection below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants