Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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 packages/go/chow/ingestvalidator/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
Expand Down
3 changes: 2 additions & 1 deletion packages/go/openapi/doc/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -22337,7 +22337,8 @@
"Groups",
"Data Quality",
"Datapipe",
"Cypher"
"Cypher",
"OpenGraph"
]
},
{
Expand Down
2 changes: 1 addition & 1 deletion packages/javascript/bh-shared-ui/src/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ export { default as CollectorCard } from './CollectorCard';
export * from './CollectorCardList';
export { default as CollectorCardList } from './CollectorCardList';
export * from './ColumnHeaders';
export * from './ConditionalTooltip';
export * from './CommunityIcon';
export { default as CommunityIcon } from './CommunityIcon';
export * from './ConditionalTooltip';
export * from './ConfirmationDialog';
export { default as ConfirmationDialog } from './ConfirmationDialog';
export * from './CreateMenu';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -439,4 +439,38 @@ describe('Rule Form', () => {
'/ui/explore?searchType=cypher&exploreSearchTab=cypher&cypherSearch=aGVsbG8%2Bd29ybGQ%3D'
);
});

it('calls handleError with ruleType when creating a rule fails', async () => {
vi.mocked(useParams).mockReturnValue({ zoneId: '1', ruleId: undefined });

server.use(
rest.post('/api/v2/asset-group-tags/:tagId/selectors', (_, res, ctx) => {
return res(
ctx.status(400),
ctx.json({
errors: [{ message: 'seeds are required' }],
})
);
})
);

render(<RuleForm />);

const nameInput = await screen.findByLabelText('Name');
await user.click(nameInput);
await user.paste('test rule');

// Submit without adding any seeds — should trigger the "seeds are required" path
await user.click(await screen.findByRole('button', { name: /Create Rule/ }));

await waitFor(() => {
expect(handleErrorSpy).toHaveBeenCalledWith(
expect.anything(),
'creating',
'rule',
expect.any(Function),
expect.objectContaining({ ruleType: expect.any(Number) })
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ const RuleForm: FC = () => {
// In the API, PATCHing with an empty seeds array ignore the array.
if (Array.isArray(diffedValues.seeds) && diffedValues.seeds.length === 0) {
return addNotification(
getErrorMessage('seeds are required', 'updating', 'rule'),
getErrorMessage('seeds are required', 'updating', 'rule', ruleType),
'privilege-zones_updating-rule',
{
anchorOrigin: { vertical: 'top', horizontal: 'right' },
Expand All @@ -190,9 +190,9 @@ const RuleForm: FC = () => {

navigate(-1);
} catch (error) {
handleError(error, 'updating', 'rule', addNotification);
handleError(error, 'updating', 'rule', addNotification, { ruleType });
}
}, [tagId, ruleId, patchRuleMutation, addNotification, navigate, ruleQuery.data, form, seeds]);
}, [tagId, ruleId, ruleType, patchRuleMutation, addNotification, navigate, ruleQuery.data, form, seeds]);

const handleCreateRule = useCallback(async () => {
try {
Expand All @@ -212,9 +212,9 @@ const RuleForm: FC = () => {

navigate(tagDetailsLink(tagId));
} catch (error) {
handleError(error, 'creating', 'rule', addNotification);
handleError(error, 'creating', 'rule', addNotification, { ruleType });
}
}, [tagId, form, seeds, createRuleMutation, addNotification, navigate, tagDetailsLink]);
}, [tagId, ruleType, form, seeds, createRuleMutation, addNotification, navigate, tagDetailsLink]);

const onSubmit: SubmitHandler<RuleFormInputs> = useCallback(() => {
if (ruleId !== '') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
//
// SPDX-License-Identifier: Apache-2.0

import { SeedTypeCypher, SeedTypeObjectId } from 'js-client-library';
import { cloneDeep } from 'lodash';
import { errorSilencer } from '../../../mocks/stderr';
import { handleError } from './utils';
import { getErrorMessage, handleError } from './utils';

const mockAxiosError = {
isAxiosError: true,
Expand Down Expand Up @@ -116,4 +117,93 @@ describe('handleError', () => {
notificationOptions
);
});

it('reports an Object ID-specific error when ruleType is SeedTypeObjectId and seeds are required', () => {
const expectedMessage = 'To create a rule using Object ID, add at least one object using the field below.';

const handleErrorSpy = vi.fn();
handleError(mockAxiosCypherError, 'creating', 'rule', handleErrorSpy, { ruleType: SeedTypeObjectId });
expect(handleErrorSpy).toHaveBeenCalledWith(
expectedMessage,
'privilege-zones_creating-rule',
notificationOptions
);
});

it('reports a Cypher-specific error when ruleType is SeedTypeCypher and seeds are required', () => {
const expectedMessage =
'To save a rule created using Cypher, the Cypher must be run first. Click "Run" to continue';

const handleErrorSpy = vi.fn();
handleError(mockAxiosCypherError, 'creating', 'rule', handleErrorSpy, { ruleType: SeedTypeCypher });
expect(handleErrorSpy).toHaveBeenCalledWith(
expectedMessage,
'privilege-zones_creating-rule',
notificationOptions
);
});

it('falls back to the default Cypher message when ruleType is undefined and seeds are required', () => {
const expectedMessage =
'To save a rule created using Cypher, the Cypher must be run first. Click "Run" to continue';

const handleErrorSpy = vi.fn();
handleError(mockAxiosCypherError, 'creating', 'rule', handleErrorSpy);
expect(handleErrorSpy).toHaveBeenCalledWith(
expectedMessage,
'privilege-zones_creating-rule',
notificationOptions
);
});

it('passes ruleType through optionalParams with empty object', () => {
const handleErrorSpy = vi.fn();
handleError(mockAxiosCypherError, 'creating', 'rule', handleErrorSpy, {});
// No ruleType provided, should fall back to default Cypher message
expect(handleErrorSpy).toHaveBeenCalledWith(
'To save a rule created using Cypher, the Cypher must be run first. Click "Run" to continue',
'privilege-zones_creating-rule',
notificationOptions
);
});
});

describe('getErrorMessage', () => {
it('returns name uniqueness message for "name must be unique"', () => {
const result = getErrorMessage('name must be unique', 'creating', 'rule');
expect(result).toBe(
'Error creating rule: rule names must be unique. Please provide a unique name for your new rule and try again.'
);
});

it('returns Object ID message when ruleType is SeedTypeObjectId and seeds are required', () => {
const result = getErrorMessage('seeds are required', 'creating', 'rule', SeedTypeObjectId);
expect(result).toContain('Object ID');
expect(result).toBe('To create a rule using Object ID, add at least one object using the field below.');
});

it('returns Cypher message when ruleType is SeedTypeCypher and seeds are required', () => {
const result = getErrorMessage('seeds are required', 'creating', 'rule', SeedTypeCypher);
expect(result).toContain('Cypher');
expect(result).toContain('Click "Run" to continue');
});

it('returns fallback Cypher message when ruleType is undefined and seeds are required', () => {
const result = getErrorMessage('seeds are required', 'creating', 'rule');
expect(result).toBe(
'To save a rule created using Cypher, the Cypher must be run first. Click "Run" to continue'
);
});

it('returns default error message for unknown API messages', () => {
const result = getErrorMessage('something unexpected', 'updating', 'zone');
expect(result).toBe(
'An unexpected error occurred while updating the zone. Message: something unexpected. Please try again.'
);
});

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.');
});
Comment on lines +205 to +208
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

});
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,20 @@
//
// SPDX-License-Identifier: Apache-2.0

import { isAxiosError } from 'js-client-library';
import { isAxiosError, SeedTypeCypher, SeedTypeObjectId, SeedTypes, SeedTypesMap } from 'js-client-library';
import { OptionsObject } from 'notistack';

export const getErrorMessage = (apiMessage: string, action: string, entity: string) => {
export const getErrorMessage = (apiMessage: string, action: string, entity: string, ruleType?: SeedTypes) => {
switch (apiMessage) {
case 'name must be unique':
return `Error ${action} ${entity}: ${entity} names must be unique. Please provide a unique name for your new ${entity} and try again.`;

case 'seeds are required':
if (ruleType === SeedTypeObjectId) {
return `To create a ${entity} using ${SeedTypesMap[SeedTypeObjectId]}, add at least one object using the field below.`;
Comment on lines +26 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} 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`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


default:
Expand All @@ -34,7 +39,8 @@ export const handleError = (
error: unknown,
action: 'creating' | 'updating' | 'deleting',
entity: 'rule' | 'zone' | 'label',
addNotification: (notification: string, key?: string, options?: OptionsObject) => void
addNotification: (notification: string, key?: string, options?: OptionsObject) => void,
optionalParams?: { ruleType?: SeedTypes }
) => {
console.error(error);

Expand All @@ -49,7 +55,7 @@ export const handleError = (
const apiMessage = errorsList.length ? errorsList[0].message : error.response?.statusText || undefined;

if (apiMessage) {
message = getErrorMessage(apiMessage, action, entity);
message = getErrorMessage(apiMessage, action, entity, optionalParams?.ruleType);
}
}

Expand Down
Loading