Skip to content

Pass device ID to logbook if available#12728

Merged
balloob merged 6 commits intodevfrom
logbook-device-id
May 20, 2022
Merged

Pass device ID to logbook if available#12728
balloob merged 6 commits intodevfrom
logbook-device-id

Conversation

@balloob
Copy link
Member

@balloob balloob commented May 19, 2022

Breaking change

Proposed change

Pass device ID to logbook if available.

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

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

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:

@bdraco

This comment was marked as outdated.

@bdraco

This comment was marked as outdated.

@bdraco
Copy link
Member

bdraco commented May 19, 2022

I fixed the missing devices here home-assistant/core#72122

@bdraco

This comment was marked as outdated.

@balloob
Copy link
Member Author

balloob commented May 19, 2022

Pushed fixes.

@bdraco
Copy link
Member

bdraco commented May 19, 2022

Retesting now

@bdraco
Copy link
Member

bdraco commented May 19, 2022

Out of scope, but the logbook card doesn't support selecting a device or entity (only entities)

@bdraco

This comment was marked as off-topic.

@bdraco

This comment was marked as outdated.

@balloob
Copy link
Member Author

balloob commented May 19, 2022

I want to do the device picker for logbook card in another PR because it will require making some decisions. Would you expect to pick a device, and then have it automatically select all entities too? And if we do that, should we even still show the entity picker? And should there be a toggle if the device should only show logbook entries for primary entities or also config/diagnostic?

@bdraco
Copy link
Member

bdraco commented May 19, 2022

I want to do the device picker for logbook card in another PR because it will require making some decisions.

I agree 100%. Was only mentioning it for testing completeness.

Would you expect to pick a device, and then have it automatically select all entities too?

Sadly since not all entities have devices or come from config entries, I think we have to select both otherwise the user is likely missing out on a lot of entities they probably want to track.

And if we do that, should we even still show the entity picker?

Previous answer applies

And should there be a toggle if the device should only show logbook entries for primary entities or also config/diagnostic?

Previous answer applies

@bdraco
Copy link
Member

bdraco commented May 19, 2022

Devices with only diagnostic entities or no entities get the full logbook.

No filter is passed to the api call
Screen Shot 2022-05-19 at 12 38 01

Screen Shot 2022-05-19 at 12 37 01

Screen Shot 2022-05-19 at 12 37 43

@bdraco

This comment was marked as outdated.

@bdraco

This comment was marked as off-topic.

@bdraco

This comment was marked as off-topic.

@bdraco
Copy link
Member

bdraco commented May 19, 2022

There is a bug with minimal responses with the last state that is the cause of #12728 (comment)

We replace it with a full state for the frontend but we get the wrong one if we have skipped all the states at the end of the list.

Opened #12732 to get the attributes from first state instead of the last. I'm going to remove the code that replaces the last state on the backend with a full state instead of a minimal state so we don't have that complexity as it was only doing so to make the frontend happy

@bdraco
Copy link
Member

bdraco commented May 19, 2022

Ok back to testing this.

@bdraco
Copy link
Member

bdraco commented May 19, 2022

diff --git a/src/panels/config/devices/ha-config-device-page.ts b/src/panels/config/devices/ha-config-device-page.ts
index 3c0477573..372334048 100644
--- a/src/panels/config/devices/ha-config-device-page.ts
+++ b/src/panels/config/devices/ha-config-device-page.ts
@@ -594,8 +598,8 @@ export class HaConfigDevicePage extends LitElement {
                       <ha-logbook
                         .hass=${this.hass}
                         .time=${this._logbookTime}
-                        .entityId=${this._entityIds(entities)}
-                        .deviceId=${this._deviceIdInList(this.deviceId)}
+                        .entityIds=${this._entityIds(entities)}
+                        .deviceIds=${this._deviceIdInList(this.deviceId)}
                         virtualize
                         narrow
                         no-icon

@bdraco

This comment was marked as outdated.

@bdraco
Copy link
Member

bdraco commented May 19, 2022

Finished testing. Looks like just the missing ss and this is GTG 👍

bdraco
bdraco previously approved these changes May 19, 2022
@bdraco
Copy link
Member

bdraco commented May 20, 2022

✅ Logbook cards
✅ Logbook from left bar
✅ Areas
✅ Devices #12728 (comment)
✅ More Info

@bdraco
Copy link
Member

bdraco commented May 20, 2022

This one is really so cool to see in action. 👍

@balloob balloob enabled auto-merge (squash) May 20, 2022 04:08
@balloob balloob merged commit bfeb907 into dev May 20, 2022
@delete-merged-branch delete-merged-branch bot deleted the logbook-device-id branch May 20, 2022 04:09
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2022
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.

3 participants