TDL-13615: Add Locations stream and TDL-13614: Add Inventory Levels stream #114
Conversation
hpatel41
requested review from
karanpanchal-crest,
dbshah1212 and
savan-chovatiya
hpatel41
changed the title
savan-chovatiya
changed the title
tests/test_pagination.py
Outdated
| # skip 'locations' stream as there is not much info about | ||
| # limit of records returned in 1 page | ||
| # Documentation: https://help.shopify.com/en/manual/locations/setting-up-your-locations | ||
| if stream in ['locations']: |
There was a problem hiding this comment.
Can we remove the location stream from the testable_streams. Remove location stream from line no. 23.
There was a problem hiding this comment.
Removed locations stream from testable_streams as per suggestion.
| actual_sync = list(LOCATIONS_OBJECT.sync()) | ||
|
|
||
| # Verify that only 2 record syncs | ||
| self.assertEqual(actual_sync, expected_sync) |
There was a problem hiding this comment.
@namrata270998 Please write test case to check the max_bookmark value in sync function
There was a problem hiding this comment.
Added an assertion to check with what value is the update_bookmark() function called.
tests/test_pagination.py
Outdated
|
|
||
| with self.subTest(store="store_2"): | ||
| conn_id = self.create_connection(original_properties=False, original_credentials=False) | ||
| self.pagination_test(conn_id, self.store_2_streams) | ||
| self.pagination_test(conn_id, self.store_2_streams - excepted_streams) |
There was a problem hiding this comment.
For this test could you please add the assertions that verify records are unique across pages by checking primary key values. It should leverage tuples in case of any compound primary keys and the assertCountEqual method. https://github.com/singer-io/tap-hubspot/blob/290e5c051839b01e43739d25e1c46b6ddef5a749/tests/test_hubspot_pagination_test.py#L118-L127
savan-chovatiya
requested review from
luandy64 and
zachharris1
and removed request for
KAllan357 and
savan-chovatiya
tests/unittests/test_locations.py
Outdated
|
|
||
| expected_sync = [LOCATION_3.to_dict(), LOCATION_4.to_dict()] | ||
| mock_get_locations_data.return_value = [ | ||
| LOCATION_1, LOCATION_2, LOCATION_3, LOCATION_4] |
There was a problem hiding this comment.
Line 32/33 make it one single line
tests/unittests/test_locations.py
Outdated
| max_bookmark = strptime_to_utc("2021-08-14T01:57:05-04:00") | ||
|
|
||
| # Verify that maximum replication key of all keys is updated as bookmark | ||
| mock_update_bookmark.assert_called_with( | ||
| utils.strftime(max_bookmark)) |
There was a problem hiding this comment.
Why do we want to have extra conversion for max_bookmark?
There was a problem hiding this comment.
Because max_bookmark is in datetime format and the bookmark date is called in the string format.
tap_shopify/__init__.py
Outdated
| raise ShopifyError(exc, 'Add read_inventory scope for access token and ' | ||
| 'Re-authorize the connection to sync ' | ||
| 'Inventory Levels and Inventory Items streams.') \ | ||
| from exc |
There was a problem hiding this comment.
Change it to "Add read_inventory scope for access token, re-authorize the connect to sync and get Inventory levels /Inventory items streams
| for parent_object in selected_parent.get_locations_data(): | ||
| inventory_levels = self.get_inventory_levels(parent_object.id, bookmark) | ||
| for inventory_level in inventory_levels: | ||
| yield inventory_level |
There was a problem hiding this comment.
why not use yield from? What is difference between line 28?
There was a problem hiding this comment.
Updated the code and used "yield from"
|
|
||
| @shopify_error_handling | ||
| def get_locations_data(self): | ||
| location_page = self.replication_object.find() |
There was a problem hiding this comment.
Are there any parameters being missed for find like API for inventory levels?
There was a problem hiding this comment.
No, the location API documentation does not have the parameters like the inventory levels.
…tream (singer-io#114) * added locations stream * pylint resolve * added location as full table * set replication key to none * test: added location in test * pylint resolve * test: set api limit 0 for locations stream * added unit test to run on cci * TDL-13614: Added Inventory Level stream * Resolved pylint failure * Added error message for read_inventory scope requirement * Resolved pylint failure * added error decorator in locations stream * skip if updated at is not found * updated location stream code to use replication key value as bookmark * Updated pagination test for locations stream * Added unit tests * Updated unit tests as per suggestion * added 2 fields in locations stream * updated the test case according to the comment * made changes according to the comments * updated code Co-authored-by: savan-chovatiya <[email protected]> Co-authored-by: savan-chovatiya <[email protected]>
Description of change
TDL-13615: Add locations data
TDL-13614: Add Inventory Levels stream
TDL-14697: Properly catch the error of missing OAuth scopes
QA steps
Risks
Rollback steps