[WIP] Implement missing API functions #8
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
==========================================
- Coverage 84.49% 75.18% -9.32%
==========================================
Files 6 6
Lines 729 826 +97
==========================================
+ Hits 616 621 +5
- Misses 113 205 +92
Continue to review full report at Codecov.
|
|
@philipflohr thanks for your work on this PR. I'm gonna make a short review of the code now and add a few comments. One thing that would be really important though are tests. The most useful and thorough tests we have right now are in test_mock_api.py, where the response from the server for certain inputs is mocked. Would you be willing to add tests for the API methods you have implemented? |
src/personio_py/models.py
Outdated
| def set_client(self, client: 'Personio'): | ||
| self._client = client | ||
|
|
There was a problem hiding this comment.
I'm not sure if there is a need for a setter here, this looks like java code to me ^^
We can change create_attendances() so that this is not needed
There was a problem hiding this comment.
Solution proposal: I could change the objects client to be public accessible
src/personio_py/models.py
Outdated
| def _create(self, client: 'Personio'): | ||
| pass | ||
| self._client.create_attendances([self]) |
There was a problem hiding this comment.
I left this part unfinished... client should have been an optional parameter and the function should choose between client and self._client or raise an error if both are None. I should probably fix this myself ^^
| Note: Attendances are created sequentially. This function stops on first error. | ||
| All attendance records before the error will be created, all records after the error will be skipped. |
There was a problem hiding this comment.
were you able to verify that this works? The documentation does not indicate what happens when there are errors in only some of the attendance records.
There was a problem hiding this comment.
This is not documented but I verified this using a personio test account
There was a problem hiding this comment.
in this case, maybe we should tell the user which attendances have been created and which have failed? We could return a dictionary like
{
'success': [list of attendance objects that were created],
'failure': [list of attendance objects that were not created],
}
There was a problem hiding this comment.
I only did a very quick test of this behavior, but IIRC the response does not contain the information where the error happened.
Raises error if no client is available
Changed Absence definition: half_day_start, half_day_end are now bool Implemented Absence.to_body_params Added test
…query Added tests for absences
…nd authentication successful
|
@klamann are the test provided for absences ok like this? |
|
@klamann can you give any update on this or do you have an idea when you'll have a little time for it? |
|
Hey @philipflohr, sorry for letting you wait. I plan to review your changes on the weekend, maybe earlier if I get the chance. |
|
There are still test missing for attendances. Could have a quick look into the absences tests? If they are ok I'll provide similar tests for attendances |
|
I had a look at the tests; what I like is that there are many tests that work with a live Personio instance and where many edge cases are covered. What I'm missing however are tests that can run as part of the CI pipeline without a live Personio instance, like those defined in Also, I'm starting to worry that this PR is getting out of hand, there's too much structural change in here (e.g. the refactoring of the tests in multiple files) and it's becoming increasingly more difficult to review. I think it would be wise to split this into several smaller PRs that are easier to edit and review. |
|
I'm closing this PR in favor of multiple smaller PRs. |
As written in #7 my work is not yet finished and is not ready to be pulled.
Besides the missing functionality there are no tests.
Also I'm actually not sure if the remote_query_id flag should default to True or False - I chose False for a simple reason: without explicitly allowing a query with this default value it's only possible to update an attendance record which already exists on the Personio servers.