RFC 104: Validation on publish

[gasman]

Rendered

A frequently-requested feature (see Wagtail issue #12505) is the ability to save draft versions of pages in an incomplete state that would not pass validation, but still enforce validation at the point that the page is published or submitted for approval. This is also a prerequisite for an effective auto-save implementation, as noted in RFC 99. This RFC sets out the requirements for such a feature, and an approach to implementing it.

[laymonage]

Thanks @gasman! Excellent write-up on the current state of validation and how autosave might come into play.

However I'm not convinced we should go through hoops of modifying the form classes and customising the validation etc.

For autosave to work, I think we'd get the best value out of it if developers don't have to learn and write yet another validation rules for their models. For users, the best value would be to make sure whatever they have worked on is not lost, which may contain invalid data when validated as a whole.

I think we should go with a different approach of capturing the current form state as-is, which can then be passed on to existing validation rules in the form at the point when a revision needs to be saved. See my comment for more details.

Thanks!

[gasman]

@tm-kn has raised the important question of whether this only covers text fields, or whether for example it would be possible to leave a foreign key field blank and still save as a draft.

The intention was very much for it to work on all field types, but I now realise there's one aspect where I've fallen into the trap of text-centric thinking: for non-text fields, this fails to satisfy the goal of "it should just work without the site implementor having to specifically accommodate it in their code". When bypassing the Django-level validation, a text field that's been left blank can still be saved to the database (and will be written as an empty string) but this is not the case for non-text fields unless null=True is specified. In other words, we're reliant on developers to remember to define a field as IntegerField(null=True) if they want it to be a required field on publish but still allow saving incomplete drafts (as distinct from IntegerField(null=True, blank=True) which would be a non-required field on publish). I don't think there's a good way around this, though.

[laymonage]

@gasman Yeah, I think there’s going to be a lot of edge cases to cover if we went down that path, e.g. database constraints.

Hence I think we should treat the form state as something else entirely and save it to the database as-is, skipping the entire validation chain for the model/form (until the form is actually saved).

[laymonage]

Thanks!

[laymonage]

Makes sense to me!

[jsma]

How might this impact scheduling publication for a future date? Would validation be run at the moment of scheduling? Apologies in advance as I haven't had a chance to checkout the draft PR to test this use case and don't have the relevant internals loaded into my working memory.

I recently had a situation where I had to programmatically add related fields to a page and schedule publication for a future date (page.save_revision(user=user, log_action=True, approved_go_live_at=future_date)), and it isn't clear to me from this RFC how "validation on publish" might impact such a workflow (whether done programmatically or via the UI).

[gasman]

A change of plan here - I found numerous places in the Wagtail test models and bakerydemo where the title field was defined as a plain FieldPanel("title") rather than using TitleFieldPanel, and it seems likely that this will be common in real-world projects too. In this situation, saving a page as draft without a title would skip the form-level validation but enforce it at the model level, resulting in an uncaught error.

Instead, we will set a required_on_save flag on the title field:

    title = models.CharField(
        verbose_name=_("title"),
        max_length=255,
        help_text=_("The page title as you'd like it to be seen by the public"),
    )
    title.required_on_save = True

which can also be used by user fields if desired. The base FieldPanel implementation will check for this flag whenever an explicit required_on_save argument has not been passed to the FieldPanel - that way, we're not reliant on the panels being configured in a certain way to ensure that the validation remains in force.

[gasman]

How might this impact scheduling publication for a future date? Would validation be run at the moment of scheduling? Apologies in advance as I haven't had a chance to checkout the draft PR to test this use case and don't have the relevant internals loaded into my working memory.

I recently had a situation where I had to programmatically add related fields to a page and schedule publication for a future date (page.save_revision(user=user, log_action=True, approved_go_live_at=future_date)), and it isn't clear to me from this RFC how "validation on publish" might impact such a workflow (whether done programmatically or via the UI).

@jsma That's a good point - within the admin interface, scheduling a page for future publication is done through the "publish" action (even though the button may be relabelled if wagtail/wagtail#12424 goes ahead) and so the validation rules applicable to publishing will be enforced at that point.

If you're scheduling them programmatically instead, things are a bit more subtle since it's bypassing the form validation and entirely reliant on model-level validation - as it stands, Page.save_revision does enforce full validation unless you specifically pass clean=False, so I believe this is handled correctly. It would definitely be worth us adding a unit test to ensure that stays in force, though.

[gasman]

A user experience issue that has arisen while implementing this for snippets: if the field (or fields) used for the model's __str__ representation has been defined in the conventional Django way (blank=False but no required_on_save flag set), it will be possible to save a draft instance with a blank string representation, which will then show up as blank in listings and other places.

I don't think there's any way we can identify those fields to be singled out for special-case treatment, and while it might be possible to implement some heuristics (such as requiring validation on draft if the field is called title or is the only field defined directly on the model), that seems like too much magic.

At least it's an easy problem to spot and correct when it does come up...

We should definitely ensure that anything making use of the string representation (or get_admin_display_title) provides a fallback such as [Foo object]. This would include listings, and log entries (the latter being where this was spotted, since the BaseLogEntry model defaults to populating the label field with this, via the get_instance_title method - and this is a required field that gets validated on save).

I'm also considering the possibility that this might be a big enough user-facing change to justify bumping the version to 7.0, especially if we can get an MVP of auto-save at the same time. (We're probably about due for another major release to clear out all the deprecated features, anyhow.)

[gasman]

Now updated to document additional details encountered while working on the final implementation in Wagtail 7.0: #104 (comment), #104 (comment). This brings the RFC in line with the final implementation, and so I'll go ahead and merge now.