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

[BUG] PeoplePicker not showing selected people when using selectionChanged callback method and setting state object

Open siddharth-vaghasia opened this issue 2 years ago • 5 comments

Describe the bug PeoplePicker not showing selected people when using selectionChanged callback method is added and if we are setting any state object using hook... When not setting any state object it is working fine.

Using MGT Graph Toolkit inside Task Module in Teams App of type Messaging Extension, I have used same concept inside Teams Tab and it is working fine...exactly same code.... so most likely issue is how teams Task module is setup ?

Below is code for reference

Component added <PeoplePicker type={PersonType.person} userType={UserType.user} selectionChanged={handleSelectionChanged} />

Callback method

  const handleSelectionChanged = (event) => {
         console.log(event);
         setPeople(event.detail);
      };

To Reproduce Expected behavior it should work as-is inside Messaging Extension task module and we should be able to set state objects

Screenshots Issue GIF ezgif com-gif-maker

Environment (please complete the following information):

  • OS: [e.g. Windows 11]
  • Browser [chrome]
  • Framework [react,]
  • Context [e.g. Microsoft Teams]
  • Version [e.g. 0.1]
  • Provider [e.g. TeamsMsal2Provider]

Additional context Add any other context about the problem here.

siddharth-vaghasia avatar Mar 01 '22 19:03 siddharth-vaghasia

Hello siddharth-vaghasia, 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 🙌

ghost avatar Mar 01 '22 19:03 ghost

This is quite interesting... Exceptionally good question! Do you have a repo we can use to speed up our repro? I'm very curious about this specific issue as this really seems like a bug or an issue with using both MGT and Tasks Modules. Thanks @siddharth-vaghasia!

sebastienlevert avatar Mar 01 '22 21:03 sebastienlevert

@sebastienlevert - sure, let me share the repo with you....

siddharth-vaghasia avatar Mar 02 '22 07:03 siddharth-vaghasia

@sebastienlevert - So I did debug the MGT library and found more insights and context to issue. Below are details

The actual issue is coming when you are initializing the MGT provider in the main component and changing the state inside the people picker selectionChanged callback method. This is because as soon as we are setting the state, somehow it is calling the initProvider method of Msal2Provider.js and it is going to trySilentSignIn method. In this method, after some iterations it is setting the providerstate to signedOut

image

And then in basecomponent.js, clearStateMethod gets calls

image

Inside clearState method there is code to clear selectedPeople array to empty

image

When I said it is working with Tabs, I found that in my Tab set my main component where I am initializing provider is just a entry point and then I am using PeoplePicker in the child component so in this case when setState method is called it does not re initialize the provider and hence does not change the state of provider to signedout.

So to confirm this issue with Tab, I did this setup in Tab and then initialized the provider in the same component where we are using PeoplePicker and then setting state object issue was reproducible..

I am not sure why the provider would initialize again if we are changing the state...

Looks like an issue, but for now, we have a workaround.... this gets me thinking where is the best place to initialize the provider?

I can still share the repo but because of SSO implementation, it has lots of configurations that we have to do to make the SSO and people picker work with MGT.. let me know

Hope this analysis help...

siddharth-vaghasia avatar Mar 02 '22 14:03 siddharth-vaghasia

I would still love to get the repro! I would say that the approach should always to configure MGT and the Auth Providers in a parent component and leave all child components to drive the show. But there might scenarios where this makes things more difficult. For example, for React SPAs, we recommend doing the provider work in the index.ts, making sure it is being usable by all the components in your solution.

sebastienlevert avatar Mar 14 '22 15:03 sebastienlevert

Do you have any recommendations for getting this to work in a SPA?

araver avatar Aug 17 '22 22:08 araver

If you have a reproduction that shows your scenario, then I'd be happy to take a look and see what I think the correct approach is.

gavinbarron avatar Aug 17 '22 23:08 gavinbarron

Here's some sample code. There's way more to the solution, but its Company IP that I can't share. OrgPeoplePicker is 5-10 components below index. indext.ts

