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.

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

Store access token in entry for Fyta #116260

Merged
merged 12 commits into from Apr 29, 2024

Conversation

dontinelli
Copy link
Contributor

Proposed change

Save the access_token and its expiration received from the API in ConfigEntry an load it in async_setup_entry to reduce unnecessary login-calls to the 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 #
  • 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:

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

You can use a minor version config entry migration for this. This way you create a central place where you update the config entry.

@dontinelli
Copy link
Contributor Author

You can use a minor version config entry migration for this. This way you create a central place where you update the config entry.

Thank you for the hint, done. Is it then save to remove the following code (respectively change it to expiration = entry.data["expiration"] or should I leave it to catch possible errors?

    expiration: datetime | None = (
        datetime.fromisoformat(entry.data.get("expiration", "")).astimezone(
            ZoneInfo(tz)
        )
        if "expiration" in entry.data
        else None
    )

@joostlek
Copy link
Member

Minor version, not a major version. Minor versions keep working after a restore of a version before this. Major versions don't, and this change doesn't affect the working on old versions

@joostlek
Copy link
Member

I think you can always expect an expiration after this migration

@dontinelli
Copy link
Contributor Author

I think you can always expect an expiration after this migration

Yes, sure. But this was not my question. Expiration is catched in coordinator. Question was, if I can assume correct expiration value in config entry or if I should leave the saveguard there.

@joostlek
Copy link
Member

joostlek commented Apr 26, 2024

What safeguard are you exactly talking about? In the code of your previous comment I see 2 about expiration not being in entry data, maybe I'm missing it

@dontinelli
Copy link
Contributor Author

What safeguard are you exactly talking about? In the code of your previous comment I see 2 about expiration not being in entry data, maybe I'm missing it
I'm asking, if this is still needed:

    expiration: datetime | None = (
        datetime.fromisoformat(entry.data.get("expiration", "")).astimezone(
            ZoneInfo(tz)
        )
        if "expiration" in entry.data
        else None
    )

Or if I should just use expiration = entry.data["expiration"]

@joostlek
Copy link
Member

But that's what I meant, after the migration is run, you can expect expiration to be there in entry.data

@dontinelli dontinelli marked this pull request as ready for review April 27, 2024 10:55
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

I'm wondering 2 things. What's the value of expiration? The Epoch? Isn't that always UTC?

How long is a token valid? Can we get a new one without username/password?

@dontinelli
Copy link
Contributor Author

dontinelli commented Apr 27, 2024

I'm wondering 2 things. What's the value of expiration? The Epoch? Isn't that always UTC?

The API returns a validity in seconds (cf. below), which is then added to the current time in fyta_cli: datetime.now() + timedelta(seconds=int(json_response["expires_in"]))
According to my understanding, now() returns local time and not UTC.

How long is a token valid? Can we get a new one without username/password?

Token is valid 5184000 seconds, i.e. 60 days. According to my understanding the renewal requires username and password.

@joostlek
Copy link
Member

Ah, in that case I agree that this is a good change.

homeassistant/components/fyta/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/fyta/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/fyta/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/fyta/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/fyta/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/fyta/coordinator.py Outdated Show resolved Hide resolved
tests/components/fyta/test_config_flow.py Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft April 27, 2024 13:37
@home-assistant
Copy link

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.

@dontinelli dontinelli marked this pull request as ready for review April 28, 2024 11:50
@home-assistant home-assistant bot requested a review from joostlek April 28, 2024 11:50
dontinelli and others added 3 commits April 29, 2024 13:15
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
homeassistant/components/fyta/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/fyta/coordinator.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft April 29, 2024 13:27
dontinelli and others added 2 commits April 29, 2024 15:27
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
@joostlek joostlek changed the title Save access_token and expiration date in ConfigEntry Store access token in entry for Fyta Apr 29, 2024
@joostlek joostlek marked this pull request as ready for review April 29, 2024 13:30
@home-assistant home-assistant bot requested a review from joostlek April 29, 2024 13:30
@joostlek
Copy link
Member

I think it would make sense to include this in 2024.5 because it will prevent doing a request every time HA boots/the integration reloads. I only see benefit in pulling this into the beta, to avoid unnecessary calls.

@joostlek joostlek added this to the 2024.5.0 milestone Apr 29, 2024
@joostlek joostlek merged commit 180e178 into home-assistant:dev Apr 29, 2024
24 checks passed
frenck pushed a commit that referenced this pull request Apr 29, 2024
* save access_token and expiration date in ConfigEntry

* add MINOR_VERSION and async_migrate_entry

* shorten reading of expiration from config entry

* add additional consts and test for config entry migration

* Update homeassistant/components/fyta/coordinator.py

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>

* Update homeassistant/components/fyta/__init__.py

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>

* omit check for datetime data type

* Update homeassistant/components/fyta/__init__.py

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>

* Update homeassistant/components/fyta/coordinator.py

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>

---------

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
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.

Please address the comments in a new PR. Thanks!

homeassistant/components/fyta/__init__.py Show resolved Hide resolved
homeassistant/components/fyta/__init__.py Show resolved Hide resolved
@dontinelli
Copy link
Contributor Author

Please address the comments in a new PR. Thanks!

Just opened a respective PR #116433.

@dontinelli dontinelli deleted the save-access-token branch April 30, 2024 06:00
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants