Tdl 14803 check api access in discovery mode #74

prijendev · 2021-09-27T07:57:28Z

Description of change

TDL-14803 Check API access in Discovery mode

In Discovery Mode, the tap makes an API call to the each stream.
Write unittest cases for the same.
Updated error handling. (TDL-15392)
Replaced get call in get_incremental_export(http.py) by call_api because it doing the same as call_api. So, now error handling will work properly with backoff as earlier backoff was not available.

Note - Added code of PR79 in this PR because it was require. So, first PR79 need to merge and once that PR merged changes will not be reflected here.

Manual QA steps

Check that tap shows error in discovery code for streams which does not have access.
Check that ticket_metrics, ticket_comments, ticket_audit stream skip 404 error.

Risks

Rollback steps

revert this branch

savan-chovatiya · 2021-09-29T12:40:20Z

tap_zendesk/discover.py

+            elif json.loads(e.args[0]).get('description') == "You are missing the following required scopes: read":
+                error_list.append(s.name)
+            else:
+                raise e


@prijendev Raise an exception from None.

Raised an exception from None.

savan-chovatiya · 2021-09-29T12:41:04Z

tap_zendesk/http.py

+    except Exception: # pylint: disable=broad-except
+        response_json = {}
+    if response.status_code != 200:
+        message = "HTTP-error-code: {}, Error: {}".format(


Use f-strings instead of format.

Replaced format with f-string.

savan-chovatiya · 2021-09-29T12:44:54Z

tap_zendesk/streams.py

+        headers = {
+        'Content-Type': 'application/json',
+        'Accept': 'application/json',
+        'Authorization': 'Bearer {}'.format(self.config['access_token'])
+        }


Suggested change

headers = {

'Content-Type': 'application/json',

'Accept': 'application/json',

'Authorization': 'Bearer {}'.format(self.config['access_token'])

}

headers = {

'Content-Type': 'application/json',

'Accept': 'application/json',

'Authorization': f'Bearer {self.config['access_token']}'

}

Replaced format with f-string.

savan-chovatiya · 2021-09-29T12:45:49Z

tap_zendesk/streams.py

+        LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",
+                            stream)


Suggested change

LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",

stream)

LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",

stream)

dbshah1212 · 2021-09-30T10:25:23Z

tap_zendesk/http.py

