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(integrations): add support for datev #2223

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

hassan254-prog
Copy link
Collaborator

Describe your changes

  • Added configuration for datev in providers.yaml
  • Added datev to docs-v2.
  • Added datev pages for Supported API groups in mint.json.
  • Added datev SVG file in template-logos folder.
  • Consolidate datev docs pages

Issue ticket number and link

EXT-76

Test

The documentation update was reviewed using npm run docs and verifying on localhost.

@hassan254-prog
Copy link
Collaborator Author

It is much simpler to understand how DATEV Online API handles authentication from the following postman collection

@hassan254-prog hassan254-prog marked this pull request as draft May 30, 2024 09:35
@hassan254-prog hassan254-prog marked this pull request as ready for review May 30, 2024 10:02

## API gotchas

- An `access_token` is valid for 15 minutes, while the `refresh_token` is valid for 11 hours.
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's quite short, I'm not sure we will be able to keep this alive (cc @khaliqgant)

Copy link
Member

Choose a reason for hiding this comment

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

No, we won't, also good reason to keep the refresh logic in the getConnectionCredentials call

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have a way to compute expiration date, store it in DB and use this date to refresh only the one that will expires soon

@@ -466,6 +466,30 @@ contentstack:
authorization_url: https://${connectionConfig.subdomain}.contentstack.com/apps/${connectionConfig.appId}/authorize
token_url: https://${connectionConfig.subdomain}.contentstack.com/apps-api/apps/token
docs: https://docs.nango.dev/integrations/all/contentstack
#Untested configuration. Please reach out if you have a test account that we can use to test it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it nice to have this comment :D
Most likely the customer that will need it will reach out to us when time comes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bodinsamuel, thank you for pointing this out. @bastienbeurier had suggested we add this comment here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh okay, miss communication then let's keep it that way ☺️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should add another pr to revert this☺️

@TBonnin TBonnin enabled auto-merge (squash) June 4, 2024 19:32
@TBonnin TBonnin merged commit 1f25469 into NangoHQ:master Jun 4, 2024
19 of 21 checks passed
authorization_url: https://login.datev.de/openid/authorize
token_url: https://api.datev.de/token
token_request_auth_method: basic
scope_seprator: ' '

Choose a reason for hiding this comment

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

Just asking, but doesn't that typo have any side effects?

Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch, I'll fix that 👌🏻

Choose a reason for hiding this comment

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

There's another one in another section btw. Thanks

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.

5 participants