-
Notifications
You must be signed in to change notification settings - Fork 47
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
TDL-14058: Update SDK and api version #105
Conversation
… update-sdk-and-api-version
'subscriptions':{ | ||
'payment_settings', | ||
'default_tax_rates', | ||
'pending_update', | ||
'automatic_tax', | ||
}, | ||
'products':{ | ||
'skus', |
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 response of the latest API(2020-08-27) version does not return skus
field.
@@ -55,12 +48,34 @@ | |||
'plans': set(), | |||
'invoice_line_items': { | |||
'tax_rates', | |||
'unique_id', |
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 response of the latest API(2020-08-27) version does not return the unique_id
field. Also, the updated
field is not available in the stripe document. So, removed it from the KNOWN_MISSING_FIELDS map.
'reversals', | ||
'reversed', | ||
}, | ||
'payouts':set(), |
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.
As per the API doc, the newest API version that is 2020-08-27, doesn't return these missing fields for the payouts object, hence removing the fields from the dictionary
@@ -224,10 +225,10 @@ def list_all_object(stream, max_limit: int = 100, get_invoice_lines: bool = Fals | |||
if dict_obj.get('data'): | |||
for obj in dict_obj['data']: | |||
|
|||
if obj['sources']: | |||
if obj.get('sources'): |
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.
obj['sources']
was throwing KeyError in the latest API version.
@@ -145,9 +225,7 @@ | |||
'invoices': {'updated'}, | |||
'plans': {'updated'}, | |||
'invoice_line_items': { | |||
'updated', |
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 updated
and subscription_item
field is not added by tap. So, removed it from the FIELDS_ADDED_BY_TAP
map.
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 believe this is a necessary field to get updates. Has the sync strategy for getting updates for a child stream changed?
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.
No, the sync strategy is not changed. But, I verified that the above fields are not available in the records by running sync mode for the invoice_line_items
stream. Tap is not adding the above fields in the records of the invoice_line_items
stream. Sync for the child stream works based on parent records and parent records would have an updated
field. That's why the above fields are removed from the FIELDS_ADDED_BY_TAP
map.
@@ -371,10 +382,13 @@ def reduce_foreign_keys(rec, stream_name): | |||
return rec | |||
|
|||
|
|||
def paginate(sdk_obj, filter_key, start_date, end_date, limit=100): | |||
def paginate(sdk_obj, filter_key, start_date, end_date, stream_name, limit=100): |
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.
Explain why you have to include the stream_name as a new parameter?
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 expand
parameter is passed in SDK request in the function and the required values are in the STREAM_TO_EXPAND_FIELDS map. So stream_name
is used to retrieve expand's a value from that map.
tap-stripe/tap_stripe/__init__.py
Lines 389 to 391 in 1223197
# Some fields are not available by default with latest API version so | |
# retrieve it by passing expand paramater in SDK object | |
expand=STREAM_TO_EXPAND_FIELDS.get(stream_name, []), |
tap_stripe/__init__.py
Outdated
# Invoice was bookmarking on `date` but in latest API version, that field is deprecated and replication key changed to `created` | ||
# kept `date` in bookmarking as it has to respect bookmark of active connection too | ||
if stream_name == 'invoices': | ||
stream_bookmark = singer.get_bookmark(Context.state, stream_name, 'date') or \ | ||
int(utils.strptime_to_utc(Context.config["start_date"]).timestamp()) |
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.
In the comment it says the bookmark has changed to Created date, whereas in the code you have added start_date? Is start_date similar to created_date? Please explain
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 replication key is changed to created
but date
was used previously so we are trying to get a bookmark from the state using the date
key first and if not found in a state then use start_date as a bookmark.
tap-stripe/tap_stripe/__init__.py
Lines 425 to 427 in 1223197
if stream_name == 'invoices': | |
stream_bookmark = singer.get_bookmark(Context.state, stream_name, 'date') or \ | |
int(utils.strptime_to_utc(Context.config["start_date"]).timestamp()) |
The previous code was also doing that like looking into state first and not found then use start_date.(Deleted line above)
tap_stripe/__init__.py
Outdated
if sub_stream_name == "invoice_line_items": | ||
sub_stream_bookmark = singer.get_bookmark(Context.state, sub_stream_name, 'date') \ | ||
or int(utils.strptime_to_utc(Context.config["start_date"]).timestamp()) | ||
else: |
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.
In the comment it is mentioned as Created, but in the code it is start_date. Looks the similar code is replicated. Can we write a function which accept these as parameters and set the appropriate bookmark?
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.
Yes, I added get_bookmark_for_stream()
function to retrive bookmark and similarly added write_bookmark_for_stream()
for writing bookmark.
# Bookmark key changed for `invoices` from `date` to `created` due to latest API change | ||
# but for `invoices` stream, the `created` field have integer type(epoch format) from starting so | ||
# converting `updated` to epoch for comparison. | ||
if stream == "invoices": | ||
updated[stream]['messages'] += [m for m in batch['messages'] | ||
if self.dt_to_ts(m['data'].get("updated")) != m['data'].get(bookmark_key)] | ||
else: | ||
updated[stream]['messages'] += [m for m in batch['messages'] | ||
if m['data'].get("updated") != m['data'].get(bookmark_key)] |
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 little bit confused here. In the previous section we are converting updated to integer type to compare with created which made sense. Why are we here doing the comparison with Updated and Updated?
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.
We are doing similar things in the previous section and above section. The only difference is in the previous section we are checking with the ==
operator to get records that are created. While in the above section we are checking with the !=
operator to get records that are updated.
# Bookmark key changed for `invoices` from `date` to `created` due to latest API change | ||
# but for `invoices` stream, the `created` field have integer type(epoch format) from starting so | ||
# converting `updated` to epoch for comparison. | ||
if stream == "invoices": | ||
created[stream]['messages'] += [m for m in batch['messages'] | ||
if self.dt_to_ts(m['data'].get("updated")) == m['data'].get(bookmark_key)] | ||
else: | ||
created[stream]['messages'] += [m for m in batch['messages'] | ||
if m['data'].get("updated") == m['data'].get(bookmark_key)] | ||
|
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 don't think this is a best practice for bookmarking. Consistency in the data types for bookmarks is what we want. This may be worth discussing with a dev, if it's unclear.
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 bookmark is written in epoch format for all streams like below in STATE.
{"type": "STATE", "value": {"bookmarks": {"charges": {"created": 1642661551}, "charges_events": {"updates_created": 1609459200}, "events": {"created": 1642661567}, "customers": {"created": 1642661570}, "customers_events": {"updates_created": 1609459200}, "plans": {"created": 1642661586}, "plans_events": {"updates_created": 1609459200}, "invoices": {"date": 1642661602}, "invoice_line_items": {"date": 1642661602}, "invoices_events": {"updates_created": 1609459200}, "invoice_items": {"date": 1642661617}, "invoice_items_events": {"updates_created": 1609459200}, "transfers": {"created": 1642661632}, "transfers_events": {"updates_created": 1609459200}, "coupons": {"created": 1642661647}, "coupons_events": {"updates_created": 1609459200}, "subscriptions": {"created": 1642661662}, "subscription_items": {"created": 1642661662}, "subscriptions_events": {"updates_created": 1609459200}, "balance_transactions": {"created": 1642661677}, "payouts": {"created": 1642661680}, "payout_transactions": {"created": 1642661680}, "payouts_events": {"updates_created": 1609459200}, "disputes": {"created": 1642661696}, "disputes_events": {"updates_created": 1609459200}, "products": {"created": 1642661711}, "products_events": {"updates_created": 1609459200}}}}
While the replication key of invoices is emitted in epoch format and date-time for other streams. (In RECORD).
We did not change the data-type to avoid column split because the created field was already there with an integer type. and handled that in a test case.
@@ -145,9 +225,7 @@ | |||
'invoices': {'updated'}, | |||
'plans': {'updated'}, | |||
'invoice_line_items': { | |||
'updated', |
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 believe this is a necessary field to get updates. Has the sync strategy for getting updates for a child stream changed?
sources = obj['sources']['data'] | ||
obj['sources'] = sources | ||
if obj['subscriptions']: | ||
if obj.get('subscriptions'): |
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.
good catch
* initial commit * added back for all streams * resolved comments * fixed cci errors * fixed typo in comment * updated comment * fixed typo in function * fixed failures * fixed error * resolved failure for discount * updated discount schema * added bugtracker
* Initial commit * Updated util.py �[D�[D�[D�[D�[D�[D�[D�[DAPI version to latest in util.py * Added support of tax_ids field * Updated discount schema * Updated datatype of percent_off field.
* Initial commit * Added back schema_keys * Uncommented expected assertion * Removed skus field from schema
…o pass (#112) * Initial commit * Updated schema file * Removed price field from schema * Removed tax_rates from schema * Removed extra fields from schema * Updated schema file * Added metadata in schema * Added invoice_line_items in KNOWN_FAILING_FIELDS * Updated price value�[D�[D�[D�[D�[Drecord value * Updated all_field test case * Merged changes * Uncommented code * Updated code comment
* initial commit with subscription_items stream * reverted back to all streams * resolved pylint * testing for subscription_items only * resolved error * added back for all streams * resolved cci errors * added commnets * added automatic fields for subscription items * removed updated from the schema * removed empty line and updated base * resolved comments * chahnged updated * reverted changes * reverted changes * removed updated from subscription_items * reverted a type change Co-authored-by: prijendev <[email protected]>
* Initial commit for dict to stripe object conversion * Resolved pylint error * Updated test cases * Added condtion to check dict type
* TDL-14058: Update SDK and api version (#105) * Updated SDK and API version * Added expand parameter for more streams * Updated replication key for invoices * added backward compability for invoice bookmark * Added unit test and code coverage report in circleci * Resolved unit test failure * Updated stripe SDK to latest version * Running all test for testing * updated stripe version in config.yml * Running all test for testing * Running all test for testing * Running all test for testing * Updated base class of tap_tester to handle replication key change of invoices * Reverted back config.yml to run dependant tests * Reverted back config.yml to run dependant tests * Updated all_field test case * Added back payout stream * Added comments in test case * Fixed key in bookmark for invoice_line_items * Fixed key in bookmark for invoice_line_items in get_bookmark * Fixed charge stream for all_field test case * Fixed payouts stream for all_field test case * Updated amount value * Resolved circleci error * Fiexed all_field test case�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[�[�[3~�[�[C�[�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[xed invoice_line_item failure in all_field test case * Fix the build_daily workflow definition * Resolved review comment * Updated all_field test case * Updated test case for customer and invoice_line_item stream * Updated util.py * Removed fields from missing fields * fixed subscription_items stream * Resolved review comment and added function for get and write bookmark * Updated code comment * Tdl 5894 update invoices stream (#115) * initial commit * added back for all streams * resolved comments * fixed cci errors * fixed typo in comment * updated comment * fixed typo in function * fixed failures * fixed error * resolved failure for discount * updated discount schema * added bugtracker * Tdl 6026 Modify 'customer' stream on 'test_all_fields.py' to pass (#109) * Initial commit * Updated util.py �[D�[D�[D�[D�[D�[D�[D�[DAPI version to latest in util.py * Added support of tax_ids field * Updated discount schema * Updated datatype of percent_off field. * Tdl 6597 Modify 'product' stream on 'test_all_fields.py' to pass (#108) * Initial commit * Added back schema_keys * Uncommented expected assertion * Removed skus field from schema * TDL-6587 Modify 'invoice_line_items' stream on 'test_all_fields.py' to pass (#112) * Initial commit * Updated schema file * Removed price field from schema * Removed tax_rates from schema * Removed extra fields from schema * Updated schema file * Added metadata in schema * Added invoice_line_items in KNOWN_FAILING_FIELDS * Updated price value�[D�[D�[D�[D�[Drecord value * Updated all_field test case * Merged changes * Uncommented code * Updated code comment * TDL-5992 modify subscription items stream (#114) * initial commit with subscription_items stream * reverted back to all streams * resolved pylint * testing for subscription_items only * resolved error * added back for all streams * resolved cci errors * added commnets * added automatic fields for subscription items * removed updated from the schema * removed empty line and updated base * resolved comments * chahnged updated * reverted changes * reverted changes * removed updated from subscription_items * reverted a type change Co-authored-by: prijendev <[email protected]> * TDL-17429 Revert back tiers field datatype conversion (#117) * Initial commit for dict to stripe object conversion * Resolved pylint error * Updated test cases * Added condtion to check dict type Co-authored-by: prijendev <[email protected]> Co-authored-by: namrata270998 <[email protected]> Co-authored-by: namrata270998 <[email protected]> Co-authored-by: Prijen Khokhani <[email protected]> * Added tax_type and metadata in subcriptions_items * added the proration_details due to cci failure * TDL-17880 Add missing test cases (#131) * Added event_type field in all schemas * Added test cases for event updates * Updated __init__ file * Updated event_type field to updated_by_event_type field name * Reverted back config.yml file changes * Updated event_type field in all_field test case. * Added missing test case assertion * fixed circleci error and updated tests * updated integration tests to resolve cci failures * removed the unused variables * removed duplicate code * resolved comments * added more comments * added new map for intermittent missing fields * updated schema missing fields map and changed the union * added proper comment Co-authored-by: prijendev <[email protected]> Co-authored-by: nevilparikh_crest <[email protected]> * Tdl 13149 sync payment intent (#127) * Initial commit for payment_intent stream * Updated bookmark test case. * Updated expand parameter for charge stream in utils.py * Updated utils.py * Removed payment_intent object from charge stream * Removed extra field from charges schema * Added card field in schema * Updated transfer_group and application_fee_amount field type * Updated transfer_group field * Updated datatype of destination field * Updated transfer_data * Added payment record creation * Resolved test case error * Resloved all fields test case error * Resolved test case error * Added comments in test case. Co-authored-by: namrata270998 <[email protected]> * Tdl 13711 upgrade json schema (#124) * updated the transform_usage type and integration tests * added plan back to the sub_items * removed the workaround for the invoices 'created' field * updated transform_usage type * added code comment * TDL 15120 add event type field in schemas (#123) * Added event_type field in all schemas * Added test cases for event updates * Updated __init__ file * Updated event_type field to updated_by_event_type field name * Reverted back config.yml file changes * Updated event_type field in all_field test case. Co-authored-by: namrata270998 <[email protected]> * updated charge.json * TDL-14354 log request_id (#135) * logged request_id for the sdk calls * unittests for debug log * changed the logic of new_list and updated unittests * added code comments * rsolved pylint error * resolved cci failures * resolved pylint errors * rsolved pylint * changed the function overwritten for logging request id * resolved PR review comments * skipped schema missing fields to pass the cci * fixed cci issue * added schema missing fields * added receipt_url in fickle fields * resolved PR comments * TDL-17879 added authentication before discovery mode and unittests (#128) * added authentication before discovery mode and unittests * resolved comments * fixed spelling error * added schema missing fields * added comment for pagination test * added detailed comment * added ticket it * fixed circleci errors * Tdl 17878 implement request timeout and retry (#126) * added configurable request timeout with default as 5 minutes * added comments * added schema missing fields missing from schema * fixed cci error * added timeout in readme * TDL-9856: ensure setup includes non-automatic balance transactions (#129) * added test case to verify we sync only automatic payout's transactions * added more comments * resolved PR review comments * resolve CCi error * added doc-string in test_automatic_payout_transactions * resolve CCi error * resolve CCi error * resolve CCi error * resolve CCi error * added try-catch in test_automatic_payout_transactions Co-authored-by: namrata270998 <[email protected]> * TDL-16966: Handle deleted invoice_line_items returned from the events updates endpoint (#137) * Added retry logic for deleted invoice line items * Resolved pylint error * Added backoff over child also * added code to skip deleted invoice line item call * resolved unittest failure * Updated all field test case. * Reverted back changes in base.py * Updated all field test case to pass cci. Co-authored-by: harshpatel4_crest <[email protected]> Co-authored-by: namrata270998 <[email protected]> * TDL-15168: Use `unique_line_item_id` for invoice updates' lines value instead of `id` (#134) * updated id field for invoice line items * resolve integration test error * updated code to handle null invoice_item for invoice line items * resolve review comments * resolved CCi failure Co-authored-by: namrata270998 <[email protected]> * TDL-9801: `payouts` incorrectly mapped to `transfers` object results in transfer objects replicated by payouts stream (#133) * added payout with transfer for payout events * updated config.yml file * removed unnecessary files * resolved integration test error * resolved integration test error * resolved review comments * fix integration test and run all fields test only * fix cci failure * fix cci failure * fix cci failure * fix cci failure, run all tests Co-authored-by: namrata270998 <[email protected]> * Tdl 17902 fix parent child relationship (#130) * update child bookmark and make child stream independent * fixed pylint * resolved PR review comments * added function comments * added child streams to bookmarks test and added a comment to all_fields * resolved cci error * resolved comments * resolved comments * resolved PR review comments * fixed circleci errors * skipped schema missing fields and removed extra checks for nested fields * skipped receipt_url as it keeps changing every time in all_fields * updated function name in unittest and resolved pylint error (#140) Updated function names of unittest after all PR merge. * TDL-18217 change lookback window logic (#138) * added configurable lookback window * added code comments * resolved comments * resolved comments * changed lookback and added integration test * added new map for intermittently missing fields * resolved events stream lookback issue * updated schema missing fields map * added lookback test in config.yml * initial commit * configurable lookback window for bothstreams with updated tests * resolved cci issue and added new assertion * fixed cci issues * addentsd comme * added lookback test in all_tests_run * changed to same default lookback for both streams * resolved PR review comments * fixed typo error * added code comments * changed the lookback loguc acc to Kyle's suggestions * updated comments * float value in lookback * removed float as giving error * handled 0 scenario for lookback * handled string 0 and fixed cci * resolved comments * skipped new fields not present in schema fields to fix cci * resolved comments * added comments in unittests * added lookback in readme * updated debug with %s instead of f-string * updated unittests for the logger statement * removed the timedelta from start_date in lookback_window test * added all_fields in lookback test deoendency * updated the configurable lookback window test * TDL-19384 optimize logic for parent child relationship to be independent (#141) * initial commit with optimized logic for parent child streams * fixed issues in the invoices and invoice_line_items sync * fixed pylint * updated unittests * TDL-19384: Added Sub Stream check * fixed unittests * resolved PR comments and fix cci issues * updated invoice_line_item to line_item * Updated the code comments * fixed typo * updated code comments and added more unittests * resolved pylint * removed uwanted comments * resolved PR comments * resolved PR comments * resolved PR review comments * added integration test for only parent * resolved PR comments * skipped some newly generated fields from all_fields * added failing field as missing nested field Co-authored-by: dbshah1212 <[email protected]> * removed skipped fields from SCHEMA_MISSING_FIELDS * added missing fields in skipped fields Co-authored-by: savan-chovatiya <[email protected]> Co-authored-by: namrata270998 <[email protected]> Co-authored-by: namrata270998 <[email protected]> Co-authored-by: nevilparikh_crest <[email protected]> Co-authored-by: Harsh <[email protected]> Co-authored-by: harshpatel4_crest <[email protected]> Co-authored-by: dbshah1212 <[email protected]>
Description of change
TDL-14058: Update SDK API Versions
https://docs.google.com/presentation/d/1HVkKmBhPuvwD854ZeMwplRNyk8oVtFEpIuFKNERL5tQ
Manual QA steps
Risks
Rollback steps