Point add redirect uri #101967

fredrike · 2023-10-13T20:52:30Z

Proposed change

Update configflow to allow redirect_uri input as required by the Minut API.

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 Unable to setup authentication with Minut Point #91583
This PR is related to issue: Fix issues with Point #100843,
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 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

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.

frenck

We require a 100% test coverage on our configuration flows; it seems like this PR doesn't provided that.

Could you take a look?

Thanks 👍

../Frenck

home-assistant · 2023-10-15T21:03:39Z

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.

fredrike · 2023-10-15T21:05:02Z

We require a 100% test coverage on our configuration flows; it seems like this PR doesn't provided that.

Could you take a look?

Thanks 👍

../Frenck

Yes, I just saw that I didn't reach full coverage, my latest commit fixes that.

fredrike · 2023-10-17T12:35:12Z

@frenck we do have 100% coverage on config_flow now.

frenck · 2023-10-22T17:27:55Z

homeassistant/components/point/config_flow.py

+        """Return current schema."""
+        return vol.Schema(
+            {
+                vol.Required(CONF_REDIRECT_URI): str,


Wait, why doesn't this implement an OAuth flow?

Yes but not using home-assistant's oauth.

I've been trying to implement using the guidelines but I don't quite understand all parts and have not found an integration that does it by the book either.

The userbase for Point is quite small, perhaps it is better to pull this integration and use hacs instead.

I've been trying to implement using the guidelines but I don't quite understand all parts and have not found an integration that does it by the book either.

There are many integrations doing oauth flows? They all use the same constructs.

The thing is, the redirect URL should not even be a question... With the default flows, it will handle that for the user.

perhaps it is better to pull this integration and use hacs instead.

I do not understand what the latter reasoning has to do with anything? Even as a HACS integration, one should be using the right constructs, right?

There are many integrations doing oauth flows? They all use the same constructs.

Could you please name a few for reference.

perhaps it is better to pull this integration and use hacs instead.

I do not understand what the latter reasoning has to do with anything? Even as a HACS integration, one should be using the right constructs, right?

The reasoning is that I don't feel I have the time to keep up with the high code quality standards. I did another PR #100843 but that was rejected due to 1. I updated the yaml 2. Once I've rewritten the flow to not use yaml the pr should be split up, this is the first stage of the split.

I feel that I've already spent too much time on this, implementing "the right" oauth flow will take me another 6-8 hours which I don't have atm. Either accept this pr (with guidelines on how to make it appropriate) or pull support for Point (the integration is currently broken as Minut have change the oauth flow).

Could you please name a few for reference.

Search for AbstractOAuth2FlowHandler usage in config_flow.py files across our codebase, we currently have 26 integrations implementing it.

Some ones that have been implemented fairly recently (and thus had recent reviews) are Twitch, Fitbit and YouTube.

The reasoning is that I don't feel I have the time to keep up with the high code quality standards.

This has nothing to do with coding standards. 🤷

Either accept this pr (with guidelines on how to make it appropriate) or pull support for Point (the integration is currently broken as Minut have change the oauth flow).

I really want to help, but let me be very clear: These kinds of statements don't help and are not a factor in merging PRs. If anything, they make me not wanting to help out and I don't see how this would be fruitful in any way.

Looking upstream at the minute point docs, they implement a really standard OAuth2 flow, which we fully support out of the box. The amount of code used by this integration can be reduced quite a bit and simplified.

Should not take more than 1-2 hours to fix/adjust I think (including import of existing configuration).

Looking upstream at the minute point docs, they implement a really standard OAuth2 flow, which we fully support out of the box. The amount of code used by this integration can be reduced quite a bit and simplified.

Should not take more than 1-2 hours to fix/adjust I think (including import of existing configuration).

Sounds great, I'll have a look at it!

jacobwilsonnet · 2023-11-18T21:03:45Z

Hi! I've been testing this new version to see if it can work with my account. This version got me a lot further into the setup. However, entities won't show up after the configuration is done.

There is a lot of this unhelpful error:

2023-11-18 15:57:40.861 WARNING (MainThread) [custom_components.point] Device is unavailable
2023-11-18 15:58:40.809 WARNING (MainThread) [custom_components.point] Device is unavailable
2023-11-18 15:59:40.835 WARNING (MainThread) [custom_components.point] Device is unavailable

But when I reload the integration, I do get this slightly more helpful error:

2023-11-18 15:59:51.982 ERROR (MainThread) [homeassistant.config_entries] Error unloading entry [MY ACCOUNT EMAIL] for binary_sensor
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/config_entries.py", line 557, in async_unload
    result = await component.async_unload_entry(hass, self)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/components/binary_sensor/__init__.py", line 175, in async_unload_entry
    return await component.async_unload_entry(entry)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/entity_component.py", line 202, in async_unload_entry
    raise ValueError("Config entry was never loaded!")
ValueError: Config entry was never loaded!
2023-11-18 15:59:51.987 ERROR (MainThread) [homeassistant.config_entries] Error unloading entry [MY ACCOUNT EMAIL] for sensor
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/config_entries.py", line 557, in async_unload
    result = await component.async_unload_entry(hass, self)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/components/sensor/__init__.py", line 136, in async_unload_entry
    return await component.async_unload_entry(entry)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/entity_component.py", line 202, in async_unload_entry
    raise ValueError("Config entry was never loaded!")
ValueError: Config entry was never loaded!

Happy to test any updates or try to help in any way I can.

github-actions · 2024-01-17T21:04:44Z

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.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

home-assistant bot added bugfix cla-signed Hacktoberfest integration: point by-code-owner Quality Scale: gold labels Oct 13, 2023

fredrike marked this pull request as ready for review October 15, 2023 20:26

fredrike added 4 commits October 15, 2023 22:27

close window after auth

ee052bd

updated config_flow

5a5904c

update tests

6546224

full coverage, simplify user_flow

6264b0a

frenck requested changes Oct 15, 2023

View reviewed changes

home-assistant bot marked this pull request as draft October 15, 2023 21:03

fredrike force-pushed the point-redirect-uri branch from 1d23a30 to 6264b0a Compare October 15, 2023 21:03

fredrike marked this pull request as ready for review October 15, 2023 21:04

home-assistant bot requested a review from frenck October 15, 2023 21:04

frenck reviewed Oct 22, 2023

View reviewed changes

frenck marked this pull request as draft October 22, 2023 17:28

fredrike mentioned this pull request Oct 23, 2023

Unable to setup authentication with Minut Point #91583

Closed

github-actions bot added the stale label Jan 17, 2024

github-actions bot closed this Jan 24, 2024

home-assistant bot removed the Hacktoberfest label Jan 24, 2024

github-actions bot locked and limited conversation to collaborators Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Point add redirect uri #101967

Point add redirect uri #101967

fredrike commented Oct 13, 2023 •

edited

Loading

frenck left a comment

home-assistant bot commented Oct 15, 2023

fredrike commented Oct 15, 2023

fredrike commented Oct 17, 2023

frenck Oct 22, 2023

fredrike Oct 22, 2023

frenck Oct 23, 2023

fredrike Oct 23, 2023

frenck Oct 23, 2023 •

edited

Loading

frenck Oct 23, 2023

fredrike Oct 23, 2023

jacobwilsonnet commented Nov 18, 2023

github-actions bot commented Jan 17, 2024

Point add redirect uri #101967

Point add redirect uri #101967

Conversation

fredrike commented Oct 13, 2023 • edited Loading

Proposed change

Type of change

Additional information

Checklist

frenck left a comment

Choose a reason for hiding this comment

home-assistant bot commented Oct 15, 2023

fredrike commented Oct 15, 2023

fredrike commented Oct 17, 2023

frenck Oct 22, 2023

Choose a reason for hiding this comment

fredrike Oct 22, 2023

Choose a reason for hiding this comment

frenck Oct 23, 2023

Choose a reason for hiding this comment

fredrike Oct 23, 2023

Choose a reason for hiding this comment

frenck Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

frenck Oct 23, 2023

Choose a reason for hiding this comment

fredrike Oct 23, 2023

Choose a reason for hiding this comment

jacobwilsonnet commented Nov 18, 2023

github-actions bot commented Jan 17, 2024

fredrike commented Oct 13, 2023 •

edited

Loading

frenck Oct 23, 2023 •

edited

Loading