feat: Handle notebook conflicts #16352

daibhin · 2023-07-03T13:09:18Z

Problem

Notebooks do not support multiplayer editing so instead we are going to render a conflict error if the content has been updated elsewhere since the last save

Changes

Return a custom 409 error
Show a warning and allow the notebook to be reloaded from the server

How did you test this code?

Manually (gif included)

daibhin · 2023-07-03T13:10:33Z

frontend/src/initKea.ts

@@ -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].includes(error.status)))
+                        (error?.status !== undefined && ![200, 201, 204, 409].includes(error.status)))


We seem to show the toast on every request. 409 doesn't seem to be used anywhere else and should be handled gracefully so thought it worth adding as a new special case here

Hmmm I don't know that this wouldn't cause issues elsewhere...

A more localized way would be to simply catch the error from the API call in the logic and check for the error status.

That would mean the little toast appears in the bottom right hand corner which seemed uncessary given the error is handled in the UI

The other option would be to add the saveNotebook action to the ERROR_FILTER_WHITELIST array. That has the effect of not showing the toast for any failed response from the server for that action which felt less good e.g. an actual 500 would not show anything in the UI despite having failed to save

posthog/frontend/src/initKea.ts

Lines 12 to 16 in 0bb4898

/*

Actions for which we don't want to show error alerts,

mostly to avoid user confusion.

*/

const ERROR_FILTER_WHITELIST = [

What we really want is a combination of the saveNotebook and 409 response. It doesn't look like there is any way to inject conditions given it is a global listener 🤔

Actually...

Maybe I can implement something within the logic. It seems to get passed as an argument to the onFailure callback

Please ignore... covered in #16352 (comment)

daibhin · 2023-07-03T13:13:31Z

frontend/src/scenes/notebooks/Notebook/notebookLogic.ts

@@ -82,10 +89,7 @@ export const notebookLogic = kea<notebookLogicType>([
                        throw new Error('Notebook not found')
                    }

-                    if (!values.notebook) {


We only load notebooks when the component mounts so I thought it was ok to remove this check as it was preventing the refreshed content from being populated into the editor

daibhin · 2023-07-03T13:15:17Z

posthog/api/notebook.py

-            instance.last_modified_by = self.context["request"].user
+        with transaction.atomic():
+            # Lock the database row so we ensure version updates are atomic
+            locked_instance = Notebook.objects.select_for_update().get(pk=instance.pk)


I'm not super familiar with Django but it seemed necessary to me that the instance needed to be loaded with select_for_update to lock the table row as part of the transaction. Is there a cleaner way to override the instance lookup within the serializer so maybe I don't need to load it a second time?

I don't know is my answer here :D
A quick google showed me that you can mark an entire view as atomic with a decorator . No idea if that would work here but worth a try? That way the entire call is in a transaction?

Not sure how that plays into what you say about needing to use select_for_update though...

Checked it out and the decorator didn't break anything but I also couldn't see an easy way of testing and handling the error....

I like that Notebook.objects.select_for_update() is super explicit though. makes me happy we're not having to rely on some dark magic

benjackwhite · 2023-07-03T13:17:16Z

frontend/src/initKea.ts

@@ -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].includes(error.status)))
+                        (error?.status !== undefined && ![200, 201, 204, 409].includes(error.status)))


Hmmm I don't know that this wouldn't cause issues elsewhere...

A more localized way would be to simply catch the error from the API call in the logic and check for the error status.

benjackwhite · 2023-07-03T13:21:04Z

frontend/src/scenes/notebooks/Notebook/notebookLogic.ts

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


I think we should instead do this in the saveNotebook call. Partly because there we can better control the error state, as well as reset the notebook to a null value or something which would stop it from flashing the old content.

Locally this loads really fast but in prod it will definitely be slower so the old version will likely still be visible for a second or so which might be confusing...

Ahh gotcha. Please ignore my comment above. This seems like a better approach

benjackwhite · 2023-07-03T13:24:31Z

posthog/api/notebook.py

-            instance.last_modified_by = self.context["request"].user
+        with transaction.atomic():
+            # Lock the database row so we ensure version updates are atomic
+            locked_instance = Notebook.objects.select_for_update().get(pk=instance.pk)


I don't know is my answer here :D
A quick google showed me that you can mark an entire view as atomic with a decorator . No idea if that would work here but worth a try? That way the entire call is in a transaction?

Not sure how that plays into what you say about needing to use select_for_update though...

benjackwhite · 2023-07-03T13:26:32Z

posthog/exceptions.py

