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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support notification_id in notify.persistent_notification #74822
Conversation
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( |
53997c5
to
6de15bf
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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. |
Still relevant. |
@KevinCathcart Could you resolve the merge conflicts in this PR? Anything to help increase the chance of this getting reviewed and merged. |
So now that I have some core dev experience, I think I'm able to follow what the code in this PR is doing and can provide an informed review. So the appropriate In the code itself, we check to see if the Finally, the test to me looks pretty good. It's creating 2 persistent notifications via the |
I just realized a potential issue with the tests in this PR. When validating the created persistent notifications, we are getting an entity from states. However, with recent changes, persistent notifications will no longer be stored in states. I'll try to find out the proper way to fetch persistent notification information. |
Ok I think the answer actually lies with test changes that would be present if this PR were to be synced with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested these changes locally. Tests passing with 100% cover to applicable files.
While I was at it I also spun this up with hass
and I can confirm it's working as expected. The service has a data field now and when notification_id
is specified it will replace existing notifications with that id and calling persistent_notification.dismiss
with that id will remove it.
Co-authored-by: Scott Giminiani <ScottG489@Gmail.com>
馃檹馃檹馃檹 |
Breaking change
Proposed change
Adds support for notification_id as a data field. As discussed in issue #68811, this is relevant in some scenarios such as when using the Alert integration, which you would prefer to update the existing notification instead of potentially creating a large number of persistent_notification entities.
Code wise this is pretty straightforward. Rather than updating the voluptuous schema for this service, I've just used the existing notify schema to make this act the same as the real platform implementations.
Type of change
Additional information
Checklist
black --fast 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
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: