Skip to content

Commit

Permalink
Fix editor type switching bug for non-standard roles (#49991) (#50068)
Browse files Browse the repository at this point in the history
This fixes a case where we start with a non-standard role that even
after resetting would cause a validation error, then go to the standard
editor only to be stuck there by the validation requirement, while the
editor UI is itself blocked.
  • Loading branch information
bl-nero authored Dec 12, 2024
1 parent d4cac2e commit cfadbd7
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 1 deletion.
44 changes: 44 additions & 0 deletions web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,50 @@ test('rendering and switching tabs for a non-standard role', async () => {
expect(screen.getByRole('button', { name: 'Update Role' })).toBeEnabled();
});

test('switching tabs triggers validation', async () => {
// Triggering validation is necessary, because server-side yamlification
// sometimes will reject the data anyway.
render(<TestRoleEditor />);
await user.clear(screen.getByLabelText('Role Name'));
expect(getStandardEditorTab()).toHaveAttribute('aria-selected', 'true');
await user.click(getYamlEditorTab());
expect(screen.getByLabelText('Role Name')).toHaveAccessibleDescription(
'Role name is required'
);
// Expect to still be on the standard tab.
expect(getStandardEditorTab()).toHaveAttribute('aria-selected', 'true');
});

test('switching tabs ignores standard model validation for a non-standard role', async () => {
// The purpose of this test is to rule out a case where we start with a
// non-standard role that even after resetting would cause a validation
// error, then go to the standard editor only to be stuck there by the
// validation requirement, while the editor UI is itself blocked.
const originalRole = withDefaults({
// This will trigger a validation error. Note that empty metadata is a very
// blunt tool here, but that's certainly one case where we can guarantee
// it's gonna cause validation errors. The real-world scenarios would be
// much more subtle, but also more likely to get fixed in future, rendering
// this test case futile.
metadata: {},
spec: {},
unsupportedField: true, // This will cause disabling the standard editor.
} as any as Role);
render(
<TestRoleEditor
originalRole={{ object: originalRole, yaml: toFauxYaml(originalRole) }}
/>
);
expect(getYamlEditorTab()).toHaveAttribute('aria-selected', 'true');
await user.click(getStandardEditorTab());
expect(
screen.getByText(/Some fields were not readable by the standard editor/)
).toBeVisible();
await user.click(getYamlEditorTab());
// Proceed, even though our validation would consider the data invalid.
expect(getYamlEditorTab()).toHaveAttribute('aria-selected', 'true');
});

test('no double conversions when clicking already active tabs', async () => {
render(<TestRoleEditor />);
await user.click(getYamlEditorTab());
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleport/src/Roles/RoleEditor/RoleEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export const RoleEditor = ({
// requires model to be valid. However, if it's OK, we reset the validator.
// We don't want it to be validating at this point, since the user didn't
// attempt to submit the form.
if (!validator.validate()) return;
if (!standardModel.roleModel.requiresReset && !validator.validate()) return;
validator.reset();

switch (activeIndex) {
Expand Down

0 comments on commit cfadbd7

Please sign in to comment.