Add config flow to synology_srm #35412

aerialls · 2020-05-09T13:11:05Z

Breaking change

The Synology SRM integration is now "config flow" compatible. The previous YAML integration is done and the configuration should be done in the UI and not in the YAML file.

Proposed change

Type of change

Migration to the "config flow" workflow. Support addition, updates and removal of entities (even if I would like feedbacks on the latest because I'm not sure of the global workflow).

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

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Update synology_srm component to use the new config flow workflow home-assistant.io#13368

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:

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

PR incoming to update the documentation.

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

Quentame

config_flow.py needs 100% coverage : https://codecov.io/gh/home-assistant/core/compare/9eb1505aa13d8d3f60ab31ca92d9598fc03852e1...50d8a7f19836b4e70ecd47491db27942c8fb5cb9/diff

Did not look at tests.

Nice work 🎉

homeassistant/components/synology_srm/__init__.py

homeassistant/components/synology_srm/router.py

homeassistant/components/synology_srm/config_flow.py

homeassistant/components/synology_srm/device_tracker.py

aerialls · 2020-05-30T19:46:45Z

Thanks @Quentame for your feedback. I've updated the PR will all your remarks and I've added more tests to increase coverage up to 100%!

Quentame · 2020-05-31T10:23:30Z

homeassistant/components/synology_srm/__init__.py

@@ -71,3 +76,9 @@ def get_srm_client_from_user_data(data) -> synology_srm.Client:
        client.http.disable_https_verify()

    return client
+
+
+def fetch_srm_device_id(client: SynologyClient):


Why are you putting this function here since it's used only in the confug_flow ?

I moved the function because it's always mocked in tests to I cannot have 100% coverage in this situation. It's also make sense in the end because I've another function just at the top to generate the client, so both functions handling the SynologyClient are at the same place.

Quentame · 2020-05-31T10:25:43Z

homeassistant/components/synology_srm/config_flow.py

@@ -46,21 +35,18 @@ async def async_step_user(self, user_input=None):
            try:
                client = get_srm_client_from_user_data(user_input)
                device_id = await self.hass.async_add_executor_job(
-                    _login_and_fetch_device_id, client
+                    fetch_srm_device_id, client


What is the device_id actually ?

A MAC adress, serial number, ...

device_id can be compared with a serial number. For instance, it's in the following format synology_irr436p_rt2600ac. It's much easier to fetch it from the API with a simple endpoint and the device ID is also written in the back of the device so it's a good device ID to display to the user IMO.

homeassistant/components/synology_srm/config_flow.py

aerialls · 2020-05-31T10:42:03Z

I pushed the change and rebased into one commit.

Quentame · 2020-05-31T12:00:32Z

Please don't squash commits when someone starts to review your PR, so we can follow the diff better (GitHub does not have the diff tool as GitLab has).

I agree this can be kinda ugly sometimes but no worries your commits will be squashed at merge 😉

Quentame · 2020-05-31T12:03:38Z

So then, you should not force push except if you rebase (not interactively)

aerialls · 2020-05-31T12:24:35Z

Right, my bad! Forgot about that. I only pushed the change on https://github.com/home-assistant/core/pull/35412/files#diff-1262a40b266ddf89f36e596d9c1b0d62R46.

stale · 2020-07-03T04:11:16Z

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.
Thank you for your contributions.

aerialls · 2020-07-03T07:05:26Z

Still waiting for comments 😄

stale · 2020-08-02T17:02:37Z

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.
Thank you for your contributions.

aerialls · 2020-08-03T09:20:35Z

I'm not using the Synology SRM router anymore and it seems nobody wants this PR merged so I will close it.

Quentame · 2020-08-03T11:50:58Z

Sorry @aerialls I was very busy those weeks 😢

@i00 Would you like to create a new PR for this ?
Then we can take care of #31523

Ping me when it's done (also available on Discord, found me in the official HA server https://discord.gg/KxdbHtS)

homeassistant added the cla-signed label May 9, 2020

project-bot bot added this to Needs review in Dev May 9, 2020

probot-home-assistant bot added breaking-change code-quality has-tests integration: synology_srm new-feature by-code-owner labels May 9, 2020

project-bot bot moved this from Needs review to By Code Owner in Dev May 9, 2020

aerialls mentioned this pull request May 9, 2020

Update synology_srm component to use the new config flow workflow home-assistant/home-assistant.io#13368

Closed

7 tasks

aerialls force-pushed the synology_srm_config_flow branch from b03eef2 to 50d8a7f Compare May 11, 2020 17:08

Quentame changed the title ~~Update synology_srm component to use the new config flow workflow~~ Add config flow to synology_srm May 30, 2020

Quentame mentioned this pull request May 30, 2020

Add Synology SRM sensor to get information from router #31523

Closed

20 tasks

Quentame requested changes May 30, 2020

View reviewed changes

Dev automation moved this from By Code Owner to Review in progress May 30, 2020

Quentame removed the code-quality label May 30, 2020

aerialls force-pushed the synology_srm_config_flow branch from afbd2cd to c101b5f Compare May 30, 2020 20:57

Quentame self-requested a review May 31, 2020 10:19

Quentame requested changes May 31, 2020

View reviewed changes

feat(synology-srm): update component to use the new config flow workflow

9377088

aerialls force-pushed the synology_srm_config_flow branch from c101b5f to 9377088 Compare May 31, 2020 10:41

stale bot added the stale label Jul 3, 2020

stale bot removed the stale label Jul 3, 2020

stale bot added the stale label Aug 2, 2020

aerialls closed this Aug 3, 2020

Dev automation moved this from Review in progress to Cancelled Aug 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config flow to synology_srm #35412

Add config flow to synology_srm #35412

aerialls commented May 9, 2020 •

edited by Quentame

Loading

Quentame left a comment

aerialls commented May 30, 2020

Quentame May 31, 2020

aerialls May 31, 2020

Quentame May 31, 2020

aerialls May 31, 2020

aerialls commented May 31, 2020 •

edited

Loading

Quentame commented May 31, 2020

Quentame commented May 31, 2020

aerialls commented May 31, 2020

stale bot commented Jul 3, 2020

aerialls commented Jul 3, 2020

stale bot commented Aug 2, 2020

aerialls commented Aug 3, 2020

Quentame commented Aug 3, 2020

Add config flow to synology_srm #35412

Add config flow to synology_srm #35412

Conversation

aerialls commented May 9, 2020 • edited by Quentame Loading

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Quentame left a comment

Choose a reason for hiding this comment

aerialls commented May 30, 2020

Quentame May 31, 2020

Choose a reason for hiding this comment

aerialls May 31, 2020

Choose a reason for hiding this comment

Quentame May 31, 2020

Choose a reason for hiding this comment

aerialls May 31, 2020

Choose a reason for hiding this comment

aerialls commented May 31, 2020 • edited Loading

Quentame commented May 31, 2020

Quentame commented May 31, 2020

aerialls commented May 31, 2020

stale bot commented Jul 3, 2020

aerialls commented Jul 3, 2020

stale bot commented Aug 2, 2020

aerialls commented Aug 3, 2020

Quentame commented Aug 3, 2020

aerialls commented May 9, 2020 •

edited by Quentame

Loading

Example entry for `configuration.yaml`:

aerialls commented May 31, 2020 •

edited

Loading