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

Project Details Page - Allow Edit and View Details #3286

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
28 changes: 28 additions & 0 deletions frontend/src/__tests__/cypress/cypress/pages/projects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,34 @@ class ProjectDetails {
);
}

showProjectResourceDetails() {
return cy.findByTestId('resource-name-icon-button').click();
}

findProjectResourceNameText() {
return cy.findByTestId('resource-name-text');
}

findProjectResourceKindText() {
return cy.findByTestId('resource-kind-text');
}

findProjectActions() {
return cy.findByTestId('project-actions');
}

showProjectActions() {
cy.findByTestId('project-actions').click();
}

findEditProjectAction() {
return cy.findByTestId('edit-project-action');
}

findDeleteProjectAction() {
return cy.findByTestId('delete-project-action');
}

findImportPipelineButton(timeout?: number) {
return cy.findByTestId('import-pipeline-button', { timeout });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ import { mockProjectK8sResource } from '~/__mocks__/mockProjectK8sResource';
import { mockRouteK8sResource } from '~/__mocks__/mockRouteK8sResource';
import { mockSecretK8sResource } from '~/__mocks__/mockSecretK8sResource';
import { mockServingRuntimeTemplateK8sResource } from '~/__mocks__/mockServingRuntimeTemplateK8sResource';
import { projectDetails } from '~/__tests__/cypress/cypress/pages/projects';
import {
deleteProjectModal,
editProjectModal,
projectDetails,
} from '~/__tests__/cypress/cypress/pages/projects';
import { ServingRuntimePlatform } from '~/types';
import {
DataSciencePipelineApplicationModel,
Expand All @@ -31,6 +35,7 @@ import {
} from '~/__tests__/cypress/cypress/utils/models';
import { mockServingRuntimeK8sResource } from '~/__mocks__/mockServingRuntimeK8sResource';
import { mockInferenceServiceK8sResource } from '~/__mocks__/mockInferenceServiceK8sResource';
import { asProjectAdminUser } from '~/__tests__/cypress/cypress/utils/mockUsers';

type HandlersProps = {
isEmpty?: boolean;
Expand Down Expand Up @@ -259,6 +264,41 @@ describe('Project Details', () => {
projectDetails.shouldBeEmptyState('Pipelines', 'pipelines-projects', true);
});

it('Shows project information', () => {
initIntercepts({ disableKServeConfig: true, disableModelConfig: true });
projectDetails.visit('test-project');
projectDetails.showProjectResourceDetails();
projectDetails.findProjectResourceNameText().should('have.text', 'test-project');
projectDetails.findProjectResourceKindText().should('have.text', 'Project');
});

it('Should not allow actions for non-provisioning users', () => {
asProjectAdminUser({ isSelfProvisioner: false });
initIntercepts({ disableKServeConfig: true, disableModelConfig: true });
projectDetails.visit('test-project');

projectDetails.findProjectActions().should('not.exist');
});

it('Should allow actions for provisioning users', () => {
asProjectAdminUser({ isSelfProvisioner: true });
initIntercepts({ disableKServeConfig: true, disableModelConfig: true });
projectDetails.visit('test-project');

projectDetails.showProjectActions();
projectDetails.findEditProjectAction().click();

editProjectModal.shouldBeOpen();
editProjectModal.findCancelButton().click();
editProjectModal.shouldBeOpen(false);

projectDetails.showProjectActions();
projectDetails.findDeleteProjectAction().click();
deleteProjectModal.shouldBeOpen();
deleteProjectModal.findCancelButton().click();
deleteProjectModal.shouldBeOpen(false);
});

it('Both model serving platforms are disabled', () => {
initIntercepts({ disableKServeConfig: true, disableModelConfig: true });
projectDetails.visit('test-project');
Expand Down
29 changes: 21 additions & 8 deletions frontend/src/components/ResourceNameTooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import {
DescriptionListDescription,
DescriptionListGroup,
DescriptionListTerm,
Flex,
FlexItem,
Popover,
Stack,
StackItem,
Expand All @@ -26,9 +28,9 @@ const ResourceNameTooltip: React.FC<ResourceNameTooltipProps> = ({
wrap = true,
}) => (
<div style={{ display: wrap ? 'block' : 'inline-flex' }}>
<span>{children}</span>
{resource.metadata?.name && (
<div style={{ display: 'inline-block', marginLeft: 'var(--pf-v5-global--spacer--xs)' }}>
<Flex gap={{ default: 'gapXs' }} alignItems={{ default: 'alignItemsCenter' }}>
<FlexItem>{children}</FlexItem>
{resource.metadata?.name && (
<Popover
position="right"
bodyContent={
Expand All @@ -41,24 +43,35 @@ const ResourceNameTooltip: React.FC<ResourceNameTooltipProps> = ({
<DescriptionListGroup>
<DescriptionListTerm>Resource name</DescriptionListTerm>
<DescriptionListDescription>
<ClipboardCopy hoverTip="Copy" clickTip="Copied" variant="inline-compact">
<ClipboardCopy
hoverTip="Copy"
clickTip="Copied"
variant="inline-compact"
data-testid="resource-name-text"
>
{resource.metadata.name}
</ClipboardCopy>
</DescriptionListDescription>
</DescriptionListGroup>
<DescriptionListGroup>
<DescriptionListTerm>Resource type</DescriptionListTerm>
<DescriptionListDescription>{resource.kind}</DescriptionListDescription>
<DescriptionListDescription data-testid="resource-kind-text">
{resource.kind}
</DescriptionListDescription>
</DescriptionListGroup>
</DescriptionList>
</StackItem>
</Stack>
}
>
<DashboardPopupIconButton icon={<OutlinedQuestionCircleIcon />} aria-label="More info" />
<DashboardPopupIconButton
data-testid="resource-name-icon-button"
icon={<OutlinedQuestionCircleIcon />}
aria-label="More info"
/>
</Popover>
</div>
)}
)}
</Flex>
</div>
);

Expand Down
100 changes: 100 additions & 0 deletions frontend/src/pages/projects/screens/detail/ProjectActions.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import * as React from 'react';
import { Dropdown, DropdownItem, MenuToggle, DropdownList } from '@patternfly/react-core';
import { useNavigate } from 'react-router-dom';
import { getDashboardMainContainer } from '~/utilities/utils';
import { AccessReviewResourceAttributes, ProjectKind } from '~/k8sTypes';
import { useAccessReview } from '~/api';
import DeleteProjectModal from '~/pages/projects/screens/projects/DeleteProjectModal';
import ManageProjectModal from '~/pages/projects/screens/projects/ManageProjectModal';

const projectEditAccessReview: AccessReviewResourceAttributes = {
group: 'project.openshift.io',
resource: 'projectrequests',
verb: 'update',
};

const projectDeleteAccessReview: AccessReviewResourceAttributes = {
group: 'project.openshift.io',
resource: 'projectrequests',
verb: 'delete',
};

type Props = {
project: ProjectKind;
};

const ProjectActions: React.FC<Props> = ({ project }) => {
const navigate = useNavigate();
const [canEdit, editRbacLoaded] = useAccessReview({
...projectEditAccessReview,
namespace: project.metadata.name,
});
const [canDelete, deleteRbacLoaded] = useAccessReview({
...projectDeleteAccessReview,
namespace: project.metadata.name,
});
const [open, setOpen] = React.useState(false);
const [deleteOpen, setDeleteOpen] = React.useState(false);
const [editOpen, setEditOpen] = React.useState(false);

if (!editRbacLoaded || !deleteRbacLoaded || (!canEdit && !canDelete)) {
return null;
}

const DropdownComponent = (
<Dropdown
onOpenChange={setOpen}
onSelect={() => setOpen(false)}
toggle={(toggleRef) => (
<MenuToggle
aria-label="Actions"
data-testid="project-actions"
variant="secondary"
ref={toggleRef}
onClick={() => {
setOpen(!open);
}}
>
Actions
</MenuToggle>
)}
isOpen={open}
popperProps={{ position: 'right', appendTo: getDashboardMainContainer() }}
>
<DropdownList>
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the mocks, should there be a divider between these "Edit" and "Delete" dropdown items?

Choose a reason for hiding this comment

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

@jenny-s51 - We don't have design guidelines for action menus yet. When I look at other areas of the product, there are pages in the dashboard that consistently display a divider above Delete. But there are places where we have just Edit and Delete in a menu without a divider (Workbenches and Permissions list items in the Project details page).

The only guidelines I see on this topic within PF are under Menu > Red text menu:

A divider should be used to separate the destructive menu items from the non-destructive items.

We aren't using this variant on the Delete menu action (and maybe we should consider that when we draft guidelines), but the point about separating destructive menu items from the non-destructive menu items is the piece that I think is relevant here. I think this recommendation should apply regardless of how many menu items there are. So I am in favor of implementing the UI as designed in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jenny-s51 @jgiardino Looking around the UI, I see:

  • Connections (recently added) does not have the separator.
  • Data connections does not have the separator
  • Cluster storage does not have the separator
  • Permissions (both user and group) do not have the separator
  • Workbenches does not have the separator
  • Pipeline servers does have the separator
  • Pipeline versions does have the separator

I can add it here but it would be mostly inconsistent. I would suggest UX file an issue to make all of the menus consistent and we address all of them at once.

Choose a reason for hiding this comment

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

@jeff-phillips-18 @jenny-s51 @jgiardino I agree with Jeff. I’d prefer to create a separate issue once we have the menu guidelines ready to update the styling for all menus at once and ensure consistency. For now, I’d vote to drop the divider from the implementation of the Project Details action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your clarity here @jgiardino. Filing a separate ticket to make all of these menus consistent sounds like a good plan @jeff-phillips-18 @simrandhaliw.

{canEdit ? (
<DropdownItem data-testid="edit-project-action" onClick={() => setEditOpen(true)}>
Edit project
</DropdownItem>
) : null}

Check warning on line 69 in frontend/src/pages/projects/screens/detail/ProjectActions.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/projects/screens/detail/ProjectActions.tsx#L69

Added line #L69 was not covered by tests
{canDelete ? (
<DropdownItem data-testid="delete-project-action" onClick={() => setDeleteOpen(true)}>
Delete project
</DropdownItem>
) : null}

Check warning on line 74 in frontend/src/pages/projects/screens/detail/ProjectActions.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/projects/screens/detail/ProjectActions.tsx#L74

Added line #L74 was not covered by tests
</DropdownList>
</Dropdown>
);

return (
<>
{DropdownComponent}
{editOpen ? (
<ManageProjectModal editProjectData={project} onClose={() => setEditOpen(false)} />
) : null}
{deleteOpen ? (
<DeleteProjectModal
deleteData={project}
onClose={(deleted) => {
setDeleteOpen(false);
if (deleted) {
navigate('/projects');

Check warning on line 91 in frontend/src/pages/projects/screens/detail/ProjectActions.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/projects/screens/detail/ProjectActions.tsx#L91

Added line #L91 was not covered by tests
}
}}
/>
) : null}
</>
);
};

export default ProjectActions;
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ import { AccessReviewResourceAttributes } from '~/k8sTypes';
import { useAccessReview } from '~/api';
import { getDescriptionFromK8sResource, getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
import useConnectionTypesEnabled from '~/concepts/connectionTypes/useConnectionTypesEnabled';
import ResourceNameTooltip from '~/components/ResourceNameTooltip';
import useCheckLogoutParams from './useCheckLogoutParams';
import ProjectOverview from './overview/ProjectOverview';
import NotebookList from './notebooks/NotebookList';
import StorageList from './storage/StorageList';
import DataConnectionsList from './data-connections/DataConnectionsList';
import ConnectionsList from './connections/ConnectionsList';
import PipelinesSection from './pipelines/PipelinesSection';
import ProjectActions from './ProjectActions';

import './ProjectDetails.scss';

Expand Down Expand Up @@ -123,7 +125,11 @@ const ProjectDetails: React.FC = () => {
alignItems={{ default: 'alignItemsFlexStart' }}
>
<img style={{ height: 32 }} src={typedObjectImage(ProjectObjectType.project)} alt="" />
<FlexItem>{displayName}</FlexItem>
<FlexItem>
<ResourceNameTooltip resource={currentProject} wrap={false}>
{displayName}
</ResourceNameTooltip>
</FlexItem>
</Flex>
}
description={<div style={{ marginLeft: 40 }}>{description}</div>}
Expand All @@ -135,6 +141,7 @@ const ProjectDetails: React.FC = () => {
}
loaded={rbacLoaded}
empty={false}
headerAction={<ProjectActions project={currentProject} />}
>
{content()}
</ApplicationsPage>
Expand Down
Loading