Skip to content
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

Add MVP person component #20290

Merged
merged 14 commits into from Feb 7, 2019

Conversation

Projects
None yet
@MartinHjelmare
Copy link
Member

MartinHjelmare commented Jan 21, 2019

Description:

This aims to solve MVP as specified here:
home-assistant/architecture#72 (comment)

  • Required name.
  • Required id. Id will set unique id.
  • Optional user id.
  • Optionally track device trackers. Last device tracker state change will set state.
  • Set device tracker state entity_id as source attribute.
  • Set coordinates of device tracker state as state attributes.
  • Restore state.

If we think this is the right direction, I'll finish up and add tests.

Related issue (if applicable):
home-assistant/architecture#72

Pull request in home-assistant.io with documentation (if applicable):
WIP

Example entry for configuration.yaml (if applicable):

person:
  - name: Martin Hjelmare
    user_id: 12345678912345678912345678912345
    device_trackers:
      - device_tracker.demo_paulus
      - device_tracker.demo_anne_therese

device_tracker:
  - platform: demo

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Add person component
* Required first name.
* Optional last name and user id.
* Optionally track device trackers. Last device tracker state change will
  set state.
* Set device tracker state entity_id as source attribute.
* Set coordinates of device tracker state as state attributes.
* Restore state.

MartinHjelmare added some commits Jan 21, 2019

@marchingphoenix

This comment was marked as off-topic.

Copy link
Contributor

marchingphoenix commented Jan 21, 2019

For the device trackers. Do we want the last update to always reflect the state or do we want some sort of logic between multiples? Or is that outside the scope of the MVP?

@bachya

This comment was marked as off-topic.

Copy link
Contributor

bachya commented Jan 21, 2019

@marchingphoenix That's probably outside the scope of MVP, but still an absolute must-have post-MVP. For instance, router-based device trackers often disconnect at weird moments when the client drops off the network; we wouldn't that to immediately change the person's overall state.

@MartinHjelmare

This comment was marked as off-topic.

Copy link
Member Author

MartinHjelmare commented Jan 21, 2019

Yes, MVP is last state. But as mentioned above and in the architecture issue, we should probably try to be smarter in future iterations.

@frenck frenck added the docs-missing label Jan 21, 2019

@balloob
Copy link
Member

balloob left a comment

I think that this is perfect for MVP.

Things out of scope for MVP, but we should consider for the future:

  • Battery level from device tracker
  • Allow storing persons in storage
  • Manage stored persons via websockets
  • Improved device tracking algorithm, including use last X states to consider movement?
Show resolved Hide resolved homeassistant/components/person/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/person/__init__.py Outdated

@balloob balloob referenced this pull request Jan 22, 2019

Closed

Add support for unloading entries to device trackers #20237

4 of 4 tasks complete
@awarecan

This comment was marked as off-topic.

Copy link
Contributor

awarecan commented Jan 22, 2019

Hope person can be tracked at room level some day.

@Hedda

This comment was marked as off-topic.

Copy link
Contributor

Hedda commented Jan 22, 2019

Out of scope for a minimum viable product of this feature but FYI; one tip for related GUI interface part of a such "Person Tracker" concept would be to in the long-term consider looking at borrowing ideas from the user interface of the Fing app when you have the accompanying Fingbox. This as the "Fing" app when you have a Fingbox on your network has a very nice GUI interface and user experience for adding multiple persons to your home and then associating devices to those persons.

Fing calls this feature "Digital Presence" and you have to give it to the Fing.io team that they got excellent user experience engineering skills, and here is an overview page how the GUI layout of this function is in the Fing app when you have a Fingbox. This describes the entire process of adding a new user and assigning devices:

https://help.fing.io/knowledge-base/digital-presence-detection/

Here is a video which goes through the app GUI and features

https://www.youtube.com/watch?v=wsvwSNunqbg

Also check out their description as well as their video describing of the related " Digital Fence" (geofence) feature:

https://help.fing.io/knowledge-base/digitalfence-feature/

https://www.youtube.com/watch?v=IYIW8KVJ-iY

While the Fing app with Fingbox primary use this specific core feature of associating devices to those people for Parental Control functions in a household, its user interface to achieve this part of an adding people and then associating devices is very user-friendly from and end-users point-of-view.

https://help.fing.com/wp-content/uploads/2018/12/Digital-presence-e1544633161743.png

Summary:

Create Users & Assign Personal Devices.

One of the most important Fingbox features is Digital Presence. To enable Digital Presence, you need to create users and assign personal devices to each of them.

From the People tab click on the circle with a plus (+) sign to create a new user. Choose the devices belonging to that person; for example, their mobile phones, computers and tablets. Then make sure to assign a device that represents that person’s presence – this is the device that reflects their movement in and out of the office. A smart phone, for example, is likely to always be on them, whereas their desktop computer stays in the office at all times.

To select the PRESENCE Device, click on the user in the user carousel and then select Devices in the pop-up. Find the device you wish to turn into the PRESENCE device and click the CHANGE button on the right-hand side of the device. This will then give you the option to state that it always travels with that user. You will then see the CHANGE become PRESENCE.

Some additional pictures from the Fing app

