-
Notifications
You must be signed in to change notification settings - Fork 344
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
feat(Webinar) - Locus API and DTO update for Webinar #3972
base: next
Are you sure you want to change the base?
Changes from all commits
eddcf83
c8c80c7
8c807dd
de1a436
ad2df04
46831bc
3c6b54f
b9f5a5d
bebcc13
4dad0f1
9c48a68
93c4121
9d982ee
b32b5ad
1388e86
2d8ac63
fb00d2a
df719cb
e172fb2
aebbbc7
2a24f75
a3b447d
952c520
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -177,7 +177,12 @@ export default class ControlsOptionsManager { | |||||||||||||||||||||||||||||||||||||||
* @memberof ControlsOptionsManager | ||||||||||||||||||||||||||||||||||||||||
* @returns {Promise} | ||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||
private setControls(setting: {[key in Setting]?: boolean}): Promise<any> { | ||||||||||||||||||||||||||||||||||||||||
private setControls(setting: { | ||||||||||||||||||||||||||||||||||||||||
[Setting.muted]?: boolean; | ||||||||||||||||||||||||||||||||||||||||
[Setting.disallowUnmute]?: boolean; | ||||||||||||||||||||||||||||||||||||||||
[Setting.muteOnEntry]?: boolean; | ||||||||||||||||||||||||||||||||||||||||
[Setting.roles]?: Array<string>; | ||||||||||||||||||||||||||||||||||||||||
}): Promise<any> { | ||||||||||||||||||||||||||||||||||||||||
LoggerProxy.logger.log( | ||||||||||||||||||||||||||||||||||||||||
`ControlsOptionsManager:index#setControls --> ${JSON.stringify(setting)}` | ||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||
|
@@ -190,6 +195,7 @@ export default class ControlsOptionsManager { | |||||||||||||||||||||||||||||||||||||||
Object.entries(setting).forEach(([key, value]) => { | ||||||||||||||||||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||||||||||||||||||
!shouldSkipCheckToMergeBody && | ||||||||||||||||||||||||||||||||||||||||
value !== undefined && | ||||||||||||||||||||||||||||||||||||||||
!Util?.[`${value ? CAN_SET : CAN_UNSET}${key}`](this.displayHints) | ||||||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||||||
error = new PermissionError(`${key} [${value}] not allowed, due to moderator property.`); | ||||||||||||||||||||||||||||||||||||||||
|
@@ -219,6 +225,14 @@ export default class ControlsOptionsManager { | |||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
case Setting.roles: | ||||||||||||||||||||||||||||||||||||||||
if (Array.isArray(value)) { | ||||||||||||||||||||||||||||||||||||||||
body.audio = body.audio | ||||||||||||||||||||||||||||||||||||||||
? {...body.audio, [camelCase(key)]: value} | ||||||||||||||||||||||||||||||||||||||||
: {[camelCase(key)]: value}; | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||
Comment on lines
+228
to
+234
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add validation for role values While the array check is good, consider adding validation for the role strings to prevent invalid roles from being processed. case Setting.roles:
if (Array.isArray(value)) {
+ // Validate that all roles are valid strings
+ if (!value.every((role) => typeof role === 'string' && role.trim().length > 0)) {
+ error = new PermissionError('Invalid role value detected');
+ break;
+ }
body.audio = body.audio
? {...body.audio, [camelCase(key)]: value}
: {[camelCase(key)]: value};
}
break; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||||||||||||||
error = new PermissionError(`${key} [${value}] not allowed, due to moderator property.`); | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
@@ -261,18 +275,21 @@ export default class ControlsOptionsManager { | |||||||||||||||||||||||||||||||||||||||
* @param {boolean} mutedEnabled | ||||||||||||||||||||||||||||||||||||||||
* @param {boolean} disallowUnmuteEnabled | ||||||||||||||||||||||||||||||||||||||||
* @param {boolean} muteOnEntryEnabled | ||||||||||||||||||||||||||||||||||||||||
* @param {array} roles which should be muted | ||||||||||||||||||||||||||||||||||||||||
* @memberof ControlsOptionsManager | ||||||||||||||||||||||||||||||||||||||||
* @returns {Promise} | ||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||
public setMuteAll( | ||||||||||||||||||||||||||||||||||||||||
mutedEnabled: boolean, | ||||||||||||||||||||||||||||||||||||||||
disallowUnmuteEnabled: boolean, | ||||||||||||||||||||||||||||||||||||||||
muteOnEntryEnabled: boolean | ||||||||||||||||||||||||||||||||||||||||
muteOnEntryEnabled: boolean, | ||||||||||||||||||||||||||||||||||||||||
roles: Array<string> | ||||||||||||||||||||||||||||||||||||||||
): Promise<any> { | ||||||||||||||||||||||||||||||||||||||||
return this.setControls({ | ||||||||||||||||||||||||||||||||||||||||
[Setting.muted]: mutedEnabled, | ||||||||||||||||||||||||||||||||||||||||
[Setting.disallowUnmute]: disallowUnmuteEnabled, | ||||||||||||||||||||||||||||||||||||||||
[Setting.muteOnEntry]: muteOnEntryEnabled, | ||||||||||||||||||||||||||||||||||||||||
[Setting.roles]: roles, | ||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,8 @@ export interface VideoProperties { | |
|
||
export interface ViewTheParticipantListProperties { | ||
enabled?: boolean; | ||
panelistEnabled?: boolean; | ||
attendeeCount?: boolean; | ||
Comment on lines
+39
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Missing test coverage for webinar control properties. The properties 🔗 Analysis chainVerify integration with webinar functionality and FedRAMP compliance. Since these properties are part of webinar controls, let's verify their integration: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check usage of new properties and related webinar functionality
# Check for usage of these properties in the codebase
echo "Checking usage of new properties..."
rg -t typescript "panelistEnabled|attendeeCount" packages/@webex/plugin-meetings/
# Check for webinar-related test coverage
echo "Checking test coverage..."
rg -t typescript "describe.*ViewTheParticipantList" packages/@webex/plugin-meetings/test/
# Check for FedRAMP compliance documentation
echo "Checking FedRAMP documentation..."
fd -t f "README|SECURITY" packages/@webex/plugin-meetings/
Length of output: 735 Script: #!/bin/bash
# Description: Check usage of new properties and related webinar functionality with corrected file types
# Check for usage of these properties in the codebase
echo "Checking usage of new properties..."
rg --type-add 'ts=**/*.ts' --type-add 'tsx=**/*.tsx' -t ts -t tsx "panelistEnabled|attendeeCount" packages/@webex/plugin-meetings/
# Check for webinar-related test coverage
echo "Checking test coverage..."
rg --type-add 'ts=**/*.ts' --type-add 'tsx=**/*.tsx' -t ts -t tsx "describe.*ViewTheParticipantList" packages/@webex/plugin-meetings/test/
# Check for FedRAMP compliance documentation
echo "Checking FedRAMP documentation..."
fd -t f 'README|SECURITY' packages/@webex/plugin-meetings/
Length of output: 883 Script: #!/bin/bash
# Description: Check usage of new properties and related webinar functionality by searching .ts and .tsx files
# Check for usage of these properties in the codebase
echo "Checking usage of new properties..."
rg "panelistEnabled|attendeeCount" packages/@webex/plugin-meetings/ --glob "*.ts" --glob "*.tsx"
# Check for webinar-related test coverage
echo "Checking test coverage..."
rg "describe.*ViewTheParticipantList" packages/@webex/plugin-meetings/test/ --glob "*.ts" --glob "*.tsx"
# Check for FedRAMP compliance documentation
echo "Checking FedRAMP documentation..."
fd -t f 'README|SECURITY' packages/@webex/plugin-meetings/
Length of output: 2319 |
||
} | ||
|
||
export type Properties = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ FullState.parse = (fullState) => ({ | |
type: fullState.type || FULL_STATE.UNKNOWN, | ||
meetingState: fullState.state, | ||
locked: fullState.locked, | ||
attendeeCount: typeof fullState.attendeeCount === 'number' ? fullState.attendeeCount : 0, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider improving type safety and default value handling. While the type check for
Consider applying these changes: - const FullState: any = {};
+ interface FullStateData {
+ type: string;
+ state?: string;
+ locked?: boolean;
+ attendeeCount?: number;
+ }
+
+ interface ParsedFullState {
+ type: string;
+ meetingState: string | undefined;
+ locked: boolean | undefined;
+ attendeeCount: number | undefined;
+ }
+
+ const FullState = {
+ parse(fullState: FullStateData): ParsedFullState => ({
type: fullState.type || FULL_STATE.UNKNOWN,
meetingState: fullState.state,
locked: fullState.locked,
- attendeeCount: typeof fullState.attendeeCount === 'number' ? fullState.attendeeCount : 0,
+ attendeeCount: typeof fullState.attendeeCount === 'number' ? fullState.attendeeCount : undefined,
});
+ };
|
||
}); | ||
|
||
FullState.getFullState = (oldFullState, newFullState) => { | ||
|
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.
🛠️ Refactor suggestion
Consider adding a Role enum for type safety
While the type definition is more specific now, consider creating an enum for valid roles to prevent runtime errors from invalid role strings.