Skip to content

Fix missing required label on confirm details page for dependent tags #87947

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

Merged
Julesssss merged 4 commits into from
Apr 17, 2026
Merged

Fix missing required label on confirm details page for dependent tags #87947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b6c1f7
Fall back to policy.requiresTag for tag required label
MelvinBot Apr 15, 2026
7bfd3f2
Fix TypeScript error: use type assertion for test objects with intent…
MelvinBot Apr 15, 2026
7d15777
Fix TypeScript error: cast through unknown for intentionally incomple…
MelvinBot Apr 15, 2026
61bd1ec
Use || instead of ?? for tag required fallback
MelvinBot Apr 15, 2026
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
2 changes: 1 addition & 1 deletion src/libs/TagsOptionsListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ function getTagVisibility({
const policyTagLists = getTagLists(policyTags);

return policyTagLists.map(({tags, required}, index) => {
const isTagRequired = required ?? false;
const isTagRequired = required || !!policy?.requiresTag;

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve per-tag-list optional state when policy requires tags

This line now treats every tag list as required whenever policy.requiresTag is true, because required || ... overrides an explicit required: false. Independent multi-level workspaces intentionally allow some lists to remain optional while the policy still requires at least one list (the guard in src/libs/OptionsListUtils/index.ts:1369-1377 only blocks making the last required list optional), so this regression makes optional lists show the “Required” label and required-field prioritization on confirm/split flows. The fallback should apply only when required is missing (null/undefined), not when it is explicitly false.

Useful? React with 👍 / 👎.

@hungvu193 hungvu193 Apr 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MelvinBot check this, don't commit the code.

@MelvinBot MelvinBot Apr 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The bot's concern is valid — the || change I pushed is wrong and should be reverted back to ??.

Why || is wrong: Independent multi-level workspaces allow admins to set individual tag lists as optional (required: false) while policy.requiresTag is true. The || operator makes ALL tag lists required whenever the global toggle is on, ignoring these per-list overrides.

Evidence:

  • WorkspaceViewTagsPage.tsx:373-398 — the per-tag-list "Required" toggle is visible for independent tags and calls setPolicyTagsRequired to set individual lists optional
  • OptionsListUtils/index.ts:1369-1380 — isMakingLastRequiredTagListOptional only prevents making the last required list optional, proving some-but-not-all optional is by design
  • ViolationsUtils.ts:163 — uses required ?? true (not ||), so the enforcement layer respects required: false. With ||, the UI would show "Required" but no violation fires — an inconsistency

The ?? operator was correct. It handles the original bug (backend sync strips required to undefined) while preserving explicit required: false overrides. If backend syncs are resetting required to false instead of undefined, that's a backend data issue to fix at the sync layer.

let shouldShow = false;

if (shouldShowTags) {
Expand Down
70 changes: 70 additions & 0 deletions tests/unit/TagsOptionsListUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,76 @@ describe('TagsOptionsListUtils', () => {
{isTagRequired: false, shouldShow: true},
]);
});

it('should fall back to policy.requiresTag when tag list required is undefined', () => {
const policyWithRequiresTag = {...mockPolicy, requiresTag: true};
// Intentionally omitting 'required' to simulate backend sync stripping the field
const policyTagsWithoutRequired = {
tagList1: {
name: 'Department',
tags: {
tag1: {name: 'Engineering', enabled: true},
tag2: {name: 'Sales', enabled: true},
},
orderWeight: 0,
},
} as unknown as PolicyTagLists;

const result = getTagVisibility({
shouldShowTags: true,
policy: policyWithRequiresTag,
policyTags: policyTagsWithoutRequired,
transaction: mockTransaction,
});

expect(result).toEqual([{isTagRequired: true, shouldShow: true}]);
});

it('should not mark tags as required when policy.requiresTag is false and tag list required is undefined', () => {
const policyWithoutRequiresTag = {...mockPolicy, requiresTag: false};
// Intentionally omitting 'required' to simulate backend sync stripping the field
const policyTagsWithoutRequired = {
tagList1: {
name: 'Department',
tags: {
tag1: {name: 'Engineering', enabled: true},
},
orderWeight: 0,
},
} as unknown as PolicyTagLists;

const result = getTagVisibility({
shouldShowTags: true,
policy: policyWithoutRequiresTag,
policyTags: policyTagsWithoutRequired,
transaction: mockTransaction,
});

expect(result).toEqual([{isTagRequired: false, shouldShow: true}]);
});

it('should mark tags as required when policy.requiresTag is true even if tag list required is false', () => {
const policyWithRequiresTag = {...mockPolicy, requiresTag: true};
const policyTagsExplicitFalse: PolicyTagLists = {
tagList1: {
name: 'Department',
required: false,
tags: {
tag1: {name: 'Engineering', enabled: true},
},
orderWeight: 0,
},
};

const result = getTagVisibility({
shouldShowTags: true,
policy: policyWithRequiresTag,
policyTags: policyTagsExplicitFalse,
transaction: mockTransaction,
});

expect(result).toEqual([{isTagRequired: true, shouldShow: true}]);
});
});

describe('getEnabledTags', () => {
Expand Down
Loading