netmanchris / pyawair

python wrapper for Awair Air Quality Sensor
Apache License 2.0
16 stars 3 forks source link

Current data is suddenly a RAW type #14

Closed danielsjf closed 6 years ago

danielsjf commented 6 years ago

Apparently the API was not working for me anymore the last two days. This is because I suddenly get an error message when calling the current data endpoint.

@deanlyoung is this supposed to happen?

@netmanchris for now I have solved it by adding an aggregate_type argument in the AwairDev class. The default option is now '15-minute'. I'll open a pull request where we can further discuss the changes and if and how you would like to integrate them.

netmanchris commented 6 years ago

@danielsjf Wow. That’s not good. :)

I haven’t seen this error at all and I’ve been using the API quite a bit over the last few days.

Happy to discuss any changes you want. I’m not really using the AwairDev class at this point, so anything that makes sense in your HassIO use-case is going to be the primary driver.

danielsjf commented 6 years ago

Ok so I tried to run the tests. Had to add a bit of code since there were some auth objects missing in the tests.

The results are not very good. From devices, only TestGetUserData, TestGetAllDevices and TestGetDevDetails succeed. All the others fail because of missing permissions for the endpoint:

From data, only TestGet15MinAverage succeeds.

If you are saying that it does work for you, could you then give it a try? It seems that my account does not have the adequate permissions. It is strange since it did work before.

netmanchris commented 6 years ago

@deanlyoung Is there any chance of having an attribute added to the account info API that would let us know the specific accounts privilege level? This would make it a lot easier to return “Upgrade you account” messages instead of failing tests. Just thinking out loud here. :)

@danielsfj Yeah. I’m thinking you’re failing because of a permissions issue in your account. I don’t think the Awair API shad implemented the permissions hack a few weeks ago. Fun of dealing with beta APIs I guess :).

I’ll test it when I get back to a PC and see if it works.

deanlyoung commented 6 years ago

@netmanchris would it make sense as a separate endpoint? Seems like a per endpoint response code might make more sense, as long as it is standardized.

netmanchris commented 6 years ago

@deanlyoung Is what you're suggesting a separate endpoint specifically for gathering the API auth level of the current user? If so, I think that's fine too. Anything that allows us to grab that piece of data to understand what the users API privileges are so that we can then make some better error handling in the event that when a user is trying to access an API they don't have access to, the code could take a look at that level the auth level and let them know they don't have access.

Thinking from an implementation in the library itself this could simply be an attribute on the AwairAuth object that could be checked by the rest of the code. Single API lookup at the time of the creation of the initial auth object would also keep the amount of API calls down.

@danielsjf Thoughts?

danielsjf commented 6 years ago

Yes I agree with @netmanchris here. The FIVE_MIN error is quite helpful for us as developers, but for end users we could give more useful messages. Something along the lines of: It appears you are in the hobbyist tier. This tier does not have access to 5 min data. Please upgrade or use the 15 minute data.

For this, we would need a way to find out what tier they are in, hence the new endpoint.

danielsjf commented 6 years ago

Something else that would be really helpful for the tests is a dummy API key for dummy data. We could use this to implement the tests for all the tiers. Preferably one dummy key for each tier.

deanlyoung commented 6 years ago

@danielsjf @netmanchris got it! Will add it to our backlog 👍🏻

danielsjf commented 6 years ago

@netmanchris let me know how you want to proceed in the meantime. Currently the HA integration is not working anymore, but the pull request should provide a fix. It is a bit annoying that the tests fail, but I think if it doesn't work for us, there is nobody out there for which it will work.

netmanchris commented 6 years ago

@danielsjf Arg! Forgot to merge this yesterday. I’ll get this done tonight and push a new version to Pypi as well.

Important to get it working with what we have right now. We can refactor to improve later as necessary.

Sound good?

danielsjf commented 6 years ago

Yes agreed! Thanks :-)

danielsjf commented 6 years ago

So there is good news and bad news. The good news is that I can again sync values after the upgrade to v8. The bad news is that I have been stupid and referred to the first value in the data array. For the 15 minute value query, this means a value of approximately a month ago. The final value is approximately now, but with roughly a 30 minute delay. I suppose this is what we will have to live with until the Awair API gets updated.

@netmanchris I have a fix and I will again create a pull request. Sorry for the hassle.

netmanchris commented 6 years ago

PR was accepted. Thinking we should probably close this issue as we start to stabilize. Thoughts?

netmanchris commented 6 years ago

BTW Also rev'd python library to 0.0.9 and pushed to Pypi

danielsjf commented 6 years ago

Yes we can close this. The solution we have now is the best one until the API is updated so nothing we can do from our side.