Skip to content
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.

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

feat(mr): create mr modal #2828

Merged
merged 1 commit into from
May 29, 2024
Merged
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
4 changes: 2 additions & 2 deletions backend/src/utils/route-security.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { FastifyReply, FastifyRequest } from 'fastify';
import { isUserAdmin } from './adminUtils';
import { getNamespaces } from './notebookUtils';
import { logRequestDetails } from './fileUtils';
import { DEV_MODE } from './constants';
import { DEV_MODE, MODEL_REGISTRY_NAMESPACE } from './constants';

const testAdmin = async (
fastify: KubeFastifyInstance,
Expand Down Expand Up @@ -73,7 +73,7 @@ const requestSecurityGuard = async (
const isReadRequest = request.method.toLowerCase() === 'get';

// Check to see if a request was made against one of our namespaces
if (![notebookNamespace, dashboardNamespace].includes(namespace)) {
if (![notebookNamespace, dashboardNamespace, MODEL_REGISTRY_NAMESPACE].includes(namespace)) {
// Not a valid namespace -- cannot make direct calls to just any namespace no matter who you are
fastify.log.error(
`User requested a resource that was not in our namespaces. Namespace: ${namespace}`,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import { mockDashboardConfig, mockDscStatus } from '~/__mocks__';
import { mockDsciStatus } from '~/__mocks__/mockDsciStatus';
import { StackCapability, StackComponent } from '~/concepts/areas/types';
import { modelRegistrySettings } from '~/__tests__/cypress/cypress/pages/modelRegistrySettings';
import { pageNotfound } from '~/__tests__/cypress/cypress/pages/pageNotFound';
import { asProductAdminUser, asProjectAdminUser } from '~/__tests__/cypress/cypress/utils/users';

const setupMocksForMRSettingAccess = () => {
asProductAdminUser();
cy.interceptOdh(
'GET /api/config',
mockDashboardConfig({
disableModelRegistry: false,
}),
);
cy.interceptOdh(
'GET /api/dsc/status',
mockDscStatus({
installedComponents: {
[StackComponent.MODEL_REGISTRY]: true,
[StackComponent.MODEL_MESH]: true,
},
}),
);
cy.interceptOdh(
'GET /api/dsci/status',
mockDsciStatus({
requiredCapabilities: [StackCapability.SERVICE_MESH, StackCapability.SERVICE_MESH_AUTHZ],
}),
);
};

it('Model registry settings should not be available for non product admins', () => {
asProjectAdminUser();
modelRegistrySettings.visit(false);
pageNotfound.findPage().should('exist');
modelRegistrySettings.findNavItem().should('not.exist');
});

it('Model registry settings should be available for product admins with capabilities', () => {
setupMocksForMRSettingAccess();
// check page is accessible
modelRegistrySettings.visit(true);
// check nav item exists
modelRegistrySettings.findNavItem().should('exist');
});

describe('CreateModal', () => {
beforeEach(() => {
setupMocksForMRSettingAccess();
});

it('should display error messages when form fields are invalid and not allow submit', () => {
modelRegistrySettings.visit(true);
cy.findByText('Create model registry').click();
// Fill in the form with invalid data
cy.get('#mr-name').clear();
cy.get('#mr-host').clear();
cy.get('#mr-port').clear();
cy.get('#mr-username').clear();
cy.get('#mr-password').clear();
cy.get('#mr-database').clear();
cy.get('#mr-database').blur();
// Assert the submit button is disabled
cy.findByTestId('modal-submit-button').should('be.disabled');
// Assert that error messages are displayed
cy.findByTestId('mr-name-error')
.should('be.visible')
.contains(
"Must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character",
);
Comment on lines +69 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

Asserting static text as defined in in our react components is often a bad idea because it's a 1:1 relationship and will only fail if we change the react component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack. I plan to give these tests a second pass as part of adding more in #2829, I'll address this feedback there and request a review from you for that one when it's ready if you don't mind.

Copy link
Contributor

@mturley mturley May 29, 2024

Choose a reason for hiding this comment

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

@christianvogt looking back at this, I think it has value beyond what you're saying -- we shouldn't just assert that components render certain text for the sake of it, but this is trying to assert that the error is shown vs not shown. We could do it with a testid instead if you prefer but imo it doesn't matter much as long as the test is asserting logic and not just content. This test will fail if the form validation logic breaks.

Copy link
Contributor

@christianvogt christianvogt May 29, 2024

Choose a reason for hiding this comment

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

My concern was the contains check and not so much the check that the error node was visible since this is the only available error text that's shown in this node.

Note that if the error text was dynamic from a server response, I would agree with asserting the text presence.

Copy link
Contributor

Choose a reason for hiding this comment

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

@christianvogt FYI I addressed this in updates to #2829

cy.findByTestId('mr-host-error').should('be.visible').contains('Host cannot be empty');
cy.findByTestId('mr-port-error').should('be.visible').contains('Port cannot be empty');
cy.findByTestId('mr-username-error').should('be.visible').contains('Username cannot be empty');
cy.findByTestId('mr-password-error').should('be.visible').contains('Password cannot be empty');
cy.findByTestId('mr-database-error').scrollIntoView();
cy.findByTestId('mr-database-error').should('be.visible').contains('Database cannot be empty');
Comment on lines +55 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

@dpanshug Selectors should be part of page objects so they are reusable when we get to e2e tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

@christianvogt FYI I addressed this in updates to #2829

});

it('should enable submit button if fields are valid', () => {
modelRegistrySettings.visit(true);
cy.findByText('Create model registry').click();
// Fill in the form with invalid data
cy.get('#mr-name').type('valid-mr-name');
cy.get('#mr-host').type('host');
cy.get('#mr-port').type('1234');
cy.get('#mr-username').type('validUser');
cy.get('#mr-password').type('strongPassword');
cy.get('#mr-database').type('myDatabase');
cy.get('#mr-database').blur();
// Assert the submit button is disabled
cy.findByTestId('modal-submit-button').should('be.enabled');
// Assert that error messages are not displayed
cy.findByTestId('mr-name-error').should('not.exist');
cy.findByTestId('mr-host-error').should('not.exist');
cy.findByTestId('mr-port-error').should('not.exist');
cy.findByTestId('mr-username-error').should('not.exist');
cy.findByTestId('mr-password-error').should('not.exist');
cy.findByTestId('mr-database-error').should('not.exist');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { appChrome } from './appChrome';

class ModelRegistrySettings {
visit(wait = true) {
cy.visit('/modelRegistrySettings');
if (wait) {
this.wait();
}
}

navigate() {
this.findNavItem().click();
this.wait();
}

private wait() {
this.findHeading();
cy.testA11y();
}

private findHeading() {
cy.findByTestId('app-page-title').should('exist');
cy.findByTestId('app-page-title').contains('Model Registry Settings');
}

findNavItem() {
return appChrome.findNavItem('Model registry settings', 'Settings');
}
}

export const modelRegistrySettings = new ModelRegistrySettings();
3 changes: 3 additions & 0 deletions frontend/src/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ export * from './prometheus/distributedWorkloads';
// Network error handling
export * from './errorUtils';

// Models for use when constructing API objects
export * from './models';

// User access review hook
export * from './useAccessReview';

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/FieldList.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';
import { Stack, StackItem, FormGroup, TextInput } from '@patternfly/react-core';
import { EnvVariableDataEntry } from '~/pages/projects/types';
import PasswordInput from '~/pages/projects/components/PasswordInput';
import PasswordInput from '~/components/PasswordInput';

export type FieldOptions = {
key: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@ import { Button, InputGroup, TextInput, InputGroupItem } from '@patternfly/react
import { EyeIcon, EyeSlashIcon } from '@patternfly/react-icons';

const PasswordInput: React.FC<React.ComponentProps<typeof TextInput>> = (props) => {
const [isPassword, setPassword] = React.useState(true);
const [isPasswordHidden, setPasswordHidden] = React.useState(true);

return (
<InputGroup>
<InputGroupItem isFill>
<TextInput {...props} type={isPassword ? 'password' : 'text'} />
<TextInput {...props} type={isPasswordHidden ? 'password' : 'text'} />
</InputGroupItem>
<InputGroupItem>
<Button
aria-label={isPassword ? 'Show password' : 'Hide password'}
aria-label={isPasswordHidden ? 'Show password' : 'Hide password'}
variant="control"
onClick={() => setPassword(!isPassword)}
onClick={() => setPasswordHidden(!isPasswordHidden)}
>
{isPassword ? <EyeSlashIcon /> : <EyeIcon />}
{isPasswordHidden ? <EyeSlashIcon /> : <EyeIcon />}
</Button>
</InputGroupItem>
</InputGroup>
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/k8sTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1302,10 +1302,11 @@ export type ModelRegistryKind = K8sResourceCommon & {
};
mysql?: {
database: string;
username: string;
host: string;
port?: number;
};
postgres: {
postgres?: {
database: string;
host?: string;
passwordSecret?: {
Expand Down
6 changes: 6 additions & 0 deletions frontend/src/pages/modelRegistrySettings/CreateModal.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// targeting specific form section title elements to override --pf-v5-c-form__section-title--FontSize, --pf-v5-c-form__section-title--FontWeight
.form-subtitle-text {
color: var(--pf-v5-global--Color--200);
gitdallas marked this conversation as resolved.
Show resolved Hide resolved
font-size: initial;
font-weight: initial;
}
Loading
Loading