TDL-19835: Adding new streams and discovery mode #38

jtilala · 2022-07-20T16:17:37Z

Description of change

Added discovery mode.
Added New streams with class-based implementation.

Manual QA steps

Run discovery mode, sync mode with state and without state

Risks

Rollback steps

revert this branch

savan-chovatiya · 2022-07-21T10:24:50Z

tap_braintree/discover.py

@@ -0,0 +1,67 @@
+from asyncio.log import logger


Remove unused import statement.

Removed unused imports

savan-chovatiya · 2022-07-21T10:25:00Z

tap_braintree/discover.py

+def get_abs_path(path):
+    return os.path.join(os.path.dirname(os.path.realpath(__file__)), path)
+
+def get_schemas():


Add function doc string.

Added doc strings for all the fuctions.

savan-chovatiya · 2022-07-21T10:25:19Z

tap_braintree/discover.py

+
+    return schemas, field_metadata
+
+def discover():


Add function doc string for all the functions.

Added doc strings for all the fuctions.

savan-chovatiya · 2022-07-21T10:25:33Z

tap_braintree/discover.py

+
+    return schemas, field_metadata
+
+def discover():


Divide this file into two files schema.py and discover.py because you are doing full refactoring.
Reference: https://github.com/singer-io/tap-github/pull/168/files

Seperated discover.py's code into schema.py and discover.py

savan-chovatiya · 2022-07-21T10:40:05Z

tap_braintree/schemas/customers.json