+    },
+    403: {
+        "raise_exception": ZendeskForbiddenError,
+        "message": "You are missing the following required scopes: read"


Will it be each time read only? If No then please put a generalized message.

Yes. Because we are calling just GET API for each stream. So, it will return the same message of read scopes.

dbshah1212 · 2021-09-30T10:26:16Z

tap_zendesk/http.py

+    },
+    404: {
+        "raise_exception": ZendeskNotFoundError,
+        "message": "There is no help desk configured at this address. This means that the address is available and that you can claim it at http://www.zendesk.com/signup"


Same as above if it is not the fixed message for 404 each time then please put a generalized message.

Updated to generalized message.

dbshah1212 · 2021-09-30T10:39:25Z

tap_zendesk/streams.py

@@ -114,13 +115,33 @@ def load_metadata(self):
    def is_selected(self):
        return self.stream is not None

+    def check_access(self):
+        '''
+        Check whether permission given to access stream resource or not.


Suggested change

Check whether permission given to access stream resource or not.

Check whether the permission was given to access stream resources or not.

dbshah1212 · 2021-09-30T10:56:21Z

tap_zendesk/streams.py

 def raise_or_log_zenpy_apiexception(schema, stream, e):
    # There are multiple tiers of Zendesk accounts. Some of them have
    # access to `custom_fields` and some do not. This is the specific
    # error that appears to be return from the API call in the event that
    # it doesn't have access.
    if not isinstance(e, zenpy.lib.exception.APIException):
        raise ValueError("Called with a bad exception type") from e
+    #If read permission not available in oauth access_token, then it returns below error.


Suggested change

#If read permission not available in oauth access_token, then it returns below error.

#If read permission is not available in OAuth access_token, then it returns the below error.

…//github.com/singer-io/tap-zendesk into TDL-14803-check-api-access-in-discovery-mode

dbshah1212 · 2021-10-27T10:51:38Z

tap_zendesk/http.py

+
+    return 400 <=status_code < 500
+
+def raise_for_error(response):


Please add comments for this function.

added comments.

dbshah1212 · 2021-10-27T10:53:53Z

tap_zendesk/streams.py

-                    else:
-                        raise e
-
+                except http.ZendeskNotFoundError:


Add comment for this change.

added comments.

karanpanchal-crest · 2021-11-01T12:31:50Z

tap_zendesk/discover.py

+        try:
+            # Here it call the check_access method to check whether stream have read permission or not.
+            # If stream does not have read permission then append that stream name to list and at the end of all streams
+            # raise forbidden error with proper message containinn stream names.


@prijendev Typo in 'containing' word in the comment

karanpanchal-crest · 2021-11-01T12:32:33Z

tap_zendesk/discover.py

    for s in STREAMS.values():
-        s = s(client)
+        s = s(client, config)


@prijendev Can you please give understandable variable name instead of 's'

In existing code they have used 's'. Change in name will reflect lot off changes in code as it is used in many place.

@prijendev Please do the variable name changes. If it means change will reflect lot off changes that's fine

@prijendev Add Comments to the code

@prijendev Add Comments to the code

Added comments to the code. And you can find detailed comments in the try block as well. Also updated the variable name s to stream

karanpanchal-crest · 2021-11-01T12:32:50Z

tap_zendesk/discover.py

+            # raise forbidden error with proper message containinn stream names.
+            s.check_access()
+        except ZendeskForbiddenError as e:
+            error_list.append(s.name) # Append stream name to the


@prijendev Incomplete comment

karanpanchal-crest · 2021-11-01T12:35:36Z

tap_zendesk/discover.py

+        except ZendeskForbiddenError as e:
+            error_list.append(s.name) # Append stream name to the
+        except zenpy.lib.exception.APIException as e:
+            err = json.loads(e.args[0]).get('error')


@prijendev Since e.args[0] is being used multiple times. Please make it a variable and get it converted in the json.loads. and re-use it further everywhere

karanpanchal-crest · 2021-11-01T12:36:34Z

tap_zendesk/discover.py

+    if error_list:
+        streams_name = ", ".join(error_list)
+        message = "HTTP-error-code: 403, Error: You are missing the following required scopes: read. "\
+                    "The account credentials supplied do not have read access for the following stream(s):  {}".format(streams_name)


@prijendev As per the best practices, we should not use format(). Please update this code

They don't updated python to the latest version. That's why format() has been kept.

karanpanchal-crest · 2021-11-01T12:37:37Z

tap_zendesk/http.py

+        response_json = {}
+    if response.status_code != 200:
+        if response_json.get('error'):
+            message = "HTTP-error-code: {}, Error: {}".format(response.status_code, response_json.get('error'))


@prijendev Don't use format() in the code

They don't updated python to the latest version. That's why format() has been kept.

karanpanchal-crest · 2021-11-01T12:38:55Z

tap_zendesk/streams.py

+                except http.ZendeskNotFoundError:
+                    # Skip stream if ticket_audit does not found for particular ticekt_id. Earlier it throwing HTTPError
+                    # but now as error handling updated, it throws ZendeskNotFoundError.
+                    message = "Unable to retrieve audits for ticket (ID: {}), record not found".format(ticket['id'])


@prijendev Don't use format()

They don't updated python to the latest version. That's why format() has been kept.

karanpanchal-crest · 2021-11-01T12:39:20Z

tap_zendesk/streams.py

            if metrics_stream.is_selected():
                try:
                    for metric in metrics_stream.sync(ticket["id"]):
                        self._buffer_record(metric)
-                except HTTPError as e:


@prijendev Why did we remove the HTTPError exception?

Because earlier it throwing HTTPError but now as error handling updated, it throws ZendeskNotFoundError. So we replaced HTTPError with ZendeskNotFoundError. Added comment in code also.

karanpanchal-crest · 2021-11-01T12:40:14Z

tap_zendesk/streams.py

+        url = self.endpoint.format(self.config['subdomain'])
+        HEADERS['Authorization'] = 'Bearer {}'.format(self.config["access_token"])
+
+        http.call_api(url, self.config.get('request_timeout', DEFAULT_TIMEOUT), params={'start_time': 1610368140, 'per_page': 1}, headers=HEADERS)


@prijendev Why is there a static time in the code?

Here we calls api to just check api permission and we need to pass required start_time parameter. It can be anything. Replaced it with constant

karanpanchal-crest · 2021-11-02T05:07:36Z

tap_zendesk/streams.py

+        url = self.endpoint.format(self.config['subdomain'])
+        HEADERS['Authorization'] = 'Bearer {}'.format(self.config["access_token"])
+
+        http.call_api(url, self.config.get('request_timeout', DEFAULT_TIMEOUT), params={'per_page': 1}, headers=HEADERS)


@prijendev Wouldn't there be a data-type issue here if config['request_timeout'] would be a number in string format?
Eg: config[request_timeout]: '200'

Updated it. Actually I had updated it in PR 79 as part of request timeout but remained that changes this PR.

karanpanchal-crest · 2021-11-02T05:08:35Z

tap_zendesk/streams.py

-
-        for page in http.get_cursor_based(url, self.config['access_token'], **kwargs):
+        # Pass `request_timeout` parameter
+        for page in http.get_cursor_based(url, self.config['access_token'], self.config.get('request_timeout', DEFAULT_TIMEOUT), **kwargs):


Same comment here as well
Please initialize this value at a common place in the file and use it everywhere

Updated it.

karanpanchal-crest · 2021-11-02T05:09:26Z

tap_zendesk/streams.py

+        '''
+        Check whether the permission was given to access stream resources or not.
+        '''
+        self.client.search("", updated_after=datetime.datetime.utcnow(), updated_before='2000-01-02T00:00:00Z', type="user")


@prijendev What are we searching here?
What does "" mean?

They are using this API call in sync. This API call is used to retrieve users records in sync. We just checking whether that stream has read permission or not by same API call which used in sync. "" parameter is just any name of query. We followed the same call as in sync.

karanpanchal-crest · 2021-11-02T06:40:09Z

tap_zendesk/discover.py

+            args0 = json.loads(e.args[0])
+            err = args0.get('error')
+
+            if isinstance(err, dict):


@prijendev Can you add same comment here as it is added in this PR - https://github.com/singer-io/tap-zendesk/pull/69/files#diff-89887ca52395ce152e9723c5595655edcd41793204060336e88ea9805c274433R142

karanpanchal-crest · 2021-11-02T07:24:31Z

tap_zendesk/streams.py

@@ -16,6 +15,12 @@
 LOGGER = singer.get_logger()
 KEY_PROPERTIES = ['id']

+REQUEST_TIMEOUT = 300
+TICKET_START_TIME = 1610368140


@prijendev Why to keep a static value. You can use start time provided in the config.json file. Correct?

Yes, updated and removed static time

KrisPersonal · 2021-11-02T17:51:19Z

tap_zendesk/__init__.py

@@ -199,7 +200,7 @@ def main():
        LOGGER.error("""No suitable authentication keys provided.""")

    if parsed_args.discover:
-        do_discover(client)
+        do_discover(client, parsed_args.config)


Please add comments to the code changes

Please add comments to the code changes

Added comments

KrisPersonal · 2021-11-02T17:52:20Z

tap_zendesk/discover.py

    for s in STREAMS.values():
-        s = s(client)
+        s = s(client, config)


@prijendev Please do the variable name changes. If it means change will reflect lot off changes that's fine

KrisPersonal · 2021-11-02T17:52:40Z

tap_zendesk/discover.py

    for s in STREAMS.values():
-        s = s(client)
+        s = s(client, config)


@prijendev Add Comments to the code

KrisPersonal

Please look at my comments

prijendev added 6 commits September 24, 2021 15:01

API call to the each stream in discovery mode done

21edee7

removed generated catalog file

bc279a5

resolved pylint errors

7663264

Resolved cyclic import pylint error

475719c

Improved unittest case civerage

7a5fd9b

Updated error message for 403 forbidden error

f8cf58b

prijendev requested review from dbshah1212 and karanpanchal-crest September 29, 2021 04:28

savan-chovatiya reviewed Sep 29, 2021

View reviewed changes

prijendev added 2 commits September 30, 2021 12:10

Updated error handling

1b9d70a

resolved pylint error

88a3760

prijendev requested a review from savan-chovatiya September 30, 2021 08:52

prijendev added 5 commits September 30, 2021 15:05

Removed empty catalog

080b2ad

Removed unused catalog file.

3b71bea

Removed unused state file

125a051

Removed unused state file

7bfdd1e

Removed unused file

e409fae

dbshah1212 reviewed Sep 30, 2021

View reviewed changes

prijendev added 9 commits October 4, 2021 17:43

Updated error message and unittest case for 404 error

46855f1

Merge branch 'TDL-14803-check-api-access-in-discovery-mode' of https:…

95e4dee

…//github.com/singer-io/tap-zendesk into TDL-14803-check-api-access-in-discovery-mode

Resolved merge conflict

f10650e

Updated check access method

15d5c64

Resolved pylint error

961fcb1

recolved unused argument error

75363da

resolved kwargs error

c47824f

Updated unittest cases

ee826c5

Updated unittest cases

3313e58

dbshah1212 self-requested a review October 27, 2021 08:36

dbshah1212 reviewed Oct 27, 2021

View reviewed changes

prijendev added 2 commits October 28, 2021 14:44

Added comment

d08b811

resolved pylint errors

bd1fd48

prijendev requested a review from dbshah1212 October 28, 2021 09:19

dbshah1212 approved these changes Nov 1, 2021

View reviewed changes

karanpanchal-crest reviewed Nov 1, 2021

View reviewed changes

prijendev added 2 commits November 1, 2021 19:13

Enhanced the code

57daa7b

Reutilized args0

0c4b706

prijendev requested a review from karanpanchal-crest November 2, 2021 02:56

karanpanchal-crest suggested changes Nov 2, 2021

View reviewed changes

Moved request_timeout parameter to common class

d87d99e

karanpanchal-crest reviewed Nov 2, 2021

View reviewed changes

Added comment

8cd80ff

umangagrawal-crest requested review from karanpanchal-crest and KrisPersonal November 2, 2021 06:51

karanpanchal-crest reviewed Nov 2, 2021

View reviewed changes

Removed static time

ef85e74

prijendev requested a review from karanpanchal-crest November 2, 2021 08:12

karanpanchal-crest approved these changes Nov 2, 2021

View reviewed changes

prijendev added 2 commits November 2, 2021 16:27

removed warning message

ef19a71

resolved pylint error

33ac8d7

KrisPersonal reviewed Nov 2, 2021

View reviewed changes

KrisPersonal requested changes Nov 2, 2021

View reviewed changes

resolved the comments

d630fca

namrata270998 requested a review from KrisPersonal November 3, 2021 08:26

dbshah1212 requested a review from namrata270998 November 3, 2021 08:54

KrisPersonal approved these changes Nov 8, 2021

View reviewed changes

dbshah1212 mentioned this pull request Nov 11, 2021

Tdl 14624 crest master #88

Merged

		LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",
		stream)

	Check whether permission given to access stream resource or not.
	Check whether the permission was given to access stream resources or not.

	#If read permission not available in oauth access_token, then it returns below error.
	#If read permission is not available in OAuth access_token, then it returns the below error.


		return 400 <=status_code < 500

		def raise_for_error(response):

Tdl 14803 check api access in discovery mode #74

Are you sure you want to change the base?

Tdl 14803 check api access in discovery mode #74

Conversation

prijendev commented Sep 27, 2021 • edited Loading

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

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

dbshah1212 Sep 30, 2021 • 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

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

namrata270998 Nov 3, 2021 • 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

prijendev Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prijendev Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prijendev Nov 1, 2021 • 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

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

Choose a reason for hiding this comment

KrisPersonal left a comment

Choose a reason for hiding this comment

prijendev commented Sep 27, 2021 •

edited

Loading

dbshah1212 Sep 30, 2021 •

edited

Loading

namrata270998 Nov 3, 2021 •

edited

Loading

prijendev Nov 1, 2021 •

edited

Loading

prijendev Nov 1, 2021 •

edited

Loading

prijendev Nov 1, 2021 •

edited

Loading