Skip to content

Adding ledger accounts and journal entries#16

Draft
schapman1974 wants to merge 2 commits intoamcintosh:mainfrom
schapman1974:add_accounting_items
Draft

Adding ledger accounts and journal entries#16
schapman1974 wants to merge 2 commits intoamcintosh:mainfrom
schapman1974:add_accounting_items

Conversation

@schapman1974
Copy link
Copy Markdown

@schapman1974 schapman1974 commented Apr 25, 2023

Purpose

The PR adds ledger accounts and a business object to the api in order to support the account/businesses endpoint. The main focus of this is to add additional functionality to the sdk that is not currently exposed.

  • list of ledger accounts get
  • journal entries get / post
  • journal entry details

@schapman1974
Copy link
Copy Markdown
Author

@amcintosh Please review this draft and let me know if this is how you would want to implement ledger_accounts and journal entries from the client.py and the additional api/business.py

@amcintosh
Copy link
Copy Markdown
Owner

Hi @schapman1974!
Thank you so much for the contribution.

Comment thread freshbooks/api/business.py Outdated
@amcintosh
Copy link
Copy Markdown
Owner

I don't think the ledger_accounts is working as expected. The result from most of the accounting endpoints is in the form of:

{
   "response": {
      "result": {
         "client": {
         }
      }
}

But for these new endpoints the result is in the form:

{
   "data": {
   }
}

So when you add the data to the model: https://github.com/amcintosh/freshbooks-python-sdk/pull/16/files#diff-cc8690904fb746159ca8b295604ae5606beb2130b5d626243c7ba954865216a5R88 the single_name is not present in the data and so you get an empty model here: https://github.com/amcintosh/freshbooks-python-sdk/blob/main/freshbooks/models.py#L54.

In the cases of this new set of accounting resources, I don't think we need single_name or list_name at all.

I think changing the __init__ to something like:

def __init__(self, client_config: SimpleNamespace, accounting_path: str,
                     delete_via_update: bool = True, missing_endpoints: Optional[List[str]] = None):

And then update https://github.com/amcintosh/freshbooks-python-sdk/pull/16/files#diff-cc8690904fb746159ca8b295604ae5606beb2130b5d626243c7ba954865216a5R88 to be:

return Result("data", data)

should work.

Something similar would need to be done for the list() call.

@amcintosh
Copy link
Copy Markdown
Owner

I think setting up some tests for the new resource set would help work out the differences.

Comment thread freshbooks/api/business.py Outdated
Comment thread freshbooks/api/business.py Outdated
Comment thread freshbooks/api/business.py
@schapman1974 schapman1974 requested a review from amcintosh May 1, 2023 04:20
Comment thread freshbooks/client.py
@property
def journal_entries(self) -> AccountingResource:
"""FreshBooks items resource with calls to get, list, create, update, delete"""
return AccountingResource(self._client_resource_config(), "journal_entries/journal_entries", "journal_entry", "journal_entries")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks like you can only create journal entries, so missing_endpoints=["list", "get", "update", "delete"] should be added.

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.

2 participants