@@ -33,6 +33,11 @@ def __init__(self, feature: Optional[str] = None) -> None:
        )


+class Conflict(APIException):


Crazy that there is no built in for this in Django 😅

May be because we're a major version behind 🙈

posthog-bot · 2023-07-03T13:31:14Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2023-07-03T17:08:02Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

pauldambra · 2023-07-03T19:49:12Z

posthog/api/test/notebooks/__snapshots__/test_notebook.ambr

+  SELECT "posthog_notebook"."id",
+         "posthog_notebook"."short_id",
+         "posthog_notebook"."team_id",
+         "posthog_notebook"."title",
+         "posthog_notebook"."content",
+         "posthog_notebook"."deleted",
+         "posthog_notebook"."version",
+         "posthog_notebook"."created_at",
+         "posthog_notebook"."created_by_id",
+         "posthog_notebook"."last_modified_at",
+         "posthog_notebook"."last_modified_by_id"
+  FROM "posthog_notebook"
+  WHERE "posthog_notebook"."id" = '01891c84-73f6-0000-8ea4-5d223f446585'::uuid
+  LIMIT 21
+  FOR
+  UPDATE /*controller='project_notebooks-detail',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/notebooks/%28%3FP%3Cshort_id%3E%5B%5E/.%5D%2B%29/%3F%24'*/


cool the snapshot captured the fir update, yay

So, if someone breaks it in future, this will highlight it to them

pauldambra

looks good to me, tried locally and it does what it says on the tin

Would be cool to move past this and on to conflict resolution but that's for another day 🙈

daibhin commented Jul 3, 2023

View reviewed changes

daibhin force-pushed the dn-feat/notebook-conflicts branch from c9d0506 to 9055be3 Compare July 3, 2023 13:12

daibhin commented Jul 3, 2023

View reviewed changes

daibhin requested a review from a team July 3, 2023 13:15

daibhin changed the title ~~Handle notebook conflicts [https://github.com/PostHog/posthog/issues/15680]~~ feat: Handle notebook conflicts Jul 3, 2023

benjackwhite reviewed Jul 3, 2023

View reviewed changes

daibhin force-pushed the dn-feat/notebook-conflicts branch from f6674e2 to d55888a Compare July 3, 2023 16:52

pauldambra reviewed Jul 3, 2023

View reviewed changes

pauldambra approved these changes Jul 4, 2023

View reviewed changes

daibhin added 3 commits July 4, 2023 16:22

Display conflict errors to a user

d38e754

Add atomic notebook update

9d7a726

Add snapshot test for atomic saving of notebooks

149bfdb

daibhin force-pushed the dn-feat/notebook-conflicts branch from ecd8c2d to 149bfdb Compare July 4, 2023 15:22

daibhin merged commit fcf09b5 into master Jul 5, 2023

daibhin deleted the dn-feat/notebook-conflicts branch July 5, 2023 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Handle notebook conflicts #16352

feat: Handle notebook conflicts #16352

daibhin commented Jul 3, 2023 •

edited

Loading

daibhin Jul 3, 2023

benjackwhite Jul 3, 2023

daibhin Jul 3, 2023

daibhin Jul 3, 2023

daibhin Jul 3, 2023

daibhin Jul 3, 2023

daibhin Jul 3, 2023

benjackwhite Jul 3, 2023

benjackwhite Jul 3, 2023

pauldambra Jul 3, 2023

benjackwhite Jul 3, 2023

benjackwhite Jul 3, 2023

daibhin Jul 3, 2023

benjackwhite Jul 3, 2023

benjackwhite Jul 3, 2023

pauldambra Jul 3, 2023

posthog-bot commented Jul 3, 2023

posthog-bot commented Jul 3, 2023

pauldambra Jul 3, 2023

pauldambra left a comment

	/*
	Actions for which we don't want to show error alerts,
	mostly to avoid user confusion.
	*/
	const ERROR_FILTER_WHITELIST = [

		@@ -33,6 +33,11 @@ def __init__(self, feature: Optional[str] = None) -> None:
		)


		class Conflict(APIException):

feat: Handle notebook conflicts #16352

feat: Handle notebook conflicts #16352

Conversation

daibhin commented Jul 3, 2023 • edited Loading

Problem

Changes

How did you test this code?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

posthog-bot commented Jul 3, 2023

📸 UI snapshots have been updated

posthog-bot commented Jul 3, 2023

📸 UI snapshots have been updated

Choose a reason for hiding this comment

pauldambra left a comment

Choose a reason for hiding this comment

daibhin commented Jul 3, 2023 •

edited

Loading