From aae61474879b9b4a2322bc3dded3f9659aeb3ded Mon Sep 17 00:00:00 2001 From: RachelElysia <71795832+RachelElysia@users.noreply.github.com> Date: Thu, 7 Aug 2025 09:00:51 -0400 Subject: [PATCH] Fleet UI: VPP Token All teams option bug fix (#31587) --- changes/30357-all-teams-dd-bug | 1 + .../EditTeamsVppModal.tests.tsx | 113 +++++++++++++++-- .../EditTeamsVppModal/EditTeamsVppModal.tsx | 120 ++++++++++++------ server/datastore/mysql/vpp.go | 13 +- server/datastore/mysql/vpp_test.go | 42 +++++- 5 files changed, 235 insertions(+), 54 deletions(-) create mode 100644 changes/30357-all-teams-dd-bug diff --git a/changes/30357-all-teams-dd-bug b/changes/30357-all-teams-dd-bug new file mode 100644 index 0000000000..5c807450ce --- /dev/null +++ b/changes/30357-all-teams-dd-bug @@ -0,0 +1 @@ +- Fleet UI: Fixed VPP token dropdown to allow user to choose "All teams" selection diff --git a/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tests.tsx b/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tests.tsx index 8db87eb38e..8c8c68bdec 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tests.tsx +++ b/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tests.tsx @@ -2,6 +2,7 @@ import { APP_CONTEXT_ALL_TEAMS_ID, APP_CONTEXT_NO_TEAM_ID, } from "interfaces/team"; +import { IMdmVppToken } from "interfaces/mdm"; import { getOptions, selectedValueFromToken, @@ -68,10 +69,24 @@ describe("EditTeamsVppModal", () => { })); describe("getOptions", () => { + // Helper for getting a pendingTeamIds array from a token + const asArr = (token: IMdmVppToken) => + selectedValueFromToken(token) + ? selectedValueFromToken(token) + .split(",") + .map((v) => v.trim()) + .filter(Boolean) + : []; + it("returns no options when another token is all teams", () => { const tokens = [allTeamsToken, piratesAndNinjasToken]; const currentToken = piratesAndNinjasToken; - const options = getOptions(availableTeams, tokens, currentToken); + const options = getOptions( + availableTeams, + tokens, + currentToken, + asArr(currentToken) + ); expect(options).toEqual([]); }); @@ -82,7 +97,12 @@ describe("EditTeamsVppModal", () => { { ...unassignedToken, id: 1338 }, ]; const currentToken = unassignedToken; - const options = getOptions(availableTeams, tokens, currentToken); + const options = getOptions( + availableTeams, + tokens, + currentToken, + asArr(currentToken) + ); expect(options).toEqual(allOptions); }); @@ -94,14 +114,24 @@ describe("EditTeamsVppModal", () => { { ...unassignedToken, id: 1338 }, ]; const currentToken = allTeamsToken; - const options = getOptions(availableTeams, tokens, currentToken); + const options = getOptions( + availableTeams, + tokens, + currentToken, + asArr(currentToken) + ); expect(options).toEqual(allOptions); }); it("excludes all teams option when any token is assigned", () => { const tokens = [unassignedToken, piratesAndNinjasToken]; const currentToken = unassignedToken; - const options = getOptions(availableTeams, tokens, currentToken); + const options = getOptions( + availableTeams, + tokens, + currentToken, + asArr(currentToken) + ); expect(options).toEqual( options.filter((o) => o.value !== APP_CONTEXT_ALL_TEAMS_ID) ); @@ -116,7 +146,14 @@ describe("EditTeamsVppModal", () => { ]; // test with unassignedToken - expect(getOptions(availableTeams, tokens, unassignedToken)).toEqual([ + expect( + getOptions( + availableTeams, + tokens, + unassignedToken, + asArr(unassignedToken) + ) + ).toEqual([ { label: "Penguins", value: 4 }, // only penguins is available ]); @@ -126,7 +163,14 @@ describe("EditTeamsVppModal", () => { APP_CONTEXT_NO_TEAM_ID, // already assigned to noTeamToken 3, // already assigned to pandasToken ]; - expect(getOptions(availableTeams, tokens, piratesAndNinjasToken)).toEqual( + expect( + getOptions( + availableTeams, + tokens, + piratesAndNinjasToken, + asArr(piratesAndNinjasToken) + ) + ).toEqual( allOptions.filter((o) => !unavailableTeamIds.includes(o.value)) ); @@ -137,7 +181,9 @@ describe("EditTeamsVppModal", () => { 1, // already assigned to piratesAndNinjasToken 2, // already assigned to piratesAndNinjasToken ]; - expect(getOptions(availableTeams, tokens, pandasToken)).toEqual( + expect( + getOptions(availableTeams, tokens, pandasToken, asArr(pandasToken)) + ).toEqual( allOptions.filter((o) => !unavailableTeamIds.includes(o.value)) ); @@ -148,7 +194,9 @@ describe("EditTeamsVppModal", () => { 2, // already assigned to piratesAndNinjasToken 3, // already assigned to pandasToken ]; - expect(getOptions(availableTeams, tokens, noTeamToken)).toEqual( + expect( + getOptions(availableTeams, tokens, noTeamToken, asArr(noTeamToken)) + ).toEqual( allOptions.filter((o) => !unavailableTeamIds.includes(o.value)) ); @@ -160,7 +208,12 @@ describe("EditTeamsVppModal", () => { 3, // already assigned to pandasToken ]; expect( - getOptions(availableTeams, [...tokens, allTeamsToken], allTeamsToken) + getOptions( + availableTeams, + [...tokens, allTeamsToken], + allTeamsToken, + asArr(allTeamsToken) + ) ).toEqual( allOptions.filter((o) => !unavailableTeamIds.includes(o.value)) ); @@ -221,4 +274,46 @@ describe("EditTeamsVppModal", () => { expect(teamIdsFromSelectedValue("2,1")).toEqual([2, 1]); }); }); + + describe("pending edit scenarios", () => { + it("shows all teams option when user removes all teams in edit UI (pendingTeamIds = [])", () => { + const tokens = [piratesAndNinjasToken]; + // simulating clearing everything in the modal before saving + const options = getOptions( + availableTeams, + tokens, + piratesAndNinjasToken, + [] + ); + expect(options.some((o) => o.value === APP_CONTEXT_ALL_TEAMS_ID)).toBe( + true + ); + }); + + it("shows only 'all teams' option when user selects all teams pending", () => { + const tokens = [unassignedToken, pandasToken]; + const options = getOptions( + availableTeams, + tokens, + unassignedToken, + [APP_CONTEXT_ALL_TEAMS_ID.toString()] // user picks 'all teams' + ); + expect(options.some((o) => o.value === APP_CONTEXT_ALL_TEAMS_ID)).toBe( + true + ); + }); + + it("hides teams already assigned to other tokens when editing assignment", () => { + const tokens = [pandasToken]; + // Simulate user picks a team not yet assigned + const options = getOptions( + availableTeams, + tokens, + unassignedToken, + ["4"] // 'Penguins' + ); + expect(options.find((o) => o.value === 3)).toBeUndefined(); // Pandas not selectable + expect(options.find((o) => o.value === 4)).not.toBeUndefined(); // Penguins selectable + }); + }); }); diff --git a/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsx b/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsx index 71b17269ec..923a08df24 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsx +++ b/frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsx @@ -24,7 +24,8 @@ interface IEditTeamsVppModalProps { } /** - * Returns an array of team ids from a token. It includes special handling for "All teams". + * Returns a string of comma-separated team ids from a token. + * Special handling for "All teams". */ export const selectedValueFromToken = (token: IMdmVppToken) => { if (!token.teams) { @@ -47,60 +48,54 @@ export const teamIdsFromSelectedValue = (selectedValue: string) => { if (selectedValue === APP_CONTEXT_ALL_TEAMS_ID.toString()) { return []; } - const ids = selectedValue.split(",").map((str) => parseInt(str, 10)); - // NOTE: We could do some extra frontend validation here like filtering out -1 (to ensure that - // we're not trying to send all teams and other teams at the same time) and checking for isNaN, - // but instead we're relying on the API to return an error if the request is invalid. + const ids = selectedValue + .split(",") + .map((str) => parseInt(str, 10)) + .filter((id) => !isNaN(id)); return ids; }; /** - * Compare two comma-separated strings of team names and returns an updated value. It includes - * special handling for "All teams". + * Compare two comma-separated strings of team ids and returns an updated value. + * Includes special handling for "All teams". */ export const updateSelectedValue = (prev: string, next: string) => { // react-select uses a string of comma-separated values for multi-select so we split it // fo get an array of selected team ids const nextParts = next.split(",").map((p) => p.trim()); if (nextParts.length === 1) { - // if only one team is selected, no need for other checks return next; } - // we need to do some special handling for "All teams" const allTeamsId = APP_CONTEXT_ALL_TEAMS_ID.toString(); - // split the previous value to get an array of team ids const prevParts = prev.split(",").map((p) => p.trim()); if (prevParts.includes(allTeamsId)) { // if "All teams" was previously selected, we need to remove it from the next selections - return nextParts.filter((p) => p !== allTeamsId).join(", "); + return nextParts.filter((p) => p !== allTeamsId).join(","); } - // if "All teams" is newly selected, we need to remove any other selections + // If "All teams" is newly selected, remove other selections if (nextParts.includes(allTeamsId)) { return allTeamsId; } - // otherwise, just return the next selections + // Otherwise, just return the next selections return next; }; const isTokenAllTeams = (token: IMdmVppToken) => token.teams?.length === 0; - const isTokenUnassigned = (token: IMdmVppToken) => token.teams === null; /** - * Returns a dictionary of team ids that are already assigned tokens other than the current token. + * Returns a dictionary of team ids already assigned (other than current token). */ const getUnavailableTeamIds = ( currentTokenId: number, tokens: IMdmVppToken[] ) => { - const unavailableTeamIds = {} as Record; + const unavailableTeamIds: Record = {}; tokens.forEach((token) => { - if (token.id === currentTokenId) { - return; - } + if (token.id === currentTokenId) return; token.teams?.forEach((team) => { unavailableTeamIds[team.team_id.toString()] = true; }); @@ -114,32 +109,67 @@ const getUnavailableTeamIds = ( export const getOptions = ( availableTeams: ITeamSummary[], tokens: IMdmVppToken[], - currentToken: IMdmVppToken + currentToken: IMdmVppToken, + pendingTeamIds: string[] ) => { - const allOptions = - availableTeams?.map((t) => ({ - label: t.name, - value: t.id, - })) || []; + const allTeamsOption = { + label: "All teams", + value: APP_CONTEXT_ALL_TEAMS_ID, + }; + // Filter for actual team options, add "All teams" to the front + const allOptions = [ + allTeamsOption, + ...availableTeams + .filter((t) => t.id !== APP_CONTEXT_ALL_TEAMS_ID) + .map((t) => ({ + label: t.name, + value: t.id, + })), + ]; + + // Determine state of pending assignment + const isPendingAllTeams = pendingTeamIds?.includes( + APP_CONTEXT_ALL_TEAMS_ID.toString() + ); + + // Case 1: All tokens are unassigned → show all options, including "All teams" if (tokens.every(isTokenUnassigned)) { - // if all tokens are unassigned, we can include all options return allOptions; } - if (tokens.some(isTokenAllTeams) && !isTokenAllTeams(currentToken)) { - // if another token is assigned to all teams, we can't assign this token to any team + // Case 2: If another token (not current) is assigned "All teams", restrict everything unless current/pending choosing "all teams" + if ( + tokens.some( + (token) => isTokenAllTeams(token) && token.id !== currentToken.id + ) && + !isPendingAllTeams + ) { return []; } - // if other tokens are assigned to specific teams, we'll filter out those team options - const unavailableTeamIds = getUnavailableTeamIds(currentToken.id, tokens); - if (!isTokenAllTeams(currentToken)) { - // if current token isn't already assigned to all teams, we'll exclude that option too - unavailableTeamIds[APP_CONTEXT_ALL_TEAMS_ID] = true; + // Case 3: If ANY other token is assigned real teams (not all teams/not unassigned)... + const anotherAssigned = tokens + .filter((t) => t.id !== currentToken.id) + .some((t) => !isTokenAllTeams(t) && !isTokenUnassigned(t)); + + // If so, and we're not actively changing this token to "All teams", REMOVE "All teams" option + let filteredOptions = allOptions; + if (anotherAssigned && !isPendingAllTeams) { + filteredOptions = allOptions.filter( + (o) => o.value !== APP_CONTEXT_ALL_TEAMS_ID + ); } - return allOptions.filter((o) => !unavailableTeamIds[o.value]); + // Get teams unavailable due to assignment to other tokens + const unavailableTeamIds = getUnavailableTeamIds(currentToken.id, tokens); + + // Return options not assigned, or that are in the pending selection + return filteredOptions.filter( + (o) => + !unavailableTeamIds[o.value.toString()] || + pendingTeamIds.includes(o.value.toString()) + ); }; const EditTeamsVppModal = ({ @@ -158,9 +188,25 @@ const EditTeamsVppModal = ({ ); const [isSaving, setIsSaving] = useState(false); + const selectedValueArr = useMemo( + () => + selectedValue + ? selectedValue + .split(",") + .map((v) => v.trim()) + .filter(Boolean) + : [], + [selectedValue] + ); + const options = useMemo(() => { - return getOptions(availableTeams || [], tokens, currentToken); - }, [availableTeams, tokens, currentToken]); + return getOptions( + availableTeams || [], + tokens, + currentToken, + selectedValueArr + ); + }, [availableTeams, tokens, currentToken, selectedValueArr]); const isAnyTokenAllTeams = useMemo(() => tokens.some(isTokenAllTeams), [ tokens, @@ -183,6 +229,8 @@ const EditTeamsVppModal = ({ onSuccess(); } catch (e) { renderFlash("error", "Couldn’t edit. Please try again."); + } finally { + setIsSaving(false); } }, [currentToken.id, selectedValue, renderFlash, onSuccess] diff --git a/server/datastore/mysql/vpp.go b/server/datastore/mysql/vpp.go index f11dd16715..25609576d8 100644 --- a/server/datastore/mysql/vpp.go +++ b/server/datastore/mysql/vpp.go @@ -1646,21 +1646,18 @@ TEAMLOOP: func checkVPPNullTeam(ctx context.Context, tx sqlx.ExtContext, currentID *uint, nullTeam fleet.NullTeamType) error { nullTeamStmt := `SELECT vpp_token_id FROM vpp_token_teams WHERE null_team_type = ?` - anyTeamStmt := `SELECT vpp_token_id FROM vpp_token_teams WHERE null_team_type = 'allteams' OR null_team_type = 'noteam' OR team_id IS NOT NULL` + anyTeamStmt := `SELECT vpp_token_id FROM vpp_token_teams WHERE (null_team_type = 'allteams' OR null_team_type = 'noteam' OR team_id IS NOT NULL) AND vpp_token_id != ?` if nullTeam == fleet.NullTeamAllTeams { var ids []uint - if err := sqlx.SelectContext(ctx, tx, &ids, anyTeamStmt); err != nil { + if err := sqlx.SelectContext(ctx, tx, &ids, anyTeamStmt, *currentID); err != nil { return ctxerr.Wrap(ctx, err, "scanning row in check vpp token null team") } + // Only blocks assignment if another token is already assigned to one or more teams. + // Allows the current token to switch from teams to "all teams" freely if len(ids) > 0 { - if len(ids) > 1 { - return ctxerr.Wrap(ctx, errors.New("Cannot assign token to All teams, other teams have tokens")) - } - if currentID == nil || ids[0] != *currentID { - return ctxerr.Wrap(ctx, errors.New("Cannot assign token to All teams, other teams have tokens")) - } + return ctxerr.Wrap(ctx, errors.New("Cannot assign token to All teams, other teams have tokens")) } } diff --git a/server/datastore/mysql/vpp_test.go b/server/datastore/mysql/vpp_test.go index c40a22666f..b39b048d96 100644 --- a/server/datastore/mysql/vpp_test.go +++ b/server/datastore/mysql/vpp_test.go @@ -33,6 +33,7 @@ func TestVPP(t *testing.T) { {"GetVPPAppByTeamAndTitleID", testGetVPPAppByTeamAndTitleID}, {"VPPTokensCRUD", testVPPTokensCRUD}, {"VPPTokenAppTeamAssociations", testVPPTokenAppTeamAssociations}, + {"VPPTokenReassignTeamsToAllTeams", testVPPTokenReassignTeamsToAllTeams}, {"GetOrInsertSoftwareTitleForVPPApp", testGetOrInsertSoftwareTitleForVPPApp}, {"DeleteVPPAssignedToPolicy", testDeleteVPPAssignedToPolicy}, {"TestVPPTokenTeamAssignment", testVPPTokenTeamAssignment}, @@ -1540,6 +1541,46 @@ func testVPPTokenAppTeamAssociations(t *testing.T, ds *Datastore) { assert.Error(t, err) } +func testVPPTokenReassignTeamsToAllTeams(t *testing.T, ds *Datastore) { + ctx := context.Background() + + // Set up two teams + team1, err := ds.NewTeam(ctx, &fleet.Team{Name: "All Teams Reassign Team 1"}) + require.NoError(t, err) + team2, err := ds.NewTeam(ctx, &fleet.Team{Name: "All Teams Reassign Team 2"}) + require.NoError(t, err) + + // Insert token + tokenData, err := test.CreateVPPTokenData(time.Now().Add(24*time.Hour), "Org For Reassign", "Loc For Reassign") + require.NoError(t, err) + tok, err := ds.InsertVPPToken(ctx, tokenData) + require.NoError(t, err) + tokenID := tok.ID + + // Assign token to team1 and team2 + upTok, err := ds.UpdateVPPTokenTeams(ctx, tokenID, []uint{team1.ID, team2.ID}) + require.NoError(t, err) + require.Len(t, upTok.Teams, 2) + + // Now, reassign to ALL TEAMS (teams = []) + upTok, err = ds.UpdateVPPTokenTeams(ctx, tokenID, []uint{}) + require.NoError(t, err) + require.NotNil(t, upTok.Teams) + require.Len(t, upTok.Teams, 0, "After reassigning to all teams, Teams should be zero-length (all teams)") + + // Confirm that the assignment is present as "All teams" + gotTok, err := ds.GetVPPToken(ctx, tokenID) + require.NoError(t, err) + require.NotNil(t, gotTok.Teams) + require.Len(t, gotTok.Teams, 0, "After reassigning to all teams, Teams should be zero-length (all teams)") + + // Now, assign back to just team1 + upTok, err = ds.UpdateVPPTokenTeams(ctx, tokenID, []uint{team1.ID}) + require.NoError(t, err) + require.Len(t, upTok.Teams, 1) + require.Equal(t, team1.ID, upTok.Teams[0].ID) +} + func testGetOrInsertSoftwareTitleForVPPApp(t *testing.T, ds *Datastore) { ctx := context.Background() @@ -2028,5 +2069,4 @@ func testGetUnverifiedVPPInstallsForHost(t *testing.T, ds *Datastore) { require.NoError(t, err) assert.Len(t, x, step.after) } - }