Skip to content

Conversation

@skgsergio
Copy link
Contributor

@skgsergio skgsergio commented Feb 4, 2021

Proposed change

The integration is currently broken for users using non-admin accounts and for some models that incorrectly reports its RTSP port wrongly (sorry! 😓).

Foscam cameras require admin account for getting the MAC address, requiring an admin account in the integration is not desirable as an operator one is good enough (and a good practice).

Old entries using the MAC address as unique_id are migrated to the new unique_id format so everything is consistent.

Related to this issue I've found that motion detection setting is also restricted to admin accounts and was throwing an error that was not helpful so I've also added proper error messages to those cases.

Also fixed unhandled invalid responses from the camera in the config flow process.

Also the RTSP port is reported incorrectly in some models which causes stream not to work, so I made RTSP port configurable again (and importing it correctly from the old config).

So to recap:

  1. Changed unique_id so it doesn't use MAC address (restricted for foscam admin accounts) and migrate old configs.
  2. Added logs to optional parts that require admin accounts (motion detection setting).
  3. Fix error handling when an invalid response is received from a camera.
  4. Update config flow tests to test new errors and consider the new unique_id.
  5. Make configurable RTSP port again, adding it to the config flow to both UI and import flows.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

Foscam cameras require admin account for getting the MAC address,
requiring an admin account in the integration is not desirable as
an operator one is good enough (and a good practice).

Old entries using the MAC address as unique_id are migrated to the
new unique_id format so everything is consistent.

Also fixed unhandled invalid responses from the camera in the
config flow process.
@skgsergio skgsergio changed the title Do not require admin account for foscam cameras (fixes #45921) Do not require admin account for foscam cameras and make RTSP port configurable Feb 4, 2021
@skgsergio
Copy link
Contributor Author

Both bug fixes are important and both changes the configuration so in order to ship them quicker I've decided to keep them in one single PR as both require entry migration.

@skgsergio skgsergio changed the title Do not require admin account for foscam cameras and make RTSP port configurable Fix foscam to work again with non-admin accounts and make RTSP port configurable again Feb 4, 2021
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@MartinHjelmare MartinHjelmare merged commit 2c74bef into home-assistant:dev Feb 5, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2021
@skgsergio skgsergio deleted the fix-foscam-with-non-admin-users branch February 7, 2021 18:37
@MartinHjelmare MartinHjelmare added this to the 2021.2.2 milestone Feb 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

4 participants