-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tdl 12262 add ticket fields stream #52
base: TDL-20356-update-function-based-to-class-based
Are you sure you want to change the base?
Tdl 12262 add ticket fields stream #52
Conversation
…L-12262-add-ticket_fields-stream
…L-12262-add-ticket_fields-stream
…L-12262-add-ticket_fields-stream
tests/test_freshdesk_pagination.py
Outdated
@@ -43,7 +43,7 @@ def test_run(self): | |||
with self.subTest(stream=stream): | |||
# Not able to generate more data as roles stream requires pro account. | |||
# So, updating page_sie according to data available. | |||
if stream == "roles": | |||
if stream == "roles" or stream == "ticket_fields": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if stream == "roles" or stream == "ticket_fields": | |
if stream in ["roles", "ticket_fields"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
tests/test_freshdesk_pagination.py
Outdated
if stream in ["roles", "ticket_fields"]: | ||
page_size = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are we setting page_size=2
in the test? I could not find it in get_properties()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are setting page_size
property in the base.py
. It was added in the PR#51 and we have raised this pr from it. Back merge remained and that's why this change was not reflected here.
…L-12262-add-ticket_fields-stream
…L-12262-add-ticket_fields-stream
tests/test_freshdesk_start_date.py
Outdated
expected_streams = self.expected_streams() | ||
|
||
# To collect "time_entries", "satisfaction_ratings" pro account is needed. Skipping them for now. | ||
expected_streams = expected_streams - {'satisfaction_ratings', 'time_entries'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we merge these two separate lines into one?
expected_streams = self.expected_streams() - {'satisfaction_ratings', 'time_entries'}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged mentioned two lines to reduce redundancy.
"portal_cc_to": { | ||
"type": ["null", "string"] | ||
}, | ||
"choices": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the type is not mentioned for choices
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the Freshdesk API documentation, choices
is the list of values. But, in the actual response, we observed different types of values including a list of strings
, plain object(dict),
an object of the list
, etc. That's why due to uncertainty in the value of this field, we kept {}
as datatype and generally we keep {}
in such cases.
tests/test_freshdesk_start_date.py
Outdated
@@ -14,6 +14,21 @@ def name(): | |||
return "tap_tester_freshdesk_start_date_test" | |||
|
|||
def test_run(self): | |||
# Streams to verify start date tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the doc string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added docstring to describe the test_run
function.
Description of change
ticket_fields
.ticket_fields
.Manual QA steps
Risks
Rollback steps