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: format SharedDB operations as human-readable list of changes #2877

Merged
merged 9 commits into from
Apr 19, 2024
269 changes: 269 additions & 0 deletions editor.planx.uk/src/@planx/graph/__tests__/formatOps.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,269 @@
import { formatOps,Graph } from "../index";

describe("Update operations", () => {
test("Updating a single property of a node", () => {
const ops = [
{
p: ["FW5G3EMBI3", "data", "text"],
oi: "Which vegetables?",
od: "Which fruits?",
},
];

expect(formatOps(flowWithChecklist, ops)).toEqual([
'Updated Checklist text from "Which fruits?" to "Which vegetables?"',
Copy link
Member Author

@jessicamcinchak jessicamcinchak Apr 17, 2024

Choose a reason for hiding this comment

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

It feels like helpful context to print the value of the updated data prop here, but it's a big difference between showing how text or title change than it is, for example, help text.

We may want to consider making a sort of "black list" of data props where we'd only show the name - eg "Updated Checklist moreInfo"- without the values? Could also possibly address via UI design by nesting changes list within accordion drawers in side panel or similar?

I plan to leave in as-is initially for explicitness, but would expect this feedback to come up pretty early in testing !

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds sensible - a mapping of fields which do and don't get their values displayed makes sense.

]);
});

test("Updating many properties of a node", () => {
const ops = [
{
p: ["FW5G3EMBI3", "data", "fn"],
oi: "fruit",
},
{
p: ["WDwUTbF7Gq", "data", "val"],
oi: "berry.blue",
},
{
p: ["xTBfSd1Tjy", "data", "val"],
oi: "banana",
},
];

expect(formatOps(flowWithChecklist, ops)).toEqual([
'Added Checklist fn "fruit"',
'Added Answer val "berry.blue"',
'Added Answer val "banana"',
]);
});
});

describe("Insert operations", () => {
test("Adding a new node to the graph", () => {
const ops = [
{
p: ["_root", "edges", 0],
li: "ubCTG9OtFw",
},
{
p: ["ubCTG9OtFw"],
oi: {
type: 8,
data: {
title: "This is a test",
color: "#c81bcb",
resetButton: false,
},
},
},
];

expect(formatOps(emptyFlow, ops)).toEqual([
'Added Notice "This is a test"',
]);
});

test("Adding a new node and its' children to the graph", () => {
const ops = [
{
p: ["_root", "edges", 0],
li: "FW5G3EMBI3",
},
{
p: ["FW5G3EMBI3"],
oi: {
type: 105,
data: {
allRequired: false,
text: "Which fruits?",
},
edges: ["WDwUTbF7Gq", "SO5XbLwSYp", "xTBfSd1Tjy"],
},
},
{
p: ["WDwUTbF7Gq"],
oi: {
data: {
text: "Blueberry",
},
type: 200,
},
},
{
p: ["SO5XbLwSYp"],
oi: {
data: {
text: "Orange",
},
type: 200,
},
},
{
p: ["xTBfSd1Tjy"],
oi: {
data: {
text: "Banana",
},
type: 200,
},
},
];

expect(formatOps(emptyFlow, ops)).toEqual([
'Added Checklist "Which fruits?"',
'Added Answer "Blueberry"',
'Added Answer "Orange"',
'Added Answer "Banana"',
]);
});

test("Adding a new child and data property to an existing node", () => {
const ops = [
{
p: ["FW5G3EMBI3", "data", "description"],
oi: "<p>Fruits contain seeds and come from the flower of a plant</p>",
},
{
p: ["FW5G3EMBI3", "edges"],
oi: ["WDwUTbF7Gq", "SO5XbLwSYp", "xTBfSd1Tjy", "zzQAMXexRj"],
od: ["WDwUTbF7Gq", "SO5XbLwSYp", "xTBfSd1Tjy"],
},
{
p: ["zzQAMXexRj"],
oi: {
data: {
text: "Strawberry",
},
type: 200,
},
},
];

expect(formatOps(flowWithChecklist, ops)).toEqual([
'Added Checklist description "<p>Fruits contain seeds and come from the flower of a plant</p>"',
"Updated order of Checklist edges",
'Added Answer "Strawberry"',
]);
});
});

