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

[Feature] Call On Hold Error Code Update #213

Merged
merged 12 commits into from
May 30, 2022
Merged

[Feature] Call On Hold Error Code Update #213

merged 12 commits into from
May 30, 2022

Conversation

AmyL219
Copy link
Contributor

@AmyL219 AmyL219 commented May 26, 2022 •
edited
Loading

Purpose

  • Add new error code related to CallOnHold feature
  • Won't do any handling for these two new added error code before GA release

MicrosoftTeams-image (34)

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

@AmyL219 AmyL219 requested review from a team as code owners May 26, 2022 17:04
@AmyL219 AmyL219 requested review from waynemo, palatter, JoshuaLai, jimchou-dev, kevinyulu and vhuseinova-msft and removed request for a team May 26, 2022 17:04
@acs-ui-bot acs-ui-bot added the task Task for completing a specific feature. label May 26, 2022
JoshuaLai
JoshuaLai approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@JoshuaLai JoshuaLai left a comment

Choose a reason for hiding this comment

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

LGTM

@AmyL219
Copy link
Contributor Author

AmyL219 commented May 27, 2022

Closed for consistency with Android side

LGTM

Inderpal said Android side couldn't add internal error code. So for consistency, I think I shouldn't merge the new added error code. What's your thought?

jimchou-dev
jimchou-dev approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@jimchou-dev jimchou-dev left a comment

Choose a reason for hiding this comment

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

LGTM

@AmyL219 AmyL219 requested a review from iaulakh May 27, 2022 22:46
kevinyulu
kevinyulu reviewed May 27, 2022
View reviewed changes
...cationUICalling/AzureCommunicationUICalling/Presentation/Manager/CompositeErrorManager.swift Outdated
if errorCode == CallCompositeErrorCode.callEvicted,
errorCode == CallCompositeErrorCode.callDenied,
errorCode == CallCompositeErrorCode.callResume,
errorCode == CallCompositeErrorCode.callHold {
Copy link
Contributor

@kevinyulu kevinyulu May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

should these errorCode be ||, looks like it will surface these call error to contoso.

  • double check if the errorCode should be exposed to contoso

@JoshuaLai
Copy link
Member

@AmyL219 i think for us it is fine. Android had already started implementing a concept of sending event codes so imagine that is what they will be leveraging. We don’t have that system in place yet. And it might be a larger reactor to start introducing it

kevinyulu
kevinyulu approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@kevinyulu kevinyulu left a comment

Choose a reason for hiding this comment

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

tested, lgtm

@AmyL219 AmyL219 merged commit 6a2da41 into feature/callOnHold May 30, 2022
@AmyL219 AmyL219 deleted the task/callHoldErrorCode branch May 30, 2022 22:52
@AmyL219 AmyL219 mentioned this pull request May 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaLai JoshuaLai JoshuaLai approved these changes

@jimchou-dev jimchou-dev jimchou-dev approved these changes

@kevinyulu kevinyulu kevinyulu approved these changes

@waynemo waynemo Awaiting requested review from waynemo waynemo was automatically assigned from Azure/acs-ui-ios-reviewer-1

@palatter palatter Awaiting requested review from palatter palatter was automatically assigned from Azure/acs-ui-ios-reviewer-1

@vhuseinova-msft vhuseinova-msft Awaiting requested review from vhuseinova-msft vhuseinova-msft was automatically assigned from Azure/acs-ui-ios-reviewer-3

@iaulakh iaulakh Awaiting requested review from iaulakh

Assignees
No one assigned
Labels
task Task for completing a specific feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AmyL219 @JoshuaLai @jimchou-dev @kevinyulu @acs-ui-bot