microsoft-graph-toolkit icon indicating copy to clipboard operation
microsoft-graph-toolkit copied to clipboard

[BUG] mgt-people component: Cannot read properties of null (reading 'displayName')

Open YuzuLi opened this issue 1 year ago • 2 comments

Describe the bug When using mgt-people component by passing a list of user ids:

<mgt-people *ngIf="!isGuestUser" [attr.user-ids]="userIds" [attr.show-max]="showMax" person-card="none"></mgt-people>

We can see the below error in console if some users have been removed and their IDs are invalid:

image

The code throwing the error is here:

https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/b9c56bcd88a7d752c7e653a80884690d6b2eb2da/packages/mgt-components/src/graph/graph.user.ts#L199

The code gets a user from cache, and the user could be null if it the ID is not valid anymore:

image

To Reproduce Steps to reproduce the behavior:

  1. Pass an ID of a deleted user to mgt-people component

Note that this doesn't guarantee a consistent reproduction. Let's say we pass the id "invalid-user-id" to mgt-people.

Sometimes there won't be a cache with invalid-user-id as key and {user: null} as value in the IndexedDB, and mgt-people won't throw but just simply omitting this user.

Sometimes there will be a cache item of invalid-user-id with {user: null} showing up in the IndexedDB, like the screenshot above, and then the error will occur.

We also have tried using fallback-details but it doesn't help. As long as there is a {user: null} cache for the removed user, this error will happen.

Expected behavior When there is an invalid user id, the mgt-people component should just ignore the user and not throw any error. If fallback-details is used, use the fallback user. If not, just omit it.

Environment (please complete the following information):

  • OS: Windows 11
  • Browser: Edge
  • Framework: Angular
  • Context: Web
  • Version: 2.6.0
  • Provider: Msal2Provider

YuzuLi avatar Sep 10 '22 05:09 YuzuLi

Hello YuzuLi, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

msftbot[bot] avatar Sep 10 '22 05:09 msftbot[bot]

Hello @YuzuLi you're absolutely right with the code section that throws this error. Making the user?.displayName optional chain should mitigate this. Are you up to make the change and post a PR?

musale avatar Sep 12 '22 07:09 musale