Add lock and unlock functions

[apmechev]

Resolves #119

[DurgNomis-drol]

Please make sure all cli jobs work ☺️

[codecov]

Codecov Report

Merging #190 (eb157e3) into master (c599bde) will decrease coverage by 0.06%.
The diff coverage is 92.91%.

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
- Coverage   94.32%   94.26%   -0.07%     
==========================================
  Files          28       30       +2     
  Lines        1463     1586     +123     
==========================================
+ Hits         1380     1495     +115     
- Misses         83       91       +8     

Flag	Coverage Δ
unittests	94.26% <92.91%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
mytoyota/controller.py	36.28% <25.00%> (-0.66%)	⬇️
mytoyota/models/lock_unlock.py	85.36% <85.36%> (ø)
mytoyota/api.py	100.00% <100.00%> (ø)
mytoyota/client.py	97.91% <100.00%> (+0.31%)	⬆️
mytoyota/const.py	100.00% <100.00%> (ø)
mytoyota/exceptions.py	100.00% <100.00%> (ø)
tests/test_lock_unlock.py	100.00% <100.00%> (ø)
tests/test_myt.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[apmechev]

unfortunately typing.Literal is not avaialbe for pyhton 3.7 so I had to revert some changes. Is it possible to re-run codecov as well, I was missing a .* when matching the path for the lock tests

[apmechev]

On a second thought, I'll also add tests for the get methods for the new data classes ✅

[DurgNomis-drol]

Thanks for contributing 🎉

I can see that you have added other things that are not directly related to lock/unlock feature, please remove these and add these in a separat PR. This will make it easier for me to review your PR.

Have you tested this locally?

[apmechev]

Updated the comments in its own commit. I've tested it on the 2022 Toyota Yaris

[joro75]

How sure are we what happens, if a Toyota car doesn't support remote lock/unlocking?
Is an exception raised? Which exception is raised? Should the exception be caught by the mytoyota library, and be converted to a specific mytoyota exception?

Would it be an idea that we also add a function that determines if the remote lock/unlock is supported by a car with a specific vin?

[apmechev]

@joro75
Updated the code with the two suggestions.

How sure are we what happens, if a Toyota car doesn't support remote lock/unlocking?

Unfortunately, the car I have available supports remote locking, so I have no way to test the API response if it isn't supported. My guess is that some kind of 4XX error will be thrown. Currently, this will be handled like any generic API error, but if you or anyone can test this and let me know the code/response, I can handle this specific case.

Would it be an idea that we also add a function that determines if the remote lock/unlock is supported by a car with a specific vin?

I can't think of a way doing this without sending a lock/unlock request, at least through an API call. Nothing in the decompiled app shows the Totyota API can respond with this capability. The only alternative I can think of is to hardcode the list of cars with this capabilities which then needs to be maintained.

Thoughts?

[joro75]

@apmechev
There are few things I would like to respond to:

Locking a car that doesn't support remote (un)lock

My Toyota probably doesn't support remote (un)lock, as the option is not shown in my MyT app. I will use your code to try to lock it, so we can see if and which error is being raised. Didn't have time yet to do this, but once I do, I will let you know.

Method to determine if remote lock is supported

Probably no specific call is present to determine if remote locking is supported. It could be that it is encoded as an available option in one of the other parameters of the Vehicle-data, but I'm not sure in which parameter it is encoded.
For now probably the best method is to retrieve the lock-status, and if it fails, we can conclude that it is not supported. I do realise that we first have to check which error is returned when we read the status om my car.

After checking your code again, I now realise that the request-id of the lock/unlock call is needed to request the status. This is a further complication, as it thus is probably not possible to request the lock-status without first calling the lock or unlock call. And we can't call lock or unlock, just to check if the remote locking is supported, as this could result in an unwanted lock or unlock operation.

Error when impossible to lock

Did you also test what happens if you try to lock the car, when it is not possible to lock it? For example because a door or the trunk is still open? Or another example is when the key is inside the car?
Does it raise an exception, or is it just silently failing? Does this mean that a program should always check the actual lock-status, after a 'remote lock' is performed, to be able to determine if the lock was successful?

I can imagine that any error-situation during a lock is represented in the status property of VehicleLockUnlockStatusResponse. Do you know, or can you investigate (and document) which other values can be present in that status property?

---

