🎉 New Source: Zenloop #7380

AlexanderBatoulis · 2021-10-26T07:16:43Z

What

Added new source Zenloop
Participating in hacktoberfest-2021 Source: Zenloop connector-contest#82

How

Added four streams for this source

Surveys
Answers
Survey Groups
Answers Survey Groups

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Test Runs

unit test

integration test

New 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-10-26T07:16:50Z

All committers have signed the CLA.

harshithmullapudi · 2021-10-27T16:22:22Z

Hey @AlexanderBatoulis thanks for the contribution. We will review this soon

alafanechere · 2021-11-04T16:18:45Z

Hey @AlexanderBatoulis , could you please fill the checklist at the bottom of the PR's description so that we can check if it's ready for review? Thanks! 🙏

marcosmarxm · 2021-11-04T17:41:46Z

airbyte-integrations/connectors/source-zenloop/acceptance-test-config.yml

+#      expect_records:
+#        path: "integration_tests/expected_records.txt"
+#        extra_fields: no
+#        exact_order: no
+#        extra_records: yes


can you remove this?

marcosmarxm

Thanks @AlexanderBatoulis small comment also can you run ./gradlew format?

AlexanderBatoulis · 2021-11-08T16:22:04Z

Thanks @AlexanderBatoulis small comment also can you run ./gradlew format?

done

AlexanderBatoulis · 2021-11-08T16:22:20Z

Hey @AlexanderBatoulis , could you please fill the checklist at the bottom of the PR's description so that we can check if it's ready for review? Thanks! 🙏

done

marcosmarxm · 2021-11-15T16:21:29Z

Hello @AlexanderBatoulis ! Sorry to not finished your contribution before the date stipulated in the contest. All contributions made before 15-November are eligible to receive the award. We're trying to review your contribution as soon as possible.

alafanechere

Thank you very much @AlexanderBatoulis for this contribution, I have some minor questions and suggestions that you'll find in my review comments.
In addition, we configured a sandbox account for Zenloop and I'm afraid that the integration tests are not passing when we fill the public_hash_id field. The https://app.zenloop.com/settings/api page provides list of hash ids for surveys, but this value is reused to build requests to survey_groups endpoint in the AnswersSurveyGroup stream (survey_groups/{self.public_hash_id}/answers). It looks like the API expects a survey group id that is different from the public hash id provided in the config (which is expected to be a survey id and not a survey group id) and it leads to 404 error.
TLDR: integration tests did not pass when setting public_hash_id to a valid value in our config.

Could you also fix conflicts with master please? 🙏

airbyte-integrations/connectors/source-zenloop/source_zenloop/source.py

alafanechere · 2021-11-15T14:04:28Z

airbyte-integrations/connectors/source-zenloop/source_zenloop/source.py

+
+class IncrementalZenloopStream(ZenloopStream, ABC):
+    # checkpoint stream reads after 100 records.
+    state_checkpoint_interval = 100


Using state_checkpoint_interval combined with a stream_slice implementation leads to checkpointing at every 100 records of each slice, is it your original intent?

airbyte-config/init/src/main/resources/seed/source_definitions.yaml

airbyte-integrations/connectors/source-zenloop/source_zenloop/spec.json

alafanechere · 2021-11-16T07:58:29Z

airbyte-integrations/connectors/source-zenloop/unit_tests/test_incremental_streams.py

+        "cursor_field": expected_cursor_field,
+        "stream_state": {expected_cursor_field: "2021-10-20T03:30:30Z"},
+    }
+    expected_stream_slice = [None]


I think you're not testing the behavior of stream_slices if public_hash_id is None

airbyte-integrations/connectors/source-zenloop/source_zenloop/source.py

alafanechere · 2021-11-16T08:34:20Z

airbyte-integrations/connectors/source-zenloop/unit_tests/test_streams.py

