Raise authentication exception when authentication fails and add delivery details API calls

corneyl commented 3 years ago

First, thanks for making this package! I'm trying to see if I can use it for building a Home Assistant integration. But for that I need a way to check if authentication failed, therefore this PR.

It adds an authentication check to PicnicApiSession.login() and PicnicAPI._get()/PicnicAPI._post() and raises an exception if authentication failed. I've also added a test, but they're written in an unusual way as they are doing actual requests to the Picnic API. Are they not meant as unit tests? As far as I've seen it's then more common to mock e.g. the Session class.

Let me know if anything needs changing!

corneyl commented 3 years ago

I guess the checks are failing because secrets.PICNIC_COUNTRY_CODE is not set?

MikeBrink commented 3 years ago

Hi,

Glad you are enjoying the wrapper and thanks for the work! The secrets should all be set, but I'll look into that tomorrow. I agree with you that the unit tests are not optimal, but is was a quick solution, and I am also really interested for when the API fails. This tells me that picnic has updated their API (I run the tests periodically).

If you are working on an integration for HA, please have a look at https://github.com/MikeBrink/home-assistant-picnic. If you want I can add you as a collaborator, since I do not yet have the time to sink the time into it that is needed. Wrapping up my masters research at the moment.

MikeBrink commented 3 years ago

Hi,

I've looked into it today, but all secrets are set and spelled correctly. I tried to reproduce the error locally with act, but had no success. Do you have any ideas? The only thing I can think of is that the PR is dependent on your secrets being set? Would be a clumsy implementation I'd think, but I can't come up with another reason. I am eager to hear your ideas on this and will wait with merging.

corneyl commented 3 years ago

Hi Mike,

I'll have a look if I can find the culprit, but it can take a few days though!

And I only found out about your custom Picnic component after I was well underway, and I've also added a config flow e.g., which is now the preferred way for HA to set-up integrations, so I'll continue working on that as well and make a PR for HA core when it's stable.

corneyl commented 3 years ago

I could reproduce the test error locally by setting the country code constant to an empty string. I guess it has to be related to how the .env file is created or how the secrets are loaded.

To fix things I've done the following:

Moved the existing tests to another directory integration_tests. Essentially tests that do real requests are not unit tests because the unit-under-test is not isolated, but are integration tests because they test de integration between client/api. One can still run them manually, but as the API is versioned it should not change (then also the Picnic app breaks).
Added new tests to the tests directory that don't do real requests, but use mocks for the Session. They only test the behaviour of the methods in this repo.
Added 2 extra API endpoints to get the delivery position/scenario, these gives information when the delivery is in progress about where the delivery truck is, who the driver is and an ETA with smaller window (20m).

Let me know what you think!

corneyl commented 3 years ago

Thanks a lot Mike!

FYI, I've made a Picnic integration PR for HA core: https://github.com/home-assistant/core/pull/47507

MikeBrink commented 3 years ago

Hi,

No thank you for your contribution! I'll update my repo for my HA integration + the forum post to point to yours. I didn't have the time to maintain and update it sadly.

MikeBrink / python-picnic-api

Raise authentication exception when authentication fails and add delivery details API calls #9