-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add addAndConfigure overload with new parameters to enable Townhall App Filtering #2725
base: main
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
change/@microsoft-teams-js-ca4c2d7e-8656-4fe0-a226-50302107b90d.json
Outdated
Show resolved
Hide resolved
…d.json Co-authored-by: Jeff Klouda <93734408+jekloudaMSFT@users.noreply.github.com>
…m/OfficeDev/microsoft-teams-library-js into cmachart/townhall_addAndConfigure
* | ||
* Additional parameters associated with a meeting | ||
*/ | ||
export interface TeamsExtraMeetingInputs { |
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.
Echoing comments on the host sdk side: it would be better to avoid naming this "Teams" since that is host-specific, "Extra" is implied by this being optional in the function interface, and "Inputs" describes how the interface is used more than what it is. Would something like "MeetingSettings", "MeetingProperties", or something along those lines be appropriate here?
@microsoft-github-policy-service agree company="Microsoft" |
[new SerializableHostEntityId(hostEntityIds), appTypes], | ||
[ | ||
new SerializableHostEntityId(hostEntityIds), | ||
(Array.isArray(appTypesOrMeetingParams) || appTypesOrMeetingParams == undefined |
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.
This change will break with older versions of host SDK. Hosts will try to read the second argument as appTypes when meetingParams is passes. Is there any problem with leaving the original argument order here and adding meetingParams as a third argument?
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.
It shouldn't break anything because the method still accepts appTypes as the second argument. The reason the new parameter precedes appTypes in the argument list is because appTypes is optional and meetingParams isn't (as Shakeel suggested).
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.
But if an app picks up this change, then runs in a host that does not have the corresponding host sdk change, then it will send a [hostEntityIds, meetingParams] message to the host, and the host will try to read that meetingParams argument as appTypes and will fail the call or continue with unexpected behavior. If the app were to instead send a [hostEntityIds, null, meetingParams] message then the older host sdk versions would just ignore the third parameter and continue to work.
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.
I see, ok let me try to rearrange the args
@@ -96,7 +96,26 @@ describe('hostEntity', () => { | |||
const promise = hostEntity.tab.addAndConfigure(mockHostEntity); | |||
const message = utils.findMessageByFunc('hostEntity.tab.addAndConfigure'); | |||
expect(message).not.toBeNull(); | |||
expect(message?.args).toEqual([mockHostEntity, null]); | |||
expect(message?.args).toEqual([mockHostEntity, null, null]); |
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 you pass appTypes here to make sure that these arguments are in the right order?
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 doc comments for the overloaded functions and see the question on argument ordering. Thanks!
[ | ||
new SerializableHostEntityId(hostEntityIds), | ||
appTypes ? appTypes : Array.isArray(appTypesOrMeetingParams) ? appTypesOrMeetingParams : undefined, | ||
!Array.isArray(appTypesOrMeetingParams) && appTypesOrMeetingParams != undefined |
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.
!Array.isArray(appTypesOrMeetingParams) && appTypesOrMeetingParams != undefined | |
!Array.isArray(appTypesOrMeetingParams) && appTypesOrMeetingParams !== undefined |
if ( | ||
(Array.isArray(appTypesOrMeetingParams) && appTypesOrMeetingParams.length === 0) || | ||
(Array.isArray(appTypes) && appTypes?.length === 0) | ||
) { | ||
throw new Error(`Error code: ${ErrorCode.INVALID_ARGUMENTS}, message: App types cannot be an empty array`); | ||
} | ||
|
||
return callFunctionInHostAndHandleResponse<HostEntityTabInstance, HostEntityTabInstance>( | ||
'hostEntity.tab.addAndConfigure', | ||
[new SerializableHostEntityId(hostEntityIds), appTypes], | ||
[ | ||
new SerializableHostEntityId(hostEntityIds), | ||
appTypes ? appTypes : Array.isArray(appTypesOrMeetingParams) ? appTypesOrMeetingParams : undefined, | ||
!Array.isArray(appTypesOrMeetingParams) && appTypesOrMeetingParams !== undefined | ||
? new SerializableMeetingParams(appTypesOrMeetingParams) | ||
: undefined, | ||
], |
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.
I find it a bit hard to parse through exactly which one is the meeting params and which one is supposed to be the app types... For readability's sake, I think it would help to do a pre-processing step and assign the correct values in some variables before doing anything, so it is easier to understand and parse the order of the arguments. Something like:
const meetingParams = !Array.isArray(appTypesOrMeetingParams) ? appTypesOrMeetingParams : undefined;
const appTypesArg = !meetingParams ? appTypesOrMeetingParams : appTypes;
This could simplify a bit all the branching logic in the if
statement for the list and in the calling function in host arguments.
{ | ||
"title": "addAndConfigure API Call - Success", | ||
"title": "addAndConfigure API Call - Success with meetingParams and appTypes", | ||
"type": "callResponse", | ||
"boxSelector": "#box_addAndConfigure", | ||
"inputValue": { | ||
"hostEntityIds": { | ||
"threadId": "threadId" | ||
}, | ||
"appTypes": ["web"], | ||
"meetingParams": { | ||
"isTownhall": true, | ||
"isStreamingThread": true | ||
} | ||
}, | ||
"expectedAlertValue": "addAndConfigure called with {\"threadId\":\"threadId\"}", | ||
"expectedAlertValue": "addAndConfigure called with {\"threadId\":\"threadId\"} and {\"appTypes\":[\"web\"]} and {\"meetingParams\":{\"isTownhall\":true,\"isStreamingThread\":true}}", |
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 consider if it's best to instead of replacing this E2E test, just keep the previous one and add this as a new one. I think it would be beneficial as the old test should still be supported thanks to the overloading of this function and could prevent us to add any bug to how this works. Still, is up to you if you find it redundant.
For more information about how to contribute to this repo, visit this page.
Description
In order to filter apps for Townhalls in the app install menu, the new calendar must pass two new properties to addAndConfigure(). These two properties indicate whether the install menu is being triggered by a Townhall and whether the app will be installed to the streaming thread.
Main changes in the PR:
Validation
Validation performed:
Unit Tests added:
Yes
End-to-end tests added:
No
Additional Requirements
Change file added:
Yes