Skip to content

Commit

Permalink
refactor: decrease complexity of ActionForm, NetworkTable, MachineFor…
Browse files Browse the repository at this point in the history
…ms (#5452)

- add eslint-plugin-complexity
- add complexity lint rule with max of 25
  • Loading branch information
petermakowski authored Jun 6, 2024
1 parent 11a9694 commit 94310e9
Show file tree
Hide file tree
Showing 9 changed files with 366 additions and 389 deletions.
3 changes: 2 additions & 1 deletion .eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const path = require("path");

module.exports = {
root: true,
plugins: ["unused-imports", "prettier"],
plugins: ["unused-imports", "complexity", "prettier"],
extends: [
"react-app",
"plugin:prettier/recommended",
Expand Down Expand Up @@ -50,6 +50,7 @@ module.exports = {
},
rules: {
"prettier/prettier": "error",
complexity: ["error", { max: 25 }],
"@typescript-eslint/no-unused-vars": "off",
"unused-imports/no-unused-imports": "error",
"unused-imports/no-unused-vars": [
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@
"eslint-config-prettier": "9.1.0",
"eslint-config-react-app": "7.0.1",
"eslint-import-resolver-typescript": "3.6.1",
"eslint-plugin-complexity": "1.0.2",
"eslint-plugin-cypress": "2.15.2",
"eslint-plugin-no-only-tests": "3.1.0",
"eslint-plugin-playwright": "1.6.1",
Expand Down
3 changes: 2 additions & 1 deletion src/app/base/components/ActionForm/ActionForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { FormikFormProps } from "@/app/base/components/FormikForm";
import FormikForm from "@/app/base/components/FormikForm";
import { useProcessing } from "@/app/base/hooks";
import type { ActionState } from "@/app/base/types";
import type { NodeActions } from "@/app/store/types/node";
import { getNodeActionLabel } from "@/app/store/utils";

const getLabel = (
Expand All @@ -28,7 +29,7 @@ const getLabel = (
// e.g. "2 machines"
modelString = `${selectedCount} ${modelName}s`;
}
return getNodeActionLabel(modelString, actionName, processing);
return getNodeActionLabel(modelString, actionName as NodeActions, processing);
};

export type ActionFormProps<V extends object, E = null> = Omit<
Expand Down
247 changes: 138 additions & 109 deletions src/app/base/components/node/networking/NetworkTable/NetworkTable.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { ValueOf } from "@canonical/react-components";
import { MainTable } from "@canonical/react-components";
import type { MainTableRow } from "@canonical/react-components/dist/components/MainTable/MainTable";
import classNames from "classnames";
Expand Down Expand Up @@ -105,6 +106,93 @@ const getSortValue = (sortKey: SortKey, row: NetworkRow) => {
return isComparable(value) ? value : null;
};

const generateColumnData = (
checkboxHandler: CheckboxHandlers<Selected> | null,
hasActions: boolean,
isABondOrBridgeParent: boolean,
link: NetworkLink | null,
nic: NetworkInterface,
node: MachineDetails | ControllerDetails,
selected: Props["selected"],
setExpanded?: SetExpanded,
setSelected?: Props["setSelected"]
) => [
{
"aria-label": Label.Name,
content: (
<NameColumn
checkSelected={checkboxHandler?.checkSelected}
checkboxSpace={
// When interfaces can't be selected then we still need to add space so
// the parent rows appear nested under the bond or bridge.
(!hasActions && isABondOrBridgeParent) || isABondOrBridgeParent
}
handleRowCheckbox={checkboxHandler?.handleRowCheckbox}
link={link}
nic={nic}
node={node}
selected={selected}
showCheckbox={!isABondOrBridgeParent && hasActions}
/>
),
},
{
"aria-label": Label.PXE,
content: !isABondOrBridgeParent && (
<PXEColumn link={link} nic={nic} node={node} />
),
className: "u-align--center",
},
{
"aria-label": Label.Speed,
content: <SpeedColumn link={link} nic={nic} node={node} />,
},
{
"aria-label": Label.Type,
content: <TypeColumn link={link} nic={nic} node={node} />,
},
{
"aria-label": Label.Fabric,
content: !isABondOrBridgeParent && (
<FabricColumn link={link} nic={nic} node={node} />
),
},
{
"aria-label": Label.Subnet,
content: !isABondOrBridgeParent && (
<SubnetColumn link={link} nic={nic} node={node} />
),
},
{
"aria-label": Label.IP,
content: !isABondOrBridgeParent && (
<IPColumn link={link} nic={nic} node={node} />
),
},
{
"aria-label": Label.DHCP,
content: !isABondOrBridgeParent && <DHCPColumn nic={nic} />,
},
...(hasActions
? [
{
"aria-label": Label.Actions,
className: "u-align--right",
content:
!isABondOrBridgeParent && nodeIsMachine(node) && setExpanded ? (
<NetworkTableActions
link={link}
nic={nic}
selected={selected}
setSelected={setSelected}
systemId={node.system_id}
/>
) : null,
},
]
: []),
];

const generateRow = (
checkboxHandler: CheckboxHandlers<Selected> | null,
fabrics: Fabric[],
Expand Down Expand Up @@ -147,96 +235,29 @@ const generateRow = (
nic,
link
);
const showCheckbox = !isABondOrBridgeParent && hasActions;
const select = showCheckbox
? {
linkId: link?.id,
nicId: nic?.id,
}
: null;
const columns = generateColumnData(
checkboxHandler,
hasActions,
isABondOrBridgeParent,
link,
nic,
node,
selected,
setExpanded,
setSelected
);

return {
className: classNames("p-table__row", {
"truncated-border": isABondOrBridgeParent,
}),
columns: [
{
"aria-label": Label.Name,
content: (
<NameColumn
checkSelected={checkboxHandler?.checkSelected}
checkboxSpace={
// When interfaces can't be selected then we still need to add space so
// the parent rows appear nested under the bond or bridge.
(!showCheckbox && hasActions) || isABondOrBridgeParent
}
handleRowCheckbox={checkboxHandler?.handleRowCheckbox}
link={link}
nic={nic}
node={node}
selected={selected}
showCheckbox={showCheckbox}
/>
),
},
{
"aria-label": Label.PXE,
content: !isABondOrBridgeParent && (
<PXEColumn link={link} nic={nic} node={node} />
),
className: "u-align--center",
},
{
"aria-label": Label.Speed,
content: <SpeedColumn link={link} nic={nic} node={node} />,
},
{
"aria-label": Label.Type,
content: <TypeColumn link={link} nic={nic} node={node} />,
},
{
"aria-label": Label.Fabric,
content: !isABondOrBridgeParent && (
<FabricColumn link={link} nic={nic} node={node} />
),
},
{
"aria-label": Label.Subnet,
content: !isABondOrBridgeParent && (
<SubnetColumn link={link} nic={nic} node={node} />
),
},
{
"aria-label": Label.IP,
content: !isABondOrBridgeParent && (
<IPColumn link={link} nic={nic} node={node} />
),
},
{
"aria-label": Label.DHCP,
content: !isABondOrBridgeParent && <DHCPColumn nic={nic} />,
},
...(hasActions
? [
{
"aria-label": Label.Actions,
className: "u-align--right",
content:
!isABondOrBridgeParent && nodeIsMachine(node) && setExpanded ? (
<NetworkTableActions
link={link}
nic={nic}
selected={selected}
setSelected={setSelected}
systemId={node.system_id}
/>
) : null,
},
]
: []),
],
columns,
"data-testid": name,
key: name,
select,
select:
!isABondOrBridgeParent && hasActions
? { linkId: link?.id, nicId: nic?.id }
: null,
sortData: {
bondOrBridge:
(isABondOrBridgeParent && nic.children[0]) ||
Expand Down Expand Up @@ -323,6 +344,33 @@ const getChild = (row: NetworkRow, rows: NetworkRow[]): NetworkRow | null => {
);
};

const compareValues = (
rowAValue: string | number | null,
rowBValue: string | number | null,
direction: ValueOf<typeof SortDirection>
) => {
if (!rowAValue && !rowBValue) {
return 0;
}
if (
(rowBValue && !rowAValue) ||
(isComparable(rowAValue) &&
isComparable(rowBValue) &&
rowAValue < rowBValue)
) {
return direction === SortDirection.DESCENDING ? -1 : 1;
}
if (
(rowAValue && !rowBValue) ||
(isComparable(rowAValue) &&
isComparable(rowBValue) &&
rowAValue > rowBValue)
) {
return direction === SortDirection.DESCENDING ? 1 : -1;
}
return 0;
};

const rowSort = (
rowA: NetworkRow,
rowB: NetworkRow,
Expand All @@ -336,21 +384,20 @@ const rowSort = (
key = "bondOrBridge";
direction = SortDirection.ASCENDING;
}

// Get the bond or bridge child rows.
const childA = getChild(rowA, rows);
const childB = getChild(rowB, rows);
const inSameBondOrBridge =
rowA.sortData.bondOrBridge === rowB.sortData.bondOrBridge;
// Get the values to sort by. When comparing bond or bridge parents (the
// "siblings" of the bond or bridge) then use the row values, otherwise use
// the value from the child (the main row that preceeds the parents) so that all
// the rows for a bond or bridge end up together.

let rowAValue = key
? getSortValue(key, childA && !inSameBondOrBridge ? childA : rowA)
: null;
let rowBValue = key
? getSortValue(key, childB && !inSameBondOrBridge ? childB : rowB)
: null;

// If the rows are in the same bond or bridge then put the child first.
if (inSameBondOrBridge) {
if (
Expand All @@ -363,33 +410,15 @@ const rowSort = (
return 0;
}
}

if (rowAValue === rowBValue) {
// Rows that have the same value need to be sorted by the bond or bridge id
// so that they don't lose their grouping.
rowAValue = getSortValue("bondOrBridge", rowA);
rowBValue = getSortValue("bondOrBridge", rowB);
}
// From this point on compare the values as normal.
if (!rowAValue && !rowBValue) {
return 0;
}
if (
(rowBValue && !rowAValue) ||
(isComparable(rowAValue) &&
isComparable(rowBValue) &&
rowAValue < rowBValue)
) {
return direction === SortDirection.DESCENDING ? -1 : 1;
}
if (
(rowAValue && !rowBValue) ||
(isComparable(rowAValue) &&
isComparable(rowBValue) &&
rowAValue > rowBValue)
) {
return direction === SortDirection.DESCENDING ? 1 : -1;
}
return 0;

return compareValues(rowAValue, rowBValue, direction);
};

export const generateUniqueId = ({ linkId, nicId }: Selected): string =>
Expand Down
Loading

0 comments on commit 94310e9

Please sign in to comment.