+def test_request_headers(patch_base_class, config):
+    stream = ZenloopStream(config["api_token"], config["date_from"], config["public_hash_id"])
+    inputs = {"stream_slice": None, "stream_state": None, "next_page_token": None}
+    expected_headers = {}
+    assert stream.request_headers(**inputs) == expected_headers
+
+
+def test_http_method(patch_base_class, config):
+    stream = ZenloopStream(config["api_token"], config["date_from"], config["public_hash_id"])
+    expected_method = "GET"
+    assert stream.http_method == expected_method
+
+
+@pytest.mark.parametrize(
+    ("http_status", "should_retry"),
+    [
+        (HTTPStatus.OK, False),
+        (HTTPStatus.BAD_REQUEST, False),
+        (HTTPStatus.TOO_MANY_REQUESTS, True),
+        (HTTPStatus.INTERNAL_SERVER_ERROR, True),
+    ],
+)
+def test_should_retry(patch_base_class, config, http_status, should_retry):
+    response_mock = MagicMock()
+    response_mock.status_code = http_status
+    stream = ZenloopStream(config["api_token"], config["date_from"], config["public_hash_id"])
+    assert stream.should_retry(response_mock) == should_retry
+
+
+def test_backoff_time(patch_base_class, config):
+    response_mock = MagicMock()
+    stream = ZenloopStream(config["api_token"], config["date_from"], config["public_hash_id"])
+    expected_backoff_time = None
+    assert stream.backoff_time(response_mock) == expected_backoff_time


These test might be redundant because you're not overriding these methods from the parent HttpStream class

alafanechere · 2021-11-16T08:35:13Z

airbyte-integrations/connectors/source-zenloop/unit_tests/test_streams.py

+    expected_parsed_object = {"answers": [{"id": 123, "name": "John Doe"}]}
+    assert next(stream.parse_response(**inputs)) == expected_parsed_object
+
+


You could add tests for SurveyGroups and Surveys to increase coverage.

alafanechere · 2021-11-16T08:44:03Z

airbyte-integrations/connectors/source-zenloop/unit_tests/test_source.py

+def test_check_connection(mocker, config):
+    source = SourceZenloop()
+    logger_mock = MagicMock()
+    assert source.check_connection(logger_mock, config) == (True, None)


This test is not passing, you might miss some mock because the real zenloop backend seems to be called... and moreover please test the behavior in case of error.

AlexanderBatoulis · 2021-11-18T19:33:57Z

Thank you very much @AlexanderBatoulis for this contribution, I have some minor questions and suggestions that you'll find in my review comments. In addition, we configured a sandbox account for Zenloop and I'm afraid that the integration tests are not passing when we fill the public_hash_id field. The https://app.zenloop.com/settings/api page provides list of hash ids for surveys, but this value is reused to build requests to survey_groups endpoint in the AnswersSurveyGroup stream (survey_groups/{self.public_hash_id}/answers). It looks like the API expects a survey group id that is different from the public hash id provided in the config (which is expected to be a survey id and not a survey group id) and it leads to 404 error. TLDR: integration tests did not pass when setting public_hash_id to a valid value in our config.

Could you also fix conflicts with master please? 🙏

Hi @alafanechere,
Thank you very much for your thorough review! I resolved the not passing integration tests by adding another request parameter for survey_group_id.
Would be great if you could take another look and let me know if there is anything outstanding.

Kind regards,
Alex

alafanechere · 2021-11-19T08:20:00Z

HI @AlexanderBatoulis , thanks for your modifications, I'll go for another review.

alafanechere

Hi @AlexanderBatoulis , I tried to run the acceptance tests but they are failing. Could you please make sure the following command runs flawlessly?
./gradlew :airbyte-integrations:connectors:source-zenloop:integrationTest

AlexanderBatoulis · 2021-11-21T11:27:15Z

Hi @alafanechere, this should run fine now - missed to update setup.py

…into augustin/source-zenloop

alafanechere

LGTM, I'm happy with the change you've made and successfully ran the acceptance tests!
Ready to merge 👏

AlexanderBatoulis added 2 commits October 26, 2021 09:34

added source zenloop

7525eba

Added Zenloop integration

7e4f47b

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Oct 26, 2021

octavia-squidington-iii added the community label Oct 26, 2021

AlexanderBatoulis added 3 commits October 26, 2021 10:32

updated PR number in change log

6b822ce

updated README in docs/integrations

2667973

updated source_definitions.yaml

2807b19

marcosmarxm added the hacktober label Oct 26, 2021

cleaned up todo comments

bd19c8c

avaidyanatha changed the title ~~New Source: Zenloop~~ 🎉 New Source: Zenloop Oct 28, 2021

marcosmarxm assigned alafanechere Nov 3, 2021

alafanechere added the zzm label Nov 4, 2021

marcosmarxm mentioned this pull request Nov 4, 2021

CI Sandbox for Zenloop #7646

Closed

marcosmarxm reviewed Nov 4, 2021

View reviewed changes

marcosmarxm requested changes Nov 4, 2021

