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-14799 added an extra check to verify if the error has message in … #69

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

Conversation

namrata270998
Copy link
Contributor

@namrata270998 namrata270998 commented Sep 16, 2021

Description of change

  • checked if the error from the exception contains a message or not by verifying it is a dictionary or not

Manual QA steps

  • For wrong token it should print the error statement

Risks

Rollback steps

  • revert this branch

Copy link
Contributor

@cosimon cosimon left a comment

Choose a reason for hiding this comment

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

There are a lot of unnecessary whitespace changes. Please undo those.

tap_zendesk/streams.py Outdated Show resolved Hide resolved
@@ -81,49 +85,59 @@ def get_bookmark(self, state):
def update_bookmark(self, state, value):
current_bookmark = self.get_bookmark(state)
if value and utils.strptime_with_tz(value) > current_bookmark:
singer.write_bookmark(state, self.name, self.replication_key, value)

singer.write_bookmark(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you undo all of these whitespace changes? If pylint is complaining about line width then you can disable that in the circleci.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you undo all of these whitespace changes? If pylint is complaining about line width then you can disable that in the circleci.

Sure, undone the changes.

tap_zendesk/streams.py Outdated Show resolved Hide resolved
error = json.loads(e.args[0]).get('error')
if isinstance(error, dict):

if error.get('message', None) == "You do not have access to this page. Please contact the account owner of this help desk for further help.":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we merge this if condition with the above one using and?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we merge this if condition with the above one using and?

Done

Co-authored-by: savan-chovatiya <80703490+savan-chovatiya@users.noreply.github.com>
Copy link
Contributor

@dbshah1212 dbshah1212 left a comment

Choose a reason for hiding this comment

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

Why build is failling?

self.assertEqual(str(ex), error_string)


def test_zenpy_exception_but_different_message_raised(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please add a comment here.

Added comments

@namrata270998 namrata270998 requested review from dbshah1212, karanpanchal-crest and KrisPersonal and removed request for KrisPersonal November 1, 2021 13:18
@dbshah1212 dbshah1212 requested review from cosimon and removed request for cosimon November 2, 2021 07:20
tap_zendesk/streams.py Show resolved Hide resolved
prijendev and others added 3 commits November 10, 2021 11:55
* API call to the each stream in discovery mode done

* removed generated catalog file

* resolved pylint errors

* Resolved cyclic import pylint error

* Improved unittest case civerage

* Updated error message for 403 forbidden error

* Updated error handling

* resolved pylint error

* Removed empty catalog

* Removed unused catalog file.

* Removed unused state file

* Removed unused state file

* Removed unused file

* Updated error message and unittest case for 404 error

* Updated check access method

* Resolved pylint error

* recolved unused argument error

* resolved kwargs error

* Updated unittest cases

* Updated unittest cases

* Removed global variable

* Improved unittest case coverage

* updated 404 error

* resolved pylint error

* Updated typo error.

* Removed f strings

* Updated error handling

* resloved pylint error

* resolved unittest case error

* Added more comments and updated code

* resolved pylint error

* updated method name

* Added timeout error code

* Resolved pylint error

* added coverage report to artifact

* added pylint back

* Added comment

* resolved pylint errors

* Enhanced the code

* Reutilized args0

* Moved request_timeout parameter to common class

* Added comment

* Removed static time

* removed warning message

* resolved pylint error

* resolved the comments

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>
* removed the buffer systesm

* removed the unnecessary methods

* fixed pylint errors

* resolved pylint errors

* added comments in test file

* Added coverage report

* added comment

* added logger for printingcount of child streams

* updated unittest

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>
@dbshah1212 dbshah1212 mentioned this pull request Nov 11, 2021
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.

7 participants