TDL 22684/sync all form ids and handle no questions error (#73)

* sync for all forms if form ids are not in config * update unittests * handle empty questions response * modify log statement * changelog and verison bump * remove unused code and update unittests
singer-io · Apr 26, 2023 · 8eef619 · 8eef619
1 parent 3723176
commit 8eef619
Show file tree

Hide file tree

Showing 7 changed files with 71 additions and 80 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+## 2.1.0
+  * Fixes/Handles following [#73](https://github.com/singer-io/tap-typeform/pull/73)
+    * Syncs data for all forms if `forms` field is missing in config and makes the field as optional
+    * Prevents tap from failing when there are no questions associated with a form
+
 ## 2.0.1
   * Adds a condition to check if `submitted_landings` stream's child key is blank [#69](https://github.com/singer-io/tap-typeform/pull/69)
 

diff --git a/setup.py b/setup.py
@@ -4,7 +4,7 @@
 
 setup(
     name="tap-typeform",
-    version="2.0.1",
+    version="2.1.0",
     description="Singer.io tap for extracting data from the TypeForm Responses API",
     author="bytcode.io",
     url="http://singer.io",

diff --git a/tap_typeform/__init__.py b/tap_typeform/__init__.py
@@ -2,51 +2,51 @@
 import singer
 from singer import utils
 from tap_typeform.discover import discover as _discover
-from tap_typeform.sync import _forms_to_list, sync as _sync
+from tap_typeform.sync import sync as _sync
 from tap_typeform.client import Client
 from tap_typeform.streams import Forms
 
-REQUIRED_CONFIG_KEYS = ["start_date", "token", "forms"]
+REQUIRED_CONFIG_KEYS = ["start_date", "token"]
 
 LOGGER = singer.get_logger()
 
 class FormMistmatchError(Exception):
     pass
 
-class NoFormsProvidedError(Exception):
-    pass
 
 def validate_form_ids(client, config):
     """Validate the form ids passed in the config"""
     form_stream = Forms()
 
+    api_forms = {form.get('id') for res in form_stream.get_forms(client) for form in res if form}
     if not config.get('forms'):
-        raise NoFormsProvidedError("No forms were provided in the config")
+        LOGGER.info("No form ids provided in config, fetching all forms")
+        return api_forms
 
-    config_forms = _forms_to_list(config)
-    api_forms = {form.get('id') for res in form_stream.get_forms(client) for form in res}
+    config_forms = set(map(str.strip, config.get("forms").split(',')))
 
     mismatched_forms = config_forms.difference(api_forms)
 
     if len(mismatched_forms) > 0:
         # Raise an error if any form-id from config is not matching
         # from ids from API response
         raise FormMistmatchError("FormMistmatchError: forms {} not returned by API".format(mismatched_forms))
+    return config_forms
 
 
 @utils.handle_top_exception(LOGGER)
 def main():
     args = utils.parse_args(REQUIRED_CONFIG_KEYS)
     config = args.config
     client = Client(config)
-    validate_form_ids(client, config)
+    valid_forms = validate_form_ids(client, config)
     if args.discover:
         catalog = _discover()
         catalog.dump()
     else:
         catalog = args.catalog \
             if args.catalog else _discover()
-        _sync(client, config, args.state, catalog.to_dict())
+        _sync(client, config, args.state, catalog.to_dict(), valid_forms)
 
 if __name__ == "__main__":
     main()
diff --git a/tap_typeform/streams.py b/tap_typeform/streams.py
@@ -1,4 +1,3 @@
-from copy import deepcopy
 import json
 import pendulum
 from datetime import datetime
@@ -173,8 +172,12 @@ def sync_obj(self, client, state, catalogs, form_id,
         stream_catalog = get_schema(catalogs, self.tap_stream_id)
         response = client.request(full_url, params=self.params)
 
+        if self.data_key not in response:
+            LOGGER.info('There are no questions associated with form {}'.format(form_id))
+            return
+
         for record in response[self.data_key]:
-            self.add_fields_at_1st_level(record,{"form_id": form_id})
+            self.add_fields_at_1st_level(record, {"form_id": form_id})
 
         write_records(stream_catalog, self.tap_stream_id, response[self.data_key])
         self.records_count[self.tap_stream_id] += len(response[self.data_key])

diff --git a/tap_typeform/sync.py b/tap_typeform/sync.py
@@ -1,15 +1,8 @@
 import singer
-import pendulum
 from tap_typeform.streams import STREAMS
 
 LOGGER = singer.get_logger()
 
-def _forms_to_list(config, keyword='forms'):
-    """
-    Splits entries into a list and strips out surrounding blank spaces.
-    """
-    return set(map(str.strip, config.get(keyword).split(',')))
-
 
 def write_schemas(stream_id, catalog, selected_streams):
     """
@@ -50,7 +43,7 @@ def get_stream_to_sync(selected_streams):
             streams_to_sync.append(stream_name)
     return streams_to_sync
 
-def sync(client, config, state, catalog):
+def sync(client, config, state, catalog, forms_to_sync):
     """
     Sync selected streams.
     """
@@ -78,7 +71,7 @@ def sync(client, config, state, catalog):
         elif not stream_obj.parent:
             write_schemas(stream, catalog, selected_streams)
 
-            for form in _forms_to_list(config):
+            for form in forms_to_sync:
 
                 stream_obj.sync_obj(client, state, catalog['streams'], form, config["start_date"],
                                     selected_streams, records_count)

diff --git a/tests/unittests/test_main.py b/tests/unittests/test_main.py
@@ -1,19 +1,20 @@
 import unittest
 from unittest import mock
-from tap_typeform import (main, NoFormsProvidedError,
-                            FormMistmatchError, validate_form_ids)
+from tap_typeform import (main,
+                          FormMistmatchError, validate_form_ids)
 from singer.catalog import Catalog
 
 
 class MockArgs:
     """Mock args object class"""
 
-    def __init__(self, config = None, catalog = None, state = {}, discover = False) -> None:
-        self.config = config 
+    def __init__(self, config=None, catalog=None, state={}, discover=False) -> None:
+        self.config = config
         self.catalog = catalog
         self.state = state
         self.discover = discover
 
+
 @mock.patch("tap_typeform.validate_form_ids")
 @mock.patch("singer.utils.parse_args")
 @mock.patch("tap_typeform._discover")
@@ -31,23 +32,23 @@ def test_discover_with_config(self, mock_sync, mock_discover, mock_args, mock_va
         Test `_discover` function is called for discover mode.
         """
         mock_discover.dump.return_value = dict()
-        mock_args.return_value = MockArgs(discover = True, config = self.mock_config)
+        mock_args.return_value = MockArgs(discover=True, config=self.mock_config)
         main()
 
         self.assertTrue(mock_discover.called)
         self.assertFalse(mock_sync.called)
 
-
     def test_sync_with_catalog(self, mock_sync, mock_discover, mock_args, mock_validate):
         """
         Test sync mode with catalog given in args.
         """
 
-        mock_args.return_value = MockArgs(config=self.mock_config, catalog=Catalog.from_dict(self.mock_catalog))
+        mock_args.return_value = MockArgs(config=self.mock_config,
+                                          catalog=Catalog.from_dict(self.mock_catalog))
         main()
 
         # Verify `_sync` is called with expected arguments
-        mock_sync.assert_called_with(mock.ANY, self.mock_config, {}, self.mock_catalog)
+        mock_sync.assert_called_with(mock.ANY, self.mock_config, {}, self.mock_catalog, mock_validate.return_value)
 
         # verify `_discover` function is not called
         self.assertFalse(mock_discover.called)
@@ -63,7 +64,7 @@ def test_sync_without_catalog(self, mock_sync, mock_discover, mock_args, mock_va
         main()
 
         # Verify `_sync` is called with expected arguments
-        mock_sync.assert_called_with(mock.ANY, self.mock_config, {}, {"schema": "", "metadata": ""})
+        mock_sync.assert_called_with(mock.ANY, self.mock_config, {}, {"schema": "", "metadata": ""}, mock_validate.return_value)
 
         # verify `_discover` function is  called
         self.assertTrue(mock_discover.called)
@@ -73,49 +74,65 @@ def test_sync_with_state(self, mock_sync, mock_discover, mock_args, mock_validat
         Test sync mode with the state given in args.
         """
         mock_state = {"bookmarks": {"projects": ""}}
-        mock_args.return_value = MockArgs(config=self.mock_config, catalog=Catalog.from_dict(self.mock_catalog), state=mock_state)
+        mock_args.return_value = MockArgs(config=self.mock_config,
+                                          catalog=Catalog.from_dict(self.mock_catalog),
+                                          state=mock_state)
         main()
 
         # Verify `_sync` is called with expected arguments
-        mock_sync.assert_called_with(mock.ANY, self.mock_config, mock_state, self.mock_catalog)
+        mock_sync.assert_called_with(mock.ANY, self.mock_config, mock_state, self.mock_catalog, mock_validate.return_value)
 
 
 @mock.patch("tap_typeform.Forms")
 class TestValidateFormIds(unittest.TestCase):
     """
     Test `validate_form_ids` function.
     """
-    
+
     def test_all_correct_forms(self, mock_forms):
         """
         Test when proper form ids are passed, No error raised.
         """
         config = {"forms": "form1,form2"}
-        mock_forms.return_value.get_forms.return_value = [[{'id': 'form1'}, {'id': 'form2'}, {'id': 'form3'}]]
+        mock_forms.return_value.get_forms.return_value = [
+            [{'id': 'form1'}, {'id': 'form2'}, {'id': 'form3'}]]
 
         # Verify no exception was raised
-        validate_form_ids(None, config)
+        api_forms = validate_form_ids(None, config)
+
+        # Assertion to test validate_form_ids return only the configured form IDs
+        self.assertEqual(api_forms, {"form1", "form2"})
 
     def test_no_form_given(self, mock_forms):
         """
-        Test when no forms are given in config, an error is raised with an expected message.
+        Test when no forms are given in config, a statement is logged with an expected message.
         """
         config = {}
-        with self.assertRaises(NoFormsProvidedError) as e:
-            validate_form_ids(None, config)
+        mock_forms.return_value.get_forms.return_value = [
+            [{'id': 'form1'}, {'id': 'form2'}, {'id': 'form3'}]]
+        with self.assertLogs(level='INFO') as log_statement:
+            api_forms = validate_form_ids(None, config)
+            self.assertEqual(log_statement.output,
+                             ['INFO:root:No form ids provided in config, fetching all forms'])
+
+        # Assertion to make sure we call the get_forms function once
+        self.assertEqual(mock_forms.return_value.get_forms.call_count, 1)
+
+        # Assertion to test validate_form_ids returns all the form IDs from API response
+        self.assertEqual(api_forms, {"form1", "form2", "form3"})
+
 
-        # Verify exception raised with expected error message
-        self.assertEqual(str(e.exception), "No forms were provided in the config")
-
     def test_mismatch_forms(self, mock_forms):
         """
         Test wrong form ids given in config raise MismatchError.
         """
         config = {"forms": "form1,form4"}
-        mock_forms.return_value.get_forms.return_value = [[{'id': 'form1'}, {'id': 'form2'}, {'id': 'form3'}]]
-
+        mock_forms.return_value.get_forms.return_value = [
+            [{'id': 'form1'}, {'id': 'form2'}, {'id': 'form3'}]]
+
         with self.assertRaises(FormMistmatchError) as e:
             validate_form_ids(None, config)
 
         # Verify exception raised with expected error message
-        self.assertEqual(str(e.exception), "FormMistmatchError: forms {} not returned by API".format({"form4"}))
+        self.assertEqual(str(e.exception),
+                         "FormMistmatchError: forms {} not returned by API".format({"form4"}))
diff --git a/tests/unittests/test_sync.py b/tests/unittests/test_sync.py
@@ -1,8 +1,7 @@
 import unittest
 from unittest import mock
-from parameterized import parameterized
 from tap_typeform.sync import (sync, get_stream_to_sync,
-                                get_selected_streams, write_schemas, _forms_to_list)
+                                get_selected_streams, write_schemas)
 
 def get_stream_catalog(stream_name, selected = False):
     return {
@@ -27,7 +26,6 @@ def get_stream_catalog(stream_name, selected = False):
     "answers": 0
 }
 
-@mock.patch("tap_typeform.sync.pendulum")
 @mock.patch("tap_typeform.sync.get_selected_streams")
 @mock.patch("tap_typeform.sync.get_stream_to_sync")
 @mock.patch("tap_typeform.sync.write_schemas")
@@ -40,14 +38,14 @@ class TestSyncFunction(unittest.TestCase):
     config = {'start_date': "START_DATE"}
 
     @mock.patch("tap_typeform.streams.Forms.sync_obj")
-    def test_syncing_form(self, mock_sync_obj, mock_write_schema, mock_sync_streams, mock_selected_streams, mock_pendulum):
+    def test_syncing_form(self, mock_sync_obj, mock_write_schema, mock_sync_streams, mock_selected_streams):
         """
         Test for `forms` stream, its sync_object is called with proper arguments.
         """
         mock_selected_streams.return_value = ['forms']
         mock_sync_streams.return_value = ['forms']
 
-        sync(mock.Mock(), self.config, {}, self.catalog)
+        sync(mock.Mock(), self.config, {}, self.catalog, {'forms'})
 
         # Verify that write schema is called once for one selected stream
         self.assertEqual(mock_write_schema.call_count, 1)
@@ -57,17 +55,15 @@ def test_syncing_form(self, mock_sync_obj, mock_write_schema, mock_sync_streams,
         mock_sync_obj.assert_called_with(mock.ANY, {}, {}, "START_DATE", ['forms'], records_count)
 
     @mock.patch("tap_typeform.streams.SubmittedLandings.sync_obj")
-    @mock.patch("tap_typeform.sync._forms_to_list")
-    def test_only_child_selected(self, mock_form_list, mock_sync_obj,
-                                    mock_write_schema, mock_sync_streams, mock_selected_streams, mock_pendulum):
+    def test_only_child_selected(self, mock_sync_obj,
+                                    mock_write_schema, mock_sync_streams, mock_selected_streams):
         """
         Test for only child selected, parent sync object is called with proper arguments.
         """
-        mock_form_list.return_value = ['form1']
         mock_selected_streams.return_value = ['answers']
         mock_sync_streams.return_value = ['submitted_landings', 'answers']
 
-        sync(mock.Mock(), self.config, {}, self.catalog)
+        sync(mock.Mock(), self.config, {}, self.catalog, {"form1"})
 
         # Verify that write schema is called once for one selected stream
         self.assertEqual(mock_write_schema.call_count, 1)
@@ -77,13 +73,11 @@ def test_only_child_selected(self, mock_form_list, mock_sync_obj,
         mock_sync_obj.assert_called_with(mock.ANY, {}, {}, 'form1', "START_DATE", ['answers'], records_count)
 
     @mock.patch("tap_typeform.streams.SubmittedLandings.sync_obj")
-    @mock.patch("tap_typeform.sync._forms_to_list")
-    def test_for_multiple_forms(self, mock_form_list, mock_sync_obj,
-                                mock_write_schema, mock_sync_streams, mock_selected_streams, mock_pendulum):
+    def test_for_multiple_forms(self, mock_sync_obj,
+                                mock_write_schema, mock_sync_streams, mock_selected_streams):
         """
         Test for only child selected, parent sync object is called with proper arguments.
         """
-        mock_form_list.return_value = ['form1', 'form2', 'form3']
         mock_selected_streams.return_value = ['submitted_landings']
         mock_sync_streams.return_value = ['submitted_landings']
         expected_calls = [
@@ -92,7 +86,7 @@ def test_for_multiple_forms(self, mock_form_list, mock_sync_obj,
             mock.call(mock.ANY, {}, {}, 'form3', "START_DATE", ['submitted_landings'], records_count),
         ]
 
-        sync(mock.Mock(), self.config, {}, self.catalog)
+        sync(mock.Mock(), self.config, {}, self.catalog, {"form1", "form2", "form3"})
 
         # Verify that write schema is called once for one selected stream
         self.assertEqual(mock_write_schema.call_count, 1)
@@ -202,24 +196,3 @@ def test_for_child_selected(self, mock_write_schema):
         self.assertEqual(mock_write_schema.call_count, 2)
         self.assertIn(mock_write_schema.mock_calls[0], expected_calls)
         self.assertIn(mock_write_schema.mock_calls[1], expected_calls)
-
-
-class TestFormsListParsing(unittest.TestCase):
-    """
-    Test form list parsing and white space handling.
-    """
-
-    @parameterized.expand([
-        ("form1,form2,form3",['form1', 'form2', 'form3']),
-        ("form1, form2, form3",['form1', 'form2', 'form3']),
-        ("    form1, form2   , form3   ",['form1', 'form2', 'form3']),
-    ])
-    def test_forms_to_list(self, forms, expected_list):
-        """
-        Test various test cases for form_list with whitespaces.
-        """
-        config = {'forms': forms}
-        form_list = _forms_to_list(config)
-
-        # Verify that returned list contains expected form_ids
-        self.assertCountEqual(form_list, expected_list)