View reviewed changes

ran ./gradlew format & removed commented code

a18a442

alafanechere requested changes Nov 16, 2021

View reviewed changes

alafanechere removed the zzm label Nov 17, 2021

AlexanderBatoulis added 2 commits November 18, 2021 20:17

Merge branch 'master' into master

b0ef1e9

resolving review comments; added own survey_group_id input parameter

98f4e55

resolving source defintion conflict

5fe8892

add creds for zenloop

145a214

alafanechere mentioned this pull request Nov 19, 2021

Publish new source zenloop #8135

Closed

alafanechere requested changes Nov 19, 2021

View reviewed changes

added responses module to setup.py

19f3b0a

alafanechere added 3 commits November 22, 2021 09:33

Merge branch 'master' of https://github.com/AlexanderBatoulis/airbyte …

a71a73b

…into augustin/source-zenloop

Merge branch 'master' into augustin/source-zenloop

52962d8

update source specs

5b82d1e

alafanechere approved these changes Nov 22, 2021

View reviewed changes

alafanechere merged commit fd90c95 into airbytehq:master Nov 22, 2021

jrhizor mentioned this pull request Nov 23, 2021

Bump Airbyte version from 0.32.5-alpha to 0.32.6-alpha #8192

Merged

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022

🎉 New Source: Zenloop (airbytehq#7380)

a009309

marcosmarxm removed the hacktober label Oct 20, 2022

octavia-squidington-iv added bounty connectors/source/zenloop labels Oct 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 New Source: Zenloop #7380

🎉 New Source: Zenloop #7380

AlexanderBatoulis commented Oct 26, 2021 •

edited

Loading

CLAassistant commented Oct 26, 2021 •

edited

Loading

harshithmullapudi commented Oct 27, 2021

alafanechere commented Nov 4, 2021

marcosmarxm Nov 4, 2021

AlexanderBatoulis Nov 8, 2021

marcosmarxm left a comment

AlexanderBatoulis commented Nov 8, 2021

AlexanderBatoulis commented Nov 8, 2021

marcosmarxm commented Nov 15, 2021

alafanechere left a comment •

edited

Loading

alafanechere Nov 15, 2021

alafanechere Nov 16, 2021

alafanechere Nov 16, 2021

alafanechere Nov 16, 2021

alafanechere Nov 16, 2021

AlexanderBatoulis commented Nov 18, 2021

alafanechere commented Nov 19, 2021

alafanechere left a comment •

edited

Loading

AlexanderBatoulis commented Nov 21, 2021

alafanechere left a comment

		expected_parsed_object = {"answers": [{"id": 123, "name": "John Doe"}]}
		assert next(stream.parse_response(**inputs)) == expected_parsed_object

🎉 New Source: Zenloop #7380

🎉 New Source: Zenloop #7380

Conversation

AlexanderBatoulis commented Oct 26, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Test Runs

Community member or Airbyter

Airbyter

CLAassistant commented Oct 26, 2021 • edited Loading

harshithmullapudi commented Oct 27, 2021

alafanechere commented Nov 4, 2021

marcosmarxm Nov 4, 2021

Choose a reason for hiding this comment

AlexanderBatoulis Nov 8, 2021

Choose a reason for hiding this comment

marcosmarxm left a comment

Choose a reason for hiding this comment

AlexanderBatoulis commented Nov 8, 2021

AlexanderBatoulis commented Nov 8, 2021

marcosmarxm commented Nov 15, 2021

alafanechere left a comment • edited Loading

Choose a reason for hiding this comment

alafanechere Nov 15, 2021

Choose a reason for hiding this comment

alafanechere Nov 16, 2021

Choose a reason for hiding this comment

alafanechere Nov 16, 2021

Choose a reason for hiding this comment

alafanechere Nov 16, 2021

Choose a reason for hiding this comment

alafanechere Nov 16, 2021

Choose a reason for hiding this comment

AlexanderBatoulis commented Nov 18, 2021

alafanechere commented Nov 19, 2021

alafanechere left a comment • edited Loading

Choose a reason for hiding this comment

AlexanderBatoulis commented Nov 21, 2021

alafanechere left a comment

Choose a reason for hiding this comment

AlexanderBatoulis commented Oct 26, 2021 •

edited

Loading

CLAassistant commented Oct 26, 2021 •

edited

Loading

alafanechere left a comment •

edited

Loading

alafanechere left a comment •

edited

Loading