-
Notifications
You must be signed in to change notification settings - Fork 859
Add Windows support for "Require all software" during setup experience. #44519
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
0e4d2f0
2ff589a
906fa4e
36181ad
b40ae04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,6 @@ import { ISoftwareTitle } from "interfaces/software"; | |
| import { INotification } from "interfaces/notification"; | ||
|
|
||
| import { NotificationContext } from "context/notification"; | ||
| import useGitOpsMode from "hooks/useGitOpsMode"; | ||
| import mdmAPI from "services/entities/mdm"; | ||
|
|
||
| import Button from "components/buttons/Button"; | ||
|
|
@@ -75,6 +74,8 @@ interface IInstallSoftwareFormProps { | |
| softwareTitles: ISoftwareTitle[] | null; | ||
| platform: SetupExperiencePlatform; | ||
| savedRequireAllSoftwareMacOS?: boolean | null; | ||
| savedRequireAllSoftwareWindows?: boolean | null; | ||
| isWindowsMdmEnabled?: boolean; | ||
| router: InjectedRouter; | ||
| refetchSoftwareTitles: () => void; | ||
| } | ||
|
|
@@ -85,31 +86,40 @@ const InstallSoftwareForm = ({ | |
| softwareTitles, | ||
| platform, | ||
| savedRequireAllSoftwareMacOS, | ||
| savedRequireAllSoftwareWindows, | ||
| isWindowsMdmEnabled = false, | ||
| router, | ||
| refetchSoftwareTitles, | ||
| }: IInstallSoftwareFormProps) => { | ||
| const noSoftwareUploaded = hasNoSoftwareUploaded(softwareTitles); | ||
| const { renderFlash, renderMultiFlash } = useContext(NotificationContext); | ||
| const { gitOpsModeEnabled } = useGitOpsMode("software"); | ||
| const [requireAllSoftwareMacOS, setRequireAllSoftwareMacOS] = useState( | ||
| savedRequireAllSoftwareMacOS ?? false | ||
| ); | ||
| const [requireAllSoftwareWindows, setRequireAllSoftwareWindows] = useState( | ||
| savedRequireAllSoftwareWindows ?? false | ||
| ); | ||
| const [isSaving, setIsSaving] = useState(false); | ||
|
|
||
| const initialSelectedSoftware = useMemo( | ||
| () => (softwareTitles ? initializeSelectedSoftwareIds(softwareTitles) : []), | ||
| [softwareTitles] | ||
| ); | ||
|
|
||
| // Track if the user changed the macOS checkbox since the last save. | ||
| // Track if the user changed the require-all checkbox since the last save. | ||
| // We don't compare against props here to avoid races with parent refetch timing. | ||
| const [touchedRequireAll, setTouchedRequireAll] = useState(false); | ||
|
|
||
| const handleChangeRequireAll = (value: boolean) => { | ||
| const handleChangeRequireAllMacOS = (value: boolean) => { | ||
| setRequireAllSoftwareMacOS(value); | ||
| setTouchedRequireAll(true); | ||
| }; | ||
|
|
||
| const handleChangeRequireAllWindows = (value: boolean) => { | ||
| setRequireAllSoftwareWindows(value); | ||
| setTouchedRequireAll(true); | ||
| }; | ||
|
|
||
| const [selectedSoftwareIds, setSelectedSoftwareIds] = useState<number[]>( | ||
| initialSelectedSoftware | ||
| ); | ||
|
|
@@ -136,7 +146,8 @@ const InstallSoftwareForm = ({ | |
| ); | ||
|
|
||
| const shouldUpdateSoftware = isSoftwareSelectionDirty; | ||
| const shouldUpdateRequireAll = platform === "macos" && touchedRequireAll; | ||
| const shouldUpdateRequireAll = | ||
| (platform === "macos" || platform === "windows") && touchedRequireAll; | ||
|
|
||
| const onClickSave = async (evt: React.FormEvent) => { | ||
| evt.preventDefault(); | ||
|
|
@@ -170,13 +181,20 @@ const InstallSoftwareForm = ({ | |
| } | ||
| } | ||
|
|
||
| // 2. macOS “require all software” update | ||
| // 2. "require all software" update (macOS or Windows) | ||
| if (shouldUpdateRequireAll) { | ||
| try { | ||
| await mdmAPI.updateRequireAllSoftwareMacOS( | ||
| currentTeamId, | ||
| requireAllSoftwareMacOS | ||
| ); | ||
| if (platform === "windows") { | ||
| await mdmAPI.updateRequireAllSoftwareWindows( | ||
| currentTeamId, | ||
| requireAllSoftwareWindows | ||
| ); | ||
| } else { | ||
|
getvictor marked this conversation as resolved.
|
||
| await mdmAPI.updateRequireAllSoftwareMacOS( | ||
| currentTeamId, | ||
| requireAllSoftwareMacOS | ||
| ); | ||
| } | ||
|
getvictor marked this conversation as resolved.
|
||
| hadSuccess = true; | ||
| setTouchedRequireAll(false); | ||
| } catch (e) { | ||
|
Comment on lines
+184
to
200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The new Windows checkbox in InstallSoftwareForm persists Extended reasoning...What the bug isThe PR adds an admin-side write path for The exact code path
Why existing code does not prevent itWindows IS routed through Step-by-step proof
Net effect: the Windows checkbox is functionally write-only on the frontend, which directly contradicts the PR title "Add Windows support for ImpactThe core feature this PR claims to deliver does not work end-to-end on Windows. Admins can toggle the setting and the value will persist, but no Windows device will ever experience the failure flow it controls. Shipping this as-is would be misleading and likely require a follow-up bug report once a customer notices. How to fix
This was independently flagged by Qodo in the same PR review as "Windows require-all not used". 🔬 also observed by qodo
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The MDM BYOD setup experience will be handled in a separate PR/story. |
||
|
|
@@ -185,7 +203,7 @@ const InstallSoftwareForm = ({ | |
| alertType: "error", | ||
| isVisible: true, | ||
| message: | ||
| "Couldn't update 'Cancel setup if software install fails'. Please try again.", | ||
| "Couldn't update 'Cancel setup if software fails'. Please try again.", | ||
| persistOnPageChange: false, | ||
| }); | ||
| } | ||
|
|
@@ -267,15 +285,54 @@ const InstallSoftwareForm = ({ | |
| /> | ||
| {platform === "macos" && ( | ||
| <div className={`${baseClass}__macos_options`}> | ||
| <Checkbox | ||
| disabled={gitOpsModeEnabled || manualAgentInstallBlockingSoftware} | ||
| value={requireAllSoftwareMacOS} | ||
| onChange={handleChangeRequireAll} | ||
| > | ||
| <TooltipWrapper tipContent="If any software fails, the end user won't be let through, and will see a prompt to contact their IT admin. Remaining software installs will be canceled."> | ||
| Cancel setup if software install fails | ||
| </TooltipWrapper> | ||
| </Checkbox> | ||
| <GitOpsModeTooltipWrapper | ||
| tipOffset={6} | ||
| position="bottom-start" | ||
| entityType="software" | ||
| renderChildren={(disableChildren) => ( | ||
| <Checkbox | ||
| disabled={ | ||
| disableChildren || manualAgentInstallBlockingSoftware | ||
| } | ||
| value={requireAllSoftwareMacOS} | ||
| onChange={handleChangeRequireAllMacOS} | ||
| > | ||
| <TooltipWrapper | ||
| tipContent="If any software fails, the end user will be prompted to restart setup. Remaining software installs will be canceled." | ||
| disableTooltip={disableChildren} | ||
| > | ||
| Cancel setup if software fails | ||
| </TooltipWrapper> | ||
| </Checkbox> | ||
| )} | ||
| /> | ||
| </div> | ||
| )} | ||
| {platform === "windows" && ( | ||
| <div className={`${baseClass}__windows_options`}> | ||
| <GitOpsModeTooltipWrapper | ||
| tipOffset={6} | ||
| position="bottom-start" | ||
| entityType="software" | ||
| renderChildren={(disableChildren) => ( | ||
| <Checkbox | ||
| disabled={disableChildren || !isWindowsMdmEnabled} | ||
| value={requireAllSoftwareWindows} | ||
| onChange={handleChangeRequireAllWindows} | ||
| > | ||
| <TooltipWrapper | ||
| tipContent={ | ||
| isWindowsMdmEnabled | ||
| ? "If any software fails, the end user will be prompted to restart setup. Remaining software installs will be canceled." | ||
| : "Turn on Windows MDM to use this option." | ||
| } | ||
| disableTooltip={disableChildren} | ||
| > | ||
| Cancel setup if software fails | ||
| </TooltipWrapper> | ||
| </Checkbox> | ||
| )} | ||
| /> | ||
| </div> | ||
| )} | ||
| <GitOpsModeTooltipWrapper | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.