Fix unreleased bugs in OS updates UI (#16604)

frontend/pages/ManageControlsPage/OSUpdates/OSUpdates.tsx

@@ -1,15 +1,23 @@
-import React, { useContext, useEffect, useState } from "react";
+import React, { useContext, useState } from "react";
 import { InjectedRouter } from "react-router";
+import { useQuery } from "react-query";
 
-import { IConfig } from "interfaces/config";
 import { AppContext } from "context/app";
 
+import { IConfig } from "interfaces/config";
+import { ITeamConfig } from "interfaces/team";
+
+import configAPI from "services/entities/config";
+import teamsAPI, { ILoadTeamResponse } from "services/entities/teams";
+
 import PremiumFeatureMessage from "components/PremiumFeatureMessage";
+import Spinner from "components/Spinner";
 
 import NudgePreview from "./components/NudgePreview";
 import TurnOnMdmMessage from "../components/TurnOnMdmMessage/TurnOnMdmMessage";
 import CurrentVersionSection from "./components/CurrentVersionSection";
 import TargetSection from "./components/TargetSection";
+import { generateKey } from "./components/TargetSection/TargetSection";
 
 export type OSUpdatesSupportedPlatform = "darwin" | "windows";
 
@@ -34,21 +42,39 @@ interface IOSUpdates {
 }
 
 const OSUpdates = ({ router, teamIdForApi }: IOSUpdates) => {
-  const { config, isPremiumTier } = useContext(AppContext);
+  const { isPremiumTier, setConfig } = useContext(AppContext);
 
-  // the default platform is mac and we later update this value when we have
-  // done more checks.
   const [
-    selectedPlatform,
-    setSelectedPlatform,
-  ] = useState<OSUpdatesSupportedPlatform>("darwin");
+    selectedPlatformTab,
+    setSelectedPlatformTab,
+  ] = useState<OSUpdatesSupportedPlatform | null>(null);
 
-  // we have to use useEffect here as we need to update our selected platform
-  // state when the app config is updated. This is usually when we get the app
-  // config response from the server and it is no longer `null`.
-  useEffect(() => {
-    setSelectedPlatform(getSelectedPlatform(config));
-  }, [config]);
+  // FIXME: We're calling this endpoint twice on mount because it also gets called in App.tsx
+  // whenever the pathname changes. We should find a way to avoid this.
+  const {
+    data: config,
+    isLoading: isLoadingConfig,
+    isError: isErrorConfig,
+    refetch: refetchAppConfig,
+  } = useQuery<IConfig, Error>(["config"], () => configAPI.loadAll(), {
+    refetchOnWindowFocus: false,
+    onSuccess: (data) => setConfig(data), // update the app context with the fetched config
+  });
+
+  const {
+    data: teamConfig,
+    isLoading: isLoadingTeam,
+    isError: isErrorTeamConfig,
+    refetch: refetchTeamConfig,
+  } = useQuery<ILoadTeamResponse, Error, ITeamConfig>(
+    ["team-config", teamIdForApi],
+    () => teamsAPI.load(teamIdForApi),
+    {
+      refetchOnWindowFocus: false,
+      enabled: !!teamIdForApi,
+      select: (data) => data.team,
+    }
+  );
 
   // Not premium shows premium message
   if (!isPremiumTier) {
@@ -59,19 +85,28 @@ const OSUpdates = ({ router, teamIdForApi }: IOSUpdates) => {
     );
   }
 
+  // FIXME: Are these checks still necessary?
   if (config === null || teamIdForApi === undefined) return null;
 
+  // FIXME: We ought to display a spinner or some disabled state whenever refetching these queries
+  // too because a slow network can cause a disconnect between the form data and the actual data and
+  // we don't want the user to be editing the form data while fresh data is being fetched. We don't
+  // have a specified UX for this yet.
+  if (isLoadingConfig || isLoadingTeam) return <Spinner />;
+
+  // FIXME: Handle error states for app config and team config (need specifications for this).
+
   // mdm is not enabled for mac or windows.
   if (
-    !config.mdm.enabled_and_configured &&
-    !config.mdm.windows_enabled_and_configured
+    !config?.mdm.enabled_and_configured &&
+    !config?.mdm.windows_enabled_and_configured
   ) {
     return <TurnOnMdmMessage router={router} />;
   }
 
-  const handleSelectPlatform = (platform: OSUpdatesSupportedPlatform) => {
-    setSelectedPlatform(platform);
-  };
+  // If the user has not selected a platform yet, we default to the platform that
+  // is enabled and configured.
+  const selectedPlatform = selectedPlatformTab || getSelectedPlatform(config);
 
   return (
     <div className={baseClass}>
@@ -85,9 +120,18 @@ const OSUpdates = ({ router, teamIdForApi }: IOSUpdates) => {
         </div>
         <div className={`${baseClass}__taget-container`}>
           <TargetSection
-            key={teamIdForApi} // we need to re-render this component when the team id changes.
+            key={generateKey({
+              currentTeamId: teamIdForApi,
+              appConfig: config,
+              teamConfig,
+            })} // FIXME: Find a better way to trigger re-rendering if these change (see FIXME above regarding refetching)
+            appConfig={config}
             currentTeamId={teamIdForApi}
-            onSelectAccordionItem={handleSelectPlatform}
+            selectedPlatform={selectedPlatform}
+            teamConfig={teamConfig}
+            onSelectPlatform={setSelectedPlatformTab}
+            refetchAppConfig={refetchAppConfig}
+            refetchTeamConfig={refetchTeamConfig}
           />
         </div>
         <div className={`${baseClass}__nudge-preview`}>

frontend/pages/ManageControlsPage/OSUpdates/components/MacOSTargetForm/MacOSTargetForm.tsx

@@ -1,6 +1,5 @@
 import React, { useContext, useState } from "react";
 import { isEmpty } from "lodash";
-import classnames from "classnames";
 
 import { APP_CONTEXT_NO_TEAM_ID } from "interfaces/team";
 import { NotificationContext } from "context/notification";
@@ -11,7 +10,6 @@ import teamsAPI from "services/entities/teams";
 import InputField from "components/forms/fields/InputField";
 import Button from "components/buttons/Button";
 import validatePresence from "components/forms/validators/validate_presence";
-import { AppContext } from "context/app";
 
 const baseClass = "mac-os-target-form";
 
@@ -78,14 +76,17 @@ interface IMacOSTargetFormProps {
   currentTeamId: number;
   defaultMinOsVersion: string;
   defaultDeadline: string;
+  refetchAppConfig: () => void;
+  refetchTeamConfig: () => void;
 }
 
 const MacOSTargetForm = ({
   currentTeamId,
   defaultMinOsVersion,
   defaultDeadline,
+  refetchAppConfig,
+  refetchTeamConfig,
 }: IMacOSTargetFormProps) => {
-  const { setConfig } = useContext(AppContext);
   const { renderFlash } = useContext(NotificationContext);
 
   const [isSaving, setIsSaving] = useState(false);
@@ -96,11 +97,8 @@ const MacOSTargetForm = ({
   >();
   const [deadlineError, setDeadlineError] = useState<string | undefined>();
 
-  const updateNoTeamConfig = async (updateData: IMacMdmConfigData) => {
-    const updatedConfig = await configAPI.update(updateData);
-    setConfig(updatedConfig);
-  };
-
+  // FIXME: This behaves unexpectedly when a user switches tabs or changes the teams dropdown while the form is
+  // submitting because this component is unmounted.
   const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => {
     e.preventDefault();
     const errors = validateForm({
@@ -116,12 +114,15 @@ const MacOSTargetForm = ({
       const updateData = createMdmConfigData(minOsVersion, deadline);
       try {
         currentTeamId === APP_CONTEXT_NO_TEAM_ID
-          ? await updateNoTeamConfig(updateData)
+          ? await configAPI.update(updateData)
           : await teamsAPI.update(updateData, currentTeamId);
         renderFlash("success", "Successfully updated minimum version!");
       } catch {
         renderFlash("error", "Couldn’t update. Please try again.");
       } finally {
+        currentTeamId === APP_CONTEXT_NO_TEAM_ID
+          ? refetchAppConfig()
+          : refetchTeamConfig();
         setIsSaving(false);
       }
     }

frontend/pages/ManageControlsPage/OSUpdates/components/NudgePreview/NudgePreview.tsx

@@ -65,6 +65,9 @@ interface INudgePreviewProps {
 }
 
 const NudgePreview = ({ platform }: INudgePreviewProps) => {
+  // FIXME: on slow connection the image loads after the text which looks weird and can cause a
+  // mismatch between the text and the image when switching between platforms. We should load the
+  // image first and then the text.
   return (
     <div className={baseClass}>
       <NudgeDescription platform={platform} />

frontend/pages/ManageControlsPage/OSUpdates/components/PlatformTabs/PlatformTabs.tsx

@@ -14,7 +14,10 @@ interface IPlatformTabsProps {
   defaultMacOSDeadline: string;
   defaultWindowsDeadlineDays: string;
   defaultWindowsGracePeriodDays: string;
-  onSelectAccordionItem: (platform: OSUpdatesSupportedPlatform) => void;
+  selectedPlatform: OSUpdatesSupportedPlatform;
+  onSelectPlatform: (platform: OSUpdatesSupportedPlatform) => void;
+  refetchAppConfig: () => void;
+  refetchTeamConfig: () => void;
 }
 
 const PlatformTabs = ({
@@ -23,14 +26,20 @@ const PlatformTabs = ({
   defaultMacOSVersion,
   defaultWindowsDeadlineDays,
   defaultWindowsGracePeriodDays,
-  onSelectAccordionItem,
+  selectedPlatform,
+  onSelectPlatform,
+  refetchAppConfig,
+  refetchTeamConfig,
 }: IPlatformTabsProps) => {
+  // FIXME: This behaves unexpectedly when a user switches tabs or changes the teams dropdown while a form is
+  // submitting.
   return (
     <div className={baseClass}>
       <TabsWrapper>
         <Tabs
+          defaultIndex={selectedPlatform === "darwin" ? 0 : 1}
           onSelect={(currentIndex) =>
-            onSelectAccordionItem(currentIndex === 0 ? "darwin" : "windows")
+            onSelectPlatform(currentIndex === 0 ? "darwin" : "windows")
           }
         >
           <TabList>
@@ -43,6 +52,8 @@ const PlatformTabs = ({
               defaultMinOsVersion={defaultMacOSVersion}
               defaultDeadline={defaultMacOSDeadline}
               key={currentTeamId}
+              refetchAppConfig={refetchAppConfig}
+              refetchTeamConfig={refetchTeamConfig}
             />
           </TabPanel>
           <TabPanel>
@@ -51,6 +62,8 @@ const PlatformTabs = ({
               defaultDeadlineDays={defaultWindowsDeadlineDays}
               defaultGracePeriodDays={defaultWindowsGracePeriodDays}
               key={currentTeamId}
+              refetchAppConfig={refetchAppConfig}
+              refetchTeamConfig={refetchTeamConfig}
             />
           </TabPanel>
         </Tabs>

frontend/pages/ManageControlsPage/OSUpdates/components/TargetSection/TargetSection.tsx

@@ -1,16 +1,8 @@
-import React, { useContext } from "react";
-import { useQuery } from "react-query";
+import React from "react";
 
-import {
-  API_NO_TEAM_ID,
-  APP_CONTEXT_NO_TEAM_ID,
-  ITeamConfig,
-} from "interfaces/team";
+import { API_NO_TEAM_ID, ITeamConfig } from "interfaces/team";
 import { IConfig } from "interfaces/config";
-import { AppContext } from "context/app";
-import teamsAPI, { ILoadTeamResponse } from "services/entities/teams";
 
-import Spinner from "components/Spinner";
 import SectionHeader from "components/SectionHeader";
 
 import MacOSTargetForm from "../MacOSTargetForm";
@@ -20,99 +12,104 @@ import { OSUpdatesSupportedPlatform } from "../../OSUpdates";
 
 const baseClass = "os-updates-target-section";
 
-const getDefaultMacOSVersion = (
-  currentTeam: number,
-  appConfig: IConfig,
-  teamConfig?: ITeamConfig
-) => {
-  return currentTeam === API_NO_TEAM_ID
+type GetDefaultFnParams = {
+  currentTeamId: number;
+  appConfig: IConfig;
+  teamConfig?: ITeamConfig;
+};
+
+const getDefaultMacOSVersion = ({
+  currentTeamId,
+  appConfig,
+  teamConfig,
+}: GetDefaultFnParams) => {
+  return currentTeamId === API_NO_TEAM_ID
     ? appConfig?.mdm.macos_updates.minimum_version ?? ""
     : teamConfig?.mdm?.macos_updates.minimum_version ?? "";
 };
 
-const getDefaultMacOSDeadline = (
-  currentTeam: number,
-  appConfig: IConfig,
-  teamConfig?: ITeamConfig
-) => {
-  return currentTeam === API_NO_TEAM_ID
+const getDefaultMacOSDeadline = ({
+  currentTeamId,
+  appConfig,
+  teamConfig,
+}: GetDefaultFnParams) => {
+  return currentTeamId === API_NO_TEAM_ID
     ? appConfig?.mdm.macos_updates.deadline || ""
     : teamConfig?.mdm?.macos_updates.deadline || "";
 };
 
-const getDefaultWindowsDeadlineDays = (
-  currentTeam: number,
-  appConfig: IConfig,
-  teamConfig?: ITeamConfig
-) => {
-  return currentTeam === API_NO_TEAM_ID
+const getDefaultWindowsDeadlineDays = ({
+  currentTeamId,
+  appConfig,
+  teamConfig,
+}: GetDefaultFnParams) => {
+  return currentTeamId === API_NO_TEAM_ID
     ? appConfig.mdm.windows_updates.deadline_days?.toString() ?? ""
     : teamConfig?.mdm?.windows_updates.deadline_days?.toString() ?? "";
 };
 
-const getDefaultWindowsGracePeriodDays = (
-  currentTeam: number,
-  appConfig: IConfig,
-  teamConfig?: ITeamConfig
-) => {
-  return currentTeam === API_NO_TEAM_ID
+const getDefaultWindowsGracePeriodDays = ({
+  currentTeamId,
+  appConfig,
+  teamConfig,
+}: GetDefaultFnParams) => {
+  return currentTeamId === API_NO_TEAM_ID
     ? appConfig.mdm.windows_updates.grace_period_days?.toString() ?? ""
     : teamConfig?.mdm?.windows_updates.grace_period_days?.toString() ?? "";
 };
 
+export const generateKey = (args: GetDefaultFnParams) => {
+  return (
+    `${args.currentTeamId}-` +
+    `${getDefaultMacOSDeadline(args)}-` +
+    `${getDefaultMacOSVersion(args)}-` +
+    `${getDefaultWindowsDeadlineDays(args)}-` +
+    `${getDefaultWindowsGracePeriodDays(args)}`
+  );
+};
+
 interface ITargetSectionProps {
+  appConfig: IConfig;
   currentTeamId: number;
-  onSelectAccordionItem: (platform: OSUpdatesSupportedPlatform) => void;
+  teamConfig?: ITeamConfig;
+  selectedPlatform: OSUpdatesSupportedPlatform;
+  onSelectPlatform: (platform: OSUpdatesSupportedPlatform) => void;
+  refetchAppConfig: () => void;
+  refetchTeamConfig: () => void;
 }
 
 const TargetSection = ({
+  appConfig,
   currentTeamId,
-  onSelectAccordionItem,
+  selectedPlatform,
+  teamConfig,
+  onSelectPlatform,
+  refetchAppConfig,
+  refetchTeamConfig,
 }: ITargetSectionProps) => {
-  const { config } = useContext(AppContext);
+  const isMacMdmEnabled = appConfig.mdm.enabled_and_configured;
+  const isWindowsMdmEnabled = appConfig.mdm.windows_enabled_and_configured;
 
-  // We make the call at this component as multiple children components need
-  // this data.
-  const { data: teamData, isLoading: isLoadingTeam } = useQuery<
-    ILoadTeamResponse,
-    Error,
-    ITeamConfig
-  >(["team-config", currentTeamId], () => teamsAPI.load(currentTeamId), {
-    refetchOnWindowFocus: false,
-    enabled: currentTeamId > APP_CONTEXT_NO_TEAM_ID,
-    select: (data) => data.team,
+  const defaultMacOSVersion = getDefaultMacOSVersion({
+    currentTeamId,
+    appConfig,
+    teamConfig,
   });
-
-  if (!config) return null;
-
-  const isMacMdmEnabled = config.mdm.enabled_and_configured;
-  const isWindowsMdmEnabled = config.mdm.windows_enabled_and_configured;
-
-  // Loading state rendering
-  if (isLoadingTeam) {
-    return <Spinner />;
-  }
-
-  const defaultMacOSVersion = getDefaultMacOSVersion(
+  const defaultMacOSDeadline = getDefaultMacOSDeadline({
     currentTeamId,
-    config,
-    teamData
-  );
-  const defaultMacOSDeadline = getDefaultMacOSDeadline(
+    appConfig,
+    teamConfig,
+  });
+  const defaultWindowsDeadlineDays = getDefaultWindowsDeadlineDays({
     currentTeamId,
-    config,
-    teamData
-  );
-  const defaultWindowsDeadlineDays = getDefaultWindowsDeadlineDays(
+    appConfig,
+    teamConfig,
+  });
+  const defaultWindowsGracePeriodDays = getDefaultWindowsGracePeriodDays({
     currentTeamId,
-    config,
-    teamData
-  );
-  const defaultWindowsGracePeriodDays = getDefaultWindowsGracePeriodDays(
-    currentTeamId,
-    config,
-    teamData
-  );
+    appConfig,
+    teamConfig,
+  });
 
   const renderTargetForms = () => {
     if (isMacMdmEnabled && isWindowsMdmEnabled) {
@@ -123,7 +120,10 @@ const TargetSection = ({
           defaultMacOSDeadline={defaultMacOSDeadline}
           defaultWindowsDeadlineDays={defaultWindowsDeadlineDays}
           defaultWindowsGracePeriodDays={defaultWindowsGracePeriodDays}
-          onSelectAccordionItem={onSelectAccordionItem}
+          selectedPlatform={selectedPlatform}
+          onSelectPlatform={onSelectPlatform}
+          refetchAppConfig={refetchAppConfig}
+          refetchTeamConfig={refetchTeamConfig}
         />
       );
     } else if (isMacMdmEnabled) {
@@ -132,6 +132,8 @@ const TargetSection = ({
           currentTeamId={currentTeamId}
           defaultMinOsVersion={defaultMacOSVersion}
           defaultDeadline={defaultMacOSDeadline}
+          refetchAppConfig={refetchAppConfig}
+          refetchTeamConfig={refetchTeamConfig}
         />
       );
     }
@@ -140,6 +142,8 @@ const TargetSection = ({
         currentTeamId={currentTeamId}
         defaultDeadlineDays={defaultWindowsDeadlineDays}
         defaultGracePeriodDays={defaultWindowsGracePeriodDays}
+        refetchAppConfig={refetchAppConfig}
+        refetchTeamConfig={refetchTeamConfig}
       />
     );
   };

frontend/pages/ManageControlsPage/OSUpdates/components/WindowsTargetForm/WindowsTargetForm.tsx

@@ -1,6 +1,5 @@
 import React, { useContext, useState } from "react";
 import { isEmpty } from "lodash";
-import classnames from "classnames";
 
 import { APP_CONTEXT_NO_TEAM_ID } from "interfaces/team";
 import { NotificationContext } from "context/notification";
@@ -11,7 +10,6 @@ import teamsAPI from "services/entities/teams";
 import InputField from "components/forms/fields/InputField";
 import Button from "components/buttons/Button";
 import validatePresence from "components/forms/validators/validate_presence";
-import { AppContext } from "context/app";
 
 const baseClass = "windows-target-form";
 
@@ -86,16 +84,17 @@ interface IWindowsTargetFormProps {
   currentTeamId: number;
   defaultDeadlineDays: string;
   defaultGracePeriodDays: string;
-  inAccordion?: boolean;
+  refetchAppConfig: () => void;
+  refetchTeamConfig: () => void;
 }
 
 const WindowsTargetForm = ({
   currentTeamId,
   defaultDeadlineDays,
   defaultGracePeriodDays,
-  inAccordion = false,
+  refetchAppConfig,
+  refetchTeamConfig,
 }: IWindowsTargetFormProps) => {
-  const { setConfig } = useContext(AppContext);
   const { renderFlash } = useContext(NotificationContext);
   const [isSaving, setIsSaving] = useState(false);
   const [deadlineDays, setDeadlineDays] = useState(
@@ -111,15 +110,8 @@ const WindowsTargetForm = ({
     string | undefined
   >();
 
-  const classNames = classnames(baseClass, {
-    [`${baseClass}__accordion-form`]: inAccordion,
-  });
-
-  const updateNoTeamConfig = async (updateData: IWindowsMdmConfigData) => {
-    const updatedConfig = await configAPI.update(updateData);
-    setConfig(updatedConfig);
-  };
-
+  // FIXME: This behaves unexpectedly when a user switches tabs or changes the teams dropdown while the form is
+  // submitting because this component is unmounted.
   const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => {
     e.preventDefault();
     const errors = validateForm({
@@ -135,7 +127,7 @@ const WindowsTargetForm = ({
       const updateData = createMdmConfigData(deadlineDays, gracePeriodDays);
       try {
         currentTeamId === APP_CONTEXT_NO_TEAM_ID
-          ? await updateNoTeamConfig(updateData)
+          ? await configAPI.update(updateData)
           : await teamsAPI.update(updateData, currentTeamId);
         renderFlash(
           "success",
@@ -144,6 +136,9 @@ const WindowsTargetForm = ({
       } catch {
         renderFlash("error", "Couldn’t update. Please try again.");
       } finally {
+        currentTeamId === APP_CONTEXT_NO_TEAM_ID
+          ? refetchAppConfig()
+          : refetchTeamConfig();
         setIsSaving(false);
       }
     }
@@ -158,7 +153,7 @@ const WindowsTargetForm = ({
   };
 
   return (
-    <form className={classNames} onSubmit={handleSubmit}>
+    <form className={baseClass} onSubmit={handleSubmit}>
       <InputField
         label="Deadline"
         tooltip="Number of days the end user has before updates are installed and the host is forced to restart."

frontend/pages/ManageControlsPage/OSUpdates/components/WindowsTargetForm/_styles.scss

@@ -1,10 +0,0 @@
-.windows-target-form {
-  input {
-    background-color: $core-white;
-  }
-
-  &__accordion-form {
-     padding: $pad-large;
-    background-color: $ui-fleet-blue-10;
-  }
-}