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

Export custom fields on Transaction #31

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

Conversation

fernandobrito
Copy link

Description of change

Add support to the custom_fields field on Transaction.
Update the documentation to clarify the trailing days logic.

QUESTIONS BEFORE MERGING

  • custom_fields is a "JSON" (key-value pairs). This is my first time playing with taps. What is the convention for such schemaless fields? Should I export it as a JSON as well, or as a string?

Example:

  • { "id": "tx_id", "custom_fields": { "a": "b" } } vs.
  • { "id": "tx_id", "custom_fields": "{ \"a\": \"b\" }" }

Picking up the PR #19.
To address the issue #17.

Thanks @KAllan357 for the useful comment on #19 (comment). You were spot on right.

Manual QA steps

  • Running it locally on transactions with such fields works:

image

Risks

Rollback steps

  • revert this branch

@cmerrick
Copy link
Contributor

cmerrick commented Jun 8, 2020

Hi @fernandobrito, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@fernandobrito
Copy link
Author

(oh no, please ignore my typo on one of the commit messages. I was planning to squash and merge anyway if that's fine, and then I can fix it)

@cmerrick
Copy link
Contributor

cmerrick commented Jun 8, 2020

You did it @fernandobrito!

Thank you for signing the Singer Contribution License Agreement.

@fernandobrito fernandobrito changed the title Export custom fields Export custom fields on Transaction Jun 8, 2020
@fernandobrito
Copy link
Author

@luandy64 let me know if you have any thoughts on that 😇

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