🐛 Source ZenDesk Support: fix 404 responses for the ticket_comments stream #6513

antixar · 2021-09-28T16:39:15Z

What

There is an issue with relevant statutes for a lot of tickets. Some of them can be deleted by users while comments loading.

How

Need to handle 404 code responses for same cases.

Pre-merge Checklist

Updating a connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

CLAassistant · 2021-09-28T16:39:19Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ antixar
❌ Maksym Pavlenok

Maksym Pavlenok seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

antixar · 2021-09-29T22:09:19Z

/test connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1288740849
❌ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1288740849
🐛 https://gradle.com/s/jvo5btsvjga64

…rt-oauth2

antixar · 2021-09-30T22:07:37Z

/test connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1292864618
✅ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1292864618
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        74      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              197     93    53%
	 source_acceptance_test/tests/test_full_refresh.py       18     11    39%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 47     20    57%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     111     11    90%
	 ------------------------------------------------------------------------
	 TOTAL                                                  853    415    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      273     75    73%
	 --------------------------------------------------------
	 TOTAL                                  313     90    71%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      273    114    58%
	 --------------------------------------------------------
	 TOTAL                                  313    129    59%

lazebnyi

LGTM

Phlair

Awesome, adding a few useful things here. Could you update the title/description to reflect all that (since it's more than just fixing the 404 bug).

Just one main concern at the moment in my comments.

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

Phlair · 2021-10-06T15:22:59Z

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

+            )
+
+        if ticket_pages:
+            last_times = sorted(ticket_pages.keys())


based on doing ticket_pages[tickets_stream.last_end_time].append(... above, won't ticket_pages just have one unique key which is Tickets(start_date=self._start_date, subdomain=self._subdomain, authenticator=self.authenticator).last_end_time?

This code seems like it expects multiple...

(relevant down to line 430)

not necessary, the option last_end_time is updated by every new request to the zendesk server. And there is an probability that a bit of tickets were updated simultaneously. For example: Some script added 3000 comments to 3000 tickets. All them would have same updated time. And as result this connector would load same last_end_time value for 3 different consistent reques. Thus this logic tries to aggregate all tickets with same last_end_time.
Sure, this probability is unlikely but I added the aggregation just in case

Understood, I was missing the knowledge that last_end_time gets updated on every request. It may already be commented somewhere but would be good to have comments in this logic as well to explain that.

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/spec.json

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/source.py

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/spec.json

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

…zendesk_support/streams.py Co-authored-by: George Claireaux <george@claireaux.co.uk>

Co-authored-by: Davin Chia <davinchia@gmail.com>

…hub.com:airbytehq/airbyte into antixar/6249-zendesk-support-not-found-comments

antixar · 2021-10-13T17:05:00Z

/test connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1338482472
✅ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1338482472
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        74      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              200     94    53%
	 source_acceptance_test/tests/test_full_refresh.py       18     11    39%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 47     20    57%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  860    419    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      275     78    72%
	 --------------------------------------------------------
	 TOTAL                                  315     93    70%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      275    114    59%
	 --------------------------------------------------------
	 TOTAL                                  315    129    59%

Phlair

Great lgtm, minor comments and code suggestions but all good to publish/merge when addressed 👍

Phlair · 2021-10-14T08:57:59Z

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/source.py

+# from airbyte_cdk.sources.streams.http.auth.token import TokenAuthenticator
+


Suggested change

# from airbyte_cdk.sources.streams.http.auth.token import TokenAuthenticator

Phlair · 2021-10-14T09:08:35Z

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

+            )
+
+        if ticket_pages:
+            last_times = sorted(ticket_pages.keys())


Understood, I was missing the knowledge that last_end_time gets updated on every request. It may already be commented somewhere but would be good to have comments in this logic as well to explain that.

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

docs/integrations/sources/zendesk-support.md

Co-authored-by: George Claireaux <george@claireaux.co.uk>

…zendesk_support/streams.py Co-authored-by: George Claireaux <george@claireaux.co.uk>

antixar · 2021-10-15T21:31:14Z

/test connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1347563098
✅ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1347563098
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        74      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              200     94    53%
	 source_acceptance_test/tests/test_full_refresh.py       18     11    39%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 47     20    57%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  860    419    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      275     78    72%
	 --------------------------------------------------------
	 TOTAL                                  315     93    70%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      275    114    59%
	 --------------------------------------------------------
	 TOTAL                                  315    129    59%

antixar · 2021-10-15T21:39:50Z

/test connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1347585203
✅ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1347585203
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        74      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              200     94    53%
	 source_acceptance_test/tests/test_full_refresh.py       18     11    39%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 47     20    57%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  860    419    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      275     78    72%
	 --------------------------------------------------------
	 TOTAL                                  315     93    70%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                 Stmts   Miss  Cover
	 --------------------------------------------------------
	 source_zendesk_support/__init__.py       2      0   100%
	 source_zendesk_support/source.py        38     15    61%
	 source_zendesk_support/streams.py      275    114    59%
	 --------------------------------------------------------
	 TOTAL                                  315    129    59%

