Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RFC 99: Preliminary work to support auto-save functionality #99

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RFC 99: Preliminary work to support auto-save functionality #99

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions text/099-preliminary-work-for-autosave.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# RFC 099: Preliminary work to support auto-save functionality

* RFC: 099
* Author: Matthew Westcott
* Created: 2024-06-20
* Last Modified: 2024-06-20

## Abstract

A much-requested feature for Wagtail is the ability to automatically save changes in the background while editing a page or snippet. Achieving this to an acceptable standard of user experience will involve completing various milestones that are best handled as separate development efforts. This RFC outlines various items of work that are pre-requisites for implementing auto-save functionality.

## Concurrent editing notifications

When multiple users have the editing interface for a page or snippet open at the same time, the last user to hit save will generally overwrite any changes made by the other users. This problem is compounded when auto-saving is active, since this overwriting will happen without the user's explicit action. To address this, we will implement a notification system that warns users when another user is editing the same item, and disables auto-saving when potentially conflicting changes are detected. This feature is covered by RFC 95.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When multiple users have the editing interface for a page or snippet open at the same time, the last user to hit save will generally overwrite any changes made by the other users. This problem is compounded when auto-saving is active, since this overwriting will happen without the user's explicit action. To address this, we will implement a notification system that warns users when another user is editing the same item, and disables auto-saving when potentially conflicting changes are detected. This feature is covered by RFC 95.
When multiple users have the editing interface for a page or snippet open at the same time, the last user to hit save will generally overwrite any changes made by the other users. This problem is compounded when auto-saving is active, since this overwriting will happen without the user's explicit action. To address this, we implemented a notification system that warns users when another user is editing the same item. This feature is covered by RFC 95.
This notification system will need to be extended to disable auto-saving when potentially conflicting changes are detected.

Rewording to make it clear what was covered in RFC 95. I’m concerned about disabling auto-saving, but need to think more about its alternatives.


## Deferring validation until publish

Currently, a revision can only be saved if it passes validation. This is undesirable because a user may spend an extended amount of time editing a page while it is in a state that would not pass validation - for example, a blog post might have a required 'category' field at the bottom of the form, and a user leaves this unset while they are writing the body of the post. In this situation, auto-save will not be able to save the user's work in progress.

To address this, we will relax the validation behaviour so that as standard, validation rules such as `blank=False` are not enforced when saving drafts. Instead, full validation will be enforced at the point that a page is published, scheduled, or submitted to a workflow.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m worried that we’ll break developers’ expectations by making the nuance between DB-level and ORM-level validation more apparent without it being something that devs knowingly opt into. Could we consider alternatives? Perhaps adding an explicit way to distinguish between "valid" and "invalid" drafts?


Since we need to create an initial database record before we can save revisions, any data that is invalid at the database level (such as a nonsensical date in a date field, or non-numeric data in an integer field) will still produce an immediate validation error. Developers will also be provided with a way to override this behaviour and enforce immediate validation for specific fields where appropriate - this could perhaps be done via an argument on `FieldPanel`. Wagtail itself will make use of this mechanism to require the title and slug fields to be filled in before a page can be saved.

(Implementation note: Django does not enforce validation rules in a model's `save` method, so we are not inherently prevented from saving data that breaks those rules. Rather, it happens in the `full_clean` method, which is part of the `Page` model's `save` logic by default but can be skipped. We will also need to prevent validation from being applied at the form level; this could possibly be done by stripping flags such as `required=True` from the derived form fields.)

Deferring validation in this way will also improve the user experience of the live preview panel, which currently pauses updated while the page is in an invalid state. This does however mean that template authors will need to code more "defensively" to ensure that their templates are preview-friendly, since they will potentially be passed invalid or blank data that would ordinarily be prevented by the validation rules.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re my above comment about developers’ expectations, I’d like to see alternatives explored, and going into more depth on the types / commonness of preview errors that this change would cause. From personal experience, it seems pretty common already that devs implement functionality without being aware of compatibility issues with previews.


## Dynamic insertion of validation error messages

