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

Add call to account status endpoint before attempting to login to fix authentication. #38

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

bfayers
Copy link

@bfayers bfayers commented Feb 12, 2021

This PR is based on the information form @shenxn here: #37 (comment)

I am also validating the account status in this to avoid unnecessary API calls to Dyson.

libpurecool/dyson.py Outdated Show resolved Hide resolved
libpurecool/dyson.py Outdated Show resolved Hide resolved
libpurecool/dyson.py Show resolved Hide resolved
@googanhiem
Copy link
Contributor

Will need to update the releases.rst just to ensure it passes the Hass PR requirements.

@bfayers
Copy link
Author

bfayers commented Feb 15, 2021

Will need to update the releases.rst just to ensure it passes the Hass PR requirements.

I figured the release/update to the library would be a different PR - I can bounce the numbers to 0.6.5 in this PR if it's preferred.

@homekitter
Copy link

Will need to update the releases.rst just to ensure it passes the Hass PR requirements.

I figured the release/update to the library would be a different PR - I can bounce the numbers to 0.6.5 in this PR if it's preferred.

I suspect that an additional test for 'Must first check account status' should be included within https://github.com/etheralm/libpurecool/blob/master/tests/test_dyson_account.py:TestDysonAccount

@@ -54,7 +54,7 @@ def login(self):
"dyson are using a self signed certificate.")

#Must first check account status
accountstatus = requests.get(f"https://{self._dyson_api_url}/v1/userregistration/userstatus?country={self._country}&email={self._email}")
accountstatus = requests.get(f"https://{self._dyson_api_url}/v1/userregistration/userstatus?country={self._country}&email={self._email}", verify=False)
Copy link

Choose a reason for hiding this comment

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

When I go to this URL I get cloudflare warning but the SSL certificate is valid. Are they redirecting away from cloudflare or something?

@timn
Copy link

timn commented Feb 27, 2021

Can we get this merged and a release being published sometime soon? I think home-assistant users are waiting eagerly for this (home-assistant/core#46400). Thank you!

@bfayers
Copy link
Author

bfayers commented Feb 27, 2021

Can we get this merged and a release being published sometime soon? I think home-assistant users are waiting eagerly for this (home-assistant/core#46400). Thank you!

Hopefully! I need to get the tests to pass but once I've done that it shouldn't be too difficult - however I wouldn't expect a merge here -- I'll probably end up having to publish it under another name on pypi (any ideas?) for now since I am willing to at least attempt to keep the library functional.

@homekitter
Copy link

@bfayers
Thank you for all the great work that you've done.
Is the merge blocked by the coveralls check?
Have you approached @etheralm to taking over the project?

@bfayers
Copy link
Author

bfayers commented Mar 4, 2021

@bfayers
Thank you for all the great work that you've done.
Is the merge blocked by the coveralls check?

Possibly - that said I don't think etheralm is particularly interested in merging & maintaining anymore

Have you approached @etheralm to taking over the project?

No, though I may just publish my fork to pypi and PR hass to move over. :/

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

Successfully merging this pull request may close these issues.

8 participants