abmantis / whirlpool-sixth-sense

Whirlpool unofficial API for 6th Sense appliances
MIT License
13 stars 12 forks source link

Add additional whirlpool_android client credentials, add support for shared devices #49

Closed NodeJSmith closed 4 months ago

NodeJSmith commented 5 months ago

This replaces the whirlpool brand client_id and client_secret with updated values that work, as the old ones were no longer valid.

This also adds the methods _get_owned_appliances, _get_shared_appliances, and _add_appliance to AppliancesManager. The fetch_appliances method will call _get_owned_appliances first and then _get_shared_appliances, ensuring that both types are added to the appliance list.

There are some other minor changes to fix failing tests, including the addition of a noop async wait_for_close method to the aiohttp mock and pinning aiohttp to below 3.9.0. The wait_for_close is actually not required any longer with aiohttp pinned to a lower version, but I figure it's better to leave it there to save the next person the headache.

abmantis commented 5 months ago

Hi, thanks for your work! The current client id/secret work for me, but not the ones you used. Can you share the link for the Android app you are using?

I'll try to review the code next week, but can't promise since it will be a busy one.

NodeJSmith commented 5 months ago

@abmantis That's so weird, I only started digging into this because I couldn't authenticate in HA with a username/password I knew was correct. I spent three days trying to figure out the issue before finally being able to authenticate with the new values. I wonder what makes the difference.

The version if 5.9.6, from this link https://www.apkmonk.com/download-app/com.whirlpool.android.wpapp/4_com.whirlpool.android.wpapp_2023-12-13.apk/

NodeJSmith commented 5 months ago

@abmantis AFAICT the app has multiple credentials for a given Region + Brand + Environment combo. Both sets of credentials are in there for US + Whirlpool + Production, for example. I haven't checked yet if it's the same for other brands. I also can't tell if there is any way to determine which ones should be used for a given login, nothing I've seen so far indicates that.

NodeJSmith commented 5 months ago

@abmantis I pushed a new commit, making the credentials a list of dictionaries and updating the auth process to try them out in sequence, returning as soon as one succeeds. If none succeed it will return None as it does currently, indicating that authentication was not successful. I think this should satisfy both situations - I tested with my login and it returned the 401 response for the initial credentials and then 200 for the second, letting me use the CLI to view my appliances. It should work for you, returning 200 on the first try. Let me know if you have any concerns, but I think this is the best solution given the weird situation.

abmantis commented 5 months ago

By the way, clever idea to pool trough the multipe creds! That way we don't need to ask the user anything.

NodeJSmith commented 5 months ago

@abmantis thanks for review, I'll get the changes and tests done in the next day or two 👍

NodeJSmith commented 5 months ago

@abmantis I got the tests added and made the changes you suggested.

There were a couple of things that got changed slightly in order to better facilitate the tests. I moved the generation of the auth url to BackendSelector so that it could be used by both the package and the tests without having to copy the logic into the tests. I also created Enums in the mock BackendSelector for brand and region, since those are required for the headers in AppliancesManager now.

I also split out some more methods, separating out the creation of the authorization body to a separate method in Auth and doing the same with getting the account_id in AppliancesManager. Both of those just made it easier to test the independent functionality in different tests.

I added some test data into the tests folder to use in the tests for AppliancesManager - I used my own data and masked any sensitive values, but I only have a washer/dryer so it's not very robust as far as what it can test. I'd love to get air conditioner and/or over data in there for better coverage.

Lastly, I added the async-timeout to the requirements so that it's still included in the install for Python >= 3.11 and I changed the type hints for dict/list to import from typing, to support older versions of python

NodeJSmith commented 5 months ago

@abmantis Fixed up the tests, I agree that this is a better way of doing this. Accidentally broke the PR, I attempted to rename the branch and that didn't work out well. I think it's back to normal now.

Let me know if you have any other concerns on the PR!

NodeJSmith commented 4 months ago

@abmantis Just wanted to check in on this, see if there was anything else you need from me

abmantis commented 4 months ago

I'm sorry for the delay in moving this forward. Thanks a lot for your work! I will release a new version to pypi now. Let me know if you need help getting it into HA!

abmantis commented 4 months ago

0.18.6 released

abmantis commented 4 months ago

@NodeJSmith do you want me to update the HA component to use this?

NodeJSmith commented 4 months ago

I've started a local branch that has the changes incorporated but I haven't gotten further than that. I'll push the branch tomorrow and start buttoning it up. Happy for any insight or contributions you want to provide though, this will be my first time contributing to HA.

abmantis commented 4 months ago

If you just want to use the newer version of this lib I think you only need to bump the version, like this: https://github.com/home-assistant/core/pull/111439

NodeJSmith commented 4 months ago

Unfortunately bumping it isn't enough, because the integration currently doesn't allow you to choose your brand, it chooses it for you based on region. Because the shared appliances call requires the proper brand header the integration needs to be updated to allow the user to choose their brand during configuration.

I have that done already but trying to install all of the requirements to run the tests took longer than I expected so I didn't get my changes pushed yet. I'll be doing that in the morning hopefully. I'll just need to add a few tests for the changes to the brand in the configuration and then I should be able to create a PR