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

feat: Allow adding fixed hosts when 'add all team members' is enabled… #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Allow adding fixed hosts when 'add all team members' is enabled… #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 23 additions & 4 deletions packages/features/eventtypes/components/AddMembersWithSwitch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const CheckedHostField = ({
helperText,
isRRWeightsEnabled,
customClassNames,
assignAllTeamMembers,
...rest
}: {
labelText?: string;
Expand All @@ -73,18 +74,26 @@ const CheckedHostField = ({
options?: Options<CheckedSelectOption>;
helperText?: React.ReactNode | string;
isRRWeightsEnabled?: boolean;
assignAllTeamMembers?: boolean;
} & Omit<Partial<ComponentProps<typeof CheckedTeamSelect>>, "onChange" | "value">) => {
return (
<div className="flex flex-col rounded-md">
<div>
{labelText ? <Label>{labelText}</Label> : <></>}
<CheckedTeamSelect
Copy link

Choose a reason for hiding this comment

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

Consider using consistent prop names: 'onChange' in CheckedHostField should match 'onOptionChange' used in CheckedTeamSelect.

🔨 See the suggested fix 
@@ -80,7 +80,7 @@
         {labelText ? <Label>{labelText}</Label> : <></>}
         <CheckedTeamSelect
           isOptionDisabled={(option) => !!value.find((host) => host.userId.toString() === option.value)}
-          onChange={(options) => {
+          onOptionChange={(options) => {
             onChange &&
               onChange(
                 options.map((option) => ({

Click Here
⚠️ Fume is an LLM-based tool and can make mistakes.

isOptionDisabled={(option) => !!value.find((host) => host.userId.toString() === option.value)}
onChange={(options) => {
assignAllTeamMembers={assignAllTeamMembers}
isFixed={isFixed}
isOptionDisabled={(option) => {
if (assignAllTeamMembers && isFixed) {
return !!value.find((host) => host.isFixed && host.userId.toString() === option.value);
}
return !!value.find((host) => host.userId.toString() === option.value);
}}
onOptionChange={(options) => {
onChange &&
onChange(
options.map((option) => ({
isFixed,
isFixed: option.isFixed ?? isFixed,
userId: parseInt(option.value, 10),
priority: option.priority ?? 2,
weight: option.weight ?? 100,
Expand Down Expand Up @@ -181,6 +190,7 @@ export type AddMembersWithSwitchProps = {
isSegmentApplicable?: boolean;
"data-testid"?: string;
customClassNames?: AddMembersWithSwitchCustomClassNames;
onClearAllClick?: VoidFunction;
};

const enum AssignmentState {
Expand All @@ -189,19 +199,23 @@ const enum AssignmentState {
ALL_TEAM_MEMBERS_ENABLED_AND_SEGMENT_APPLICABLE = "ALL_TEAM_MEMBERS_ENABLED_AND_SEGMENT_APPLICABLE",
ALL_TEAM_MEMBERS_ENABLED_AND_SEGMENT_NOT_APPLICABLE = "ALL_TEAM_MEMBERS_ENABLED_AND_SEGMENT_NOT_APPLICABLE",
TEAM_MEMBERS_IN_SEGMENT_ENABLED = "TEAM_MEMBERS_IN_SEGMENT_ENABLED",
FIXED_HOSTS_STATE = "FIXED_HOSTS_STATE",
}

function getAssignmentState({
assignAllTeamMembers,
assignRRMembersUsingSegment,
isAssigningAllTeamMembersApplicable,
isSegmentApplicable,
isFixedHosts,
}: {
assignAllTeamMembers: boolean;
assignRRMembersUsingSegment: boolean;
isAssigningAllTeamMembersApplicable: boolean;
isSegmentApplicable?: boolean;
isFixedHosts?: boolean;
}) {
if (isFixedHosts) return AssignmentState.FIXED_HOSTS_STATE;
if (assignAllTeamMembers) {
return isSegmentApplicable
? AssignmentState.ALL_TEAM_MEMBERS_ENABLED_AND_SEGMENT_APPLICABLE
Expand Down Expand Up @@ -246,6 +260,7 @@ export function AddMembersWithSwitch({
teamId,
isSegmentApplicable,
customClassNames,
onClearAllClick,
...rest
}: AddMembersWithSwitchProps) {
const { t } = useLocale();
Expand All @@ -262,6 +277,7 @@ export function AddMembersWithSwitch({
assignRRMembersUsingSegment,
isAssigningAllTeamMembersApplicable: automaticAddAllEnabled,
isSegmentApplicable,
isFixedHosts: isFixed,
});

const onAssignAllTeamMembersInactive = () => {
Expand Down Expand Up @@ -300,10 +316,11 @@ export function AddMembersWithSwitch({

case AssignmentState.TOGGLES_OFF_AND_ALL_TEAM_MEMBERS_NOT_APPLICABLE:
case AssignmentState.TOGGLES_OFF_AND_ALL_TEAM_MEMBERS_APPLICABLE:
case AssignmentState.FIXED_HOSTS_STATE:
return (
<>
<div className="mb-2">
{assignmentState === AssignmentState.TOGGLES_OFF_AND_ALL_TEAM_MEMBERS_APPLICABLE && (
{assignmentState === AssignmentState.TOGGLES_OFF_AND_ALL_TEAM_MEMBERS_APPLICABLE && !isFixed && (
<AssignAllTeamMembers
assignAllTeamMembers={assignAllTeamMembers}
setAssignAllTeamMembers={setAssignAllTeamMembers}
Expand All @@ -324,6 +341,8 @@ export function AddMembersWithSwitch({
placeholder={placeholder ?? t("add_attendees")}
isRRWeightsEnabled={isRRWeightsEnabled}
customClassNames={customClassNames?.teamMemberSelect}
assignAllTeamMembers={assignAllTeamMembers}
onClearAllClick={onClearAllClick}
/>
</div>
</>
Expand Down
29 changes: 24 additions & 5 deletions packages/features/eventtypes/components/CheckedTeamSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,19 @@ export const CheckedTeamSelect = ({
value = [],
isRRWeightsEnabled,
customClassNames,
onClearAllClick,
onOptionChange,
Copy link

Choose a reason for hiding this comment

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

The props interface needs to be updated to include 'assignAllTeamMembers' and 'isFixed' props since they are being used in the implementation.

🔨 See the suggested fix 
@@ -45,12 +45,16 @@
   value = [],
   isRRWeightsEnabled,
   customClassNames,
+  assignAllTeamMembers,
+  isFixed,
   ...props
 }: Omit<Props<CheckedSelectOption, true>, "value" | "onChange"> & {
   value?: readonly CheckedSelectOption[];
   onChange: (value: readonly CheckedSelectOption[]) => void;
   isRRWeightsEnabled?: boolean;
   customClassNames?: CheckedTeamSelectCustomClassNames;
+  assignAllTeamMembers?: boolean;
+  isFixed?: boolean;
 }) => {
   const isPlatform = useIsPlatform();
   const [priorityDialogOpen, setPriorityDialogOpen] = useState(false);
@@ -201,4 +205,4 @@
   }
 };
 
-export default CheckedTeamSelect;
+export default CheckedTeamSelect;

Click Here
⚠️ Fume is an LLM-based tool and can make mistakes.

assignAllTeamMembers,
isFixed,
Copy link

Choose a reason for hiding this comment

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

The type definition uses 'onChange' but the implementation uses 'onOptionChange' - please update the type to use 'onOptionChange' for consistency.

🔨 See the suggested fix 
@@ -48,7 +48,7 @@
   ...props
 }: Omit<Props<CheckedSelectOption, true>, "value" | "onChange"> & {
   value?: readonly CheckedSelectOption[];
-  onChange: (value: readonly CheckedSelectOption[]) => void;
+  onOptionChange: (value: readonly CheckedSelectOption[]) => void;
   isRRWeightsEnabled?: boolean;
   customClassNames?: CheckedTeamSelectCustomClassNames;
 }) => {
@@ -149,7 +149,7 @@
 
                 <Icon
                   name="x"
-                  onClick={() => props.onChange(value.filter((item) => item.value !== option.value))}
+                  onClick={() => props.onOptionChange(value.filter((item) => item.value !== option.value))}
                   className={classNames(
                     "my-auto ml-2 h-4 w-4",
                     customClassNames?.selectedHostList?.listItem?.removeButton
@@ -166,14 +166,14 @@
             isOpenDialog={priorityDialogOpen}
             setIsOpenDialog={setPriorityDialogOpen}
             option={currentOption}
-            onChange={props.onChange}
+            onChange={props.onOptionChange}
             customClassNames={customClassNames?.priorityDialog}
           />
           <WeightDialog
             isOpenDialog={weightDialogOpen}
             setIsOpenDialog={setWeightDialogOpen}
             option={currentOption}
-            onChange={props.onChange}
+            onChange={props.onOptionChange}
             customClassNames={customClassNames?.weightDialog}
           />
         </>
@@ -201,4 +201,4 @@
   }
 };
 
-export default CheckedTeamSelect;
+export default CheckedTeamSelect;

Click Here
⚠️ Fume is an LLM-based tool and can make mistakes.

...props
}: Omit<Props<CheckedSelectOption, true>, "value" | "onChange"> & {
}: Omit<Props<CheckedSelectOption, true>, "value"> & {
value?: readonly CheckedSelectOption[];
onChange: (value: readonly CheckedSelectOption[]) => void;
onOptionChange: (value: readonly CheckedSelectOption[]) => void;
isRRWeightsEnabled?: boolean;
customClassNames?: CheckedTeamSelectCustomClassNames;
onClearAllClick?: () => void;
assignAllTeamMembers?: boolean;
isFixed?: boolean;
}) => {
const isPlatform = useIsPlatform();
const [priorityDialogOpen, setPriorityDialogOpen] = useState(false);
Expand All @@ -72,6 +79,12 @@ export const CheckedTeamSelect = ({
isMulti
className={customClassNames?.hostsSelect?.select}
innerClassNames={customClassNames?.hostsSelect?.innerClassNames}
onChange={(newValue, { action }) => {
onOptionChange(newValue);
if (action === "clear" && onClearAllClick) {
onClearAllClick();
}
}}
Copy link

Choose a reason for hiding this comment

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

Add a key prop to the Fragment wrapper or remove it if not needed, as React requires keys for proper list reconciliation.

{...props}
/>
{/* This class name conditional looks a bit odd but it allows a seemless transition when using autoanimate
Expand Down Expand Up @@ -149,7 +162,13 @@ export const CheckedTeamSelect = ({

Copy link

Choose a reason for hiding this comment

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

The icon click handler still uses props.onChange but the prop has been renamed to onOptionChange. This should be updated for consistency.

🔨 See the suggested fix 
@@ -149,7 +149,7 @@
 
                 <Icon
                   name="x"
-                  onClick={() => props.onChange(value.filter((item) => item.value !== option.value))}
+                  onClick={() => props.onOptionChange(value.filter((item) => item.value !== option.value))}
                   className={classNames(
                     "my-auto ml-2 h-4 w-4",
                     customClassNames?.selectedHostList?.listItem?.removeButton
@@ -166,14 +166,14 @@
             isOpenDialog={priorityDialogOpen}
             setIsOpenDialog={setPriorityDialogOpen}
             option={currentOption}
-            onChange={props.onChange}
+            onChange={props.onOptionChange}
             customClassNames={customClassNames?.priorityDialog}
           />
           <WeightDialog
             isOpenDialog={weightDialogOpen}
             setIsOpenDialog={setWeightDialogOpen}
             option={currentOption}
-            onChange={props.onChange}
+            onChange={props.onOptionChange}
             customClassNames={customClassNames?.weightDialog}
           />
         </>
@@ -201,4 +201,4 @@
   }
 };
 
-export default CheckedTeamSelect;
+export default CheckedTeamSelect;

Click Here
⚠️ Fume is an LLM-based tool and can make mistakes.

<Icon
name="x"
onClick={() => props.onChange(value.filter((item) => item.value !== option.value))}
onClick={() => {
let newOption: CheckedSelectOption[] = [];
if (isFixed && assignAllTeamMembers) {
newOption = [{ ...option, isFixed: false }];
}
onOptionChange([...value.filter((item) => item.value !== option.value), ...newOption]);
}}
className={classNames(
"my-auto ml-2 h-4 w-4",
customClassNames?.selectedHostList?.listItem?.removeButton
Expand All @@ -166,14 +185,14 @@ export const CheckedTeamSelect = ({
isOpenDialog={priorityDialogOpen}
setIsOpenDialog={setPriorityDialogOpen}
option={currentOption}
onChange={props.onChange}
onChange={onOptionChange}
customClassNames={customClassNames?.priorityDialog}
/>
<WeightDialog
isOpenDialog={weightDialogOpen}
setIsOpenDialog={setWeightDialogOpen}
option={currentOption}
onChange={props.onChange}
onChange={onOptionChange}
customClassNames={customClassNames?.weightDialog}
/>
</>
Expand Down
33 changes: 31 additions & 2 deletions packages/features/eventtypes/components/tabs/assignment/EventTeamAssignmentTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,28 @@ const FixedHosts = ({
const { t } = useLocale();
const { getValues, setValue } = useFormContext<FormValues>();

const hasActiveFixedHosts = isRoundRobinEvent && getValues("hosts").some((host) => host.isFixed);
const currentHosts = getValues("hosts");
const hasActiveFixedHosts = isRoundRobinEvent && currentHosts.some((host) => host.isFixed);

const [isDisabled, setIsDisabled] = useState(hasActiveFixedHosts);

const setAllHostsAsRR = () =>
setValue(
"hosts",
teamMembers.map((teamMember) => {
const host = currentHosts.find((host) => host.userId === parseInt(teamMember.value, 10));
return {
isFixed: false,
userId: parseInt(teamMember.value, 10),
priority: host?.priority ?? 2,
weight: host?.weight ?? 100,
// if host was already added, retain scheduleId
scheduleId: host?.scheduleId || teamMember.defaultScheduleId,
};
}),
{ shouldDirty: true }
);

return (
<div className={classNames("mt-5 rounded-lg", customClassNames?.container)}>
{!isRoundRobinEvent ? (
Copy link

Choose a reason for hiding this comment

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

setAllHostsAsRR function doesn't preserve fixed host status when assignAllTeamMembers is true. This could cause fixed hosts to unexpectedly become RR hosts.

🔨 See the suggested fix 
@@ -353,38 +353,38 @@
             />
           </>
         )}
-        <AddMembersWithSwitch
-          teamId={teamId}
-          teamMembers={teamMembers}
-          value={value}
-          onChange={onChange}
-          assignAllTeamMembers={assignAllTeamMembers}
-          setAssignAllTeamMembers={setAssignAllTeamMembers}
-          isSegmentApplicable={isSegmentApplicable}
-          automaticAddAllEnabled={true}
-          isRRWeightsEnabled={isRRWeightsEnabled}
-          isFixed={false}
-          containerClassName={assignAllTeamMembers ? "-mt-4" : ""}
-          onActive={() => {
-            const currentHosts = getValues("hosts");
-            setValue(
-              "hosts",
-              teamMembers.map((teamMember) => {
-                const host = currentHosts.find((host) => host.userId === parseInt(teamMember.value, 10));
-                return {
-                  isFixed: false,
-                  userId: parseInt(teamMember.value, 10),
-                  priority: host?.priority ?? 2,
-                  weight: host?.weight ?? 100,
-                  // if host was already added, retain scheduleId
-                  scheduleId: host?.scheduleId || teamMember.defaultScheduleId,
-                };
-              }),
-              { shouldDirty: true }
-            );
-          }}
-          customClassNames={customClassNames?.addMembers}
-        />
+          <AddMembersWithSwitch
+            teamId={teamId}
+            teamMembers={teamMembers}
+            value={value}
+            onChange={onChange}
+            assignAllTeamMembers={assignAllTeamMembers}
+            setAssignAllTeamMembers={setAssignAllTeamMembers}
+            isSegmentApplicable={isSegmentApplicable}
+            automaticAddAllEnabled={true}
+            isRRWeightsEnabled={isRRWeightsEnabled}
+            isFixed={false}
+            containerClassName={assignAllTeamMembers ? "-mt-4" : ""}
+            onActive={() => {
+              const currentHosts = getValues("hosts");
+              setValue(
+                "hosts",
+                teamMembers.map((teamMember) => {
+                  const host = currentHosts.find((host) => host.userId === parseInt(teamMember.value, 10));
+                  return {
+                    isFixed: host?.isFixed ?? false,
+                    userId: parseInt(teamMember.value, 10),
+                    priority: host?.priority ?? 2,
+                    weight: host?.weight ?? 100,
+                    // if host was already added, retain scheduleId
+                    scheduleId: host?.scheduleId || teamMember.defaultScheduleId,
+                  };
+                }),
+                { shouldDirty: true }
+              );
+            }}
+            customClassNames={customClassNames?.addMembers}
+          />
       </div>
     </div>
   );

Click Here
⚠️ Fume is an LLM-based tool and can make mistakes.

Expand Down Expand Up @@ -216,6 +234,9 @@ const FixedHosts = ({
switchContainerClassName={customClassNames?.container}
onCheckedChange={(checked) => {
if (!checked) {
if (assignAllTeamMembers) {
setAllHostsAsRR();
}
const rrHosts = getValues("hosts")
.filter((host) => !host.isFixed)
.sort((a, b) => (b.priority ?? 2) - (a.priority ?? 2));
Expand All @@ -236,6 +257,7 @@ const FixedHosts = ({
setAssignAllTeamMembers={setAssignAllTeamMembers}
automaticAddAllEnabled={!isRoundRobinEvent}
isFixed={true}
onClearAllClick={() => setAllHostsAsRR()}
onActive={() => {
const currentHosts = getValues("hosts");
setValue(
Expand Down Expand Up @@ -531,7 +553,14 @@ const Hosts = ({
teamMembers={teamMembers}
Copy link

Choose a reason for hiding this comment

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

Add duplicate checking between fixed and non-fixed hosts using Set or array filtering to prevent the same host from appearing in both pools based on userId.

🔨 See the suggested fix 
@@ -354,19 +354,26 @@
           </>
         )}
         <AddMembersWithSwitch
-          teamId={teamId}
-          teamMembers={teamMembers}
-          value={value}
-          onChange={onChange}
-          assignAllTeamMembers={assignAllTeamMembers}
-          setAssignAllTeamMembers={setAssignAllTeamMembers}
-          isSegmentApplicable={isSegmentApplicable}
-          automaticAddAllEnabled={true}
-          isRRWeightsEnabled={isRRWeightsEnabled}
-          isFixed={false}
-          containerClassName={assignAllTeamMembers ? "-mt-4" : ""}
-          onActive={() => {
-            const currentHosts = getValues("hosts");
+              teamId={teamId}
+              teamMembers={teamMembers}
+              value={value}
+              onChange={onChange}
+              assignAllTeamMembers={assignAllTeamMembers}
+              setAssignAllTeamMembers={setAssignAllTeamMembers}
+              isSegmentApplicable={isSegmentApplicable}
+              automaticAddAllEnabled={true}
+              isRRWeightsEnabled={isRRWeightsEnabled}
+              isFixed={false}
+              containerClassName={assignAllTeamMembers ? "-mt-4" : ""}
+              onActive={() => {
+                const currentHosts = getValues("hosts");
+                const nonFixedTeamMembers = teamMembers.filter((member) => {
+                  const fixedHostUserIds = currentHosts.filter((h) => h.isFixed).map((h) => h.userId);
+                  return !fixedHostUserIds.includes(parseInt(member.value, 10));
+                });
+                setValue(
+                  "hosts",
+                  nonFixedTeamMembers.map((teamMember) => {
             setValue(
               "hosts",
               teamMembers.map((teamMember) => {
@@ -534,27 +541,31 @@
                   onChange([...value.filter((host: Host) => !host.isFixed), ...updatedHosts(changeValue)]);
                 }}
                 assignAllTeamMembers={assignAllTeamMembers}
-                setAssignAllTeamMembers={setAssignAllTeamMembers}
-                isRoundRobinEvent={true}
-                customClassNames={customClassNames?.fixedHosts}
-              />
-              <RoundRobinHosts
-                orgId={orgId}
-                teamId={teamId}
-                teamMembers={teamMembers}
-                value={value}
-                onChange={(changeValue) => {
-                  const hosts = [...value.filter((host: Host) => host.isFixed), ...updatedHosts(changeValue)];
-                  onChange(hosts);
-                }}
-                assignAllTeamMembers={assignAllTeamMembers}
-                setAssignAllTeamMembers={setAssignAllTeamMembers}
-                customClassNames={customClassNames?.roundRobinHosts}
-                isSegmentApplicable={isSegmentApplicable}
-              />
-            </>
-          ),
-          MANAGED: <></>,
+          setAssignAllTeamMembers={setAssignAllTeamMembers}
+          isRoundRobinEvent={true}
+          customClassNames={customClassNames?.fixedHosts}
+        />
+        <RoundRobinHosts
+          orgId={orgId}
+          teamId={teamId}
+          teamMembers={teamMembers.filter((member) => {
+            // Filter out members that are already fixed hosts
+            const fixedHostUserIds = value.filter((h) => h.isFixed).map((h) => h.userId);
+            return !fixedHostUserIds.includes(parseInt(member.value, 10));
+          })}
+          value={value}
+          onChange={(changeValue) => {
+            const hosts = [...value.filter((host: Host) => host.isFixed), ...updatedHosts(changeValue)];
+            onChange(hosts);
+          }}
+          assignAllTeamMembers={assignAllTeamMembers}
+          setAssignAllTeamMembers={setAssignAllTeamMembers}
+          customClassNames={customClassNames?.roundRobinHosts}
+          isSegmentApplicable={isSegmentApplicable}
+        />
+      </>
+    ),
+    MANAGED: <></>,
         };
         return !!schedulingType ? schedulingTypeRender[schedulingType] : <></>;
       }}

Click Here
⚠️ Fume is an LLM-based tool and can make mistakes.

value={value}
onChange={(changeValue) => {
onChange([...value.filter((host: Host) => !host.isFixed), ...updatedHosts(changeValue)]);
onChange([
...value.filter(
(host: Host) =>
!host.isFixed &&
!changeValue.some((changedHost) => changedHost.userId === host.userId)
),
...updatedHosts(changeValue),
]);
}}
assignAllTeamMembers={assignAllTeamMembers}
setAssignAllTeamMembers={setAssignAllTeamMembers}
Expand Down