Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add talk api #53

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add talk api #53

wants to merge 8 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 19, 2021 •
edited by ghost
Loading

Description of change

Adds new streams from the ZenDesk Talk API:

  • talk_phone_nuber
  • talk_chats

Required changes before merge:

Manual QA steps

  • tested in my test environment

Risks

  • might break because we upgrade from singer 5.2.1 to 5.9.0. I did not get any issues during my tests.
  • I did not check if breaking changes happen during zenpy 2.0.0 and 2.0.22

Rollback steps

  • revert this branch

@ghost ghost mentioned this pull request Jan 26, 2021
Open
@leo-schick
Copy link

@kspeer825 This PR has been created with an old account from mine. I did now a update & rebase so this is now ready to be merged. Could you merge it - or close it and I will create a new one since this account does not exist anymore.
I can't put this PR from Draft to Reday to be merged mode anymore.

@jtilala jtilala marked this pull request as ready for review June 27, 2022 10:31
@jtilala
Copy link
Contributor

jtilala commented Jun 27, 2022

Hi @leo-schick, I have converted your PR from draft mode to Ready to review.

@leo-schick
Copy link

@jtilala I did a rebase now on branch master. Can you do the review/merge as well?

@jtilala jtilala mentioned this pull request Jul 6, 2022
Merged
dbshah1212
dbshah1212 reviewed Sep 8, 2022
View reviewed changes
tap_zendesk/schemas/archive/talk_availabilities.json Outdated
"type": ["object"],
"properties": {
"agent_id": {
"type": ["integer"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"type": ["integer"]
"type": ["integer", "null"]

Copy link

Choose a reason for hiding this comment

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

Done

dbshah1212
dbshah1212 reviewed Sep 8, 2022
View reviewed changes
tap_zendesk/schemas/talk_calls.json Outdated
"enum": ["queue", "web widget"]
},
"completion_status": {
"type": ["string"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Add null in type.

Copy link

Choose a reason for hiding this comment

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

Done

dbshah1212
dbshah1212 suggested changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@dbshah1212 dbshah1212 left a comment

Choose a reason for hiding this comment

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

I provided some null addition comments and this branch is out-of-date with the base branch. Can you please rebase?

@leo-schick
Copy link

@dbshah1212 Rebase is done.

dbshah1212
dbshah1212 reviewed Sep 8, 2022
View reviewed changes
tap_zendesk/schemas/talk_calls.json Outdated
"description": "The call was answered by an agent who is a member of the call's default group, if group routing is used"
},
"direction": {
"type": ["string"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add "null" to handle the empty value. Do add null at all places.

Copy link

Choose a reason for hiding this comment

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

Done

@dbshah1212
Copy link
Contributor

Looks good to me now, @KrisPersonal integration tests are not running for this branch as it is raised from the personal repo. Can we merge this one of the singer branches and then merge that in master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbshah1212 dbshah1212 Awaiting requested review from dbshah1212

@KrisPersonal KrisPersonal Awaiting requested review from KrisPersonal

@RushiT0122 RushiT0122 Awaiting requested review from RushiT0122

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leo-schick @jtilala @dbshah1212