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

WIP: Added support for Monzo Bank #17218

Closed
wants to merge 76 commits into from
Closed

Conversation

TomAFrench
Copy link

@TomAFrench TomAFrench commented Oct 7, 2018

Description:

This platform adds support for reading account/pot balances and the sum of the day's transactions from the Monzo API.

Currently WIP as I'm waiting on a PR being accepted upstream so I can use the proper PyPi package.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#6585

Example entry for configuration.yaml (if applicable):

monzo:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

homeassistant/components/sensor/monzo.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/monzo.py Outdated Show resolved Hide resolved
def request_app_setup(hass, config, add_entities, config_path,
discovery_info=None):
"""Assist user with configuring the Monzo dev application."""
configurator = hass.components.configurator
Copy link
Member

Choose a reason for hiding this comment

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

The configurator is deprecated. We want to use config entries now.

https://developers.home-assistant.io/docs/en/config_entries_index.html

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll have a look at that and update. Thanks.

Can set up sensors (all sensors are set up. ignoring configuration.yaml)

There exists extraneous code due to experimentation
Stopped explicitly passing hass into MonzoFlowHandler
@TomAFrench
Copy link
Author

We can now fully set up the sensors from within the integrations page of HA and refreshes of the token is properly reflected in the config entry so there's no bad refresh token errors.

I still have an issue with having to refresh the page in order to be redirected properly to Monzo from the link step of the config flow, that's a more general issue with the handling of links within the HA instance though. Writing this though makes me think I should possibly just send the user directly to the Monzo authentication page, it's a bit of an ugly link but it's better than the user having to refresh manually.

Still WIP as the upstream PR not accepted yet and there's still some neatening/fixes to do on things behind the scenes.

data=new_data)


class MonzoEntity(Entity):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this extra class really adds any value.

new_data = {**self.config_entry.data,
'tokens': new_token,
CONF_LAST_SAVED_AT: int(time.time())}
self._hass.config_entries.async_update_entry(
Copy link
Member

Choose a reason for hiding this comment

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

Not allowed to call async methods from a sync context

expires_at=expires_at,
refresh_callback=self.update_config_entry)

self.client = Monzo.from_oauth_session(oauth_client)
Copy link
Member

Choose a reason for hiding this comment

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

Is this doing any I/O ? It's instantiated within an async context so it's not allowed to do any I/O

@TomAFrench
Copy link
Author

Hey, thanks for the feedback. Sorry for the async issues, it's my first time using it. I'll go through and fix those up along with the other points.

Coursework is starting to hit so might take a little bit though.

@balloob
Copy link
Member

balloob commented Nov 27, 2018

This PR seems to have gone stale. Closing it. You can open it again when you finished it.

@vmbruno
Copy link

vmbruno commented Aug 23, 2019

This is such a good idea! @TomAFrench do you have any plans to reopen this PR in the future? I'd love to help get this into the next release 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants