[BUG] Header missing from people search

[FredrikEkstroem]

Describe the bug The people picker does not send the header "X-PeopleQuery-QuerySources: Mailbox,Directory” in the query https://graph.microsoft.com/v1.0/me/people?$search=%22Some%20Name%22&$top=6&$filter=personType/class%20eq%20%27Person%27. This means not all results are returned, only people "which are your saved contacts or people you are most likely to interact with", see documentation here . In our scenario this means we have many cases where the people picker returns 0 results, even though the user exist.

To Reproduce Steps to reproduce the behavior: Add a people picker

<PeoplePicker
  className={styles.pickerControl}
  selectionMode="single"
  ref={user}
/>

Search for someone and check the network traffic

Expected behavior Header should be added to include all people in search

Environment (please complete the following information):

• OS: Windows
• Browser: edge, chrome
• Framework: React, MGT-SPFX
• Context: Sharedpoint
• Version @microsoft/[email protected]
• Provider Msal2Provider/SimpleProvider

Additional context Releated bugs: #1224 #1723 I suspect it might also be the explanation for this #1387

Probably could be fixed if header was added to graph.people.ts something like

graphResult = await graph
      .api('/me/people')
      .search('"' + query + '"')
      .top(top)
      .filter(filter)
      .middlewareOptions(prepScopes(scopes))
      .headers({"X-PeopleQuery-QuerySources": "Mailbox,Directory"})
      .get();

and graph.user.ts

batch.get(personQuery, `/me/people?$search="${personQuery}"`, ['people.read'], {"X-PeopleQuery-QuerySources": "Mailbox,Directory"});

I'm not sure if it will affect anything else though...

[ghost]

Hello FredrikEkstroem, 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 🙌

[sebastienlevert]

Wow. Today I learnt something new, thanks @FredrikEkstroem!

I think there is some value in having the "regular" headerless query, but I'd love to allow more granularity for some of our components. For the PeoplePicker specifically, do you think having something like this would be useful?

<PeoplePicker
  className={styles.pickerControl}
  selectionMode="single"
  querySources={['Mailbox', 'Directory']}
  consistencyLevel={'Eventual'}
  ref={user}
/>

By default, querySources would not be set and set only when specified. Same for consistencyLevel.

[FredrikEkstroem]

Keeping it as is by default might be a good idea not to introduce changes in behaviour for people doing upgrades and also makes it more simple if you don't want to send a header, on the other hand the way query sources and consistency level is specified above I believe is how most people think the component work today. The ootb SharePoint people picker today sends query sources, but not consistency level. I think I could go both ways.

Another more flexible way, but maybe not as simple if you use the component for the first time would be to just allow specifying of the headers you want directly, like

<PeoplePicker
  className={styles.pickerControl}
  selectionMode="single"
  headers={{"X-PeopleQuery-QuerySources": "Mailbox,Directory", "consistencyLevel": "Eventual"}}
  ref={user}
/>

or even allowing for both approaches

[musale]

I'm leaning towards setting your own headers attribute. This reduces the number of attributes to keep track of and it is even easier on the backend as you can set all the required headers once.

[sebastienlevert]

What happens when we have multiple endpoints called by a single component? We send the headers to all the calls? Hoping they don't fail? That's my only concern.

I'd love to offer headers on all components to help dev drive the behavior of the endpoints, so it's actually a really great proposal @FredrikEkstroem

[HiltonGiesenow]

@sebastienlevert are you concerned because People Picker calls either /me/people or /users, depending on the settings (I haven't checked this myself, just basing this on what I've read in the mgt docs here. If that's the case, maybe there could also be an attribute to specify which 'user' endpoint you want to use?

[HiltonGiesenow]

I've been looking a bit at the code now, and from what I can see, the code's pretty clear on which endpoint it's calling. For instance, at https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/5709ebd27d5cf2b107cccca43e3339a294df38ea/packages/mgt-components/src/graph/graph.user.ts#L53, the method knows it's calling '/users' and at https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/ae546e25d0f4e8bbcd1ebab974a7e3ca04c8e819/packages/mgt-components/src/graph/graph.people.ts#L142 it knows it's calling '/me/people'. What I mean is, there's no if [x] then call this endpoint, otherwise call this one. This means we don't need to have mixed headers, that might not apply to the situation - when one endpoint is used, we can add the headers that it needs very easily, and similarly for the other endpoint.

Aside from that, there's an argument to be made for just doing the 'right' thing - as @FredrikEkstroem said above, this is actually the way people -probably- think it's working today anyway - it was a nasty surprise recently for me when it wasn't, and was excluding certain users. If we don't want to make it the default, because we don't want to break existing codebases, I want to suggest it should be a pretty obvious switch on the controls, for example (bad example but...) "search-depth" or "search-visility", with options like 'only relevant' and 'entire directory'.

I'm happy to work on this, by the way...

[FredrikEkstroem]

In this specific example with the people picker (my main concern at the moment) I'm more leaning towards setting a default value since that's how people think it works. Allowing for sending in any custom value to overwrite the default headers would be more flexible so if there are more headers in the future, it's just to send that in, no need to upgrade your mgt version.

Something like below

/** Headers default for this call */
const defaultHeaders:{[header: string]: string;} = {"X-PeopleQuery-QuerySources": "Mailbox,Directory"};
/** default headers merged with user defined headers */
const headers:{[header: string]: string;} = userDefinedHeaders ?? {...defaultHeaders, ...userDefinedHeaders} : defaultHeaders;

To make it easier when coding I would also suggest adding comments to the type definition to describe how it should be used, maybe also a description of what the default value is, something like below

export type PeoplePickerProps = {
	/** Default value: xxxxx. 
	 * Used to send in extra headers to the graph call
	 * Example:
	 * {"consistencyLevel": "Eventual"}
	 */
	headers?:{[header: string]: string;}
	...
}

I'm not 100% sure I found the right place in the code, but might be here mgt-react\src\generated\react.ts? Or are those autogenerated some way?

[musale]

@FredrikEkstroem sorry for the late reply. Yes, the react files are auto generated.

[sebastienlevert]

@FredrikEkstroem Can you confirm that v2.6.2 solved this issue? @HiltonGiesenow provided a great PR for this topic! Thanks!

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.