Migrate Hikvision integration to config flow#158279
Conversation
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @mezz64, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
8a0a487 to
24dcd68
Compare
64ac443 to
c78d95d
Compare
d3eb10a to
a18bf82
Compare
a18bf82 to
2c1a048
Compare
2c1a048 to
4e409e3
Compare
These functions extend the event detection capabilities to support NVRs with non-standard notification methods (record, email, beep) in addition to the existing 'center' and 'HTTP' methods. - get_nvr_events: Fetches events from NVR with broader notification support - inject_events_into_camera: Injects discovered events into HikCamera - HikCamera.inject_events: New method to inject events into camera's state Based on home-assistant/core#158279
4e409e3 to
23332a7
Compare
The entry object is only used to access runtime_data, so we don't need to store the entry itself.
Use requests.exceptions.RequestException instead of bare Exception for more precise error handling when connecting to Hikvision devices.
Remove the reauthentication flow as requested by reviewer. This can be added in a follow-up PR.
Preserve the user-configured name from YAML configuration when importing to config entry. Falls back to device name or host.
Use HOMEASSISTANT_DOMAIN for the successful YAML import deprecation issue, matching the pattern used by other integrations like nederlandse_spoorwegen. This uses the shared translation strings.
- Remove reauth flow tests (reauth removed for follow-up PR) - Use requests.exceptions.RequestException instead of Exception - Update deprecated_yaml issue tests to use HOMEASSISTANT_DOMAIN
- Merge mock_hikcamera and mock_hikcamera_config_flow into single fixture - Patch both HikCamera locations using combined context manager pattern - Remove unnecessary MagicMock assignments (autospec handles methods) - Fix fetch_attributes to use return_value assignment syntax
- Use combined mock_hikcamera fixture - Reuse result variable instead of result2, result3 - Remove unnecessary async_block_till_done() calls - Add unique_id assertions to verify entry uniqueness - Merge error tests to include recovery (end in CREATE_ENTRY)
- Merge test_setup_entry and test_unload_entry into single test - Remove runtime_data internal assertions - Remove test_setup_entry_no_device_id (implementation detail) - Remove test_setup_entry_default_name/type tests
- Use issue_registry fixture instead of getting from hass - Use combined mock_hikcamera fixture from conftest - Consolidate entity tests (unique_id, attributes covered in main test)
Use snapshot_platform helper to capture entity registry and state snapshots for all binary sensor entities.
- Import CONF_DELAY from homeassistant.const (existing constant) - Define CONF_IGNORED, DEFAULT_DELAY, DEFAULT_IGNORED locally in binary_sensor.py to match original code structure
- Add test for device_id None case in async_setup_entry - Add test for import flow when device_id is None - Add test for YAML import abort creating issue - Add test for binary sensor update callback - Remove unused timer cancellation code (dead code from delay feature)
fabbe62 to
c11fb3f
Compare
|
Anything you need from me for this PR? |
joostlek
left a comment
There was a problem hiding this comment.
Awesome!
Will you also look into becoming codeowner? And maybe send me a message on Discord
|
@ptarjan The documentation also needs to be updated |
Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>
* Add get_nvr_events and inject_events_into_camera functions These functions extend the event detection capabilities to support NVRs with non-standard notification methods (record, email, beep) in addition to the existing 'center' and 'HTTP' methods. - get_nvr_events: Fetches events from NVR with broader notification support - inject_events_into_camera: Injects discovered events into HikCamera - HikCamera.inject_events: New method to inject events into camera's state Based on home-assistant/core#158279 * Add tests for get_nvr_events and inject_events_into_camera Tests cover: - Parsing NVR events with various notification methods (record, email, beep, center) - Skipping videoloss events (used for watchdog) - Handling connection errors gracefully - Handling non-200 responses - Handling invalid XML - Injecting events into camera event_states - Preventing duplicate channel events * Refactor: Add notification_methods parameter to get_event_triggers Instead of a separate get_nvr_events function, extend the existing get_event_triggers method to accept a notification_methods parameter. This allows users to specify which notification methods to accept (defaults to {'center', 'HTTP'} for backwards compatibility). For NVRs, users can pass VALID_NOTIFICATION_METHODS which includes 'record', 'email', and 'beep' in addition to the defaults. Changes: - Add notification_methods parameter to get_event_triggers() - Remove duplicate get_nvr_events function - Export VALID_NOTIFICATION_METHODS constant for easy use - Update tests to cover new parameter functionality --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: mezz64 <2854333+mezz64@users.noreply.github.com>
| device_id = camera.get_id() | ||
| device_name = camera.get_name | ||
| except requests.exceptions.RequestException: | ||
| _LOGGER.exception( |
There was a problem hiding this comment.
We only log unknown exceptions including the stack trace.
There was a problem hiding this comment.
We normally catch broad exception too. Haven't you used our scaffold script to generate the config flow? It has a template to follow.
There was a problem hiding this comment.
No i never used the script, I'll check what it does and see if I missed anything. Thanks.
Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>
Migrate the Hikvision integration from a legacy YAML-based integration to the modern config flow-based system. In the process of this migration I was able to fix the creation of my binary sensors. Now I have:
Breaking change
Users with existing YAML configuration will see a deprecation message.
Proposed change
Type of change
Additional information
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: