feat(calling): move apis, listeners from calling client to line #3073

Shreyas281299 · 2023-09-11T19:35:43Z

COMPLETES #SPARK-428389

This pull request addresses

Required changes for line class.

Move below from callingClient to line:
- makeCall()
- incomingCallListener()
- getCallManager()
Add below new APIs in calling client and line:

CallingClient:
- getActiveCalls()
- getConnectedCall()
Line:
- getCall()

Change call event from callingClient:incomingCall to line:incomingCall.
To store lineData in the callManager. Added lineDict in call manager that holds the {deviceId: lineId} information. Also added addLine and getLine api to populate and fetch the lineDict
Changes in the samples page
- event handler for line:incoming_call event
- changes from callingClient.makeCall to line.makeCall

Change Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Tooling change
Internal code refactor

The following scenarios where tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

I certified that

I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

arun3528 · 2023-09-12T03:33:04Z

packages/calling/src/CallingClient/CallingClient.ts

+      const {lineId} = this.lineDict[deviceId];
+      activeCalls[lineId] = [];
+      Object.keys(allCalls).forEach((correlationId) => {
+        if (allCalls[correlationId].getLineId() === lineId) {


i am little confused with the logic here ,

Why does someone want to receive all active calls in all line ?

if that's the intention do you think we need to maintain the keys of those calls in a array when ever the call gets active , any thoughts

@arun3528 So the structure this API will return is in the form of key-value where the key is the lineId and the value is an array of the calls active on that line.
The intention here is to just return that data back to the user if they want to fetch the information of all the calls that are active. When we say active here, it just means call is present but it can be in any state.

arun3528 · 2023-09-12T03:33:08Z

packages/calling/src/CallingClient/CallingClient.ts

+  /**
+   *
+   */
+  public getActiveCalls(): Record<string, ICall[]> {


needs some description for the function

@Kesari3008 Can we have an optional parameter lineId which will only return calls from a a single line?

Suggested change

public getActiveCalls(): Record<string, ICall[]> {

public getActiveCalls(lineId?: string): Record<string, ICall[]> {

Yeah, we can do that I guess. Let's discuss on it Monday

We will do this as part of separate PR and discuss the approach for the same

arun3528 · 2023-09-12T03:33:51Z

packages/calling/src/CallingClient/CallingClient.ts

+  /**
+   *
+   */
+  public getConnectedCall(): ICall | undefined {


same here need comments

do you think its better to take line id as a parameter

@arun3528 The purpose of creating this API was that if the user wants to fetch the information regarding which call is in a connected state at any point in time, they can call this API and get the call object which will have lineId as well.
Now they have this information on which line which call is in the connected state. Hence we need not provide lineId as a parameter here.

If we take lineId as a parameter here then the user has to keep providing each lineId one by one, calling this API multiple times and find which call is in the connected state.

Added description

Kesari3008

I can see unused import in few files, please check and remove them

packages/calling/src/CallingClient/calling/call.ts

packages/calling/src/Events/types.ts

Kesari3008 · 2023-09-20T08:17:02Z

packages/calling/src/CallingClient/CallingClient.ts

+    let connectCall;
+    const allCalls = this.callManager.getActiveCalls();
+    Object.keys(allCalls).forEach((correlationId) => {
+      if (allCalls[correlationId].isConnected() && !allCalls[correlationId].isHeld()) {


Do we really need to specify a second condition here too? For a call which is on hold state, isConnected should return false for that call so just one condition should suffice here. Let's discuss on this if needed

For a held call, isConnected returns true and isHeld returns false. That's why both the conditions are required.

I am not sure if we should keep it like that, ideally isConnected should be false if the call is put on hold

packages/calling/src/CallingClient/CallingClient.ts

packages/calling/src/CallingClient/calling/callManager.ts

packages/calling/src/CallingClient/line/line.test.ts

packages/calling/src/CallingClient/calling/call.ts

packages/calling/src/CallingClient/CallingClient.ts

BhargavSatya · 2023-09-22T09:47:00Z

packages/calling/src/CallingClient/CallingClient.ts

+      const {lineId} = this.lineDict[deviceId];
+      activeCalls[lineId] = [];
+      Object.keys(calls).forEach((correlationId) => {
+        if (calls[correlationId].lineId === lineId) {


Don't we have a getter on lineId? Is lineId a public attribute?

Suggested change

if (calls[correlationId].lineId === lineId) {

if (calls[correlationId].getLineId() === lineId) {

We dont have a getter for lineId. Yes, lineId is a public attribute.

packages/calling/src/CallingClient/CallingClient.ts

BhargavSatya · 2023-09-22T10:37:21Z

packages/calling/src/CallingClient/calling/call.ts

@@ -2659,5 +2663,6 @@ export const createCall = (
  dir: CallDirection,
  deviceId: string,
  deleteCb: DeleteRecordCallBack,
-  indicator: ServiceIndicator
-): ICall => new Call(activeUrl, webex, dest, dir, deviceId, deleteCb, indicator);
+  indicator: ServiceIndicator,


Please add lineId in the description

Kesari3008 · 2023-09-22T19:08:36Z

packages/calling/src/CallingClient/CallingClient.ts

+  /**
+   *
+   */
+  public getActiveCalls(): Record<string, ICall[]> {


Yeah, we can do that I guess. Let's discuss on it Monday

Kesari3008 · 2023-09-22T19:11:19Z

packages/calling/src/CallingClient/CallingClient.ts

+    let connectCall;
+    const allCalls = this.callManager.getActiveCalls();
+    Object.keys(allCalls).forEach((correlationId) => {
+      if (allCalls[correlationId].isConnected() && !allCalls[correlationId].isHeld()) {


I am not sure if we should keep it like that, ideally isConnected should be false if the call is put on hold

packages/calling/src/CallingClient/CallingClient.ts

packages/calling/src/CallingClient/calling/callManager.ts

packages/calling/src/CallingClient/line/line.test.ts

packages/calling/src/CallingClient/CallingClient.test.ts

packages/calling/src/CallingClient/calling/callManager.ts

Kesari3008 · 2023-09-25T13:38:41Z

packages/calling/src/CallingClient/CallingClient.test.ts

+      mockCall2['connected'] = true;
+      mockCall2['held'] = true;
+      mockCall2['earlyMedia'] = false;
+      mockCall2['callStateMachine'].state.value = 'S_CALL_ESTABLISHED';


Shouldn't the state here be S_CALL_HOLD ?

Yes, it should be S_CALL_HOLD. Changed

Kesari3008

Please take care of below points:

One minor comment in UT file.
isConnected state needs to be corrected so let's make a note of it.
Ensure all the conflicts are resolved before merge and all UT's are passing.
Remove extra imports if there are any present.
Found one UT in callManager.test.ts file showing error for localAudioTrack being passed to call.answer(). Please fix that before merge.

BhargavSatya · 2023-09-26T03:10:05Z

Please take care of Priya's approval comments.

…x#3073) Co-authored-by: Shreyas Sharma <[email protected]>

Shreyas281299 added the validated If the pull request is validated for automation. label Sep 11, 2023

Shreyas281299 requested a review from a team as a code owner September 11, 2023 19:35

arun3528 reviewed Sep 12, 2023

View reviewed changes

arun3528 requested changes Sep 12, 2023

View reviewed changes

Shreyas281299 force-pushed the SPARK-428389-Move-apis-listeners-from-callingClient-to-line branch 5 times, most recently from f3e448f to ae7eb2f Compare September 14, 2023 08:49

Kesari3008 requested changes Sep 20, 2023

View reviewed changes

Shreyas281299 force-pushed the SPARK-428389-Move-apis-listeners-from-callingClient-to-line branch from ae7eb2f to 6a52910 Compare September 21, 2023 08:48

BhargavSatya self-requested a review September 21, 2023 11:12

BhargavSatya reviewed Sep 22, 2023

View reviewed changes

packages/calling/src/CallingClient/calling/call.ts Outdated Show resolved Hide resolved

BhargavSatya reviewed Sep 22, 2023

View reviewed changes

packages/calling/src/CallingClient/CallingClient.ts Outdated Show resolved Hide resolved

BhargavSatya reviewed Sep 22, 2023

View reviewed changes

packages/calling/src/CallingClient/CallingClient.ts Outdated Show resolved Hide resolved

BhargavSatya reviewed Sep 22, 2023

View reviewed changes

Kesari3008 requested changes Sep 22, 2023

View reviewed changes

Shreyas281299 force-pushed the SPARK-428389-Move-apis-listeners-from-callingClient-to-line branch 4 times, most recently from 472b7a0 to eeb0204 Compare September 25, 2023 11:21

Kesari3008 reviewed Sep 25, 2023

View reviewed changes

Kesari3008 approved these changes Sep 25, 2023

View reviewed changes

BhargavSatya approved these changes Sep 26, 2023

View reviewed changes

Shreyas281299 force-pushed the SPARK-428389-Move-apis-listeners-from-callingClient-to-line branch from eeb0204 to a3ca42d Compare September 26, 2023 05:54

feat(calling): move apis, listeners from calling client to line

701551b

Shreyas281299 force-pushed the SPARK-428389-Move-apis-listeners-from-callingClient-to-line branch from a3ca42d to 701551b Compare September 27, 2023 05:30

Shreyas281299 merged commit f09f3b2 into webex:next Sep 27, 2023
10 of 11 checks passed

Shreyas281299 added a commit to Shreyas281299/webex-js-sdk that referenced this pull request Oct 3, 2023

feat(calling): move apis, listeners from calling client to line (webe…

eee16bf

…x#3073) Co-authored-by: Shreyas Sharma <[email protected]>

Kesari3008 pushed a commit to Kesari3008/webex-js-sdk that referenced this pull request Oct 3, 2023

feat(calling): move apis, listeners from calling client to line (webe…

81c9385

…x#3073) Co-authored-by: Shreyas Sharma <[email protected]>

Shreyas281299 deleted the SPARK-428389-Move-apis-listeners-from-callingClient-to-line branch August 6, 2024 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(calling): move apis, listeners from calling client to line #3073

feat(calling): move apis, listeners from calling client to line #3073

Shreyas281299 commented Sep 11, 2023 •

edited

Loading

arun3528 Sep 12, 2023

Kesari3008 Sep 12, 2023

arun3528 Sep 12, 2023

Shreyas281299 Sep 21, 2023

BhargavSatya Sep 22, 2023

Kesari3008 Sep 22, 2023

Kesari3008 Sep 25, 2023

arun3528 Sep 12, 2023

Kesari3008 Sep 12, 2023

Shreyas281299 Sep 21, 2023

Kesari3008 left a comment

Kesari3008 Sep 20, 2023

Shreyas281299 Sep 21, 2023

Kesari3008 Sep 22, 2023

BhargavSatya Sep 22, 2023

Shreyas281299 Sep 25, 2023

BhargavSatya Sep 22, 2023

Shreyas281299 Sep 25, 2023

Kesari3008 Sep 22, 2023

Kesari3008 Sep 22, 2023

Kesari3008 Sep 25, 2023

Shreyas281299 Sep 26, 2023

Kesari3008 left a comment

BhargavSatya commented Sep 26, 2023

	public getActiveCalls(): Record<string, ICall[]> {
	public getActiveCalls(lineId?: string): Record<string, ICall[]> {

	if (calls[correlationId].lineId === lineId) {
	if (calls[correlationId].getLineId() === lineId) {

feat(calling): move apis, listeners from calling client to line #3073

feat(calling): move apis, listeners from calling client to line #3073

Conversation

Shreyas281299 commented Sep 11, 2023 • edited Loading

COMPLETES #SPARK-428389

This pull request addresses

Change Type

The following scenarios where tested

I certified that

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kesari3008 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kesari3008 left a comment

Choose a reason for hiding this comment

BhargavSatya commented Sep 26, 2023

Shreyas281299 commented Sep 11, 2023 •

edited

Loading