Even with the above change, there will still be situations where a validation error is triggered on saving a draft, such as a blank title or a slug that is already in use. For a conventional form submission, the entire form is re-rendered with error messages inserted next to the relevant fields. When auto-saving is introduced, we will need to communicate these errors to the user without performing a page reload. The placement of these errors varies according to the panel and field; for example, errors within a StreamField are attached to the block that caused the error, and `TabbedInterface` additionally shows the count of errors as a badge on the tab. Errors also need to be indicated on the minimap.

Currently, the [Telepath](https://wagtail.github.io/telepath/) library is used within StreamField to provide a client-side API for block objects that are defined in Python and would conventionally only support server-side rendering, and the same approach can be used across the whole form. We will implement Telepath adapters for panel objects, which will provide a method to obtain a client-side panel object when given a DOM fragment. This object will then provide a method for inserting error messages into the panel.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’d be nice to also cover the criteria when those error messages would be displayed – assuming we’d fields / Telepath to keep track of whether fields are "touched" to decide whether to display the error messages or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be any way for us to instead treat autosaves as a different kind of revision?

Revisions, because they kind of act as a proxy, could be treated differently to actual model saving. Bypassing some validation for the sake of convenience.

From here, we would need to solve the UX problem of 'your work is saved but not fully saved'.

Confluence does a good job of this, it's pretty intuitive, treating saves similar to what we treat publish as. When you see the page listed it says 'unpublished changes'. You edit and then click save or publish to apply your 'work in progress '.

So our autosave is less about saving an actual version but allowing for a work in progress, per user even, that allows you to come back later and actually save your work.

Confluence won't trigger any workflows or other 'proper' validation until you've actually saved things. Confluence also won't show your 'temporary' work in the proper revision listing.

I really like it's approach, it let's me work on something but not share with my team until I'm ready.

I also think the same 'temporary' revision mechanism is used for their pseudo-concurrent editing.

Just an idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I put a similar question/idea on the newer RFC https://github.com/wagtail/rfcs/pull/104/files#r1865306971


## Further applications for Telepath panel adapters (optional)

Having Telepath adapters for panels would open up the possibility of further enhancements which, while not strictly necessary for auto-save, may improve the consistency of the codebase.

Firstly, the client-side panel object could provide a method for retrieving the current value of any fields within that panel. Calling this on the top-level panel would return a dictionary of all field values, which could then be submitted to a JSON API endpoint to save the draft. This would avoid the need to construct a Django Form object to process the submission, which may provide a way to bypass form-level validation.

Secondly, the adapter could provide support for dynamically inserting panels, much as StreamField does; this would allow sharing more of the implementation between StreamField and InlinePanel, and provide functionality such as duplication and inserting at arbitrary positions, which is currently missing from InlinePanel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In light of other discussions at the core team meeting, just flagging that this proposed approach will have larger impacts on our planned Stimulus migration path. With the underlying impacts to removal of inline scripts and jQuery.

I really think that we should ensure we've fully validated a DOM / FormData based approach before going down this path.

Having Telepath baked in, for a core concern potentially, for any other field and custom admin DOM implementation is a risk we need to think about. We'll also need to deal with JS data structures that may be at odds with the DOM/FormData.

Finally, in terms of error patching into the DOM. I really think if we POSTed a submit, changes the request header to accept JSON, our save method could return a errors structure mostly out of the box using Django clean methods. From here we have an object of named (field names associated in our DOM) errors, as strings or html. This can be patched into the DOM using setCustomValidity on the field, via its name.

Then we can use the existing system of JS form /field constraint validation for as much enhanced UI around message presentation as needed.

https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation#the_constraint_validation_api

This also means the 'ownership' of what to do when there's a field error can be split out to a different controller. Or at the PanelController level.

It's worth a POC if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gasman @thibaudcolas @laymonage - here's a very basic POC branch that builds our existing Stimulus approaches to bake in a simple (ignorant) auto-save behaviour.

lb-/wagtail@54a4c19

Observe the error from 'async save' based on the JSON response, with updated tab/minimap counts
Screenshot 2024-10-31 at 3 42 24 PM

POC implementation

  1. UnsavedController -> when it's determined that there are changes in the client that are different, do an async POST with the form's FormData. Add accepts JSON as a header to this request.
  2. In our Page save POST handler, process this roughly as a normal save, except when we get the accepts JSON, in which case we return the errors as JSON and also flush the session messages.
  3. Back in UnsavedController, we map through fields and mark any matching fields as invalid, this is 100% a browser DOM API. setCustomValidity & reportValidity
  4. Using our CloneController we listen for the browser invalid event which gets fired when custom validity is called.
  5. Our CloneController captures the event (important, we use capture:true as invalid does not bubble by default, we also use preventDefault to NOT show the browser native popover as we have our own, better & more accessible approach). And creates a new cloned message from the data on the field, which is available at input.validationMessage.
  6. We make sure that the Tabs error count gets updated by listening to any message adding/clearing - supported out of the box today with the CountController and event listeners.
  7. We make sure the minimap gets updated by listening to the same events.

What we get

  1. When the user makes ANY changes to the form, after the initial check delay, if the values in the form are different to the initial load, we will save the changes to the Page model.
  2. If the save is successful, great. Otherwise we do not save it, but we do update fields with any messaging.
  3. When errors come into the DOM, the minimap/tabs count also get updated.
  4. Rather than reinventing 'code that checks for edits across all the possible universe of form changes', we just build on the existing approach in UnsavedController
  5. Almost no major new JavaScript required, building on existing controllers that do smaller dedicated jobs (a. do stuff when there are unsaved changes, b. put a message element into the DOM when there's a specific event, c. update error counts when there is a specific event fired).

What's not covered

  • A lot obviously, this would not be production ready, however...
  • We would probably want to update the unsaved messages / sync up somehow
  • We probably need to consider if we actually want the errors to show and the best practice here
  • JS input elements only support a single string, no HTML or data structures for errors, we could be fine with this and it's probably a safer way to go, but we would need to do some work to get HTML error messaging working this way if we wanted. A better approach would be to clean the messages on the server and just send down simpler messages. Leave the 'HTML' errors for actual saves maybe.
  • Robust code with tests.
  • Nuances of FormData submission, e.g. we may NOT want to submit comments or other values like next to the server when async auto-saving, we may NOT want to send files either. This comes down to UX and expectations of the autosave functionality.
  • We really should simplify how our different aspects of code 'look' for errors on the screen, but that's probably not critical for auto-save.
  • Proper handling of the server-side aspect of auto-saving, a lot depends on the implementation goals but at the moment this POC will trigger all the workflow, hooks and everything else as if it's a full save.
  • Nuances of radio/checkboxes with name being used across multiple inputs. In fact there's probably a bug right now in our UnsavedController for this, I will raise when I get the chance.
  • Error messages that need to show 'within' inputs (e.g. mapped to parts within rich text or StreamField), due to these not really being discrete inputs. I understand this is a huge chunk of work but it's possible to build on the above to add this, even the StreamField controller could listen to the invalid event, stop propagation and do it's own parsing of the validityMessage or similar.

It's worth calling out that the more we build on our own constructs for message, DOM representation and other things, the more it forces us away from building on the browser and it's capabilities.

Sometimes it's a bit of a fight to get the most out of the browser, but we do get so much 'for free' and we also get a pretty functional out of the box behaviour from 'just a Django widget' that I think is a worthy goal when it comes to developers using and building on Wagtail.

I like to think that we should give the power of the complexity/functionality of StreamField when we/devs need it, but without having to force all our HTML/JS to follow that paradigm up the whole DOM when it's probably not required. We want our JS/DOM to 'wear the right hat' if possible.

Rant over, I hope this is helpful and is a good discussion starter.

## Open Questions
Copy link
Member

@thibaudcolas thibaudcolas Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t have to be open questions, just things I’d want the RFC to cover.

Suggested change
## Open Questions
## Open Questions
### How do revisions work in the auto-save model?
### What’s the support goal for auto-saving snippets?
### What’s the support goal for auto-saving arbitrary Django models?
### What’s the support goal for fields that don’t have ready-made Telepath adapters?
(not clear to me how common that is nor how much work it would be to make them compatible).


// Include any questions until Status is ‘Accepted’