-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add QuickBooks connector #922
base: main
Are you sure you want to change the base?
Add QuickBooks connector #922
Conversation
…undgametexas/parsons into krausse-quickbooks-connector
I am running all the linting commands and it's still not passing the linting tests. Not sure what to do at this point. |
Success! I had pasted in the API docs from QB Time and they had all these problems with line length and trailing whitespaces that took forever to fix. But there is a vscode extension for flake8 that highlights all the issues. That helped as opposed to reading that long list of lines in the flake8 output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Matthew! So sorry this ended up taking so long to review, we didn't end up communicating well about who was reviewing what. Overall looks great, just a few minor changes need to merge.
""" | ||
Instantiate the QuickBooks class. | ||
|
||
TODO: Add information about the difference between accounting vs time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal, but would be nice not to publish the docs with a 'todo' in them. You can always link to a guide distinguishing the two if you don't want to have to write up the distinction yourself.
token: str | ||
A valid QuickBooks Auth Token. Not required if ``QB_AUTH_TOKEN`` env | ||
variable set. Find instructions to create yours here: | ||
https://developer.intuit.com/app/developer/qbo/docs/develop/authentication-and-authorization/oauth-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the restructured text to markup docstrings to (ie https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#external-links). Here's an example of someone doing this in a Parsons connector: https://github.com/move-coop/parsons/blob/main/parsons/aws/aws_async.py#L30
variable set. Find instructions to create yours here: | ||
https://developer.intuit.com/app/developer/qbo/docs/develop/authentication-and-authorization/oauth-2.0 | ||
|
||
Quickbooks API Documentation: https://tsheetsteam.github.io/api_docs/#introduction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if you can reformat so this is just a link with text "Quickbooks API Documentation" that'd be great - much easier for the docs reader
Only jobcodes with a parent_id set to one of these values will be returned. | ||
Additionally you can use 0 to get only the top-level jobcodes. | ||
|
||
Then get the id of any results with has_children=yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting of this looks a little weird. Have you built the docs and checked that it looks okay?
@requests_mock.Mocker() | ||
def test_get_jobcodes_with_params(self, mock_request): | ||
# Arrange | ||
mock_request.get(requests_mock.ANY, json=mock_jobcodes_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused why this data is here and not in test_quickbooks_data.py
. It's not so much data as to make the tests unreadable, but it is inconsistent.
result = self.qb.get_groups() | ||
|
||
# Assert | ||
self.assertIsInstance(result, Table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests only test whether the result is a Table, but you return an empty table for most of the methods. So you might also want to test the number of rows > 0 for all of these, and/or test the specific data being returned for each test.
This pull request adds a QuickBooks connector to the Parsons library, which allows users to pull time tracking data from QuickBooks Time.
This includes methods for getting data out of the software like users, groups, jobcodes, timesheets, schedules.