RFC 95: Concurrent editing notifications #95
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,81 @@ | ||||||
| # RFC 95: Concurrent editing notifications | ||||||
|
|
||||||
| * RFC: 95 | ||||||
| * Author: Matthew Westcott | ||||||
| * Created: 2024-03-13 | ||||||
| * Last Modified: 2024-03-13 | ||||||
|
|
||||||
| ## Abstract | ||||||
|
|
||||||
| 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. | ||||||
|
|
||||||
| ## Specification | ||||||
|
|
||||||
| We introduce a new model `EditingSession` that tracks the users who currently have the 'edit' view for a given object open. This consists of a generic foreign key to the object, a foreign key to the user, and a 'last seen' timestamp. | ||||||
|
|
||||||
| ### 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. | ||||||
|
|
||||||
| When a user visits the 'edit' view for a page or snippet, an EditingSession record is created for that object and user, with the current timestamp, and the EditingSession's ID is passed as a JS-accessible value on the template. | ||||||
|
|
||||||
| 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 There was a problem hiding this comment. Discussed today, not a big concern but certainly worth considering as a requirement for the implementation. |
||||||
|
|
||||||
| 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 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" A dedicated release endpoint also leaves a path open for adding websocket communication in future, if we want to get fancier :-) |
||||||
|
|
||||||
| 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? |
||||||
|
|
||||||
| > [!NOTE] | ||||||
| > This will also identify cases where the current user has the edit view open in multiple tabs. We might consider highlighting this in the list as a special case, and / or removing duplicate users from the list. | ||||||
|
|
||||||
| 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 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. |
||||||
|
|
||||||
| > [!NOTE] | ||||||
| > The `visibilitychange` event does not guarantee that the user has actually left the page - according to the Mozilla docs, it can occur when switching tabs or applications on mobile, for example. However, it seems to be the best option available across all browsers. | ||||||
|
|
||||||
| ### 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 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 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. Are we adding a I suppose it depends on how we want to handle "multiple sessions of the same user" scenario. Making the We're using the revision IDs to handle the concurrent editing notifications, and it doesn't seem to require the 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 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. |
||||||
|
|
||||||
| ### Distinguishing between "currently viewing" and "currently editing" | ||||||
|
|
||||||
| 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.
Suggested change
|
||||||
|
|
||||||
| ### Notifying users on changes | ||||||
|
|
||||||
| We wish to notify users when another user makes changes to the object they are currently editing. This is possible for pages, and snippets inheriting from `RevisionMixin`. | ||||||
|
|
||||||
| The edit view passes the current revision ID as a JS-accessible value on the template. This is passed as a parameter to the 'ping' endpoint, and the 'ping' endpoint response will include a list of revision IDs for the object that are newer than the passed revision ID, along with the user who created the revision and the timestamp. | ||||||
|
|
||||||
| If this list is non-empty, the edit view will display a notification within the 'active users' listing, against the user who made the change, saying "{username} has saved a new version". (This may require adding that user to the list, since it's likely that they have ended their editing session at this point.) The notification will include options to 'dismiss' and 'refresh'. | ||||||
|
|
||||||
| 'Dismiss' will close the notification, and add the revision ID to a list of dismissed revision IDs (stored as a local JS variable for the current view) which will be ignored in any subsequent 'ping' responses. | ||||||
|
|
||||||
| 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. |
||||||
|
|
||||||
| ### Applicability to auto-save functionality | ||||||
|
|
||||||
| It is expected that the approach presented here will remain largely in place when auto-save functionality is implemented. Auto-saving will occur as a background request that can take the place of the 'ping' endpoint whenever changes have been made, with the 'ping' request still being made at 30s intervals when no changes have been made. Auto-save will be initially active when the user opens the edit view, but will be paused if a notification of another user editing the item is received. In the simplest implementation, it will remain paused until the user reloads the edit view (either by selecting 'refresh' on the notification, or explicitly saving or publishing their changes and accepting the confirmation dialog to overwrite the other user's changes), at which point a new editing session will be started with auto-save active. | ||||||
|
|
||||||
| The "has unsaved changes" flag on EditingSession will largely fall out of use at this point, since users will generally be sending 'save' requests rather than 'ping' requests when they have unsaved changes. (The exception is when that user's auto-save state has been paused through the mechanism described above.) A non-editing user who has the 'edit' view open while another user makes an edit will see that user's state transition directly from 'viewing' to the "has saved a new version" notification. If the non-editing user dismisses that notification, the editing user should continue to be shown in the 'editing' state until their editing session ends. | ||||||
|
|
||||||
| ## Open Questions | ||||||
|
|
||||||
| // Include any questions until Status is ‘Accepted’ | ||||||
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.
Would the audit logs (perhaps only those with the
wagtail.editaction) 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_changedflag 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.deleteaction 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.