fix(UI): Conditional error message based on ruleType for PZ Create Rule functionality Fixes BED-7291#2414
fix(UI): Conditional error message based on ruleType for PZ Create Rule functionality Fixes BED-7291#2414
Conversation
…le functionality Fixes BED-7291 - In bh-shared-ui RuleForm component pass ruleType vlaue into handleError and getErrorMessage utility functions, handleCreateRule and handlePatchRule dependency arrays - In bh-shared-ui utils file: - In handleError util function pass in optionalParams optional function param object value with ruleType optional key to pass to getErrorMessage helper function - In getErrorMessage util function utilize ruleType to conditionally return specific message strings based on ruleType vlaue of SeedTypeObjectId vs. SeedTypeCypher vs. no ruleType val passed in Fixes: BED-7291
merging with main
📝 WalkthroughWalkthroughThe changes enhance error handling in the PrivilegeZones Save workflow by adding ruleType context to error messages for Cypher and Object ID seed types. Test coverage is expanded for error scenarios, utility functions are extended with specialized error messaging logic, and a minor OpenAPI schema update adds OpenGraph tagging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts (1)
133-157:SeedTypeCyphertest and fallback test assert the same string — indirect evidence of a redundant branch.Lines 133-144 (
ruleType: SeedTypeCypher) and lines 146-157 (noruleType) expect an identical message. This confirms that the explicitSeedTypeCypherbranch inutils.ts(lines 28-30) is currently a no-op whenSeedTypesMap[SeedTypeCypher] === "Cypher". Consider adding a comment inutils.tsexplaining the intent of that branch (e.g., anticipated future rename of the Cypher seed type label), or collapsing it into the fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts` around lines 133 - 157, Tests show identical expected messages for the SeedTypeCypher-specific branch and the undefined fallback, indicating the SeedTypeCypher branch in utils.ts (see handleError and SeedTypesMap / SeedTypeCypher usage) is redundant; either collapse the conditional into the fallback or keep the branch but add a clear comment above the SeedTypeCypher case explaining the intent (e.g., anticipating a future label/name change for the Cypher seed type) so it’s not mistaken for dead code—update handleError accordingly and adjust or annotate the SeedTypeCypher logic to reflect the chosen approach.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx (1)
443-475: Tighten theruleTypeassertion and suppressconsole.error.Two minor issues:
- Loose assertion (line 472):
expect.any(Number)only confirms a number was passed. Since the form initialises withruleType: SeedTypeObjectId(=1) and the test never changes the seed type, assert the exact value to catch regressions:💡 More precise assertion
- expect.objectContaining({ ruleType: expect.any(Number) }) + expect.objectContaining({ ruleType: SeedTypeObjectId })Add the import at the top of the file (already imported on line 18, so no change needed there for
SeedTypeCypher; addSeedTypeObjectId):-import { AssetGroupTagSelector, AssetGroupTagSelectorAutoCertifyAllMembers, SeedTypeCypher } from 'js-client-library'; +import { AssetGroupTagSelector, AssetGroupTagSelectorAutoCertifyAllMembers, SeedTypeCypher, SeedTypeObjectId } from 'js-client-library';
- Missing
console.errorsuppression:handleErrorcallsconsole.error(error)internally. The 400 mock response will surface in test output. Other error-path tests in this file (e.g. line 229) silence it withconsole.error = vi.fn().💡 Suppress console noise
+ console.error = vi.fn(); vi.mocked(useParams).mockReturnValue({ zoneId: '1', ruleId: undefined });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx` around lines 443 - 475, Update the failing test to (1) assert the exact ruleType value instead of expect.any(Number) by checking for SeedTypeObjectId (ensure SeedTypeObjectId is imported/available in the test) when verifying handleErrorSpy, and (2) mute console.error before triggering the error path (e.g., set console.error = vi.fn() or similar) so the mocked 400 response doesn't print noise; locate the assertion that calls handleErrorSpy and the test setup block that triggers the POST error in RuleForm.test.tsx to apply these changes.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
28-31: Remove redundantSeedTypeCypherbranch—it produces the same output as the fallback.The
else if (ruleType === SeedTypeCypher)branch at lines 28–29 generates an identical string to the default fallback at line 31. SinceSeedTypesMap[SeedTypeCypher]evaluates to'Cypher', both code paths produce'To save a {entity} created using Cypher, the Cypher must be run first. Click "Run" to continue'. Tests at lines 133–144 and 146–157 confirm this equivalence. Simplify by removing the dead branch unless forward-compatibility with futureSeedTypesMapchanges is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts` around lines 28 - 31, The branch checking for SeedTypeCypher is redundant because SeedTypesMap[SeedTypeCypher] equals 'Cypher' and produces the same message as the fallback; remove the else if (ruleType === SeedTypeCypher) branch and return the default fallback message for Cypher cases instead, keeping the code that uses ruleType, SeedTypesMap and entity intact so other seed types still get special messages (update any affected unit tests referencing SeedTypeCypher if necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts`:
- Around line 205-208: The test currently asserts a hardcoded "To create"
message while calling getErrorMessage('seeds are required', 'updating', 'zone',
SeedTypeObjectId), so update the expectation to reflect the passed action;
change the expected string to use the action ("updating") for the Object ID
message (i.e., make the expected message match what getErrorMessage will return
once the verb is driven by the action parameter) so the test aligns with
getErrorMessage and SeedTypeObjectId usage.
In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts`:
- Around line 26-27: The message for the Object ID branch currently hardcodes
"To create" and ignores the action parameter (seen in handlePatchRule and the
ruleType/SeedTypeObjectId branch), so update the template to use the passed-in
action (e.g., `${action} a ${entity} using ${SeedTypesMap[SeedTypeObjectId]},
add at least one object using the field below.`) so the text reflects creating
vs updating; adjust any tests expecting the previous fixed wording if needed.
---
Nitpick comments:
In
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx`:
- Around line 443-475: Update the failing test to (1) assert the exact ruleType
value instead of expect.any(Number) by checking for SeedTypeObjectId (ensure
SeedTypeObjectId is imported/available in the test) when verifying
handleErrorSpy, and (2) mute console.error before triggering the error path
(e.g., set console.error = vi.fn() or similar) so the mocked 400 response
doesn't print noise; locate the assertion that calls handleErrorSpy and the test
setup block that triggers the POST error in RuleForm.test.tsx to apply these
changes.
In
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts`:
- Around line 133-157: Tests show identical expected messages for the
SeedTypeCypher-specific branch and the undefined fallback, indicating the
SeedTypeCypher branch in utils.ts (see handleError and SeedTypesMap /
SeedTypeCypher usage) is redundant; either collapse the conditional into the
fallback or keep the branch but add a clear comment above the SeedTypeCypher
case explaining the intent (e.g., anticipating a future label/name change for
the Cypher seed type) so it’s not mistaken for dead code—update handleError
accordingly and adjust or annotate the SeedTypeCypher logic to reflect the
chosen approach.
In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts`:
- Around line 28-31: The branch checking for SeedTypeCypher is redundant because
SeedTypesMap[SeedTypeCypher] equals 'Cypher' and produces the same message as
the fallback; remove the else if (ruleType === SeedTypeCypher) branch and return
the default fallback message for Cypher cases instead, keeping the code that
uses ruleType, SeedTypesMap and entity intact so other seed types still get
special messages (update any affected unit tests referencing SeedTypeCypher if
necessary).
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/go/chow/ingestvalidator/schema.gopackages/go/openapi/doc/openapi.jsonpackages/javascript/bh-shared-ui/src/components/index.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts
| it('uses the correct entity name in the Object ID message', () => { | ||
| const result = getErrorMessage('seeds are required', 'updating', 'zone', SeedTypeObjectId); | ||
| expect(result).toBe('To create a zone using Object ID, add at least one object using the field below.'); | ||
| }); |
There was a problem hiding this comment.
Test confirms action is ignored in the Object ID message — downstream corroboration of the utils.ts issue.
The assertion expect(result).toBe('To create a zone using Object ID, ...') with action: 'updating' passed in explicitly documents that the hardcoded "To create" verb in utils.ts line 27 ignores action. This aligns with the minor issue flagged in utils.ts. If that verb is corrected there, update this test expectation accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts`
around lines 205 - 208, The test currently asserts a hardcoded "To create"
message while calling getErrorMessage('seeds are required', 'updating', 'zone',
SeedTypeObjectId), so update the expectation to reflect the passed action;
change the expected string to use the action ("updating") for the Object ID
message (i.e., make the expected message match what getErrorMessage will return
once the verb is driven by the action parameter) so the test aligns with
getErrorMessage and SeedTypeObjectId usage.
| if (ruleType === SeedTypeObjectId) { | ||
| return `To create a ${entity} using ${SeedTypesMap[SeedTypeObjectId]}, add at least one object using the field below.`; |
There was a problem hiding this comment.
Hardcoded "To create" ignores the action parameter in the Object ID branch.
For the patch/update flow, handlePatchRule can reach this message with action = 'updating', yet the returned string always says "To create a ...". The utils.test.ts at line 205-208 already confirms this by calling with action: 'updating' while expecting "To create a zone...".
Either incorporate action into the message, or accept that the verb is intentionally fixed (and remove action from the template here). As-is it's misleading on the update path.
💡 Suggested fix to use action context
- if (ruleType === SeedTypeObjectId) {
- return `To create a ${entity} using ${SeedTypesMap[SeedTypeObjectId]}, add at least one object using the field below.`;
+ if (ruleType === SeedTypeObjectId) {
+ return `To ${action} a ${entity} using ${SeedTypesMap[SeedTypeObjectId]}, add at least one object using the field below.`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ruleType === SeedTypeObjectId) { | |
| return `To create a ${entity} using ${SeedTypesMap[SeedTypeObjectId]}, add at least one object using the field below.`; | |
| if (ruleType === SeedTypeObjectId) { | |
| return `To ${action} a ${entity} using ${SeedTypesMap[SeedTypeObjectId]}, add at least one object using the field below.`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts`
around lines 26 - 27, The message for the Object ID branch currently hardcodes
"To create" and ignores the action parameter (seen in handlePatchRule and the
ruleType/SeedTypeObjectId branch), so update the template to use the passed-in
action (e.g., `${action} a ${entity} using ${SeedTypesMap[SeedTypeObjectId]},
add at least one object using the field below.`) so the text reflects creating
vs updating; adjust any tests expecting the previous fixed wording if needed.
slimi-so
changed the title
| } else if (ruleType === SeedTypeCypher) { | ||
| return `To save a ${entity} created using ${SeedTypesMap[SeedTypeCypher]}, the ${SeedTypesMap[SeedTypeCypher]} must be run first. Click "Run" to continue`; | ||
| } | ||
| return `To save a ${entity} created using Cypher, the Cypher must be run first. Click "Run" to continue`; |
There was a problem hiding this comment.
Not fully read on the context of this feature, but for this default returned error should we consider something a bit more generic ? Seems like if there was an error that wasn't related to Cypher that did not match an above case, it would show as related to a Cypher which would not necessarily be true.
|
|
||
| case 'seeds are required': | ||
| if (ruleType === SeedTypeObjectId) { | ||
| return `To create a ${entity} using ${SeedTypesMap[SeedTypeObjectId]}, add at least one object using the field below.`; |
There was a problem hiding this comment.
This is an interesting comment from coderabbit, seems like the bug addressed in this PR is related to creating and updating. Yet when updating the error includes "To create" which could be misleading. Maybe something like the cypher one? where it says To save a... So it would say To save a rule using ObjectID... in either created or updating and it would make sense.
2 participants
Description
*- In bh-shared-ui RuleForm component pass ruleType vlaue into handleError and getErrorMessage utility functions, handleCreateRule and handlePatchRule dependency arrays
Motivation and Context
Resolves <BED-7291>
Why is this change required? What problem does it solve?
It was giving wrong error message for creating rule with empty Object ID in Privilege Zones page. Now it gives the correct error message for trying to create a rule with empty Object ID in Privilege Zones page
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
By manual testing in browser and by unit tests
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style