describe("Remove operations", () => {
test("Removing a node from the graph", () => {
const ops = [
{
p: ["_root", "edges", 1],
ld: "FW5G3EMBI3",
},
{
p: ["WDwUTbF7Gq"],
od: {
data: {
text: "Blueberry",
},
type: 200,
},
},
{
p: ["SO5XbLwSYp"],
od: {
data: {
text: "Orange",
},
type: 200,
},
},
{
p: ["xTBfSd1Tjy"],
od: {
data: {
text: "Banana",
},
type: 200,
},
},
{
p: ["FW5G3EMBI3"],
od: {
type: 105,
data: {
allRequired: false,
text: "Which fruits?",
},
edges: ["wBwRtMce7c", "SO5XbLwSYp", "xTBfSd1Tjy"],
},
},
];

expect(formatOps(flowWithChecklist, ops)).toEqual([
'Removed Answer "Blueberry"',
'Removed Answer "Orange"',
'Removed Answer "Banana"',
'Removed Checklist "Which fruits?"',
]);
});

test("Removing a child of a node", () => {
const ops = [
{
p: ["FW5G3EMBI3", "edges"],
oi: ["WDwUTbF7Gq", "xTBfSd1Tjy"],
od: ["WDwUTbF7Gq", "SO5XbLwSYp", "xTBfSd1Tjy"],
},
{
p: ["SO5XbLwSYp"],
od: {
data: {
text: "Orange",
},
type: 200,
},
},
];

expect(formatOps(flowWithChecklist, ops)).toEqual([
"Updated order of Checklist edges",
'Removed Answer "Orange"',
]);
});

test.todo("Removing a data property of an existing node");
});

const emptyFlow: Graph = {
_root: {
edges: [],
},
};

const flowWithChecklist: Graph = {
_root: {
edges: ["FW5G3EMBI3"],
},
FW5G3EMBI3: {
type: 105,
data: {
allRequired: false,
text: "Which fruits?",
},
edges: ["WDwUTbF7Gq", "SO5XbLwSYp", "xTBfSd1Tjy"],
},
WDwUTbF7Gq: {
data: {
text: "Blueberry",
},
type: 200,
},
SO5XbLwSYp: {
data: {
text: "Orange",
},
type: 200,
},
xTBfSd1Tjy: {
data: {
text: "Banana",
},
type: 200,
},
};
80 changes: 80 additions & 0 deletions editor.planx.uk/src/@planx/graph/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -499,3 +499,83 @@ export const sortIdsDepthFirst =
(a, b) => allNodeIdsSorted.indexOf(a) - allNodeIdsSorted.indexOf(b),
);
};

