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

Financials API initial commit #352

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Financials API initial commit #352

wants to merge 4 commits into from

Conversation

teamoo
Copy link
Contributor

@teamoo teamoo commented Nov 7, 2019

This file introduces the financials API endpoint. It is currently only available on the Graph API beta version or via the BusinessCentral API endpoint.

One additional dependency is created by this module (inflection).

Many endpoints are currently only available on the business central API endpoint. These are not yet fully implemented, whereas all endpoints accessible via Graph API are already fully implemented, but not all CRUD operations are yet tested for all entities.

Navigation properties are always loaded in the current implementation.

The object can be called like this:

scopes = ['Financials.ReadWrite.All']

credentials = (client_id, client_secret)

protocol = MSGraphProtocol(api_version='beta')
credentials = (client_id, client_secret)

account = Account(credentials, protocol=protocol)

if not account.is_authenticated:
    account.authenticate(scopes=scopes)

financials = Financials(parent=account)
company = financials.get_companies()

Currently, you cannot grant application access, only delegated access.
If using the BusinessCentral API endpoint, make sure to grant access not for "Microsoft Graph", but for "Dynamics 365 Business Central".

The Company object contains access to most objects as they are company-specific.

This file introduces the financials API endpoint. It is currently only available on the Graph API beta version or via the BusinessCentral API endpoint.

One additional dependency is created by this module (inflection).

Many endpoints are currently only available on the business central API endpoint. These are not yet fully implemented, whereas all endpoints accessible via Graph API are already fully implemented, but not all CRUD operations are yet tested for all entities.

Navigation properties are always loaded in the current implementation.
@alejcas
Copy link
Member

alejcas commented Nov 7, 2019

This is absolutely great @teamoo!

Let me go through the code and I'll report back

@teamoo
Copy link
Contributor Author

teamoo commented Nov 8, 2019

Sure, feel free to change things. I took a slightly different approach than you. In my opinion, you have a lot of boilerplate in the current classes, a lot of repetition in the code...event thought about using data classes (python>=3.7).

Copy link
Member

@alejcas alejcas left a comment

Choose a reason for hiding this comment

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

Hi @teamoo I've reviewed the code and wrote some comments,

Just a few comments:

  1. There is no need for the inflection dependency. This is already covered by the stringcase dependency. The casing functions are also incorporated into the protocols (see the code). And the ApiComponent gives each class a self._cc ('cc' means convert case) method that is an alias for self.protocol.convert_case.
from stringcase import snake_case, pascal_case

print(snake_case('myUnitDimension'))
# results in 'my_unit_dimension'

print(camelcase('my_unit_dimension'))
# results in 'myUnitDimension'
  1. Although I like the approach of auto converting everything received from the api into a property like you did, it can generate problems. If you receive a keyword that is a reserved keyword in python this will cause a SyntaxError. For example if you try to add 'from' or 'class' to a class it will fail.
    This can be overcome by checking if the keyword is a reserved keyword and set the keyword with a modification instead.

Also, there are some properties that need to be set more like an instance of another class (such as Attachments for messages and others that need some adjustments (like body on messages).

Maybe this approach can be set to the other classes in the library. Will have to investigate a little more.

  1. You should accept a Query instance when getting collection of items.
    Simply update the data of the query with q.as_params()

Comment on lines +34 to +35
if kwargs.pop('main_resource', None):
main_resource = kwargs.pop('main_resource', None)
Copy link
Member

Choose a reason for hiding this comment

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

If you pop the main_resource then the next call to pop inside the if will always return None.

O365/financials.py Outdated Show resolved Hide resolved
O365/financials.py Outdated Show resolved Hide resolved
O365/financials.py Show resolved Hide resolved
O365/financials.py Outdated Show resolved Hide resolved
@alejcas
Copy link
Member

alejcas commented Nov 21, 2019

hi @teamoo can you change the code about the inflection dependency? There's any reason to maintain this dependency?

This way I can merge the PR.

@teamoo
Copy link
Contributor Author

teamoo commented Nov 23, 2019

Hi @janscas,

yes sure. Sorry I was busy with projects, will do that next week.

@teamoo
Copy link
Contributor Author

teamoo commented Dec 1, 2019

Sorry again, Deadline on a client project and time off next week. I will Update it in December.

@jeroenhabets
Copy link

@teamoo it's been rather quiet on your O365 Financials API pull request. Any change you'd be able to wrap it up soon?

@teamoo
Copy link
Contributor Author

teamoo commented Jun 24, 2020

Hi @jeroenhabets ,

sorry, no chance to update it at the moment. If it is a problem for you guys, feel free to discard it.

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