Add Traffic Rule switches to UniFi Network #118821

bdowden · 2024-06-04T16:12:39Z

Breaking change

Proposed change

Add switch entities for unifi traffic rules. This is branched from ViViDboarder's PR with fixes and implementations for the suggestions in that PR. I know what it's like to have a newborn and how time slips away.

It's also my first PR for home assistant so suggestions/clarifications on some classes are welcome!

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)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant

Hi @bdowden

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 · 2024-06-04T16:12:47Z

Hey there @Kane610, mind taking a look at this pull request as it has been labeled with an integration (unifi) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of unifi can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign unifi Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

EliSchleifer · 2024-06-05T00:39:45Z

@bdowden - you need to sign the CLA for copyright issues before this can move forward

bdowden · 2024-06-05T01:04:03Z

@EliSchleifer I did. When I go to the link it says no pending request.

EliSchleifer · 2024-06-05T01:08:47Z

You should be able to mark the PR as ready for review. It’s currently marked as draft. Also probably needs to rerun those CI jobs to clear the block.

…

________________________________ From: bdowden ***@***.***> Sent: Tuesday, June 4, 2024 6:04:25 PM To: home-assistant/core ***@***.***> Cc: Eli Schleifer ***@***.***>; Mention ***@***.***> Subject: Re: [home-assistant/core] Add Traffic Rule switches to UniFi Network (PR #118821) @EliSchleifer<https://github.com/EliSchleifer> I did. When I go to the link it says no pending request. — Reply to this email directly, view it on GitHub<#118821 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJVCPWXDS3MNWVVJG5G3IDZFZPZTAVCNFSM6AAAAABIY22SEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBYGY3DCMRSGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Kane610

First round of comments, I need to think about the updatecoordinator and read up on it a bit

homeassistant/components/unifi/hub/entity_loader.py

homeassistant/components/unifi/hub/hub.py

homeassistant/components/unifi/switch.py

homeassistant/components/unifi/hub/entity_loader.py

home-assistant · 2024-06-07T08:38:24Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

bdowden · 2024-06-07T22:46:02Z

First round of comments, I need to think about the updatecoordinator and read up on it a bit

@Kane610 I wasn't sure about updatecoordinator either. I saw your comment in the other PR that a simple polling mechanism to get the updated status of the traffic rule would be fine and began to implement that. I saw that there was an update coordinator that did polling based on a given time so I didn't want to reinvent the wheel.

Stale

Kane610

I have rewritten test fixtures in the integration so you would need to adapt to the new testing style, should affect you very much. Two other minor things to correct.

homeassistant/components/unifi/switch.py

homeassistant/components/unifi/hub/hub.py

homeassistant/components/unifi/hub/entity_loader.py

bdowden · 2024-07-13T20:26:51Z

@Kane610 Do you mind helping me out? I can't figure out how to get the traffic rule config added to the entities. I've spent far longer than I care to admit looking for the magic code and I can't find it :(

james-fry · 2024-07-16T21:18:48Z

Really looking forward to this, thank you for making HA better 👍

Me too - great work guys!

Kane610 · 2024-07-21T11:02:23Z

@Kane610 Do you mind helping me out? I can't figure out how to get the traffic rule config added to the entities. I've spent far longer than I care to admit looking for the magic code and I can't find it :(

Sorry, I missed this.

Run only the traffic rule test and do some printing in the entity_loader should give you everything you need

…e appended to support v2 api requests Fix traffic rule unit tests;

tests/components/unifi/conftest.py

Kane610

Almost there

homeassistant/components/unifi/switch.py

tests/components/unifi/test_switch.py

…ffic rule unique ID prefix

bdowden · 2024-07-23T01:14:19Z

Almost there

re: Update Coordinator

This is an interesting situation. Currently it won't reset the polling timer because the update coordinator isn't tied specifically to the traffic rules; instead it uses its timer to call a function on the entity loader to update a group of entities (it just so happens that it's only traffic rules).

I think there are benefits to resetting the timer for any entities that are updated outside of the update coordinator. As it's currently written, though, it's polling for a group of entities. To support resetting for a single item, I think there would have to be a single coordinator per entity; for only traffic rules is fine but there may be performance issues with multiples

tests/components/unifi/conftest.py

tests/components/unifi/test_switch.py

Kane610

Some minor things left to handle

homeassistant/components/unifi/hub/hub.py

homeassistant/components/unifi/switch.py

tests/components/unifi/test_switch.py

…efault code

Kane610 · 2024-07-30T15:26:17Z

Thanks!

Kane610 · 2024-07-31T06:08:01Z

homeassistant/components/unifi/switch.py

+        control_fn=async_traffic_rule_control_fn,
+        device_info_fn=async_unifi_network_device_info_fn,
+        is_on_fn=lambda hub, traffic_rule: traffic_rule.enabled,
+        name_fn=lambda traffic_rule: traffic_rule.description,


As there is a name property in the test data for traffic rules, why did you go for description over name?

home-assistant bot previously requested changes Jun 4, 2024

View reviewed changes

home-assistant bot added cla-needed has-tests integration: unifi new-feature labels Jun 4, 2024

home-assistant bot assigned Kane610 Jun 4, 2024

home-assistant bot added the Quality Scale: platinum label Jun 4, 2024

home-assistant bot added cla-recheck cla-signed and removed cla-recheck cla-needed labels Jun 4, 2024

bdowden mentioned this pull request Jun 4, 2024

Add Traffic Rule switches to UniFi Network #104671

Closed

20 tasks

bdowden marked this pull request as ready for review June 5, 2024 01:12

bdowden requested a review from Kane610 as a code owner June 5, 2024 01:12

bdowden force-pushed the unifi-traffic-rules branch from b467351 to 1be4ee5 Compare June 5, 2024 01:48

bdowden requested a review from home-assistant[bot] June 5, 2024 01:49

Kane610 requested changes Jun 7, 2024

View reviewed changes

home-assistant bot marked this pull request as draft June 7, 2024 08:38

Fetochini approved these changes Jun 7, 2024

View reviewed changes

bdowden marked this pull request as ready for review June 7, 2024 22:47

home-assistant bot requested a review from Kane610 June 7, 2024 22:47

Kane610 requested changes Jun 22, 2024

View reviewed changes

homeassistant/components/unifi/switch.py Show resolved Hide resolved

homeassistant/components/unifi/hub/hub.py Outdated Show resolved Hide resolved

homeassistant/components/unifi/hub/entity_loader.py Show resolved Hide resolved

home-assistant bot marked this pull request as draft June 22, 2024 11:36

Begin updating traffic rule unit tests

46018a0

bdowden force-pushed the unifi-traffic-rules branch from 131070d to 46018a0 Compare July 13, 2024 20:25

For the mock get request, allow for meta and data properties to not b…

45a11fb

…e appended to support v2 api requests Fix traffic rule unit tests;

Kane610 reviewed Jul 21, 2024

View reviewed changes

tests/components/unifi/conftest.py Outdated Show resolved Hide resolved

inspect path to determine json response instead of passing an argument

5ff230c

bdowden requested a review from Kane610 July 22, 2024 01:19

Kane610 requested changes Jul 22, 2024

View reviewed changes

bdowden added 2 commits July 22, 2024 21:01

Remove entity id parameter from tests; remove unused code; rename tra…

ffb4487

…ffic rule unique ID prefix

Remove parameter with default.

cb4b040

Kane610 reviewed Jul 23, 2024

View reviewed changes

tests/components/unifi/conftest.py Outdated Show resolved Hide resolved

Kane610 reviewed Jul 23, 2024

View reviewed changes

tests/components/unifi/test_switch.py Outdated Show resolved Hide resolved

Kane610 requested changes Jul 23, 2024

View reviewed changes

tests/components/unifi/test_switch.py Outdated Show resolved Hide resolved

tests/components/unifi/test_switch.py Outdated Show resolved Hide resolved

More code removal;

7962a03

bdowden marked this pull request as ready for review July 24, 2024 11:34

home-assistant bot requested a review from Kane610 July 24, 2024 11:34

Kane610 requested changes Jul 25, 2024

View reviewed changes

home-assistant bot marked this pull request as draft July 25, 2024 11:45

Rename copy/paste variable; remove commented code; remove duplicate d…

68187b4

…efault code

bdowden marked this pull request as ready for review July 30, 2024 01:31

home-assistant bot requested a review from Kane610 July 30, 2024 01:31

Kane610 approved these changes Jul 30, 2024

View reviewed changes

home-assistant bot added the code-owner-approved label Jul 30, 2024

Kane610 merged commit 18a7d15 into home-assistant:dev Jul 30, 2024
26 checks passed

Kane610 reviewed Jul 31, 2024

View reviewed changes

github-actions bot locked and limited conversation to collaborators Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Traffic Rule switches to UniFi Network #118821

Add Traffic Rule switches to UniFi Network #118821

bdowden commented Jun 4, 2024 •

edited

Loading

home-assistant bot left a comment

home-assistant bot commented Jun 4, 2024

EliSchleifer commented Jun 5, 2024

bdowden commented Jun 5, 2024

EliSchleifer commented Jun 5, 2024 via email

Kane610 left a comment

home-assistant bot commented Jun 7, 2024

bdowden commented Jun 7, 2024

Kane610 left a comment

bdowden commented Jul 13, 2024

james-fry commented Jul 16, 2024

Kane610 commented Jul 21, 2024

Kane610 left a comment

bdowden commented Jul 23, 2024

Kane610 left a comment

Kane610 commented Jul 30, 2024

Kane610 Jul 31, 2024

Add Traffic Rule switches to UniFi Network #118821

Add Traffic Rule switches to UniFi Network #118821

Conversation

bdowden commented Jun 4, 2024 • edited Loading

Breaking change

Proposed change

Type of change

Additional information

Checklist

home-assistant bot left a comment

Choose a reason for hiding this comment

home-assistant bot commented Jun 4, 2024

EliSchleifer commented Jun 5, 2024

bdowden commented Jun 5, 2024

EliSchleifer commented Jun 5, 2024 via email

Kane610 left a comment

Choose a reason for hiding this comment

home-assistant bot commented Jun 7, 2024

bdowden commented Jun 7, 2024

Kane610 left a comment

Choose a reason for hiding this comment

bdowden commented Jul 13, 2024

james-fry commented Jul 16, 2024

Kane610 commented Jul 21, 2024

Kane610 left a comment

Choose a reason for hiding this comment

bdowden commented Jul 23, 2024

Kane610 left a comment

Choose a reason for hiding this comment

Kane610 commented Jul 30, 2024

Kane610 Jul 31, 2024

Choose a reason for hiding this comment

bdowden commented Jun 4, 2024 •

edited

Loading