-
Notifications
You must be signed in to change notification settings - Fork 2
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
Changes from all commits
2c0cccf
efdd614
aa51801
0a4338f
2dd3841
d2dc1a0
a0ab1e3
9962a6d
9635f25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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?"', | ||
]); | ||
}); | ||
|
||
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, | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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");
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Future scope:
Don't think this is necesary just yet as recent changes to publishing are testing well, but just putting it on our radar ! |
There was a problem hiding this comment.
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
ortitle
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 !
There was a problem hiding this comment.
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.