Skip to content

Commit

Permalink
Add snapshot test for atomic saving of notebooks
Browse files Browse the repository at this point in the history
  • Loading branch information
@daibhin
daibhin committed Jul 4, 2023
1 parent 9d7a726 commit 149bfdb
Show file tree
Hide file tree
Showing 7 changed files with 751 additions and 36 deletions.
2 changes: 1 addition & 1 deletion frontend/src/initKea.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export function initKea({ routerHistory, routerLocation, beforePlugins }: InitKe
if (
!ERROR_FILTER_WHITELIST.includes(actionKey) &&
(error?.message === 'Failed to fetch' || // Likely CORS headers errors (i.e. request failing without reaching Django)
(error?.status !== undefined && ![200, 201, 204, 409].includes(error.status)))
(error?.status !== undefined && ![200, 201, 204].includes(error.status)))
) {
lemonToast.error(
`${identifierToHuman(actionKey)} failed: ${
Expand Down
12 changes: 5 additions & 7 deletions frontend/src/scenes/notebooks/Notebook/Notebook.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,15 +170,17 @@ export function Notebook({ shortId, editable = false }: NotebookProps): JSX.Elem
<BindLogic logic={notebookLogic} props={{ shortId }}>
<div className={clsx('Notebook', !isExpanded && 'Notebook--compact')}>
<FloatingSlashCommands />
{!notebook && notebookLoading ? (
{conflictWarningVisible ? (
<NotebookConflictWarning />
) : !notebook && notebookLoading ? (
<div className="space-y-4 px-8 py-4">
<LemonSkeleton className="w-1/2 h-8" />
<LemonSkeleton className="w-1/3 h-4" />
<LemonSkeleton className="h-4" />
<LemonSkeleton className="h-4" />
</div>
) : !notebook ? (
<NotFound object={'notebook'} />
<NotFound object="notebook" />
) : isEmpty && !editable ? (
<div className="NotebookEditor">
<h1>
Expand Down Expand Up @@ -212,11 +214,7 @@ export function Notebook({ shortId, editable = false }: NotebookProps): JSX.Elem
browser. It's a great place to gather ideas before turning into a saved Notebook!
</LemonBanner>
) : null}
{conflictWarningVisible ? (
<NotebookConflictWarning />
) : (
<EditorContent editor={_editor} className="flex flex-col flex-1" />
)}
<EditorContent editor={_editor} className="flex flex-col flex-1" />
</>
)}
</div>
Expand Down
53 changes: 30 additions & 23 deletions frontend/src/scenes/notebooks/Notebook/notebookLogic.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ export const notebookLogic = kea<notebookLogicType>([
false,
{
showConflictWarning: () => true,
loadNotebook: () => false,
loadNotebookSuccess: () => false,
},
],
}),
loaders(({ values, props, actions }) => ({
notebook: [
undefined as NotebookType | undefined,
null as NotebookType | null,
{
loadNotebook: async () => {
// NOTE: This is all hacky and temporary until we have a backend
let response: NotebookType | undefined
let response: NotebookType | null

if (props.shortId === SCRATCHPAD_NOTEBOOK.short_id) {
response = {
Expand All @@ -80,7 +80,8 @@ export const notebookLogic = kea<notebookLogicType>([
version: 0,
}
} else if (props.shortId.startsWith('template-')) {
response = values.notebookTemplates.find((template) => template.short_id === props.shortId)
response =
values.notebookTemplates.find((template) => template.short_id === props.shortId) || null
} else {
response = await api.notebooks.get(props.shortId)
}
Expand All @@ -89,7 +90,10 @@ export const notebookLogic = kea<notebookLogicType>([
throw new Error('Notebook not found')
}

values.editor?.commands.setContent(response.content)
if (!values.notebook) {
// If this is the first load we need to override the content fully
values.editor?.commands.setContent(response.content)
}

return response
},
Expand All @@ -99,28 +103,37 @@ export const notebookLogic = kea<notebookLogicType>([
return values.notebook
}

const response = await api.notebooks.update(values.notebook.short_id, {
version: values.notebook.version,
content: notebook.content,
title: notebook.title,
})
try {
const response = await api.notebooks.update(values.notebook.short_id, {
version: values.notebook.version,
content: notebook.content,
title: notebook.title,
})

// If the object is identical then no edits were made, so we can safely clear the local changes
if (notebook.content === values.localContent) {
actions.clearLocalContent()
}
// If the object is identical then no edits were made, so we can safely clear the local changes
if (notebook.content === values.localContent) {
actions.clearLocalContent()
}

return response
return response
} catch (error: any) {
if (error.code === 'conflict') {
actions.showConflictWarning()
return null
} else {
throw error
}
}
},
},
],

newNotebook: [
undefined as NotebookType | undefined,
null as NotebookType | null,
{
duplicateNotebook: async () => {
if (!values.notebook) {
return
return null
}

// We use the local content if set otherwise the notebook content. That way it supports templates, scratchpad etc.
Expand Down Expand Up @@ -230,12 +243,6 @@ export const notebookLogic = kea<notebookLogicType>([
saveNotebookSuccess: sharedListeners.onNotebookChange,
loadNotebookSuccess: sharedListeners.onNotebookChange,

saveNotebookFailure: ({ errorObject }) => {
if (errorObject.code === 'conflict') {
actions.showConflictWarning()
}
},

exportJSON: () => {
const file = new File(
[JSON.stringify(values.editor?.getJSON())],
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/scenes/notebooks/NotebookScene.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ export const scene: SceneExport = {
export function NotebookScene(): JSX.Element {
const { notebookId, mode } = useValues(notebookSceneLogic)
const { setNotebookMode } = useActions(notebookSceneLogic)
const { notebook, notebookLoading } = useValues(notebookLogic({ shortId: notebookId }))
const { notebook, notebookLoading, conflictWarningVisible } = useValues(notebookLogic({ shortId: notebookId }))
const { exportJSON } = useActions(notebookLogic({ shortId: notebookId }))
const { selectNotebook, setNotebookSideBarShown } = useActions(notebookSidebarLogic)
const { selectedNotebook, notebookSideBarShown } = useValues(notebookSidebarLogic)

if (!notebook && !notebookLoading) {
if (!notebook && !notebookLoading && !conflictWarningVisible) {
return <NotFound object="notebook" />
}

Expand Down
2 changes: 1 addition & 1 deletion posthog/api/notebook.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def update(self, instance: Notebook, validated_data: Dict, **kwargs) -> Notebook
before_update = None

with transaction.atomic():
# Lock the database row so we ensure version updates are atomic
# select_for_update locks the database row so we ensure version updates are atomic
locked_instance = Notebook.objects.select_for_update().get(pk=instance.pk)

if validated_data.keys():
Expand Down
Loading

0 comments on commit 149bfdb

Please sign in to comment.