Thanks in advance for all the work you have already done to add this feature. I'm sorry that I'm still asking for additional improvements.

[joro75]

I have added the exception ToyotaActionNotSupported. This exception is raised when the 'Lock' or 'Unlock' action is not supported on a vehicle. (In this case it internally receives a HTTP 403 (FORBIDDEN) response). I have also fixed the unittests for the lock/unlock functions.

[DurgNomis-drol]

I think we are really close now 🎉some minor things below

[apmechev]

In terms of errors when locking, at least in my case, all errors look like the response below. I tested leaving the passenger door open and leaving the keys inside. I would guess that the LU code stands for LockUnlock, so it's strange that both cases returned LU0004. Maybe some vehicles only return a generic lock request error and that's all I'm getting.

{'id': 'c5421d34-25e7-4b16-93a8-c5edefa1d268',
 'status': 'error',
 'errorCode': 'LU0004',
 'requestTimestamp': '2022-11-03T20:11:32.322Z',
 'type': 'controlLock'}

As for doing the polling inside the client.lock/unlock functions, I think that's a good idea. I was debating whether to leave it to the user to do the polling, but I don't imagine the user do anything else than poll for the request status to no longer be inprogress

[joro75]

@apmechev
Thanks for the update on what happens when an error is present during the lock operation.

I suggest that we have to make a decision now, who is responsible for checking if an error is present during the lock/unlock:

1. The mytoyota library
   When the set_lock_vehicle or set_unlock_vehicle is called, the actual (un)lock action is performed, and if that doesn't raise an exception, also the get_lock_status is called. If the get_lock_status returns a successful status, the set_lock_vehicle or set_unlock_vehicle can return.
   The disadvantage of this, is that the set_lock_vehicle is blocking and waiting for the lock to be succesful, which can take several seconds??
   It may also be necessary to call the get_lock_status multiple times, as long as the status is 'inprogress'.

2. The user-program which is using the mytoyota library
   When the set_lock_vehicle or set_unlock_vehicle is called, the actual (un)lock action is performed, and the function directly returns. It will be the task of the user-program to also check the actual result status.
   The disadvantage of this, is that the user-program may be unaware of the specific error code that is returned by the vehicle, as it is an implementation detail for the toyota car.

I do have a preference for the second decision, as it allows the user-program to decide if and how the status is checked. Blocking code in a library is also not a good design.
I do would like to advise that we add an 'is_in_progress', 'is_error' and 'is_completed' property on the VehicleLockUnlockStatusResponse, so the user-program can easily call the property. In the property we can implement the actual error code and status check. THe user-program then doesn't need to implement the toyota specific details of the returned status.

What is your opinion?

[apmechev]

@joro75

The disadvantage of this, is that the set_lock_vehicle is blocking and waiting for the lock to be succesful, which can take several seconds??
It may also be necessary to call the get_lock_status multiple times, as long as the status is 'inprogress'.

This is indeed a downside, the action takes 5-10 seconds to complete, so the library will have to ping multiple times, as well as make the call blocking (so that no lock command is sent while unlocking and vice versa). The second solution above will also allow clients to implement their own back-off strategy as well.

we add an 'is_in_progress', 'is_error' and 'is_completed' property on the VehicleLockUnlockStatusResponse

Sounds like a good plan to me, I was already in the process of implementing an error_code property. I would suggest that we have is_success instead of is_completed, since IMO both success and error requests are completed. I think the verbage used in the Toyota API considers 'completed' as 'successfully completed' which I don't entirely agree with

[joro75]

@apmechev
I agree. Do you add a commit with the is_succes and error_code properties?
In my opionion, we can then accept and finalize this PR. 👍

[joro75]

@apmechev
The actual code implementation is OK for me, however there are some trailing whitespace characters on the empty lines. Please use pre-commit to ensure that our checks for code cleanness and code conformance are running during the git commit.

I hope you understand that I can only approve this PR, if the automated checks and builds are all passing. And there are 4 failing checks at this moment.

Changes have already been incorporated

[joro75]

@apmechev: Many thanks for this improvement! 👍 It took some time to get it finalized and tested, but it is a very good addition to the library!

[apmechev]

@joro75, thanks it was fun!
I wasn't able to squeeze one last commit in, so I created a small separate PR to update the tests

[DurgNomis-drol]

Awesome job guys, and a special thanks to you @joro75 for helping it get over the line 🎉 🎂