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

TDL-14887: Respect field selection #67

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

Conversation

hpatel41
Copy link
Contributor

@hpatel41 hpatel41 commented Sep 2, 2021

Description of change

TDL-14887: Respect field selection

Added code change for field selection and transformation.

Manual QA steps

Risks

Rollback steps

  • revert this branch

@@ -134,6 +138,8 @@ def sync_data(self):
'ModifiedDate',
content_area.get('ModifiedDate'))

singer.write_records(table, [content_area])
with Transformer() as transformer:

Choose a reason for hiding this comment

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

@harshpatel4crest Can you make this code generic among all the streams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done the code change

if message['action'] == 'upsert'][0]

if stream == 'list':
expected_keys = expected_keys - {

Choose a reason for hiding this comment

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

@harshpatel4crest Why are we extracting fields like 'SendClassification', 'PartnerProperties', etc. from the expected kets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are not retrievable. So, added a comment regarding that.

'SendClassification',
'PartnerProperties'}
elif stream == 'subscriber':
expected_keys = expected_keys - {

Choose a reason for hiding this comment

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

Same question as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are not retrievable. So, added a comment regarding that.

@@ -59,6 +59,12 @@ def get_catalog_keys(self):
def parse_object(self, obj):
return project(obj, self.get_catalog_keys())

# a function to write records with applying transformation
def write_records_with_transform(self, record, catalog, table): # pylint: disable=no-self-use

Choose a reason for hiding this comment

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

@harshpatel4crest Can you please try resolving this pylint error instead of disabling it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hpatel41 hpatel41 marked this pull request as ready for review September 9, 2021 08:00
@hpatel41 hpatel41 requested a review from cosimon September 13, 2021 13:00
Copy link
Contributor

@kspeer825 kspeer825 left a comment

Choose a reason for hiding this comment

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

Tests approved 👍

Copy link
Contributor

@KrisPersonal KrisPersonal left a comment

Choose a reason for hiding this comment

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

For all endpoints, please separate out schema from .py file to schema and json file.

Copy link
Contributor

@KrisPersonal KrisPersonal left a comment

Choose a reason for hiding this comment

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

For all endpoints, please separate out schema from .py file to schema and json file.

@KrisPersonal
Copy link
Contributor

We have to ensure the extracted .json schema files are the same as the generated schemas. With the schema extracted, it’ll be easier to focus on the sync-specific code in each of those files

@hpatel41 hpatel41 requested a review from KrisPersonal October 8, 2021 10:52
@hpatel41
Copy link
Contributor Author

hpatel41 commented Oct 8, 2021

We have to ensure the extracted .json schema files are the same as the generated schemas. With the schema extracted, it’ll be easier to focus on the sync-specific code in each of those files

Separated schemas from .py files into .json files.


LOGGER = singer.get_logger()


class CampaignDataAccessObject(DataAccessObject):

SCHEMA = with_properties({
Copy link
Contributor Author

@hpatel41 hpatel41 Oct 12, 2021

Choose a reason for hiding this comment

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

Removed schema.
Similarly for other files too.

import singer

from tap_exacttarget.client import request
from tap_exacttarget.dao import DataAccessObject
from tap_exacttarget.schemas import with_properties
Copy link
Contributor Author

@hpatel41 hpatel41 Oct 12, 2021

Choose a reason for hiding this comment

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

Removed unused import as the schema is removed.
Similarly for other files too.

@@ -1,64 +0,0 @@
def with_properties(properties):
Copy link
Contributor Author

@hpatel41 hpatel41 Oct 12, 2021

Choose a reason for hiding this comment

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

Removed this file, as the schema is separated from the .py file. Hence, created a definitions.json for the common (reference) fields used in the schema.

hpatel41 and others added 7 commits October 13, 2021 11:44
…nResetErrors (#71)

* added backoff for certain errors

* resolve pylint

* updated decorator location

* added unittests

* added comment

* added comments
* updated error message when generating auth_stub

* made changes according to the comments

* updated the code acording to the comments

* updated the tap tester image

* updated pylint and astroid to latest version

* updated the code as, on updating tap tester image it was throwing cascading errors

* updated config.yml file

* updated the start date for integration tests as per the params

* removed scenario as new tap-tester version does not support it

* updated start date in the base file test
* added best practices

* resolve pylint

* resolve test failure

* test: updated the test cases

* test: updated some test cases

* updated the code as per comments

* resolve test case failure

* updated the code as per comments

* resolve integration test failure

* resolve tes case failure

* resolve test case failure

* add data extension stream in test cases

* added data extension incremental stream

* test: run bookmark test

* test: run bookmark test

* test: run bookmark test, debug failing test

* test: pylint resolve

* test: run all data extenstion stream test in bookmark test

* test: updated data extension code

* updated the test to run data extension stream

* run all tests

* added sys.exit and updated pylint and astroid to latest versions

* resolve pylint

* updated the files as per comments

* updated the code
…stream does not bookmark correctly (#75)

* make keys automatic

* pylint resolve

* add full replication test case

* added code change for data extension stream

* pylint resolve

* added comment

* added comments

* added comment in base file

* updated discovery test and removed full replication test

* updated the code

* added a comment explaining subscriber and list subscriber syncing

* added comments
@hpatel41 hpatel41 mentioned this pull request Oct 13, 2021
@hpatel41 hpatel41 changed the base branch from automatic-fields to master October 14, 2021 04:51
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