Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: migration to v9 api version (tests passing) #320

Merged
merged 13 commits into from
May 30, 2024

Conversation

andreabak
Copy link
Contributor

Additions to #303 with v9 migration fixes and all tests passing.

@AuHau
Copy link
Owner

AuHau commented May 21, 2024

This is so dope! Thanks so much for the work! 🙏 I will review it hopefully tomorrow.

@andreabak
Copy link
Contributor Author

@AuHau I also have a branch where I've upgraded the reports API to v3 (even though v2 is not deprecated yet): andreabak@f6e2d0e
Lmk if you'd like to include this in your repo, maybe in a separate PR (it's stacked on this branch, so it'll have to wait for this to be merged first)

@AuHau AuHau self-requested a review May 22, 2024 05:42
@Doslin
Copy link

Doslin commented May 28, 2024

I'm very happy to see this PR. Is there any progress so far?

@andreabak
Copy link
Contributor Author

I'm very happy to see this PR. Is there any progress so far?

@Doslin The the code is awaiting review from @AuHau and full CI checks haven't run AFAICT. Locally all tests including integration ones are passing. There's also some small additional TODO work on the CLI side for user invites, but that's probably a niche feature anyway.

I've been using my branch in other projects since a week now (mostly entries fetching, filtering and reports) and it seems to be working fine.
While waiting on the PR to move forward, you can already test it out from my forked repo using something like this for pip install / requirements.txt: togglCli @ git+https://github.com/andreabak/toggl-cli@feat/api-v9

Copy link
Owner

@AuHau AuHau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for pushing this to finishing line!
Here are just few initial comments, if you would be so kind to work them in.
I noticed you have added few "TODO: check" todos, so if you could either expend them to what needs to be done or remove them that would be appriciated.

I am currently trying to run the tests locally, which for some reason does not work for me and it seems the same problem is on CI (Authentication problem). I have already identified some other issues as part of that endeavor, that I am fixing up.

Comment on lines +233 to +235
output = [self.entity_cls.deserialize(config=config, **entry) for entry in fetched_entities]
if order == 'desc':
return output[::-1]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Definitely more clean and Pythony

toggl/api/models.py Outdated Show resolved Hide resolved
toggl/api/models.py Outdated Show resolved Hide resolved
toggl/api/models.py Show resolved Hide resolved
toggl/api/models.py Outdated Show resolved Hide resolved
@AuHau
Copy link
Owner

AuHau commented May 29, 2024

I'm very happy to see this PR. Is there any progress so far?

Yes there is, I am slowly reviewing it and testing it locally as the test coverage is unfortunately not great and hence before merging this sort of big change I would like to be sure it won't be broken.

@andreabak
Copy link
Contributor Author

I am currently trying to run the tests locally, which for some reason does not work for me and it seems the same problem is on CI (Authentication problem). I have already identified some other issues as part of that endeavor, that I am fixing up.

@AuHau idk if maybe related, locally I've been able to run the non-premium tests by creating a new account on toggl. And in order to make that work I had to change the workspace ID in both configs to the non-premium one, because the premium config was being used as well. I tried to look into it, but didn't come to a quick conclusion of why it was doing that, and moved on (since despite both configs, only non-premium tests seemed to run anyway). Maybe it was something wrong with my pytest config/cli or maybe there's something that needs to be tweaked in the tests setup code.

@andreabak
Copy link
Contributor Author

@AuHau I've pushed the changes to the invite / organization code (separate commits so they're easier to review before squashing them).
I've tested the invite CLI command and it seems to work properly.

@andreabak andreabak requested a review from AuHau May 29, 2024 07:50
@AuHau
Copy link
Owner

AuHau commented May 29, 2024

Thanks! I will try to have a look on it again tomorrow. I need to figure out why tests do not work for me.

Btw. you say that tests are passing for you. Did you also run the integration tests? (I assume so) How did you go about running those? Did you set Toggl token in env. variable and then they worked?

@andreabak
Copy link
Contributor Author

Btw. you say that tests are passing for you. Did you also run the integration tests? (I assume so) How did you go about running those? Did you set Toggl token in env. variable and then they worked?

@AuHau Yes, I've developed the changes against the integration tests and they all pass (non-premium only tho).
To make them run I had to set TOGGL_API_TOKEN in the env + changing the default_wid in the tests configs to the workspace id of my test account. (I also set log_cli = true and log_cli_level = DEBUG in pytest config and -s cli switch for debugging verbosity, but that shouldn't be relevant)

Copy link
Owner

@AuHau AuHau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice thanks a lot! 🙏

I have finally managed to run the tests and discovered some more bugs. I will merge this PR to my branch and fix that up there.

@AuHau AuHau merged commit 8663c74 into AuHau:feat/new-api-version May 30, 2024
0 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants