Add context to event trigger by OnFreund · Pull Request #7182 · home-assistant/frontend

OnFreund · 2020-10-01T09:25:37Z

Proposed change

Add automation editor support to context in event triggers.
The backend is implemented in home-assistant/core#40932.

Type of change

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

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add context to event trigger documentation home-assistant.io#14678

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

balloob · 2020-10-01T10:00:28Z

src/data/automation.ts

  platform: "event";
  event_type: string;
  event_data: any;
+  context: any;


This is not true. Right now context is always context_id: string, user_id?: string

balloob · 2020-10-01T10:01:10Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

        .defaultValue=${event_data}
        @value-changed=${this._dataChanged}
      ></ha-yaml-editor>
+      <ha-yaml-editor


We shouldn't just put up a YAML editor. Instead we know that context ID can only have a user that is relevant for matching, so we should show a drop down where the user can pick users to match.

zsarnett · 2020-10-11T06:15:22Z

@OnFreund Any progress?

OnFreund · 2020-10-11T08:30:41Z

@zsarnett I think I understand how to show a user drop down using ha-user-picker but I'm not sure how to allow selection of multiple users.

bramkragten · 2020-10-12T09:17:58Z

Check https://github.com/home-assistant/frontend/blob/dev/src/components/entity/ha-entities-picker.ts or https://github.com/home-assistant/frontend/blob/dev/src/components/ha-form/ha-form-multi_select.ts if you really want to get fancy.

OnFreund · 2020-10-12T14:11:37Z

Thanks @bramkragten, ha-entities-picker pointed me in the right direction. The view aspect is working now, but something isn't working well with change propagation. I'm not exactly sure how ha-automation-trigger-event is propagating its value, but right now it isn't working for me.

bramkragten · 2020-10-15T13:27:54Z

src/data/automation.ts

  platform: "event";
  event_type: string;
  event_data: any;
+  context: ContextConstraint;


Is context mandatory?

it's optional, I'll change this.

bramkragten · 2020-10-15T13:28:56Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

+          "ui.panel.config.automation.editor.triggers.type.event.context_user_pick"
+        )}
+        .hass=${this.hass}
+        .value=${this._wrapUsersInArray(context)}


Why do we need to do this?

the user_id constraint can contain either a single user id or an array of them. This normalizes it to an array. Is there a better way to do this?

bramkragten · 2020-10-15T13:30:02Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

+          "ui.panel.config.automation.editor.triggers.type.event.context_user_pick"
+        )}
+        .hass=${this.hass}
+        .value=${this._wrapUsersInArray(context)}


Suggested change

.value=${this._wrapUsersInArray(context)}

.value=${this._wrapUsersInArray(context?.user_id)}

bramkragten · 2020-10-15T13:32:21Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

+        .hass=${this.hass}
+        .value=${this._wrapUsersInArray(context)}
+        .users=${this._users}
+        @value-changed=${this._valueChanged}


We can't use the default _valueChanged handler, as this value needs to be merged in the context object first.

bramkragten · 2020-10-15T13:35:12Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

+        .hass=${this.hass}
+        .value=${this._wrapUsersInArray(context)}
+        .users=${this._users}
+        @value-changed=${this._valueChanged}


Suggested change

@value-changed=${this._valueChanged}

@value-changed=${this._usersChanged}

private _usersChanged(ev) { ev.stopPropagation(); fireEvent(this, "value-changed", { value: { ...this.trigger, context: {...this.trigger.context, user_id: ev.detail.value} }, }); }

Thanks, that did the trick!

bramkragten · 2020-10-15T13:37:22Z

src/data/automation.ts

  event_data: any;
+  context: ContextConstraint;


Suggested change

event_data: any;

context: ContextConstraint;

event_data?: any;

context?: ContextConstraint;

And you should guard for context being undefined.

bramkragten · 2020-10-15T13:38:01Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