+                }
+            }
+        },
+        "android_pay_cards": {


Looks like all the *_cards related fields have the same kind of schema. Can you check it once, if those are the same then use schema reference for it?

Added reference schema in require schemas

savan-chovatiya · 2022-07-21T11:32:01Z

tap_braintree/schemas/settlement_batch_summary.json

+            "items": {
+                "type": [
+                    "null",
+                    "objects"


I think this should be object and not objects.

Corrected field type value

savan-chovatiya · 2022-07-21T11:32:51Z

tap_braintree/schemas/settlement_batch_summary.json

+                    "settlement_date": {
+                        "type": [
+                            "null",
+                            "strings"


I think this should be string and not strings.

Corrected field type value

savan-chovatiya · 2022-07-21T11:36:21Z

tap_braintree/streams.py

@@ -0,0 +1,366 @@
+from readline import replace_history_item


Remove unused import statement.

Removed unused imports

dbshah1212 · 2022-07-21T11:37:26Z

tap_braintree/schemas/customers.json

+                            "array"
+                        ],
+                        "items": {
+                            "$ref": "subscription.json#/"


Do we have separate stream for subscriptions? If not then we can move subscription.json into the shared folder.

Updated customer schema

dbshah1212 · 2022-07-21T11:40:53Z

tap_braintree/schemas/disputes.json

+                }
+            }
+        },
+        "transaction": {


I can see multiple occurrences of it, can we create a reference for it?

Added schema reference in require's schema

dbshah1212 · 2022-07-21T11:41:41Z

tap_braintree/schemas/merchant_accounts.json

+                "object"
+            ],
+            "properties": {
+                "address_details": {


address_details used multiple times, we can create a reference of it.

removed this stream, so no need to create reference schema for this

dbshah1212 · 2022-07-21T11:57:52Z

tap_braintree/streams.py

+DEFAULT_TIMESTAMP = "1970-01-01T00:00:00Z"
+LOGGER = singer.get_logger()
+
+SDK_CALL = {


Instead of creating dict, pass this in the stream object.

Updated streams.py as per your suggestions

savan-chovatiya · 2022-07-21T12:00:46Z

tap_braintree/streams.py

+    replication_keys = "updated_at"
+    key_properties = ["id"]
+
+    def sync_endpoint(self, gateway, config, schema, state, selected_streams, sync_streams):


Looks like all the classes have only 3 kinds of sync_endpoint() function. Create 3 parent classes with specific sync_endpoint() as per functionality and extend those from all the classes. Also, move common attribute variables to those parent classes.
Also move sync_without_window, sync_with_window and sync_full_table method to specific classes with common name sync.

Updated streams.py and created 3 parent classes for sync.

umangagrawal-crest · 2022-07-27T05:35:39Z

setup.py

@@ -10,7 +10,7 @@
      classifiers=['Programming Language :: Python :: 3 :: Only'],
      py_modules=['tap_braintree'],
      install_requires=[
-          'singer-python==5.5.0',
+          'singer-python==5.12.2',
          'requests==2.20.0',
          'braintree==3.53.0',


Can we use the latest version of braintree i.e. 4.16.0

updated version of the Braintree

umangagrawal-crest · 2022-07-27T09:07:59Z

tap_braintree/discover.py

+
+def discover():
+    """
+    Generate catalog for call the streams


Suggested change

Generate catalog for call the streams

Run the discovery mode, prepare the catalog file, and return the catalog.

Updated doc strings

umangagrawal-crest

Please update setup.py and include changes same as https://github.com/singer-io/tap-quickbooks/blob/master/setup.py#L31-L36
Also, please check the grammar for all the doc string.

umangagrawal-crest · 2022-07-27T09:20:50Z

tap_braintree/__init__.py

-
-    # increment through each day (20k results max from api)
-    for start, end in daterange(period_start, period_end):
+    LOGGER.info("Starting discovery")


Suggested change

LOGGER.info("Starting discovery")

LOGGER.info("Starting discover mode")

Updated logger messages

umangagrawal-crest · 2022-07-27T09:21:03Z

tap_braintree/__init__.py

+    LOGGER.info("Starting discovery")
+    catalog = discover()
+    json.dump(catalog.to_dict(), sys.stdout, indent=2)
+    LOGGER.info("Finished discover")


Suggested change

LOGGER.info("Finished discover")

LOGGER.info("Finished discover mode")

Updated logger messages as per your suggestions

umangagrawal-crest · 2022-07-27T09:31:31Z

tap_braintree/schemas/plans.json

+                "array"
+            ],
+            "items": {
+                "$ref": "add_on.json#/"


Suggested change

"$ref": "add_on.json#/"

"$ref": "add_ons.json#/"

Updated all schema with the correct name of the schema

umangagrawal-crest · 2022-07-27T09:31:51Z

tap_braintree/schemas/plans.json

+                "array"
+            ],
+            "items": {
+                "$ref": "discount.json#/"


Suggested change

"$ref": "discount.json#/"

"$ref": "discounts.json#/"

Updated all schema with the correct name of the schema

umangagrawal-crest

References used in the streams schema are not resolved

umangagrawal-crest · 2022-07-27T10:29:48Z

tap_braintree/schemas/subscriptions.json

+                "array"
+            ],
+            "items": {
+                "$ref": "add_on.json#/"


Suggested change

"$ref": "add_on.json#/"

"$ref": "add_ons.json#/"

Updated all schema with the correct name of the schema

savan-chovatiya · 2022-07-28T06:37:03Z

tap_braintree/schemas/shared/address_details.json

@@ -0,0 +1,32 @@
+{


Please update package_data in setup.py and include a shared schema path in MANIFEST.in

Added shared path in setup.py

savan-chovatiya · 2022-07-28T06:48:29Z

tests/unittests/test_main.py

+        if test_value[0]:
+            call_count = mocked_discover.call_count
+        else:
+            call_count = mocked_sync.call_count


Don't use if-else in a test case to decide call_count. Create separate test cases in such scenarios. Use @parameterized.expand for common scenarios only.

Seperated unit tests for sync mode and discover mode

savan-chovatiya · 2022-07-28T07:04:01Z

tap_braintree/schemas/transactions.json

-        "paypal_details": {
-            "anyOf": [
-                {
-                    "type": "object",


Any reason to remove many fields from the existing schema.

TDL-19835: Adding new streams and discovery mode

1522ee4

jtilala requested review from dbshah1212 and savan-chovatiya July 20, 2022 16:18

Jay Tilala added 3 commits July 21, 2022 11:10

add primary key to records for settlement_batch_summary

8112ab6

make logger CAPITAL

45a53aa

make logger to LOGGER

6be4e77

savan-chovatiya reviewed Jul 21, 2022

View reviewed changes

dbshah1212 reviewed Jul 21, 2022

View reviewed changes

savan-chovatiya reviewed Jul 21, 2022

View reviewed changes

Jay Tilala added 5 commits July 22, 2022 13:52

address given comments for sync

41f35f9

taking bookamrk with highest value

959eb09

added unitest for sync

bf0b898

add unittest test cases

251ba65

add unittest cases

58cf292

jtilala requested review from dbshah1212, savan-chovatiya and umangagrawal-crest July 27, 2022 05:14

umangagrawal-crest reviewed Jul 27, 2022

View reviewed changes

updated SDK version

332f3cf

umangagrawal-crest reviewed Jul 27, 2022

View reviewed changes

umangagrawal-crest suggested changes Jul 27, 2022

View reviewed changes

umangagrawal-crest reviewed Jul 27, 2022

View reviewed changes

umangagrawal-crest suggested changes Jul 27, 2022

View reviewed changes

added schema json

a24dc67

umangagrawal-crest reviewed Jul 27, 2022

View reviewed changes

savan-chovatiya reviewed Jul 28, 2022

View reviewed changes

Jay Tilala added 7 commits August 1, 2022 18:13

addressed comments

c39f276

updated setup.py

3974762

updated add all unitests

6a294ae

Name change in streams.py

d2e9edf

updated setup.py

1bc7d2a

updated doc strings

426f555

add ref in transactions

485b2cb

	Generate catalog for call the streams
	Run the discovery mode, prepare the catalog file, and return the catalog.

	LOGGER.info("Starting discovery")
	LOGGER.info("Starting discover mode")

	LOGGER.info("Finished discover")
	LOGGER.info("Finished discover mode")

TDL-19835: Adding new streams and discovery mode #38

Are you sure you want to change the base?

TDL-19835: Adding new streams and discovery mode #38

Conversation

jtilala commented Jul 20, 2022

Description of change

Manual QA steps

Risks

Rollback steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savan-chovatiya Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umangagrawal-crest left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umangagrawal-crest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtilala Jul 22, 2022 •

edited

Loading

jtilala Aug 2, 2022 •

edited

Loading

jtilala Aug 2, 2022 •

edited

Loading

jtilala Aug 2, 2022 •

edited

Loading

jtilala Aug 2, 2022 •

edited

Loading

jtilala Aug 2, 2022 •

edited

Loading

savan-chovatiya Jul 21, 2022 •

edited

Loading

umangagrawal-crest left a comment •

edited

Loading

jtilala Aug 2, 2022 •

edited

Loading