Manage data policy list view #892
Conversation
GowthamShanmugam
commented
Jun 6, 2023
•
GowthamShanmugam
commented
- Find all assigned policies to the app and display them in a list view.
- A dummy action is added for bulk policy un assign the policy.
- A dummy action is added for individual policy-wise un assign the policy.
- A dummy action is added for to view policy configuration info
- Dummy primary action button to switch the assign policy view.
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
2 times, most recently
from
19cc70b
to
720c8dc
Compare
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
from
720c8dc
to
6bec61a
Compare
|
/hold |
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
from
6bec61a
to
4fc92df
Compare
Screencast.from.2023-06-07.14-09-23.webm |
|
/hold cancel |
|
TODO on next PR: Unassign policy: Assign policy: |
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
from
4fc92df
to
2789b18
Compare
| @@ -0,0 +1,77 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
I don;t think we need "/mco/components/modals/app-manage-data-policy/components/" directory within "packages/mco/components/" (components within components).
"app-manage-data-policy-modal.tsx" can just directly be inside "app-manage-data-policy".
There was a problem hiding this comment.
yes, I thought the same. But the reason I used the component folder is, I kept all hook-calling logic inside the parser folder, Becuase parsers will grow for different app types. If I keep "app-manage-data-policy-modal.tsx" outside of the component then the component call hierarchy won't be correct. call will be like:
acm-action-callback.tsx -> parser/argo-application-set-parser.tsx -> ../app-manage-data-policy-modal.tsx
This is the reason i kept one more folder.
| @@ -0,0 +1,23 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
Name of the directory "app-manage-policies", instead of "app-manage-policy".
Since it is multi select table, meaning there can be multiple policies per app.
| const modalViewContext = () => { | ||
| switch (state.modalViewContext) { | ||
| case ModalViewContext.POLICY_LIST_VIEW: | ||
| return ( | ||
| <> | ||
| <ModalHeader | ||
| title={t('Manage Policy')} | ||
| description={t( | ||
| 'Assign policy to protect the application and ensure quick recovery. Unassign policy from an application when they no longer require to be managed.' | ||
| )} | ||
| /> | ||
| <ModalListView | ||
| dataPolicyInfo={applicaitonInfo?.dataPolicies} | ||
| state={state} | ||
| dispatch={dispatch} | ||
| /> | ||
| </> | ||
| ); | ||
| case ModalViewContext.ASSIGN_POLICY_VIEW: | ||
| return <>ASSIGN_POLICY_VIEW</>; | ||
| case ModalViewContext.DISABLE_POLICY_VIEW: | ||
| return <>DISABLE_POLICY_VIEW</>; | ||
| default: | ||
| return <>POLICY_LIST_VIEW</>; | ||
| } | ||
| }; |
There was a problem hiding this comment.
should be a new FC in itself...
There was a problem hiding this comment.
i just dont want to use && condition or any other condition on render, I thought to use the switch, that is why i created it as a function. but your are right, it should be a component
| }; | ||
|
|
||
| return ( | ||
| <React.Fragment> |
There was a problem hiding this comment.
| <React.Fragment> | |
| <> |
| <StatusBox loadError={loadError} loaded={loaded} /> | ||
| )} | ||
| </Modal> | ||
| </React.Fragment> |
There was a problem hiding this comment.
| </React.Fragment> | |
| </> |
| case ModalViewContext.DISABLE_POLICY_VIEW: | ||
| return <>DISABLE_POLICY_VIEW</>; | ||
| default: | ||
| return <>POLICY_LIST_VIEW</>; |
There was a problem hiding this comment.
should this be returning component corresponding to "ModalViewContext.POLICY_LIST_VIEW", instead of plain text ??
There was a problem hiding this comment.
its just dummy text, upcoming PR will replace this with proper component
| import { Text } from '@patternfly/react-core'; | ||
| import { ActionsColumn, IAction } from '@patternfly/react-table'; | ||
| import { Td } from '@patternfly/react-table'; | ||
| import { formatDateTimeWithZone } from '../../../../../utils'; |
There was a problem hiding this comment.
from '@odf/mco/...'
| } from '../utils/reducer'; | ||
| import { DataPolicyType } from '../utils/types'; | ||
|
|
||
| const getRow = (dataPolicyInfo: DataPolicyType[], searchText: string) => { |
There was a problem hiding this comment.
| const getRow = (dataPolicyInfo: DataPolicyType[], searchText: string) => { | |
| const getRows = (dataPolicyInfo: DataPolicyType[], searchText: string) => { |
| const getRow = (dataPolicyInfo: DataPolicyType[], searchText: string) => { | ||
| const rows: TableRowType<DataPolicyType>[] = []; | ||
| dataPolicyInfo?.forEach((policyInfo) => { | ||
| policyInfo?.policyName?.includes(searchText) && | ||
| rows.push({ | ||
| data: policyInfo, | ||
| rowId: policyInfo?.policyName, | ||
| isDisabled: false, | ||
| }); | ||
| }); | ||
| return rows; | ||
| }; |
There was a problem hiding this comment.
nits:
| const getRow = (dataPolicyInfo: DataPolicyType[], searchText: string) => { | |
| const rows: TableRowType<DataPolicyType>[] = []; | |
| dataPolicyInfo?.forEach((policyInfo) => { | |
| policyInfo?.policyName?.includes(searchText) && | |
| rows.push({ | |
| data: policyInfo, | |
| rowId: policyInfo?.policyName, | |
| isDisabled: false, | |
| }); | |
| }); | |
| return rows; | |
| }; | |
| const getRow = (dataPolicyInfo: DataPolicyType[], searchText: string) => | |
| dataPolicyInfo?.reduce((rowsAcc, policyInfo) => { | |
| policyInfo?.policyName?.includes(searchText) && | |
| rowsAcc.push({ | |
| data: policyInfo, | |
| rowId: policyInfo?.policyName, | |
| isDisabled: false, | |
| }); | |
| }, []); |
| const indexOfLastRow = currentPage * perPage; | ||
| const indexOfFirstRow = indexOfLastRow - perPage; | ||
| return [indexOfFirstRow, indexOfLastRow]; |
There was a problem hiding this comment.
if:
currentPage = 1
perPage = 10
then:
indexOfLastRow = 10
indexOfFirstRow = 0
won;t it make 11 rows in total ??
There was a problem hiding this comment.
nits: naming wise this can be confusing... better to do "end+1" while using slice later...
There was a problem hiding this comment.
rows?.slice(0, 10) will return values from 0 to 9th index of array, i dont want to complicate the logic but adding one (or) subtracting 1
| <Td translate={null} dataLabel={policyName}> | ||
| {policyName} | ||
| </Td> | ||
| <Td translate={null} dataLabel={policyType}> |
There was a problem hiding this comment.
are there diff type of policies as well ?? why do we have name of the column as "Policy type" ??
There was a problem hiding this comment.
As per UX design, the modal is designed to support different types of policy, but do we really need to support multiple types of policy in the future is not yet decided. Supporting multiple types might affect the UI performance. for now we are keeping like this.
| {isValidated ? ( | ||
| <StatusIconAndText title={status} icon={<GreenCheckCircleIcon />} /> | ||
| ) : ( | ||
| <StatusIconAndText | ||
| title={status} | ||
| icon={<RedExclamationCircleIcon />} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
| {isValidated ? ( | |
| <StatusIconAndText title={status} icon={<GreenCheckCircleIcon />} /> | |
| ) : ( | |
| <StatusIconAndText | |
| title={status} | |
| icon={<RedExclamationCircleIcon />} | |
| /> | |
| )} | |
| { | |
| <StatusIconAndText title={status} icon={isValidated ? <GreenCheckCircleIcon /> : <RedExclamationCircleIcon />} /> | |
| } |
| <Td translate={null} isActionCell> | ||
| <ActionsColumn items={RowActions(t)} isDisabled={false} /> | ||
| </Td> |
There was a problem hiding this comment.
why not Kebab that we use everywhere else ??
There was a problem hiding this comment.
kebab is for launch modal, here i just want to switch the context
| @@ -0,0 +1,17 @@ | |||
| .mco-modal-col-padding { | |||
There was a problem hiding this comment.
| .mco-modal-col-padding { | |
| .mco-modal__table--padding { |
| padding: 0 3rem 0 1rem; | ||
| } | ||
|
|
||
| .mco-modal-box__body { |
There was a problem hiding this comment.
nits:
| .mco-modal-box__body { | |
| .mco-modal__body { |
| ) => { | ||
| setPage(newPage); | ||
| }; |
There was a problem hiding this comment.
nits:
| ) => { | |
| setPage(newPage); | |
| }; | |
| ) => | |
| setPage(newPage); |
| ) => { | ||
| onSearchChange(value); | ||
| }; | ||
|
|
||
| const onToggle = () => { | ||
| setIsExpanded(!isExpanded); | ||
| }; | ||
|
|
||
| const onSelect = () => { | ||
| setIsExpanded(false); | ||
| }; |
There was a problem hiding this comment.
nits:
| ) => { | |
| onSearchChange(value); | |
| }; | |
| const onToggle = () => { | |
| setIsExpanded(!isExpanded); | |
| }; | |
| const onSelect = () => { | |
| setIsExpanded(false); | |
| }; | |
| ) => onSearchChange(value); | |
| const onToggle = () => setIsExpanded(!isExpanded); | |
| const onSelect = () => setIsExpanded(false); |
| export const PrimaryActionHeader: React.FC<PrimaryActionHeaderType> = ({ | ||
| title, | ||
| actionTitle, | ||
| disabled, | ||
| onClick, | ||
| }) => ( | ||
| <div className="odf-primary-action-title odf-primary-action--row"> | ||
| <Text component="h3">{title}</Text> | ||
| <div> | ||
| <Button | ||
| variant={ButtonVariant.primary} | ||
| onClick={onClick} | ||
| isDisabled={disabled} | ||
| > | ||
| {actionTitle} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
I think we can easily re-use ListPageCreateLink in order to replicate this component ??
There was a problem hiding this comment.
create is for to link, which will redirect the page, may be ListPageCreateButton might work, i need to check
There was a problem hiding this comment.
i tried both. ListPageCreateLink is for redirecting. For ListPageCreateButton unit testing is throws an error. Looks like one is using ListPageCreateButton.
| <ToolbarItem variant="search-filter"> | ||
| <SearchInput | ||
| placeholder="Search" | ||
| aria-label={t('Search input')} | ||
| value={searchValue} | ||
| onChange={onSearch} | ||
| /> | ||
| </ToolbarItem> | ||
| <ToolbarItem> |
There was a problem hiding this comment.
can't we re-use ListPageFilter ??
There was a problem hiding this comment.
i checked, ListPageFilter is designed for name and label-wise filters. It is comes with either name dropdown or label dropdown, in my design both are not present.
| ) => void; | ||
| }[]; | ||
| onSearchChange?: React.Dispatch<React.SetStateAction<string>>; | ||
| }; |
There was a problem hiding this comment.
are we sure we need new component and we can not re-use any of the existing "ListPage" related components exposed by console ??
cc @GowthamShanmugam
There was a problem hiding this comment.
ListPage dont has a dropdown button, also, the search box is designed for name and level-wise filtering. In manage policy modal mockups, we dont have such filtering.
| </Tbody> | ||
| </TableComposable> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
is there no way we can re-use VirtualizedTable ?? do we need to use TableComposable Tbodyetc from PF to implement this ??
cc @GowthamShanmugam
There was a problem hiding this comment.
i am not sure VirtualizedTable is providing a selectable option for each row and select all options. Also, we are using import { Table, TableHeader, TableBody } from '@patternfly/react-table'; in select-nodes-table.tsx but it is not that flexible.
There was a problem hiding this comment.
VirtualizedTable has a rowData prop, whatever is passed to this "rowData" is then passed on to each row that is added via "RowRenderer" FC which is passed to Row prop.
Maybe we can try adding first column of each row as a checkbox in this "RowRenderer", and pass a "setRows" callback via rowData prop ??
There was a problem hiding this comment.
I have a select all in column header also
There was a problem hiding this comment.
mm. looks like it might work, let me check
There was a problem hiding this comment.
i checked VirtualizedTable code, which does not support selectable table
| React.useEffect(() => { | ||
| const onKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Shift') { | ||
| setShifting(true); | ||
| } | ||
| }; | ||
| const onKeyUp = (e: KeyboardEvent) => { | ||
| if (e.key === 'Shift') { | ||
| setShifting(false); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', onKeyDown); | ||
| document.addEventListener('keyup', onKeyUp); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', onKeyDown); | ||
| document.removeEventListener('keyup', onKeyUp); | ||
| }; | ||
| }, [setShifting]); |
There was a problem hiding this comment.
do we even need to support such cases ??
I know PF might be supporting this, but, does tables in console support this ??
There was a problem hiding this comment.
i will check and remove this
| isSelecting: boolean | ||
| ) => { | ||
| // If the user is shift + selecting the checkboxes, then all intermediate checkboxes should be selected | ||
| if (shifting && recentSelectedRowIndex !== null) { |
There was a problem hiding this comment.
I don;t think we need to support this if console tables do not support this. If they does, then it might make sense to have it for us as well.
There was a problem hiding this comment.
this table is multi-select, and console tables are different, I am unable to find any other component which gives a selectable table, that why I created this selectable table component, which means this use case is also new, we may support this or we may not. Even i dont like this shifting option :)
| } from '@patternfly/react-table'; | ||
| import { useCustomTranslation } from '../useCustomTranslationHook'; | ||
|
|
||
| const isRowsSelectable = (row: TableRowType) => !row?.isDisabled; |
There was a problem hiding this comment.
| const isRowsSelectable = (row: TableRowType) => !row?.isDisabled; | |
| const isRowSelectable = (row: TableRowType) => !row?.isDisabled; |
| ); | ||
| }; | ||
|
|
||
| const isRowsSelected = (rowId: any, selectedRows: TableRowType[]) => |
There was a problem hiding this comment.
| const isRowsSelected = (rowId: any, selectedRows: TableRowType[]) => | |
| const areRowsSelected = (rowId: any, selectedRows: TableRowType[]) => |
| // Note that we perform the sort as part of the component's render logic and not in onSort. | ||
| let sortedRows: TableRowType[] = sortRows( | ||
| activeSortIndex, | ||
| activeSortDirection, | ||
| rows, | ||
| columns | ||
| ); |
There was a problem hiding this comment.
why bother to sort as a part of render ??
There was a problem hiding this comment.
aren;t we doing something which is not needed, also this will happen everytime component re-renders...
There was a problem hiding this comment.
I will check this why, the I referred patternfly example
There was a problem hiding this comment.
The funny thing is sort is always used to render level everywhere, what I am assuming is, since the list is a watcher, it needs to be sorted always. Since the arrow asc or dec is displayed always, it looks like it needs to sorted always
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
4 times, most recently
from
63b2676
to
ae881ef
Compare
|
/retest |
| @@ -77,7 +78,7 @@ const DRPolicyRow: React.FC<RowProps<DRPolicyKind, RowData>> = ({ | |||
| {obj?.metadata?.name} | |||
| </TableData> | |||
| <TableData {...tableColumnInfo[1]} activeColumnIDs={activeColumnIDs}> | |||
| {condition?.status === 'True' ? t('Validated') : t('Not Validated')} | |||
| {isDRPolicyValidated(obj) ? t('Validated') : t('Not Validated')} | |||
There was a problem hiding this comment.
Do we need to backport this potentially?
There was a problem hiding this comment.
its, not a fix, just moved the condition?.status === 'True' under util function just to reuse the logic in multiple places
| sortField: string | ||
| ) => { | ||
| const negation = c !== SortByDirection.asc; | ||
| const aValue = _.get(a, sortField, ''); |
There was a problem hiding this comment.
| const aValue = _.get(a, sortField, ''); | |
| const aValue = a?.[sortField] || ''; |
This should work.
| const selectedRowIds = selectedRows?.map((selectedRow) => | ||
| getUID(selectedRow) |
There was a problem hiding this comment.
| const selectedRowIds = selectedRows?.map((selectedRow) => | |
| getUID(selectedRow) | |
| const selectedRowIds = selectedRows?.map(getUID) |
nit: you could do something like this as well.
| ) => { | ||
| const selectedRowIds = selectedRows?.map((selectedRow) => | ||
| getUID(selectedRow) | ||
| ); |
There was a problem hiding this comment.
const selecteableRowsIds = selectableRows.map(getUID)
return ( selectableRowsIds?.every(rowID => selectedRowIds.includes(rowID)))
Would this make it easier to read? WDYT?
There was a problem hiding this comment.
yes, but still keeping the length check to avoid a corner case: every will return true for an empty array, and select all checkbox will be checked for empty rows.
| }; | ||
|
|
||
| type SelectableTableProps = <T extends K8sResourceCommon>( | ||
| props: TableProps<T> |
There was a problem hiding this comment.
| props: TableProps<T> | |
| props: React.PropsWithoutRef<TableProps<T>> |
| <> | ||
| <Modal | ||
| title={ | ||
| state.modalViewContext !== ModalViewContext.ASSIGN_POLICY_VIEW && |
There was a problem hiding this comment.
What do we do if it is AssignPolicyView?
There was a problem hiding this comment.
Assign policy view is a wizard flow, and wizard component will comeup with its own header
There was a problem hiding this comment.
i talk to UX about reusing the same header for assign policy, and he agreed. i am going to remove this condition
| SET_ERROR = 'SET_ERROR', | ||
| } | ||
|
|
||
| export const managePolicyState = (): ManagePolicyState => ({ |
There was a problem hiding this comment.
Does this need to be a function?
There was a problem hiding this comment.
not exactly, changing to object
| }, | ||
| ]; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
could we generate coulmns once instad of running getColumns(t) multiple times?
There was a problem hiding this comment.
Still looks the same to me.
There was a problem hiding this comment.
i want to use the column name under the row render component. It means column names are used between two different components. if i move get column as object under PolicyListViewTable, then i can't access it from PolicyListViewTableRow.
| children, | ||
| }) => { | ||
| return ( | ||
| <div className="mco-manage-policies__panelContent--col-padding"> |
There was a problem hiding this comment.
I do not see a scss file associated with this file? Please add a scss file and add these styles there
| children, | ||
| }) => { | ||
| return ( | ||
| <div className="mco-manage-policies__panelHeader"> |
There was a problem hiding this comment.
where are the styles defined?
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
from
ae881ef
to
72f30e6
Compare
| !_.isEmpty(drPolicy) | ||
| ? [ | ||
| { | ||
| apiVersion: drPolicy?.apiVersion, |
There was a problem hiding this comment.
| apiVersion: drPolicy?.apiVersion, | |
| apiVersion: drPolicy.apiVersion, |
| kind: drPolicy?.kind, | ||
| metadata: drPolicy?.metadata, | ||
| // TODO: For multiple DRPC find least recently created | ||
| assignedOn: drpcInfo?.[0]?.metadata?.creationTimestamp, | ||
| // TODO: For multiple DRPC summarize the activity | ||
| activity: getCurrentActivity(drpcInfo?.[0]?.status, t), | ||
| isValidated: isDRPolicyValidated(drPolicy), | ||
| schedulingInterval: drPolicy?.spec?.schedulingInterval, | ||
| replicationType: findDRType(drClusters), |
There was a problem hiding this comment.
are all these optional chaining really required after you have tested it to be not empty already?
There was a problem hiding this comment.
ack i will check and remove unwanted optional symbol
| export const getClustersFromPlacementDecision = ( | ||
| placementDecision: ACMPlacementDecisionKind | ||
| ): string[] => | ||
| placementDecision?.status?.decisions?.map( |
There was a problem hiding this comment.
This is not performing find. It's doing a map.
There was a problem hiding this comment.
i changed function to getClustersFromPlacementDecision
| apiVersion: application?.apiVersion, | ||
| kind: application?.kind, | ||
| metadata: application?.metadata, | ||
| workloadNamespace: getRemoteNSFromAppSet(application), |
There was a problem hiding this comment.
| workloadNamespace: getRemoteNSFromAppSet(application), | |
| workloadNamespace: getRemoteNamespaceFromAppSet(application), |
nit
| @@ -0,0 +1,232 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
The name of the file should be ApplicationSetModal.tsx or something related to that since it exports that component.
There was a problem hiding this comment.
This file has parser logic and calls the AppManagePoliciesModal component, the actual modal component is called in app-manage-policies-modal.tsx. Instead of renaming the file, i am renaming the component to ApplicationSetParser
| SET_ERROR = 'SET_ERROR', | ||
| } | ||
|
|
||
| export const initialPolicyState: ManagePolicyState = { |
There was a problem hiding this comment.
Why does this need to be a function?
There was a problem hiding this comment.
this is not a function, this is an object
There was a problem hiding this comment.
i already changed to object
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
2 times, most recently
from
35b4b8a
to
8852d35
Compare
| import * as React from 'react'; | ||
| import { ArgoApplicationSetModel } from '@odf/mco/models'; | ||
| import { ArgoApplicationSetKind } from '@odf/mco/types'; | ||
| import { getGVKFromK8Resource } from '@odf/mco/utils'; |
There was a problem hiding this comment.
Use getReference it's already part of shared.
|
|
||
| return ( | ||
| <> | ||
| {gvk === referenceForModel(ArgoApplicationSetModel) && ( |
There was a problem hiding this comment.
Is this supposed to launch a modal for Argo applications only? Are there other applications on which this would perform nothing?
There was a problem hiding this comment.
DR is going to support multiple application types, Argo applications is one type, and in 4.15 we support another type
| }); | ||
|
|
||
| return ( | ||
| <> |
| <StatusBox loadError={props.loadError} loaded={props.loaded} /> | ||
| )} | ||
| </Modal> | ||
| </> |
| import { ApplicationType } from './utils/types'; | ||
|
|
||
| export const AppManagePoliciesModal: React.FC<AppManagePoliciesModalProps> = ( | ||
| props |
There was a problem hiding this comment.
| props | |
| {isOpen, ... |
Destructure all props here itself.
| ]; | ||
|
|
||
| const PolicyListViewTableRow: React.FC<RowComponentType<DataPolicyType>> = ( | ||
| props |
There was a problem hiding this comment.
nit: destructure props here.
| }; | ||
|
|
||
| export const PolicyListViewTable: React.FC<PolicyListViewTableProps> = ( | ||
| props |
There was a problem hiding this comment.
nit: destructure props here.
| } from '@patternfly/react-core'; | ||
| import { useCustomTranslation } from '../useCustomTranslationHook'; | ||
|
|
||
| export const ActionDropdown: React.FC<ActionDropdownProps> = (props) => { |
| ); | ||
|
|
||
| export const PolicyListViewToolBar: React.FC<PolicyListViewToolBarProps> = ( | ||
| props |
There was a problem hiding this comment.
nit: destructure props here
Signed-off-by: Gowtham Shanmugasundaram <[email protected]>
GowthamShanmugam
force-pushed
the
app_manage_policy
branch
from
8852d35
to
04fafa5
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, GowthamShanmugam The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test odf-console-e2e-aws |