/**
* Translates a list of ShareDB operations into a human-readable change summary.
* See https://github.com/ottypes/json0?tab=readme-ov-file#summary-of-operations
*/
export const formatOps = (graph: Graph, ops: Array<OT.Op>): string[] => {
const output: string[] = [];

// Updating a node or its properties (update = delete + insert)
const handleUpdate = (node: Node, op: OT.Object.Replace) => {
if (op.od.type && op.oi.type) {
output.push(
`Replaced ${TYPES[op.od.type]} "${op.od.data?.title || op.od.data?.text
}" with ${TYPES[op.oi.type]} "${op.oi.data?.title || op.oi.data?.text
}"`,
);
} else if (op.p.includes("data")) {
output.push(
`Updated ${node.type ? TYPES[node.type] : "node"} ${op.p?.[2]} from "${op.od}" to "${op.oi
}"`,
);
} else if (op.p.includes("edges")) {
output.push(
`Updated order of ${node.type ? TYPES[node.type] : "graph"} edges`,
);
}
};

// Updating the _root list (update = list insert or list delete)
const handleRootUpdate = (op: OT.Array.Replace) => {
if (op.p.includes("edges") && op.p.includes("_root")) {
output.push(`Re-ordered the graph`);
}
};

// Adding (inserting) a node or its properties
const handleAdd = (node: Node, op: OT.Object.Add) => {
if (op.oi.type) {
output.push(
`Added ${TYPES[op.oi.type]} "${op.oi.data?.title || op.oi.data?.text
}"`,
);
} else if (op.p.includes("data")) {
output.push(`Added ${node.type ? TYPES[node.type] : "node"} ${op.p?.[2]} "${op.oi}"`);
} else if (op.p.includes("edges")) {
output.push(`Added ${node.type ? TYPES[node.type] : "node"} to branch`);
}
};

// Removing (deleting) a node or its properties
const handleRemove = (node: Node, op: OT.Object.Remove) => {
if (op.od.type) {
output.push(
`Removed ${TYPES[op.od.type]} "${op.od.data?.title || op.od.data?.text
}"`,
);
} else if (op.p.includes("data")) {
output.push(`Removed ${node.type ? TYPES[node.type] : "node"} ${op.p?.[2]} "${op.od}"`);
} else if (op.p.includes("edges")) {
output.push(`Removed ${node.type ? TYPES[node.type] : "node"} from branch`);
}
};

ops.map((op) => {
const node = graph[op.p?.[0]];
const operationTypes = Object.keys(op);

if (operationTypes.includes("od") && operationTypes.includes("oi")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If we made this a type guard, we could avoid the casting here if we wanted to -

const isUpdate = (op: OT.Op): op is OT.Object.Replace => {
  const operationTypes = Object.keys(op);
  return operationTypes.includes("od") && operationTypes.includes("oi");
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is super useful to keep in mind & I'll definitley come back to it - will personally feel more confident full type-guarding after more testing of various operation scenarios !

handleUpdate(node, op as OT.Object.Replace);
} else if (operationTypes.includes("oi")) {
handleAdd(node, op as OT.Object.Add);
} else if (operationTypes.includes("od")) {
handleRemove(node, op as OT.Object.Remove);
} else if (operationTypes.includes("li") && operationTypes.includes("ld")) {
handleRootUpdate(op as OT.Array.Replace);
}
});

return output;
jessicamcinchak marked this conversation as resolved.
Show resolved Hide resolved
};
Copy link
Member Author

@jessicamcinchak jessicamcinchak Apr 17, 2024

Choose a reason for hiding this comment

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

Future scope:

  • Live edits are tracked via operations, which this formatter "translates" into something human readable
  • Publishing changes are summarised via a JSON diff of two flow structures, which we summarise as alteredNodes - currently this may not perfectly match up to the summarised operations
  • For more consistency in the future between "edit history" & "publishing changes", our publishing JSON diff could first be mapped to a JSON OT using a package like this https://github.com/kbadk/json0-ot-diff - then both could share this tranlator

Don't think this is necesary just yet as recent changes to publishing are testing well, but just putting it on our radar !

10 changes: 5 additions & 5 deletions editor.planx.uk/src/lib/featureFlags.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// add/edit/remove feature flags in array below
const AVAILABLE_FEATURE_FLAGS = [] as const;
const AVAILABLE_FEATURE_FLAGS = ["UNDO"] as const;

type FeatureFlag = (typeof AVAILABLE_FEATURE_FLAGS)[number];

Expand All @@ -25,7 +25,7 @@ const activeFeatureFlags = (() => {
*/
export const toggleFeatureFlag = (
featureFlag: FeatureFlag,
autoReload = true
autoReload = true,
) => {
const supportedFlag = AVAILABLE_FEATURE_FLAGS.includes(featureFlag);

Expand All @@ -35,13 +35,13 @@ export const toggleFeatureFlag = (
activeFeatureFlags.add(featureFlag);
} else {
throw new Error(
`${featureFlag} is not a supported feature flag, try again. Available flags are: ${AVAILABLE_FEATURE_FLAGS}`
`${featureFlag} is not a supported feature flag, try again. Available flags are: ${AVAILABLE_FEATURE_FLAGS}`,
);
}

localStorage.setItem(
"FEATURE_FLAGS",
JSON.stringify(Array.from(activeFeatureFlags))
JSON.stringify(Array.from(activeFeatureFlags)),
);

if (autoReload) window.location.reload();
Expand Down Expand Up @@ -71,6 +71,6 @@ if (process.env.REACT_APP_ENV !== "test") {
]
.sort()
.join(", ")}`
: `🎏 no active feature flags`
: `🎏 no active feature flags`,
);
}
Loading
Loading