Defer validation of required text fields until publish time by gasman · Pull Request #12923 · wagtail/wagtail

gasman · 2025-02-25T13:22:51Z

Implements RFC 104 for required fields on page and snippet models.

squash-labs · 2025-02-25T14:00:33Z

Manage this branch in Squash

Test this branch here: https://featurevalidate-on-publish-3-97bsx.squash.io

laymonage

I tested it and it works very well! Just a few non-blocking comments, otherwise it looks good to me, thanks!

laymonage · 2025-03-17T08:09:00Z

    def get_cell_context_data(self, instance, parent_context):
        context = super().get_cell_context_data(instance, parent_context)
+        if not str(context["value"]).strip():
+            context["value"] = "(blank)"


Happy to go with this for now, but I wonder if the columns should support something similar to ModelAdmin's empty_value_display more generally?

Seems like a good idea! Added in 453ef62 (and made translatable along the way).

laymonage · 2025-03-17T09:08:02Z

+            # If required_on_save is not explicitly set, treat it as false unless:
+            # - it corresponds to a model field with required_on_save=True (such as page title)
+            # - it corresponds to a non-null, non-text-typed model field (in which case a blank value
+            #   is not valid at the database level)


Makes sense to me!

laymonage · 2025-03-17T09:14:47Z

+    @property
+    def action_name(self):
+        return self.action_name_and_method[0]
+
+    @property
+    def action_method(self):
+        return self.action_name_and_method[1]


Thank you, I've been thinking of doing some kind of mapping between the action name and method, but I think this works just as well!

laymonage · 2025-03-17T09:19:58Z

+    if result.strip() == "":
+        result = f"{obj.__class__.__name__} object ({obj.pk})"


Should this (and the other fallbacks, i.e. in audit log and table column) be translatable? Django's isn't, but we might want to do so as it's user-facing.

Made translatable in a70e582.

laymonage · 2025-03-17T10:23:08Z

        if self.form.is_valid():
            return self.form_valid(self.form)
        else:
+            self.form.restore_required_fields()


Nice catch!

laymonage · 2025-03-17T10:28:26Z

+        If ``clean=False`` is passed, the page is saved without validation. This is appropriate for updates that only
+        change metadata such as `latest_revision` while keeping content and page location unchanged.

-        By default, pages are validated using ``full_clean()`` before attempting to
-        save changes to the database, which helps to preserve validity when restoring
-        pages from historic revisions (which might not necessarily reflect the current
-        model state). This validation step can be bypassed by calling the method with
-        ``clean=False``.
+        If ``clean=True`` is passed (the default), and the page has ``live=True`` set, the page is validated using
+        :meth:`~django.db.models.Model.full_clean` before saving.
+
+        If ``clean=True`` is passed, and the page has `live=False` set, only the title and slug fields are validated.


Not sure if we ever did it inside docstrings, but does the change warrant a versionchanged admonition? If we don't want it in the docstring, maybe we can add it below the .. automethod:: save in the Markdown file.

Yep, that can't hurt... now added in e3f674a.

laymonage

One non-blocking comment about empty_value_display, but otherwise this looks good to me. Thanks!

laymonage · 2025-03-19T06:48:17Z

            value = unlocalize(value)
-        context["value"] = value
+
+        if not str(value).strip():


Just want to note that Django's logic is as follows:

If the value of a field is None, an empty string, or an iterable without elements, Django will display - (a dash). You can override this with AdminSite.empty_value_display:

https://docs.djangoproject.com/en/stable/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_display

I think taking None into consideration so that users can override empty_value_display to customise its display would be a common use case. However, we might want to change the default value to a dash like Django, as otherwise it would display as an empty string.

It's not in the scope of this PR though, but I thought I'd raise this since we're just about to add the support. Also happy to do this in a separate PR.

Yep, since this is a potential change of behaviour on existing listings, i think that's best left for a separate PR.

…ModelForm

…om performing it

…ethod

… from_date field The fix to EventPageForm.clean addresses a logic bug that existed before the change to validation - if `date_from` was left blank, it would be omitted from `cleaned_data` and so retrieving it would raise an uncaught KeyError.

