#1753 people search to include all users in tenant

[HiltonGiesenow]

Closes #1753 #1743 #1665 #1666 (and some others)

PR Type

Bugfix

Description of the changes

Per the discussion, a certain header needed to be set, otherwise the search was limiting results only to 'relevant' users. A similar issue existed with the /users endpoint, but it seems the required headers are now being included for that ("consistencyLevel": "Eventual"), so it makes sense that this search should 'just work' as well.

PR checklist

• [x] Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
• [x] All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
• [] Stories have been added and existing stories have been tested (nothing was changed on the front end components, so didn't test stories)
• [ ] Added appropriate documentation. Docs PR: (component should 'just work' now - docs needed?)
• [x] License header has been added to all new source files (yarn setLicense)
• [x] Contains NO breaking changes

[ghost]

Thank you for creating a Pull Request @HiltonGiesenow.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

• [ ] I have verified a documentation PR has been linked and is approved (or not applicable)
• [ ] I have ran this PR locally and have tested the fix/feature
• [ ] I have verified that stories have been added to storybook (or not applicable)
• [ ] I have tested existing stories in storybook to verify no regression has occured
• [ ] I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

[sebastienlevert]

What I'm wondering is if this causes a breaking change for our existing users. If we add the header, the results will be fairly different from what they are used to... Would that be considered a breaking change or an enhancement? What you think @gavinbarron?

[HiltonGiesenow]

Of course I'm happy to implement either way, but I'll give you my thinking and see what you'd suggest is best.

At the moment, because there are two endpoints that can be, and are, used in the code, they are sometimes used in the same component based on different conditions. As an example, mgt-person-picker has this:

 if (this.userType === UserType.contact || this.userType === UserType.user) {
                // we might have a user-filters property set, search for users with it.
                if (this.userIds && this.userIds.length) {
                  // has the user-ids proerty set
                  people = await getUsersForUserIds(graph, this.userIds, input, this._userFilters);
                } else {
                  people = await findUsers(graph, input, this.showMax, this._userFilters);
                }
              } else {
                if (this.userIds && this.userIds.length) {
                  // has the user-ids proerty set
                  people = await getUsersForUserIds(graph, this.userIds, input, this._userFilters);
                } else {
                  people = (await findPeople(graph, input, this.showMax, this.userType, this._peopleFilters)) || [];
                }
              }

Basically, depending on what's been set, in this case, the code may use findUsers or it may use findPeople. findUsers has .header('ConsistencyLevel', 'eventual'), whereas findPeople currently doesn't have the header it requires.

In another example, in mgt-person-cards, if the user sets the userId property it behaves differently from when the user sets the personQuery property, which I presume most people are setting to email address (code is lines 857 - 875 if you want to check). This is very counter-intutitive I think - if you set the email address, it might NOT find the user, but if you set the id it will ALWAYS find the user - I'd presume, as a developer, that it would be able to find the person equally well for both unique properties.

Incidentally, I see lots of places where batch.get is called in these 'user'/'people' scenarios, and it doesn't have the headers applied either, and I'm not sure if that's likely to give people similar issues. For instance, in graph.user.ts, ``findUsers(as mentioned) sets the header, butgetUsersForUserIds` doesn't. These might well be giving similar issues, but I haven't seen much in the issues logs. In contrast, there are a bunch of issues logged that would get fixed with the initial code change I've submitted, so it's clearly an issue people are expecting to work better oob. I'm suspecting more people don't see it because, in many scenarios, having the people picker for example only find 'related' colleagues is totally sufficient (e.g. choose 'my manager') - that's certainly why I'd not hit this before now.

[gavinbarron]

Well that is an interesting change. One that could be considered a breaking change as it will change the nature of the data returned for these queries. I'll admit a degree of unfamiliarity with the default behavior here. I think it would be best if the component added a property to control the breadth of the search being done, with the current behavior being the default when no search breadth is specified. I do like the idea of a flexible property like a headers object. While I do share the same concern as @sebastienlevert around inavertently passing an invalid header for a given call, if I understand you correctly in #1743 @HiltonGiesenow we can avoid that case by simply not applying the passed headers object when execution takes the batch get path?

[HiltonGiesenow]

Thanks @gavinbarron for jumping here. A property would certainly make sense, as it allows the developer to switch between 'scopes', so to speak - the "users related to me" scope versus the "company wide" scope. However, here's where that would get tricky - see my sample above from mgt-person-picker. One code path uses findUsers and the other users findPeople. findUsers ALWAYS uses the 'company-wide' scope at the moment, and findUsers ALWAYS uses the "users related to me" scope.

If we introduce a property like 'searchScope' and default it to "users related to me", then it would be, so to speak, a 'non-breaking' change for findUsers. However, findPeople would now technically be incorrect in it's default form (i.e. we'd effectively be introducing a breaking change for that operation.

In the grander scheme, to me at least, it makes sense that the picker should not by default limit to just "users related to me" scope. I think people would generally expect a picker, if I insert an known email address or type a known name, to be able to find a user that I know exists in my org. At the moment, this doesn't happen, and there's no way to even make it do so. I would suggest this is the 'sensible' default in this case? Of course we can -also- add a property to switch between these.

To summarise, as I understand it:

1. we should add a property to allow switching between scopes
2. we should default it to...(to be clarified, my vote is 'company-wide' scope)

How does that sound?

[gavinbarron]

Apologies for the delay getting back to you here @HiltonGiesenow I've spent a little time to try and comprehend this issue a bit more, bear with me as a I get to grips with things.

We have two search modes in the people picker at present, findUsers and findPeople. Based on my understanding from reading the comments in the existing code and the documentation that we have findUsers is the path that searches the whole organization, while findPeople targets those users who are relevant to the current user.

We have three user-type search modes available on the people picker, any, user, and contact

Currently we're using the findPeople path when the user-type is any and the findUsers path for user or contact. That feels off to me. My belief is that we should be using findUsers for any and user and findPeople for contact. To me that would provide a search experience in line with what the documentation suggests and would be a refactoring of the existing code flows without introducing a new header or component property. Now this also is a breaking change in that it entirely changes the internal behavior of the components, but I believe that it's better to have our component working the way our documentation says they should.

Now it's entirely possible that I'm missing something here, so please let me know if that's the case.

[HiltonGiesenow]

Once again, thanks for taking the time to look at this properly, so no worries about the delayed response.

It actually does go deeper than this. Remember, People Picker is just one element at play here. Another is the Person Card component, for instance. Depending on whether you set email address or user id, it might/might not work. For example, in my test tenant, the following shows nothing:

    <PersonCard personQuery="[email protected]" />

but the following works fine.

    <PersonCard userId="...-2db8-43fb-a809-4fc4e60d4870" />

As a dev, it seems pretty weird that one works but the other doesn't. Once again, in this case, Person Card uses findPeople for the email-based search, but another endpoint for the id-based search. As a theoretical workaround, we could require people to do a search for email, based on Id, but that seems an unreasonable requirement and not one I'd expect to need to do - I'd think, at first glance, that email vs Id would be interchangeable. It's also not always easy/feasible to force the developer to do this in all cases.

To your point about the user type ('any', 'user', 'contact'), my understanding of those was different - 'user' should find a user -within- your org (what the docs call 'organizational users'), 'contact' would find someone who's in the directory but not an actual user in the org (for example, an External user with an entry therefore in the Azure AD), and 'any' would therefore include both of the other two options. With this in mind, once again, setting the option to 'any' OR 'user' should enable me, I'd think, to find "any user within my org". Maybe I'm misreading the docs in this regard? If not, then I feel we come back to needing to make this change as it's currently 'broken', and we could possibly introduce an option to switch types (see my previous comment).

[gavinbarron]

Thanks for the additional context.

I see the problem regarding the PersonCard now, and yes, we should try to address this friction point.

Regarding the contact user type, it includes Personal Contacts and Implicit Contacts, i.e. Organizational Users that the user has interacted with in a meaningful way. Given this I feel like we need to ensure that any changes we make to the flow of these searches preserves the existing intent while improving the scope of the search.

For the Person Card it feels like we should be implicitly passing down a parameter to findPeople which triggers the use of the wider search scope via the QuerySources header.

For People Picker it feels to me that we should be using the wider scoped search in findPeople whe the any user type search is used, but not when the contact type is used, as that would be quite a shift in behavior from what I believe the intent there is.

I think that this will address the issues that have been raised on this front in a clean, non-breaking fashion, and avoid introducing new configuration properties to the people picker.

[HiltonGiesenow]

That sounds good - I'll chew on it a bit and look in the code how that could work, thanks @gavinbarron

If I understand, we're looking to:

1. Update "Person" and "Person Card" to always use the expand scope
2. Update People Picker to use expanded scope when type is "any" or "user" (i.e. "org user"), but NOT for "Contact"

does that about cover it?

[gavinbarron]

Almost. For user-type any there should be no need to expand the scope of the search as it uses the findUsers path already. Thanks @HiltonGiesenow

[HiltonGiesenow]

oh yeah, of course. Ok, will work on this update and re-submit

[HiltonGiesenow]

I've been wrapping up several multi-year projects and then on vacation for a bit, so had to pause on this but I'm looking at it again. Just fyi, it affects all of these in some way.

• Person
• PersonCard
• PeoplePicker
• People

I'm working on it though. Suddenly my test tenant is not returning results even for the required header ("'X-PeopleQuery-QuerySources" set to "Mailbox,Directory"), in case anyone has any thoughts?

[HiltonGiesenow]

I've finished this off. I found out what was wrong in my test tenant - maybe useful for someone in an edge case - the user I was testing on hadn't been assigned a license. Once I did that, I was able to use them as a test case.

Aside from that, I've made the changes we discussed above. Looking forward to your feedback @gavinbarron

[HiltonGiesenow]

sure thing - thanks for the support and guidance, much appreciated