https://help.fing.com/wp-content/uploads/2018/12/Digital-presence-e1544633161743.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181212-164934-e1544635013765.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181207-181059-e1544208350801.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181212-170359-e1544634955638.png
https://help.fing.com/wp-content/uploads/2018/05/WhatsApp-Image-2018-05-23-at-11.54.20-1-320x569.jpeg
https://help.fing.com/wp-content/uploads/2018/12/fence-300x203.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181207-200337-1-320x525.png
https://help.fing.com/wp-content/uploads/2018/03/Screenshot-Device-List-320x569.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181207-200343-e1544214089109.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181207-200411-e1544214398728.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181207-200418-e1544214500444.png
https://help.fing.com/wp-content/uploads/2018/12/Screenshot_20181207-200453-1-e1544215077466.png
https://help.fing.com/wp-content/uploads/2018/12/filter-options-300x51.png

@Landrash

This comment has been minimized.

Copy link
Contributor

Landrash commented Jan 22, 2019

Copied to the Architecture issue home-assistant/architecture#72
Goes against what's suggested but user_id usually is the only thing that doesn't change in most systems. Since people can change their name etc.
Wouldn't a user_id be simpler since names can often contain characters that needs to be normalized but it might be desired to have these still shown in the UI correctly?

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 22, 2019

Please don't comment on this PR about things outside MVP scope. That's what we have the architecture issue for. I'll hide the comments I think are out of scope.

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 22, 2019

@Landrash are you suggesting we should base the entity_id of the person entity on user_id instead of name? This is relevant to MVP, I think.

@Landrash

This comment has been minimized.

Copy link
Contributor

Landrash commented Jan 22, 2019

@MartinHjelmare That is correct.

@MartinHjelmare MartinHjelmare changed the title RFC: Add person component RFC: Add MVP person component Jan 22, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 22, 2019

I think that we should base unique_id on user_id, and entity_id on name. That way a user can also change entity_id.

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 22, 2019

Just to make sure, when we say user_id, we mean the 32 character string generated for each user during user registration?

@Landrash

This comment has been minimized.

Copy link
Contributor

Landrash commented Jan 22, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 23, 2019

@Landrash the user_id is inherently connected with a home assistant user account. We might want to track a person without an account, yes. Then that entity will not have a user_id and no unique_id. That is ok I think. Did you mean something else?

@MartinHjelmare MartinHjelmare changed the title RFC: Add MVP person component Add MVP person component Jan 23, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 24, 2019

it is kinda annoying that persons for non-users would have no unique ID…

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 24, 2019

Yeah. First and last name together would probably be unique and stable for a home installation. Although they will be configurable of course.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 24, 2019

Storage mode and automatic generated IDs, management via UI via websocket commands 🙈

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 24, 2019

I don't have a better idea at the moment.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 24, 2019

I think that for YAML configuration, we should just allow people to configure it manually. When we add storage mode, we will generate them on the fly.

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 24, 2019

Ok, so I'll add an option for unique_id, that if set overrides user_id as unique_id?

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 25, 2019

nah, let's not use user_id as unique ID at all. Let's add ID as a mandatory field.

When we add storage support (as I want to be able to manage this via UI), all persons backed by YAML will be marked as read-only (based on IDs)

MartinHjelmare added some commits Jan 26, 2019

DOMAIN = 'person'

PERSON_SCHEMA = vol.Schema({
vol.Required(CONF_ID): cv.string,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 26, 2019

Author Member

Should we validate this with the entity_registry, similar as how we do for user_id?

This comment has been minimized.

@balloob

balloob Jan 27, 2019

Member

Not sure if it's necessary. Just has to be unique among persons.

for person_conf in conf:
user_id = person_conf.get(CONF_USER_ID)
if (user_id is not None
and await hass.auth.async_get_user(user_id) is None):

This comment has been minimized.

@balloob

balloob Jan 27, 2019

Member

Should we skip it completely or just let it be added without a user_id ?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 27, 2019

Author Member

Not sure. I can see arguments for both ways.

@balloob
Copy link
Member

balloob left a comment

Seems good 🎉

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 27, 2019

I'll start on documentation now.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 31, 2019

@MartinHjelmare shall I tag this PR for the beta?

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Jan 31, 2019

I would like to hold off on that. I want to try and make the source entity_id always be the device tracker that provided last state. Then I need to fix the docs. I don't have so much time this week.

@balloob

balloob approved these changes Feb 7, 2019

Copy link
Member

balloob left a comment

I am going ahead and merge this so I can add a 2nd storage option using the storage helper.

@balloob balloob merged commit 5f76628 into home-assistant:dev Feb 7, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 93.11%
Details

@wafflebot wafflebot bot removed the in progress label Feb 7, 2019

@MartinHjelmare MartinHjelmare referenced this pull request Feb 9, 2019

Merged

Add person component #8458

2 of 2 tasks complete

@MartinHjelmare MartinHjelmare deleted the MartinHjelmare:add-person-component branch Feb 9, 2019

@klaasnicolaas

This comment has been minimized.

Copy link
Member

klaasnicolaas commented Feb 10, 2019

@niemyjski

This comment has been minimized.

Copy link

niemyjski commented Feb 20, 2019

It seems like a user account would be tied to a person entity of which you could also attach roles to.

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

@Joshfindit

This comment has been minimized.

Copy link

Joshfindit commented Feb 24, 2019

Request from an end-user: Could the documentation be updated to cover the following?

  1. Where the user_id can be found? Not just for the logged-in user, but for other users.
  2. What the requirements are for the id:. Could it just be 1 and 2? alyssa and seth?
@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

MartinHjelmare commented Feb 24, 2019

Please open an issue in our documentation repo instead.
https://github.com/home-assistant/home-assistant.io/issues

Merged PRs should not be used for enhancement discussion or bug reports.

Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Feb 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.