Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update Sonarr config flow to standardize ports #127625

Merged
merged 5 commits into from
Nov 10, 2024
Merged

Update Sonarr config flow to standardize ports #127625

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab020ba
Update Sonarr config flow to standardize ports
Khabi Oct 5, 2024
d108ef1
Remove debugging
Khabi Oct 5, 2024
30768fb
Merge branch 'dev' into sonarr_config_flow
Khabi Oct 5, 2024
33410c9
Update homeassistant/components/sonarr/config_flow.py
Khabi Oct 22, 2024
0e71a11
Merge branch 'dev' into sonarr_config_flow
frenck Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions homeassistant/components/sonarr/config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,13 @@ async def async_step_user(
errors = {}

if user_input is not None:
# aiopyarr defaults to the service port if one isn't given
# this is counter to standard practice where http = 80
Khabi marked this conversation as resolved.
Show resolved Hide resolved
# and https = 443.
if CONF_URL in user_input:
url = yarl.URL(user_input[CONF_URL])
user_input[CONF_URL] = f"{url.scheme}://{url.host}:{url.port}{url.path}"

if self.entry:
user_input = {**self.entry.data, **user_input}

Expand Down
2 changes: 1 addition & 1 deletion tests/components/sonarr/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
MOCK_REAUTH_INPUT = {CONF_API_KEY: "test-api-key-reauth"}

MOCK_USER_INPUT = {
CONF_URL: "http://192.168.1.189:8989",
CONF_URL: "http://192.168.1.189:8989/",
Copy link
Member

Choose a reason for hiding this comment

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

why the extra slash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My assumption is that at least a few users will probably have this proxied behind some kind of service an may have to have a path component, if no path is specified then yarl just does a / in place of it.

reference: https://github.com/home-assistant/core/pull/127625/files#diff-e4368ac4899b15f980f4e784edaa60285fd6f7b04847463d414d86e17f4a2e87R105

>>> url = yarl.URL("https://my.exmaple.com")
>>> url.path
'/'

CONF_API_KEY: "MOCK_API_KEY",
}
32 changes: 30 additions & 2 deletions tests/components/sonarr/test_config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,34 @@ async def test_cannot_connect(
assert result["errors"] == {"base": "cannot_connect"}


async def test_url_rewrite(
hass: HomeAssistant,
mock_sonarr_config_flow: MagicMock,
mock_setup_entry: None,
) -> None:
"""Test the full manual user flow from start to finish."""
result = await hass.config_entries.flow.async_init(
DOMAIN,
context={CONF_SOURCE: SOURCE_USER},
)

assert result["type"] is FlowResultType.FORM
assert result["step_id"] == "user"

user_input = MOCK_USER_INPUT.copy()
user_input[CONF_URL] = "https://192.168.1.189"
result = await hass.config_entries.flow.async_configure(
result["flow_id"],
user_input=user_input,
)

assert result["type"] is FlowResultType.CREATE_ENTRY
assert result["title"] == "192.168.1.189"

assert result["data"]
assert result["data"][CONF_URL] == "https://192.168.1.189:443/"
Comment on lines +53 to +78
Copy link
Member

Choose a reason for hiding this comment

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

Should we parametrize this test and just throw a lot of scenarios to it so we are sure this keeps working like we expect? (We have seen some YARL issues in the last. weeks, so it would be useful to avoid regressions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did notice when I added this to the radarr component that the tests were a little more rounded out. I'm not opposed to it, but might be a little bit before I have the time to sit down and knock that out.

Copy link
Member

Choose a reason for hiding this comment

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

In the mean time, this is already an improvement, so let's move forward with this one at least.



async def test_invalid_auth(
hass: HomeAssistant, mock_sonarr_config_flow: MagicMock
) -> None:
Expand Down Expand Up @@ -145,7 +173,7 @@ async def test_full_user_flow_implementation(
assert result["title"] == "192.168.1.189"

assert result["data"]
assert result["data"][CONF_URL] == "http://192.168.1.189:8989"
assert result["data"][CONF_URL] == "http://192.168.1.189:8989/"


async def test_full_user_flow_advanced_options(
Expand Down Expand Up @@ -175,7 +203,7 @@ async def test_full_user_flow_advanced_options(
assert result["title"] == "192.168.1.189"

assert result["data"]
assert result["data"][CONF_URL] == "http://192.168.1.189:8989"
assert result["data"][CONF_URL] == "http://192.168.1.189:8989/"
assert result["data"][CONF_VERIFY_SSL]


Expand Down