+
  public static get defaultConfig() {
-    return { event_type: "", event_data: {} };
+    return { event_type: "", event_data: {}, context: { user_id: [] } };


No need to add this

Suggested change

return { event_type: "", event_data: {}, context: { user_id: [] } };

return { event_type: "" };

bramkragten · 2020-10-15T14:03:45Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

+    const empty: string[] = [];
+    return empty.concat(context.user_id || []);


Suggested change

const empty: string[] = [];

return empty.concat(context.user_id || []);

if (typeof user_id === "string") {

return [user_id];

}

return user_id;

bramkragten · 2020-10-15T14:05:56Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

    `;
  }

+  private _wrapUsersInArray(context): string[] {


Suggested change

private _wrapUsersInArray(context): string[] {

private _wrapUsersInArray(user_id: string | string[] | undefined): string[] {

bramkragten · 2020-10-15T14:07:00Z

src/panels/config/automation/trigger/types/ha-automation-trigger-event.ts

  }
+
+  protected firstUpdated(): void {
+    if (!this._users) {


It can't be defined here, so you can skip this check.

OnFreund · 2020-10-15T14:35:04Z

src/components/user/ha-users-picker.ts

+  private async _addUser(event: PolymerChangedEvent<string>) {
+    event.stopPropagation();
+    const toAdd = event.detail.value;
+    (event.currentTarget as any).value = "";


@bramkragten this doesn't work, because ha-user-picker is doing this:

const newValue = ev.detail.item.dataset.userId; if (newValue !== this._value) {

And changing the value doesn't change the selected item. What's the best way to resolve this?

That's actually weird, because it's comparing to ev.detail.item.dataset.userId, yet setting to ev.detail.value:

private _userChanged(ev) { const newValue = ev.detail.item.dataset.userId; if (newValue !== this._value) { this.value = ev.detail.value; setTimeout(() => { fireEvent(this, "value-changed", { value: newValue }); fireEvent(this, "change"); }, 0); } }

Yeah, that doesn't seem correct. I think newValue should be ev.detail.value?

This is really strange. So ev.detail.value doesn't even exist (which means that this.value = ev.detail.value; is probably wrong), and ev.newValue is always undefined.
It seems like ev.detail.item.dataset.userId is indeed the right way to grab the currently selected user, but it's unclear how to programmatically select a user...

This is a red herring - this method on ha-user-picker isn't even called by setting value to "", and it's unclear that value even has any meaning (I thought it was called, because it was called twice when picking a user, so I assumed one of those was a result of setting value to "").
I'm completely stumped 🤷‍♂️

I'll put this PR on my todo list :-)

Any chance we can get it in time for the beta? The core feature is already merged

I'll try 🙈

Should be fixed now

bramkragten · 2020-10-21T12:17:04Z

If a user removes all users, should the user_id be removed or should it be an empty list?

OnFreund · 2020-10-21T12:19:06Z

Thanks @bramkragten!

If a user removes all users, should the user_id be removed or should it be an empty list?

It should be removed

bramkragten · 2020-10-21T12:27:05Z

@OnFreund can you review/test this?

OnFreund · 2020-10-21T12:49:22Z

Looks great, and working. The only thing that's slightly off is that when you add a user, the drop down only contains users who haven't been selected, yet if you open it again after adding a user, it will show all users.

bramkragten · 2020-10-21T13:32:43Z

Not seeing that? How can I reproduce that?

OnFreund · 2020-10-21T13:37:59Z

Start with no users
Add one user
Second dropdown only shows "No User", and remaining users. Select one of them
Third dropdown created with "No User" and remaining users.
Second dropdown now has all users.

bramkragten · 2020-10-21T14:33:32Z

@OnFreund can you try again?

OnFreund · 2020-10-21T14:39:02Z

Perfect!

homeassistant added the cla-signed label Oct 1, 2020

balloob reviewed Oct 1, 2020

View reviewed changes

OnFreund added 2 commits October 12, 2020 17:08

Add context to event trigger

d98f21a

User picker instead of yaml

840b2f3

OnFreund force-pushed the event_context branch from 640ca9a to 840b2f3 Compare October 12, 2020 14:09

OnFreund added 5 commits October 12, 2020 17:13

Remove console.log

f3ed5bd

Fix lint

40afd5a

More lint fixes

3979a48

var->let

186c4fc

let-const

257d066

bramkragten reviewed Oct 15, 2020

View reviewed changes

OnFreund commented Oct 15, 2020

View reviewed changes

OnFreund and others added 2 commits October 16, 2020 11:08

Fix propagation

60de9ef

Fix user picker

b51e330

Remove user id when none selected

62b2f57

bramkragten added 2 commits October 21, 2020 15:41

Move fetching users to users picker

fe954d3

Only allows selecting user once

010c589

Finals tweaks and localize

7cd4287

bramkragten approved these changes Oct 21, 2020

View reviewed changes

clean up

c1b9bb2

bramkragten merged commit 30f34ee into home-assistant:dev Oct 21, 2020

bramkragten mentioned this pull request Oct 21, 2020

20201021.0 #7422

Merged

OnFreund deleted the event_context branch October 22, 2020 09:34

github-actions bot locked and limited conversation to collaborators Jul 5, 2022

	.value=${this._wrapUsersInArray(context)}
	.value=${this._wrapUsersInArray(context?.user_id)}

	@value-changed=${this._valueChanged}
	@value-changed=${this._usersChanged}

	return { event_type: "", event_data: {}, context: { user_id: [] } };
	return { event_type: "" };

		const empty: string[] = [];
		return empty.concat(context.user_id \|\| []);

-    const empty: string[] = [];
-    return empty.concat(context.user_id || []);
+    if (typeof user_id === "string") {
+    	return [user_id];
+    }
+    return user_id;

	private _wrapUsersInArray(context): string[] {
	private _wrapUsersInArray(user_id: string \| string[] \| undefined): string[] {

Conversation

OnFreund commented Oct 1, 2020

Proposed change

Type of change

Additional information

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsarnett commented Oct 11, 2020

Uh oh!

OnFreund commented Oct 11, 2020

Uh oh!

bramkragten commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OnFreund commented Oct 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramkragten Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramkragten Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramkragten Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramkragten commented Oct 21, 2020

Uh oh!

OnFreund commented Oct 21, 2020

Uh oh!

bramkragten commented Oct 21, 2020

Uh oh!

OnFreund commented Oct 21, 2020

Uh oh!

bramkragten commented Oct 12, 2020 •

edited

Loading

bramkragten Oct 15, 2020 •

edited

Loading

bramkragten Oct 15, 2020 •

edited

Loading

bramkragten Oct 16, 2020 •

edited

Loading