antixar · 2021-10-15T21:55:32Z

/publish connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1347619283
✅ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/1347619283

sherifnada · 2021-10-15T22:33:40Z

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/spec.json

@@ -23,6 +23,17 @@
        "default": "api_token",
        "description": "Zendesk service provides 2 auth method: API token and oAuth2. Now only the first one is available. Another one will be added in the future",
        "oneOf": [
+          {
+            "type": "object",
+            "title": "OAuth2.0 authorization (not implemented)",


Why are we adding this if it's not implemented? A user who sees this will lose confidence in the product

This was planned as temporary gap for the oauth future functionality. I remember to CI tests checked that oneOf items have to include more than 1 subitem. But now I see CI passes this test without problems.
You're right to it will be able to confuse users.
I've added a PR with correction.

ah got it. thanks for making the change!

sherifnada · 2021-10-15T22:36:05Z

@antixar is this PR also introducing oauth support? I am asking because the title doesn't reflect it, so we should either make sure to update the title appropriately

Also, please make sure to update the PR description in the future -- it's not clear to the reader what the root cause exactly is or how the PR solves it unless they read the whole PR which in this case is not trivial

antixar · 2021-10-16T08:36:44Z

@sherifnada , Initially this PR included correction for 2 critical bugs (they were fixed 2 weeks ago). Unfortunately oauth2 development is blocked. Thus I splitted all oauth2 logic to another PR. It will be bumped by separate version.

…tream (airbytehq#6513) * fix 404 responses for the ticket_comments stream * add unit test * add unit test * add oauth2 access token * Update airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py Co-authored-by: George Claireaux <george@claireaux.co.uk> * switching among auth methods * Update docs/integrations/sources/zendesk-support.md Co-authored-by: Davin Chia <davinchia@gmail.com> * add log message for 404 errors * Update docs/integrations/sources/zendesk-support.md Co-authored-by: George Claireaux <george@claireaux.co.uk> * Update airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py Co-authored-by: George Claireaux <george@claireaux.co.uk> * remove oauth logic from this PR * remove oauth logic from this PR Co-authored-by: Maksym Pavlenok <maksym.pavlenok@globallogic.com> Co-authored-by: George Claireaux <george@claireaux.co.uk> Co-authored-by: Davin Chia <davinchia@gmail.com>

fix 404 responses for the ticket_comments stream

87d02f5

github-actions bot added the area/connectors Connector related issues label Sep 28, 2021

antixar self-assigned this Sep 28, 2021

gl-pix added 2 commits September 29, 2021 18:59

add unit test

a873a6d

add unit test

274a195

antixar requested a review from lazebnyi September 29, 2021 16:01

antixar temporarily deployed to more-secrets September 29, 2021 16:02 Inactive

antixar linked an issue Sep 29, 2021 that may be closed by this pull request

Zendesk Source incremental sync is very slow #6400

Closed

jrhizor temporarily deployed to more-secrets September 29, 2021 22:11 Inactive

gl-pix added 2 commits September 30, 2021 13:06

Merge remote-tracking branch 'origin' into antixar/6285-zendesk-suppo…

72108cf

…rt-oauth2

add oauth2 access token

d8d6058

github-actions bot added the area/documentation Improvements or additions to documentation label Sep 30, 2021

antixar temporarily deployed to more-secrets September 30, 2021 22:07 Inactive

jrhizor temporarily deployed to more-secrets September 30, 2021 22:09 Inactive

This was linked to issues Oct 1, 2021

Zendesk Support Still Failing - Error on comments.json #6249

Closed

Source Zendesk Support: support oauth #6285

Closed

antixar requested a review from midavadim October 1, 2021 14:20

lazebnyi approved these changes Oct 1, 2021

View reviewed changes

antixar requested a review from Phlair October 4, 2021 15:48

Phlair reviewed Oct 6, 2021

View reviewed changes

midavadim mentioned this pull request Oct 7, 2021

CDK HttpStream authenticator property does not return oauth object #6843

Closed

midavadim reviewed Oct 7, 2021

View reviewed changes

thomasclavet mentioned this pull request Oct 7, 2021

Zendesk Support Still Failing - Error on comments.json #6249

Closed

Update airbyte-integrations/connectors/source-zendesk-support/source_…

9f98748

…zendesk_support/streams.py Co-authored-by: George Claireaux <george@claireaux.co.uk>

antixar temporarily deployed to more-secrets October 8, 2021 09:44 Inactive

antixar requested a review from Phlair October 8, 2021 11:28

switching among auth methods

7df812c

Update docs/integrations/sources/zendesk-support.md

d455f30

Co-authored-by: Davin Chia <davinchia@gmail.com>

antixar temporarily deployed to more-secrets October 12, 2021 15:38 Inactive

gl-pix added 2 commits October 13, 2021 19:47

Merge branch 'antixar/6249-zendesk-support-not-found-comments' of git…

4a831f9

…hub.com:airbytehq/airbyte into antixar/6249-zendesk-support-not-found-comments

add log message for 404 errors

2da950d

antixar temporarily deployed to more-secrets October 13, 2021 17:05 Inactive

antixar requested a review from davinchia October 13, 2021 17:05

jrhizor temporarily deployed to more-secrets October 13, 2021 17:07 Inactive

Phlair approved these changes Oct 14, 2021

View reviewed changes

antixar and others added 2 commits October 15, 2021 20:04

Update docs/integrations/sources/zendesk-support.md

716e8c1

Co-authored-by: George Claireaux <george@claireaux.co.uk>

Update airbyte-integrations/connectors/source-zendesk-support/source_…

0f25474

…zendesk_support/streams.py Co-authored-by: George Claireaux <george@claireaux.co.uk>

antixar temporarily deployed to more-secrets October 15, 2021 17:06 Inactive

antixar removed a link to an issue Oct 15, 2021

Source Zendesk Support: support oauth #6285

Closed

remove oauth logic from this PR

f0bcd10

antixar temporarily deployed to more-secrets October 15, 2021 21:31 Inactive

jrhizor temporarily deployed to more-secrets October 15, 2021 21:33 Inactive

remove oauth logic from this PR

18903ef

antixar temporarily deployed to more-secrets October 15, 2021 21:39 Inactive

jrhizor temporarily deployed to more-secrets October 15, 2021 21:41 Inactive

jrhizor temporarily deployed to more-secrets October 15, 2021 21:57 Inactive

antixar merged commit 592f7fe into master Oct 15, 2021

antixar deleted the antixar/6249-zendesk-support-not-found-comments branch October 15, 2021 22:20

sherifnada reviewed Oct 15, 2021

View reviewed changes

jrhizor mentioned this pull request Oct 19, 2021

Bump Airbyte version from 0.30.20-alpha to 0.30.21-alpha #7145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source ZenDesk Support: fix 404 responses for the ticket_comments stream #6513

🐛 Source ZenDesk Support: fix 404 responses for the ticket_comments stream #6513

antixar commented Sep 28, 2021 •

edited

Loading

CLAassistant commented Sep 28, 2021 •

edited

Loading

antixar commented Sep 29, 2021 •

edited by github-actions bot

Loading

antixar commented Sep 30, 2021 •

edited by github-actions bot

Loading

lazebnyi left a comment

Phlair left a comment •

edited

Loading

Phlair Oct 6, 2021

Phlair Oct 6, 2021

antixar Oct 8, 2021

Phlair Oct 14, 2021

antixar commented Oct 13, 2021 •

edited by github-actions bot

Loading

Phlair left a comment

Phlair Oct 14, 2021

Phlair Oct 14, 2021

antixar commented Oct 15, 2021 •

edited by github-actions bot

Loading

antixar commented Oct 15, 2021 •

edited by github-actions bot

Loading

antixar commented Oct 15, 2021 •

edited by github-actions bot

Loading

sherifnada Oct 15, 2021

antixar Oct 16, 2021

sherifnada Oct 17, 2021

sherifnada commented Oct 15, 2021

antixar commented Oct 16, 2021

		# from airbyte_cdk.sources.streams.http.auth.token import TokenAuthenticator

🐛 Source ZenDesk Support: fix 404 responses for the ticket_comments stream #6513

🐛 Source ZenDesk Support: fix 404 responses for the ticket_comments stream #6513

Conversation

antixar commented Sep 28, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

CLAassistant commented Sep 28, 2021 • edited Loading

antixar commented Sep 29, 2021 • edited by github-actions bot Loading

antixar commented Sep 30, 2021 • edited by github-actions bot Loading

lazebnyi left a comment

Choose a reason for hiding this comment

Phlair 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

antixar commented Oct 13, 2021 • edited by github-actions bot Loading

Phlair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antixar commented Oct 15, 2021 • edited by github-actions bot Loading

antixar commented Oct 15, 2021 • edited by github-actions bot Loading

antixar commented Oct 15, 2021 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada commented Oct 15, 2021

antixar commented Oct 16, 2021

antixar commented Sep 28, 2021 •

edited

Loading

CLAassistant commented Sep 28, 2021 •

edited

Loading

antixar commented Sep 29, 2021 •

edited by github-actions bot

Loading

antixar commented Sep 30, 2021 •

edited by github-actions bot

Loading

Phlair left a comment •

edited

Loading

antixar commented Oct 13, 2021 •

edited by github-actions bot

Loading

antixar commented Oct 15, 2021 •

edited by github-actions bot

Loading

antixar commented Oct 15, 2021 •

edited by github-actions bot

Loading

antixar commented Oct 15, 2021 •

edited by github-actions bot

Loading