Skip to content

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

Merged
kethan1122 merged 6 commits intomasterfrom
TDL-22684/sync-all-form-ids
Apr 26, 2023
Merged

TDL 22684/sync all form ids and handle no questions error#73
kethan1122 merged 6 commits intomasterfrom
TDL-22684/sync-all-form-ids

Conversation

@kethan1122
Copy link
Contributor

@kethan1122 kethan1122 commented Apr 14, 2023

Description of change

Handles/Fixes:

  • Syncs data for all forms if no form ID is provided in config.
  • Currently, tap fails with an following error when there are no questions associated with a form. Handles this scenario by logging a statement and continues to sync data for other streams.
  • Updates unittests
CRITICAL 'fields'
Traceback (most recent call last):
  File "/Users/Documents/project/taps/tap-typeform/tap_typeform/__init__.py", line 54, in <module>
    main()
  File "/Users/Documents/project/taps/tap-typeform/typeform-venv/lib/python3.9/site-packages/singer_python-5.10.0-py3.9.egg/singer/utils.py", line 229, in wrapped
    return fnc(*args, **kwargs)
  File "/Users/Documents/project/taps/tap-typeform/tap_typeform/__init__.py", line 51, in main
    _sync(client, config, args.state, catalog.to_dict(), valid_forms)
  File "/Users/Documents/project/taps/tap-typeform/typeform-venv/lib/python3.9/site-packages/tap_typeform-2.0.1-py3.9.egg/tap_typeform/sync.py", line 76, in sync
    stream_obj.sync_obj(client, state, catalog['streams'], form, config["start_date"],
  File "/Users/Documents/project/taps/tap-typeform/typeform-venv/lib/python3.9/site-packages/tap_typeform-2.0.1-py3.9.egg/tap_typeform/streams.py", line 176, in sync_obj
    for record in response[self.data_key]:
KeyError: 'fields'

Manual QA steps

  • Tested without having forms field in config and noticed data being extracted for all the forms.
  • Tested with forms field value as "" and None and noticed data being extracted for all the forms.
  • Tested with single valid form_id in config and noticed data being extracted only for that single form id.
  • Tested with multiple valid form_ids which are comma separated in config and noticed data being extracted only for those form IDs
  • Created a form with no questions and made sure tap logs a statement which reads There are no questions associated with form without failing.

Risks

  • Low, all the existing connections should be having forms field configured. This changes will not have any impact on them

Rollback steps

  • revert this branch

Copy link
Contributor

@dsprayberry dsprayberry left a comment

Choose a reason for hiding this comment

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

We can remove the NoFormsProvidedError class since it will no longer be used.

We should probably move api_forms into the if not config.get('forms') to ensure we're not making extraneous calls when we already have user provided forms.

Can we also add unit test assertions around fetching forms (via api_forms) when none are provided in addition to raising the log warning?

"""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}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be within the "if not" block on 24 to make sure we're not getting all forms even when a user has provided forms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also needed to validate the form ids configured by the user, so keeping it as it is

Choose a reason for hiding this comment

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

Agree with @kethan1122 ; this step is used to verify that the form id's provided are valid ids. It now is also used to capture all forms from the account.

@kethan1122 kethan1122 requested a review from dsprayberry April 17, 2023 14:24
@aaronbannin
Copy link

Thanks @kethan1122 and @dsprayberry!

@kethan1122 kethan1122 merged commit 8eef619 into master Apr 26, 2023
@kethan1122 kethan1122 deleted the TDL-22684/sync-all-form-ids branch April 26, 2023 05:14
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.

3 participants