Enhance Snoo integration to support logging diaper changes#149599
Enhance Snoo integration to support logging diaper changes#149599kevin-david wants to merge 3 commits intohome-assistant:devfrom
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @kevin-david
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
bot
added
cla-needed
has-tests
integration: snoo
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
| "--security-opt", | ||
| "label=disable" | ||
| ], | ||
| "mounts": [ |
There was a problem hiding this comment.
I will remove this before finalizing the PR, but wondering - is there a better way to do this?
There was a problem hiding this comment.
Still interested in knowing but removed now
| _raise_baby_not_found_error(baby_id, available_babies) | ||
|
|
||
| start_time = call.data.get("start_time") | ||
| if start_time is not None and start_time.tzinfo is None: |
There was a problem hiding this comment.
Not sure if this is the best approach, but got the timezone data working for me locally
| @@ -0,0 +1,72 @@ | |||
| """Test services for the Snoo integration.""" | |||
There was a problem hiding this comment.
Not sure on optimal test patterns, but these seemed like OK cases.
kevin-david
force-pushed
the
snoo-diapers
branch
3 times, most recently
from
0ef2805 to
81785a3
Compare
|
@Lash-L ready for a review here when you are, I think! Also added docs here: home-assistant/home-assistant.io#40371 |
| "Diaper change logged for baby %s: %s", baby_id, call.data["diaper_types"] | ||
| ) | ||
|
|
||
| return result.to_dict() |
There was a problem hiding this comment.
It's a DiaperActivity - mainly thought it could be useful to get the ID back (plus any sent info, server time, etc.) in case it was going to be used for something else. happy to remove this (and/or the logging) if it's too much noise
| def _raise_no_config_error() -> None: | ||
| """Raise error when no Snoo configuration is found.""" | ||
| raise HomeAssistantError("No Snoo configuration found") | ||
|
|
||
| def _raise_baby_not_found_error(baby_id: str, available_babies: str) -> None: | ||
| """Raise error when baby ID is not found.""" | ||
| raise HomeAssistantError( | ||
| f"Baby ID '{baby_id}' not found. Available baby IDs: {available_babies}" | ||
| ) | ||
|
|
||
| config_entries = call.hass.config_entries.async_entries(DOMAIN) | ||
| if not config_entries: | ||
| _raise_no_config_error() |
There was a problem hiding this comment.
Can one snoo integration handle multiple babies? Is there possible data that we can show for a baby so we can make it a Device? (this is a weird thing to say lol). Because then we can use a device selector and we don't have to iterate over all the entries
There was a problem hiding this comment.
I was thinking something similar. I think a device per baby would be helpful.
You'd have one device for the actual snoo and then one device for the baby. The baby part of the api is not a part that I have touched much - but I think that would be helpful for future improvements.
There was a problem hiding this comment.
Well this was fun, but now it supports baby... Devices 😄
Got pretty large because of it, but contained to the most recent commit. I can split that out as a separate PR if need be
| options: | ||
| - "Wet" | ||
| - "Dirty" |
There was a problem hiding this comment.
Let's make the values lowercase and use a translation key to translate these
There was a problem hiding this comment.
So - tried to use a translation key here, e.g. [%key:component::snoo::services::log_diaper_change::fields::diaper_types::options::dirty%] but was showing up as that literal string. Was I doing that wrong or did you mean something else?
Also in strings.json when trying to add wet/dirty... * [ERROR] [TRANSLATIONS] Invalid strings.json: extra keys not allowed @ data['services']['log_diaper_change']['fields']['diaper_types']['options']. Got {'wet': 'Wet', 'dirty': 'Dirty'}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There are many other things that could be logged as well, but this is a start!
kevin-david
force-pushed
the
snoo-diapers
branch
from
81785a3 to
d714b5e
Compare
| ] | ||
|
|
||
|
|
||
| BABY_BINARY_SENSOR_DESCRIPTIONS: list[SnooBabyBinarySensorEntityDescription] = [ |
There was a problem hiding this comment.
Presumably these are changable, but to keep this simple(r) I just left these as (binary in this case) sensors for now.
There was a problem hiding this comment.
Do note that once this is released you would have to deprecate it, so it's probably easier to find out if it is changable, as it will probably save you time in the end
There was a problem hiding this comment.
Ahh I didn't realize that, which seems obvious now. Since the original intent of this PR was the service, I'm thinking I go back to that, add the device, but none of the sensors. What do you think?
| _LOGGER.warning("No coordinators found for config entry %s", entry.entry_id) | ||
| continue | ||
|
|
||
| for coordinator in coordinators.values(): |
There was a problem hiding this comment.
Theoretically we could require the parent/Snoo device ID as well, but that seemed redundant when we can just filter down to Baby devices via services.yaml for one less input, and I suspect this loop will be a 1-2 iterations for most people
| device: dr.DeviceEntry | None = device_registry.async_get(baby_device_id) | ||
| if not device: | ||
| _raise_device_not_found_error(baby_device_id) | ||
| assert device is not None |
There was a problem hiding this comment.
Not sure if this is right, but got mypy happy b/c the _raise_* methods are the ones doing the blowing up
kevin-david
force-pushed
the
snoo-diapers
branch
from
d714b5e to
6e5de4c
Compare
| if update_interval is None: | ||
| update_interval = timedelta(minutes=5) |
| async def _async_update_data(self) -> None: | ||
| """Fetch baby data.""" | ||
| status = await self.baby.get_status() | ||
| self.data = status |
There was a problem hiding this comment.
return the new baby status instead of assigning it to self.data
| super().__init__(coordinator) | ||
| self.baby = coordinator.baby | ||
| self.entity_description = description | ||
| self._attr_unique_id = f"snoo_baby_{coordinator.baby.baby_id}_{description.key}" |
There was a problem hiding this comment.
I think we can remove snoo here
| ] | ||
|
|
||
|
|
||
| BABY_BINARY_SENSOR_DESCRIPTIONS: list[SnooBabyBinarySensorEntityDescription] = [ |
There was a problem hiding this comment.
Do note that once this is released you would have to deprecate it, so it's probably easier to find out if it is changable, as it will probably save you time in the end
| icon="mdi:minus", | ||
| value_fn=lambda data: data.settings.minimalLevel, | ||
| ), | ||
| SnooBabySensorEntityDescription( | ||
| key="minimal_level_volume", | ||
| translation_key="minimal_level_volume", | ||
| icon="mdi:volume-low", | ||
| value_fn=lambda data: data.settings.minimalLevelVolume, | ||
| ), | ||
| SnooBabySensorEntityDescription( | ||
| key="responsiveness_level", | ||
| translation_key="responsiveness_level", | ||
| icon="mdi:knob", |
There was a problem hiding this comment.
Let's keep this for a followup then :)
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
@kevin-david Please address or reply to the open review comments, and mark your PR as "ready for review", it won't be merged otherwise. |
|
Wow I had the same idea to work on this! We must both have new kiddos! I created this PR for the python_snoo library to just get a list of babies (and not have to go through a snoo device's list of baby IDs) since babies shouldn't be tied to a specific device. (Parents may sell device or use it for another kid after their baby is past 6 months, but may want to continue tracking diapers/feedings.) So this may have an impact on your PR, hopefully it can make it easier/more logical. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
5 participants
There are many other things that could be logged as well, but this is a start! Totally open to feedback here, this is my first home assistant core PR.
Proposed change
Introduce functionality via a service call to make calls to log data via the Happiest Baby / Snoo backend, available via the phone app.
Type of change
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: