RFC 95: Concurrent editing notifications#95
Conversation
laymonage
left a comment
laymonage
left a comment
There was a problem hiding this comment.
Thanks! I think I'm quite happy with the general approach here (an EditingSession model, a client-side behaviour that pings the server, etc.). I have a few questions, but they're somewhat closer to implementation details than the general concept.
| We add a 'ping' endpoint view that receives an EditingSession ID as a parameter, validates that the EditingSession belongs to the current user, and updates the 'last seen' timestamp to the current time. It returns a JSON response containing a list of all EditingSessions other than the current one that match this object and have a timestamp within the last minute. | ||
|
|
||
| > [!NOTE] | ||
| > It may also be necessary to perform a permission check to verify that the current user has edit permission on the object, so that users cannot use the 'ping' endpoint as a means to track user activity outside of their own area of the site. (This cannot happen if going through the 'edit' view is the only way to acquire an EditingSession ID, but see the "Handling unknown EditingSession IDs" section below for a setup where this could be bypassed.) |
There was a problem hiding this comment.
Yet another reason why we need something like #92 so we can check the permissions for non-page models without relying on the viewset's permission_policy 😄
There was a problem hiding this comment.
Discussed today, not a big concern but certainly worth considering as a requirement for the implementation.
|
|
||
| The list of active users will also be included in the initial rendered response of the edit view, so that we don't have to wait for the initial 'ping' response for this information to be displayed. | ||
|
|
||
| When a user leaves the edit view (as determined by a [`visibilitychange` event](https://developer.mozilla.org/en-US/docs/Web/API/Document/visibilitychange_event) changing visibility to 'hidden'), the 'release' endpoint is called (using [`sendBeacon`](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon)) to delete the EditingSession. |
There was a problem hiding this comment.
Should we also consider "idle" sessions? When a user switches to a different tab, it is not uncommon for them to return to the editing tab not long after. Do we want to immediately release the session, or mark it as inactive? Or, maybe just leave it as-is, and if the timestamp is e.g. 30 seconds ago < timestamp < 60 seconds ago), display it as idle.
If so, we might need to use pagehide event (or beforeunload, but that seems less ideal) to call the 'release' endpoint.
There was a problem hiding this comment.
Proposed design variation for Wagtail: desaturated user avatar.
TBC whether we do this based on feasibility. We can update the RFC to leave some room for this, it doesn’t feel crucial for the RFC’s design decisions to reflect those requirements.
|
|
||
| ### Handling unknown EditingSession IDs | ||
|
|
||
| Given the above cleanup process, and the potential for a `visibilitychange` event to occur without the user actually leaving the page, it's possible that a browser could resume sending 'ping' requests for a given EditingSession ID some time after the record has been deleted. In this situation, we would want the user to reappear in the list of active sessions. To handle this, requests to the 'ping' endpoint should pass the object's ID and content type in addition to the EditingSession ID, and in the case that the EditingSession is not found, the view will create a new one. The ID of the EditingSession (whether retrieved or newly created) will also be included in the response, so that subsequent 'ping' requests can be made with the new ID. |
There was a problem hiding this comment.
Are we adding a UniqueConstraint on content_type, object_id, and user? If so, maybe we could use the content_type and object_id as the identifier when interacting with the API? I'm not sure if there would be a need to use the EditingSession's ID directly if this were the case.
I suppose it depends on how we want to handle "multiple sessions of the same user" scenario. Making the EditingSession unique on the generic FK and the user would prevent us from differentiating the sessions, but I don't know if we need that. It doesn't make much sense displaying the same user multiple times in the header (and if we make the query distinct – perhaps we should make it unique in the first place?).
We're using the revision IDs to handle the concurrent editing notifications, and it doesn't seem to require the EditingSession directly (other than for piggybacking the requests to update the client JS state). If a user saves a new revision in a different tab, the current spec doesn't seem to have any issues displaying it when the user clicks save. Unless I'm missing something...
There was a problem hiding this comment.
It feels to me that the editing session is an instance of editing the object, not an instance of a user editing an object, at least on the backend side of things. I think that's what you want to be modelling. In the UI, you can still display it as a single user, even if one user has multiple editing sessions. It would make sense to generate a separate identifier (e.g. UUID) for each editing session and identify it that way (with filtering by the user ID from the session as well).
I don't see a reason for abstracting editing sessions in this way at the database level. I think the database should store each instance of an editing session (one user may multiple tabs where each tab is an editing session). If the UI or public API needs a simplified version, it can be done separately in the querying code.
I think the implementation should be as simple as possible. An example of unnecessary complexity you are introducing is, if there's one session only and you use the update_or_create() call, is there a possibility that one session calls the release endpoint when you close the tab and that deletes the session, and the user disappears for a short period and is no longer "currently editing", even if they are still editing.
I can't see what outcome enforcing one editing session per user in the database would bring. I can only see negatives such as: introducing unnecessary complexity in the code and making the database work harder for zero gain, plus introducing complexity usually means you multiply number of surprises you will come by when you implement this.
Also, an example reason why you might want to display multiple sessions on the UI is for users to realise that they have the same object opened in multiple tabs.
There was a problem hiding this comment.
Good point, I was also slightly concerned about the additional complexity that the unique constraint would bring. OK then, I think we can proceed without making it unique per user+object. I also thought about adding a UUID as well, although I'm not sure what it would be useful for (other than preventing enumeration).
There was a problem hiding this comment.
Discussed today, this feels important to consider during implementation, but not worth confirming 100% in the RFC.
|
|
||
| ### Base implementation - tracking active sessions | ||
|
|
||
| A minimal implementation would simply show a list of other users with an active editing session on the same object, without any notification of when those users actually make changes. This would be the only functionality available for non-versioned snippets, since we need a revision ID to identify whether the last saved version is the one that the current user is editing. |
There was a problem hiding this comment.
Would the audit logs (perhaps only those with the wagtail.edit action) be a reasonable substitute instead of the revisions? It would allow us to have the feature for non-versioned snippets. Maybe use revisions if supported, and fall back to the audit logs, or maybe just use the latter to simplify the implementation.
We might also be able to use the BaseLogEntry.content_changed flag to only show the warning if the other user's "save" action actually changed the content. If we use revisions, we'll always have to display the warning, even if the other user might have only clicked "save" without changing anything. Not sure how reliable the flag is, though.
Also, we could detect if there's a wagtail.delete action and display some information that the object was deleted by which user. If we use revisions, we can still do this by making the ping API's response indicate that the object is no longer available (and optionally query the audit log to include the information of which user deleted it). Speaking of which, I don't think we've discussed the deletion case when we initially designed the feature...
There was a problem hiding this comment.
For now we’ll focus on models that are revisioned. It’s a good idea to use the audit log for this but doesn’t feel worth the effort.
|
|
||
| ### Clean-up of outdated sessions | ||
|
|
||
| Since we cannot guarantee that the 'release' endpoint will be called in all cases (for example, if the user's browser crashes), it is possible that stale EditingSession records will be left in the database. These will not affect functionality (since we disregard sessions older than a minute), but over time they may impair performance and bloat the database. These could be cleaned up with a scheduled task, but the extra work for site owners to set this up is probably not worth the benefit. I therefore propose that we perform this clean-up in the edit view, at the point where an EditingSession record is created. This will delete all EditingSession records that are more than one hour old. |
There was a problem hiding this comment.
This might be more of an implementation detail – but if we're using generic FK to the object, we probably will need to provide a model mixin with a default GenericRelation and tell the developers to add it to their model so that the EditingSession gets deleted when the model instance is deleted. Or perhaps, tell developers to add the GenericRelation themselves.
Or, if that sounds like a hassle when you have a lot of models, maybe we need to introduce a management command to be run periodically after all...
Maybe we can get away with doing the deletion in the generic DeleteView instead, but this won't cover cases where the deletion happens programmatically.
There was a problem hiding this comment.
Discussion: let’s do this on creation of new records. Later, switch to background tasks APIs (RFC #72).
There was a problem hiding this comment.
Proposed additions to open questions based on today’s discussion:
## Do we want support models that aren’t revisioned?
No
## Do we want to implement “idle session” logic?
Maybe. TBC during implementation
## How do we tackle multiple editing sessions by the same user?
No special treatment for this (TBC further down the line).
## How do we handle the page having been deleted?
Error handling in the API endpoints + UI|
|
||
| ### Base implementation - tracking active sessions | ||
|
|
||
| A minimal implementation would simply show a list of other users with an active editing session on the same object, without any notification of when those users actually make changes. This would be the only functionality available for non-versioned snippets, since we need a revision ID to identify whether the last saved version is the one that the current user is editing. |
There was a problem hiding this comment.
For now we’ll focus on models that are revisioned. It’s a good idea to use the audit log for this but doesn’t feel worth the effort.
| We add a 'ping' endpoint view that receives an EditingSession ID as a parameter, validates that the EditingSession belongs to the current user, and updates the 'last seen' timestamp to the current time. It returns a JSON response containing a list of all EditingSessions other than the current one that match this object and have a timestamp within the last minute. | ||
|
|
||
| > [!NOTE] | ||
| > It may also be necessary to perform a permission check to verify that the current user has edit permission on the object, so that users cannot use the 'ping' endpoint as a means to track user activity outside of their own area of the site. (This cannot happen if going through the 'edit' view is the only way to acquire an EditingSession ID, but see the "Handling unknown EditingSession IDs" section below for a setup where this could be bypassed.) |
There was a problem hiding this comment.
Discussed today, not a big concern but certainly worth considering as a requirement for the implementation.
|
|
||
| The list of active users will also be included in the initial rendered response of the edit view, so that we don't have to wait for the initial 'ping' response for this information to be displayed. | ||
|
|
||
| When a user leaves the edit view (as determined by a [`visibilitychange` event](https://developer.mozilla.org/en-US/docs/Web/API/Document/visibilitychange_event) changing visibility to 'hidden'), the 'release' endpoint is called (using [`sendBeacon`](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon)) to delete the EditingSession. |
There was a problem hiding this comment.
Proposed design variation for Wagtail: desaturated user avatar.
TBC whether we do this based on feasibility. We can update the RFC to leave some room for this, it doesn’t feel crucial for the RFC’s design decisions to reflect those requirements.
|
|
||
| ### Clean-up of outdated sessions | ||
|
|
||
| Since we cannot guarantee that the 'release' endpoint will be called in all cases (for example, if the user's browser crashes), it is possible that stale EditingSession records will be left in the database. These will not affect functionality (since we disregard sessions older than a minute), but over time they may impair performance and bloat the database. These could be cleaned up with a scheduled task, but the extra work for site owners to set this up is probably not worth the benefit. I therefore propose that we perform this clean-up in the edit view, at the point where an EditingSession record is created. This will delete all EditingSession records that are more than one hour old. |
There was a problem hiding this comment.
Discussion: let’s do this on creation of new records. Later, switch to background tasks APIs (RFC #72).
|
|
||
| ### Handling unknown EditingSession IDs | ||
|
|
||
| Given the above cleanup process, and the potential for a `visibilitychange` event to occur without the user actually leaving the page, it's possible that a browser could resume sending 'ping' requests for a given EditingSession ID some time after the record has been deleted. In this situation, we would want the user to reappear in the list of active sessions. To handle this, requests to the 'ping' endpoint should pass the object's ID and content type in addition to the EditingSession ID, and in the case that the EditingSession is not found, the view will create a new one. The ID of the EditingSession (whether retrieved or newly created) will also be included in the response, so that subsequent 'ping' requests can be made with the new ID. |
There was a problem hiding this comment.
Discussed today, this feels important to consider during implementation, but not worth confirming 100% in the RFC.
|
|
||
| The above implementation will show all users who have the 'edit' view open for a given object, regardless of whether they have made any changes. It would be helpful to distinguish between users who are currently editing the object and those who are simply viewing it. | ||
|
|
||
| To do this, we add a 'has unsaved changes' boolean field to the EditingSession model. When the edit view sends a 'ping' request, it will use the existing 'dirty fields' mechanism to determine whether the current user has unsaved changes, and pass this flag as a parameter to the 'ping' endpoint. The 'ping' endpoint will then update this field along with the timestamp, and the list of existing sessions it returns will also include this flag. The edit view can then apply a visual distinction to those users in the list. |
There was a problem hiding this comment.
| To do this, we add a 'has unsaved changes' boolean field to the EditingSession model. When the edit view sends a 'ping' request, it will use the existing 'dirty fields' mechanism to determine whether the current user has unsaved changes, and pass this flag as a parameter to the 'ping' endpoint. The 'ping' endpoint will then update this field along with the timestamp, and the list of existing sessions it returns will also include this flag. The edit view can then apply a visual distinction to those users in the list. | |
| To do this, we add a 'has unsaved changes' boolean field to the EditingSession model. When the edit view sends a 'ping' request, it will use the existing 'dirty fields' mechanism (accepting its [known shortcomings](https://github.com/wagtail/wagtail/issues/7188)) to determine whether the current user has unsaved changes, and pass this flag as a parameter to the 'ping' endpoint. The 'ping' endpoint will then update this field along with the timestamp, and the list of existing sessions it returns will also include this flag. The edit view can then apply a visual distinction to those users in the list. |
|
|
||
| We also add a 'release' endpoint view that receives an EditingSession ID as a parameter, validates that the EditingSession belongs to the current user, and deletes the EditingSession. | ||
|
|
||
| A JavaScript handler is added to the edit view that sends a ping request to the 'ping' endpoint every 30 seconds, and updates the list of active users based on the response. |
There was a problem hiding this comment.
Do we want to make this duration configurable?
There was a problem hiding this comment.
This looks solid to me. There are a few points that need further thinking but they feel more like implementation details / things we can refine later, without the base design being affected too much.
Making this as Final Comment Period as other feedback so far has been very positive.
| > [!NOTE] | ||
| > It may also be necessary to perform a permission check to verify that the current user has edit permission on the object, so that users cannot use the 'ping' endpoint as a means to track user activity outside of their own area of the site. (This cannot happen if going through the 'edit' view is the only way to acquire an EditingSession ID, but see the "Handling unknown EditingSession IDs" section below for a setup where this could be bypassed.) | ||
|
|
||
| We also add a 'release' endpoint view that receives an EditingSession ID as a parameter, validates that the EditingSession belongs to the current user, and deletes the EditingSession. |
There was a problem hiding this comment.
If there's a ping every 30 seconds then do we really need a release endpoint too? Expired editing sessions will be cleared up soon enough anyway. And I guess when a document becomes hidden (or idle) then setInterval/Timeout calls for the ping will need to be suspended explicitly in javascript regardless.
There was a problem hiding this comment.
We’ve discussed indicating it differently if a session is "idle", "active", or simply over, so this feels useful to me.
There was a problem hiding this comment.
I don't know what events will be used for this (a few were mentioned already). Regardless, if there's a periodic ping I don't see why a separate release is necessary. If we're relying only on pings then switching between tabs quickly won't immediately trigger release and activate events right away, only if up to 30 seconds passed. That's not just simpler but more practical behaviour I think. Btw, on desktop switching between apps doesn't trigger a visibilitychange event, so it's not like we can be precise about this anyway.
But this isn't really an issue, more of an implementation detail. Just thought it was weird.
There was a problem hiding this comment.
30 seconds is quite a long time when it comes to "realtime" feedback, and it's liable to mount up - we've specified 60 seconds of not receiving pings as the threshold where they're assumed to have left the page (to allow for minor network outages and lag), and it might be another 30 seconds before the person at the other end makes another ping and sees the updated information. Having the 'release' endpoint doesn't add much more complexity, and I can see scenarios where the extra perceived responsiveness makes it worthwhile, such as when the two users are in the same office...
"Hey, it says you're editing the homepage"
"Oh yeah, let me close that - OK, it's all yours"
"Hmm, I refreshed the page, and it's still saying you have it open"
A dedicated release endpoint also leaves a path open for adding websocket communication in future, if we want to get fancier :-)
|
|
||
| If the current user has not made any changes, the 'refresh' option will immediately reload the page. If the current user has made changes, the 'refresh' option will display a confirmation dialog, warning the user that they will lose their changes if they proceed. If they confirm, the page will be reloaded. | ||
|
|
||
| When the user performs any save or publish action, if a 'ping' response has returned any revisions at any point (including ones that were dismissed), the user will be shown a confirmation dialog warning them that they will be overwriting existing changes made by other users. If they proceed, the save or publish action will be performed as normal. |
There was a problem hiding this comment.
There will be still a chance that another person's revision is overridden in the 30 second window. E.g. people start updating the same intranet page after a meeting. So I'd still expect users to be told about that after they've successfully saved and skipped/overridden another revision, not just when a new revision is detected beforehand. (I suspect users would have complained about this more had they known they wouldn't be told about it when it happens)
There was a problem hiding this comment.
That’d be nice. Not sure it’s worth the trouble.
There was a problem hiding this comment.
These concurrent notifications are great but I think it's actually more important to have confidence about knowing that I didn't cause any data loss by overwriting other people's edits.
There was a problem hiding this comment.
If we're trying to cater for those kinds of rapid-fire editing scenarios, I think there's a point where nothing less than a Google Docs style collaborative editor will be acceptable. I don't know that point is 30 seconds, 10 seconds, or lower... (FWIW, based on this feedback I'm inclined to reduce the ping interval to 10 seconds.)
Having said that, it's probably not a huge leap to handle this more rigorously: the 'save' endpoint would be passed the last known revision ID just as the 'ping' endpoint does, so that it can identify when an intermediate save has happened. If so - and an explicit "force overwrite" flag is not passed with the submission - this would be handled equivalently to a validation error, re-rendering the form without saving. At that point they'd be shown the same confirmation dialog as when a conflicting edit is detected through the 'ping' mechanism - and the "yes, proceed" option in that confirmation dialog would pass the "force overwrite" flag.
laymonage
left a comment
laymonage
left a comment
There was a problem hiding this comment.
I think Balazs raised very good points here, and I'm happy with the responses to the questions (I'd assume they also do, based on the 👍 reaction). I think the RFC as-is provides enough detail to start the implementation while still having enough room to experiment and adapt the details as we encounter any challenges. Thanks all!
|
🚢🚢🚢🚢 |
5 participants
Rendered view
Currently, if two users edit the same page or snippet at the same time, the second user to save their changes will overwrite the first user's changes. This doesn't seem to be a common problem, and can often be avoided through communication - however, it will need to be addressed in advance of implementing auto-saving, as this increases the likelihood of users overwriting each other's changes unknowingly.
The "holy grail" of avoiding edit conflicts would be a real-time collaborative editing feature where changes are reflected immediately on the other user's screen, but this will not happen any time soon. A locking mechanism has been proposed, but this raises further implementation questions (for example, who has authority to override a lock in the case that an editor leaves a page open and goes on holiday?) and would be overly disruptive.
This RFC proposes an approach where users are notified of any other users editing the same item. Users will not be blocked from editing, but will be provided with information about the potential for conflicts, and how to resolve them when they do occur.