import React from "react";
import ReactDOM from "react-dom";
import "./index.css";
import { App } from "./App";
import reportWebVitals from "./reportWebVitals";
import { msalConfig } from "./AuthConfig";
import { PublicClientApplication } from "@azure/msal-browser";
import { MsalProvider } from "@azure/msal-react";
import { Providers } from "@microsoft/mgt-element";
import { Msal2Provider } from "@microsoft/mgt-msal2-provider";
import { initializeIcons } from "@fluentui/react";
import { BrowserRouter } from "react-router-dom";
import { ConfirmationServiceProvider } from "./components/shared/ConfirmDialog/ConfirmDialog";
initializeIcons();

const root = document.getElementById("root");
const msalInstance = new PublicClientApplication(msalConfig);
Providers.globalProvider = new Msal2Provider({ publicClientApplication: msalInstance });
ReactDOM.render(
	<React.StrictMode>
		<BrowserRouter>
			<ConfirmationServiceProvider>
				<MsalProvider instance={msalInstance}>
					<App />
				</MsalProvider>
			</ConfirmationServiceProvider>
		</BrowserRouter>
	</React.StrictMode>,
	root
);

// If you want to start measuring performance in your app, pass a function
// to log results (for example: reportWebVitals(console.log))
// or send to an analytics endpoint. Learn more: https://bit.ly/CRA-vitals
reportWebVitals();

OrgPeoplePicker.tsx

import { getTheme, Label } from "@fluentui/react";
import { MgtPeoplePicker, PeoplePicker, UserType } from "@microsoft/mgt-react";
import * as React from "react";

export enum PickerType {
	single = "single",
	multiple = "multple",
}

export type IOrgPeoplePickerProps = {
	disabled?: boolean;
	userId?: string | undefined;
	userIds?: string[];
	label?: string;
	required?: boolean;
} & (
	| {
			type: PickerType.single;
			userId: string | undefined;
			onChange: (id?: string) => void;
	  }
	| {
			type: PickerType.multiple;
			userIds: string[];
			onChange: (id: string[]) => void;
	  }
);

export function OrgPeoplePicker(props: IOrgPeoplePickerProps) {
	const theme = getTheme();
	const [test, setTest] = React.useState<boolean>(false);
	const ref = React.useRef<MgtPeoplePicker>();
	const { type, userId, userIds } = props;

	const users = React.useMemo(() => {
		if (type === PickerType.multiple) {
			return userIds || [];
		} else if (userId && type === PickerType.single) {
			return [userId];
		} else {
			return [];
		}
	}, [type, userId, userIds]);

	const cssStyle: object = {
		"--input-border": `1px ${theme.semanticColors.inputBorder} solid`,
	};
	console.log("USERS", users);
	return (
		<div style={cssStyle}>
			{props.label && (
				<Label
					onClick={() => {
						console.log("SEL PEOPLE", ref.current?.selectedPeople, users);
					}}
					required={props.required}>
					{props.label}
				</Label>
			)}

			<PeoplePicker
				ref={ref}
				defaultSelectedUserIds={[...users]}
				selectionMode={"multple"}
				userType={UserType.user}
				userFilters="userType eq 'Member'"
				disabled={props.disabled}
				selectionChanged={(e) => {
					setTest(!test);
				}}></PeoplePicker>

			<div>{users.length}</div>
		</div>
	);
}

araver avatar Aug 18 '22 13:08 araver

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.

ghost avatar Aug 22 '22 14:08 ghost

Thanks for your repro @araver I took a look and cut it back further and then worked out wht I think is going on for you. In React whenenver a state object is updated then the component that state belongs to and all children are re-rendered, this can leat to some undesirable side-effects, like re-initializing the global MGT provider if you have that inside a component which gets re-rendered. Additionally in your example you ae setting the defaultSelectedUserIds prop, this is only used on the initial render of the mgt-people-pickerm what you really want to be using is the selectedPeople prop.

Ive taken your example and converted it to a sample that I think is working as intended on StackBlitz: https://stackblitz.com/edit/mgt-starter-react-ts-yndzjp?file=Picker.tsx,App.tsx,index.tsx

Plase let us know if this helps of not.

gavinbarron avatar Aug 23 '22 23:08 gavinbarron

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.

ghost avatar Aug 28 '22 00:08 ghost