It is clear from the test models and bakerydemo that title is frequently declared with a plain FieldPanel rather than TitleFieldPanel, so we should not rely on the presence of TitleFieldPanel or an explicit required_on_save flag on the FieldPanel to enforce form-level validation on the title.

… non-text-type model fields

…r scheduling As per wagtail/rfcs#104 (comment) - unless clean=False is explicitly passed, validation should be applied, including on fields that would accept nulls at the database level.

…ests for creating/editing invalid draft snippets

… views

…wing form after an error

…lank This surfaces an issue with SearchPromotion having a broken string representation when the query string is None, so fix that too

gasman added the type:Enhancement New features, feature requests, improvements to existing functionality, enhancements label Feb 25, 2025

gasman added this to the 6.5* milestone Feb 25, 2025

gasman marked this pull request as draft February 25, 2025 13:23

gasman force-pushed the feature/validate-on-publish-3 branch from ce5b027 to 1fda901 Compare March 5, 2025 17:16

gasman self-assigned this Mar 5, 2025

gasman marked this pull request as ready for review March 5, 2025 18:04

gasman requested a review from laymonage March 5, 2025 18:04

lb- added the status:Needs Review Ready for review by maintainers, awaiting code review, pending feedback label Mar 9, 2025

gasman mentioned this pull request Mar 12, 2025

Defer validation of StreamField min block counts when saving drafts #12964

Merged

laymonage reviewed Mar 17, 2025

View reviewed changes

laymonage approved these changes Mar 19, 2025

View reviewed changes

gasman added 18 commits March 19, 2025 13:33

Add mechanism for deferring required field validation on WagtailAdmin…

9899754

…ModelForm

Refactor page create/edit views to decouple identifying the action fr…

6fce521

…om performing it

Set default slug / draft_title / locale independently of full_clean m…

38a1433

…ethod

Relax model validation when saving pages as draft

8c7b04c

Fix test models to use TitleFieldPanel for the title field

d2888fc

Defer validation on FieldPanel unless required_on_save passed

46c6926

Add tests for validate-on-publish when creating pages

21d6589

Add tests for validate-on-publish when editing

fde6029

Enforce validation on save for FieldPanels corresponding to non-null,…

2c7052f

… non-text-type model fields

Test that validation is still enforced on save_revision when saving o…

3834f91

…r scheduling As per wagtail/rfcs#104 (comment) - unless clean=False is explicitly passed, validation should be applied, including on fields that would accept nulls at the database level.

Defer validation when saving snippets as draft

e69f716

Add fallback for blank string representations in audit log, and add t…

ce37f71

…ests for creating/editing invalid draft snippets

Use fallback for blank string representations on model index and edit…

a471ef5

… views

Preserve required asterisks on validation-deferred fields when re-sho…

3030397

…wing form after an error

Support deferring validation on StreamField

78f6de4

Add an empty_value_display property on Column for use when value is b…

1ecc739

…lank This surfaces an issue with SearchPromotion having a broken string representation when the query string is None, so fix that too

gasman added 2 commits March 19, 2025 13:33

Make get_latest_str fallback translatable

b1beca3

Add versionchanged note for new Page.save validation

0df0a20

gasman force-pushed the feature/validate-on-publish-3 branch from e3f674a to 0df0a20 Compare March 19, 2025 13:34

gasman merged commit c38db51 into wagtail:main Mar 19, 2025
17 of 19 checks passed

github-project-automation bot moved this to ✅ Done in Wagtail 7.0 release planning Mar 19, 2025

github-project-automation bot moved this to Done in Triage trial Mar 19, 2025

lb- removed the status:Needs Review Ready for review by maintainers, awaiting code review, pending feedback label Mar 19, 2025

This was referenced May 21, 2025

Support fields being required only when publishing, not when saving drafts #12505

Closed

RFC 108: Autosave MVP wagtail/rfcs#108

Open

		if result.strip() == "":
		result = f"{obj.__class__.__name__} object ({obj.pk})"

Uh oh!

Conversation

gasman commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

squash-labs bot commented Feb 25, 2025

Manage this branch in Squash

Uh oh!

laymonage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laymonage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gasman commented Feb 25, 2025 •

edited

Loading