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

Updating Chat prop to include support for external users. #14711

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

Updating Chat prop to include support for external users. #14711

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a3adea
Updating Chat prop to include support for external users.
malexanderlim Nov 21, 2024
2c5e1c3
Bump
malexanderlim Nov 21, 2024
a0dc9a4
Committed CodeRabbit suggestions for error handling and caching for p…
malexanderlim Nov 21, 2024
bf6fd91
Adding docs link and original description.
malexanderlim Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 43 additions & 3 deletions components/microsoft_teams/microsoft_teams.app.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import constants from "./common/constants.mjs";
export default {
type: "app",
app: "microsoft_teams",
description: "**Personal accounts are not currently supported by Microsoft Teams.** Refer to Microsoft's documentation [here](https://learn.microsoft.com/en-us/graph/permissions-reference#remarks-7) to learn more.",
description: "Connect and interact with Microsoft Teams, supporting both internal and external communications.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume removing the dcumentation link is intentional here

propDefinitions: {
team: {
type: "string",
Expand Down Expand Up @@ -58,16 +58,50 @@ export default {
chat: {
type: "string",
label: "Chat",
description: "Team Chat within the organization (No external Contacts)",
description: "Team Chat (internal and external contacts)",
async options({ prevContext }) {
const response = prevContext.nextLink
? await this.makeRequest({
path: prevContext.nextLink,
})
: await this.listChats();

const myTenantId = await this.getAuthenticatedUserTenant();
const options = [];

for (const chat of response.value) {
const members = chat.members.map((member) => member.displayName);
const messages = await this.makeRequest({
path: `/chats/${chat.id}/messages?$top=50`,
});

const members = await Promise.all(chat.members.map(async (member) => {
let displayName = member.displayName;

if (!displayName && messages.value.length > 0) {
const userMessage = messages.value.find((msg) =>
msg.from?.user?.id === member.userId);
if (userMessage?.from?.user?.displayName) {
displayName = userMessage.from.user.displayName;
}
}

if (!displayName) {
try {
const userDetails = await this.makeRequest({
path: `/users/${member.userId}`,
});
displayName = userDetails.displayName;
} catch (err) {
displayName = "Unknown User";
}
}

const isExternal = member.tenantId !== myTenantId || !member.tenantId;
return isExternal
? `${displayName} (External)`
: displayName;
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and performance

The member resolution logic has several areas for improvement:

  1. Error handling is generic and could miss specific API error cases
  2. Multiple sequential API calls could impact performance
  3. No caching mechanism for user details

Consider implementing these improvements:

          const members = await Promise.all(chat.members.map(async (member) => {
-            let displayName = member.displayName;
+            // Cache key using userId
+            const cacheKey = `user_${member.userId}`;
+            let displayName = member.displayName || this._userCache?.get(cacheKey);
 
-            if (!displayName && messages.value.length > 0) {
+            if (!displayName) {
               const userMessage = messages.value.find((msg) =>
                 msg.from?.user?.id === member.userId);
               if (userMessage?.from?.user?.displayName) {
                 displayName = userMessage.from.user.displayName;
               }
-            }
 
-            if (!displayName) {
               try {
                 const userDetails = await this.makeRequest({
                   path: `/users/${member.userId}`,
                 });
                 displayName = userDetails.displayName;
+                // Cache the result
+                this._userCache?.set(cacheKey, displayName);
               } catch (err) {
-                displayName = "Unknown User";
+                if (err.statusCode === 404) {
+                  displayName = "User Not Found";
+                } else if (err.statusCode === 403) {
+                  displayName = "Access Denied";
+                } else {
+                  displayName = "Unknown User";
+                }
+                console.error(`Failed to fetch user details for ${member.userId}:`, err);
               }
             }

Committable suggestion skipped: line range outside the PR's diff.


options.push({
label: members.join(", "),
value: chat.id,
Expand Down Expand Up @@ -144,6 +178,12 @@ export default {
: reduction;
}, api);
},
async getAuthenticatedUserTenant() {
const { value } = await this.client()
.api("/organization")
.get();
return value[0].id;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling to tenant ID retrieval

The method assumes the organization API will always return results. Add proper error handling for edge cases.

     async getAuthenticatedUserTenant() {
-      const { value } = await this.client()
-        .api("/organization")
-        .get();
-      return value[0].id;
+      try {
+        const { value } = await this.client()
+          .api("/organization")
+          .get();
+        
+        if (!value || value.length === 0) {
+          throw new Error("No organization found");
+        }
+        
+        return value[0].id;
+      } catch (error) {
+        console.error("Failed to fetch tenant ID:", error);
+        throw new Error("Unable to determine tenant ID");
+      }
     },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async getAuthenticatedUserTenant() {
const { value } = await this.client()
.api("/organization")
.get();
return value[0].id;
},
async getAuthenticatedUserTenant() {
try {
const { value } = await this.client()
.api("/organization")
.get();
if (!value || value.length === 0) {
throw new Error("No organization found");
}
return value[0].id;
} catch (error) {
console.error("Failed to fetch tenant ID:", error);
throw new Error("Unable to determine tenant ID");
}
},

async authenticatedUserId() {
const { id } = await this.client()
.api("/me")
Expand Down
Loading