Tag Length Input Validation #11119

Temidayo32 · 2023-10-23T23:12:34Z

This PR resolves #11054. As reported in the issue, the document page throwing a 500-network error, while on the frontend side, the Update button spins endlessly. I successfully reproduced the error before attempting a fix. Here is what the fix looks like from the frontend side.

Thanks to @BertrandBordage for pointing out the issue and for making useful suggestions in resolving the issue.

Please check the following:

Do the tests still pass?¹
Does the code comply with the style guide?
- Run make lint from the Wagtail root.
For Python changes: Have you added tests to cover the new/fixed behaviour?
For front-end changes: Did you test on all of Wagtail’s supported environments?²
- Please list the exact browser and operating system versions you tested:
- Please list which assistive technologies ³ you tested:
For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

squash-labs · 2023-10-23T23:14:54Z

Manage this branch in Squash

Test this branch here: https://temidayo32document-xdsqt.squash.io

wagtail/documents/forms.py

BertrandBordage

Thank you very much for tackling this @Temidayo32! 🤟
Your approach is correct, but it will unfortunately work only for documents. The same issue will happen with images and for custom models and pages that use tags.

I made a detailed review with a point-by-point suggestion of what I think would be the ideal solution. Keep in mind that I didn't have time to test it though, I'm just pointing out what I found in the source on GitHub.

BertrandBordage · 2023-10-24T22:19:07Z

docs/extending/extending_draftail.md

Are the changes to this file in the right PR? 😉

BertrandBordage · 2023-10-24T22:19:27Z

wagtail/models/i18n.py

Are the changes to this file in the right PR? 😉

BertrandBordage · 2023-10-24T22:28:41Z

wagtail/documents/templates/wagtaildocs/documents/add.html

+            var form = document.querySelector("form[action*='wagtaildocs:add']");
+            if (form) {
+                form.addEventListener("submit", function (event) {
+                    var tagInput = form.querySelector("#id_doc-11-tags");


Does #id_doc-11-tags exist on all document pages? Is 11 the ID of the document, here?

This is naive. I should correct for this. Thanks for pointing it out

BertrandBordage · 2023-10-24T22:50:40Z

wagtail/documents/templates/wagtaildocs/documents/add.html

+        document.addEventListener("DOMContentLoaded", function () {
+            var form = document.querySelector("form[action*='wagtaildocs:add']");
+            if (form) {
+                form.addEventListener("submit", function (event) {
+                    var tagInput = form.querySelector("#id_doc-11-tags");
+                    if (tagInput) {
+                        var tags = tagInput.value.split(",");
+                        var maxTagLength = 100;
+                        for (var i = 0; i < tags.length; i++) {
+                            if (tags[i].trim().length > maxTagLength) {
+                                var errorPlaceholder = form.querySelector("[data-field-errors-placeholder]");
+                                errorPlaceholder.innerHTML = '<p class="error-message">Tag(s) are over 100 characters</p>';
+                                event.preventDefault();
+                                return;
+                            }
+                        }
+                    }
+                });
+            }
+        });


This will work only for documents ;) This does not solve the issue for other places where we use the AdminTagWidget.
Fixing it for all places should be in AdminTagWidget itself. So here:

wagtail/wagtail/admin/templates/wagtailadmin/widgets/tag_widget.html

Lines 1 to 2 in 56c62cc

{% include 'django/forms/widgets/text.html' %}

<p class="help">{{ widget.help_text }}</p>

That being said, the proper fix should be done in TagController, which was introduced by @lb- in PR #10100. There is no easy way to create errors though like you did, so we have to find another way. What I suggest is to simply modify cleanTag, which currently looks like this:

wagtail/client/src/controllers/TagController.ts

Lines 83 to 85 in 56c62cc

cleanTag(val: string) {

return val && val[0] !== '"' && val.indexOf(' ') > -1 ? `"${val}"` : val;

}

If we simply truncate the tag to the maximum length allowed, then we don't even have to think about raising errors, the user will understand that the input has a maximum length.

Here is what I suggest we do:

1. Pass the max_length from TagBase.name in the context of AdminTagWidget, so that the JavaScript side knows what is the limit set in the backend

# This goes in `AdminTagWidget.get_context`: context["widget"]["attrs"]["data-w-tag-options-value"] = json.dumps( { "allowSpaces": getattr(settings, "TAG_SPACES_ALLOWED", True), "tagLimit": getattr(settings, "TAG_LIMIT", None), "autocompleteOnly": not free_tagging, "tagMaxLength": TagBase._meta.get_field("name").max_length, } )

2. Truncate the tag in the frontend to never be more than the maximum length, in TagController.ts

/** * Double quote a tag if it contains a space * and if it isn't already quoted. * Truncate it if it exceeds max length. */ cleanTag(val: string) { const cleanedTag = val && val[0] !== '"' && val.indexOf(' ') > -1 ? `"${val}"` : val; const maxLength = this.optionsValue["tagMaxLength"]; if (maxLength) { return cleanedTag.substring(0, maxLength); } return cleanedTag; }

3. Modify TagField.clean so that the end of it raises a validation error when one of the tags is too long

max_length = TagBase._meta.get_field("name").max_length if any(len(tag) > max_length for tag in tags): raise forms.ValidationError( _("Tags cannot be longer than %s characters long.") % max_length, ) return tags

4. Modify BaseDocumentForm.Meta, BaseImageForm.Meta (and BaseMediaForm.Meta in wagtailmedia)

Remove "tags": AdminTagWidget, from widgets

Add field_classes = {"tags": TagField}

With only these 4 changes, we should cover perfectly all the models using tags in Wagtail. For the TagController.ts changes, you will have to compile the frontend.

@BertrandBordage I think it might be not be completely feasible at the moment to specify a field_classes for BaseDocumentForm. It appears all widgets must be passed into the widget attribute as it is part of the convention Wagtail used for working. And then TagField doesn't work when added directly as tags into widgets.

Here is some documentation: https://docs.wagtail.org/en/stable/reference/settings.html#wagtaildocs-document-form-base.

Here is some docs from the source code:

If the base form specifies the 'tags' widget as a plain unconfigured AdminTagWidget,
# substitute one that correctly passes the tag model used on the document model.
# (If the widget has been overridden via WAGTAILDOCS_DOCUMENT_FORM_BASE, leave it
# alone and trust that they know what they're doing)

So I think based on this, it would be the case that specifying field_classes might break WAGTAILDOCS_DOCUMENT_FORM_BASE so it doesn't work.

BertrandBordage · 2023-10-24T22:51:43Z

wagtail/documents/forms.py

+    def clean_tags(self):
+        tags = self.cleaned_data["tags"]
+        max_tag_length = 100
+        if any(len(tag) > max_tag_length for tag in tags):
+            raise forms.ValidationError("Tag(s) are over 100 characters")
+        return tags


This would work only for tags in documents. To apply this fix to all the tag fields and documents as well, see my comment below :)

@BertrandBordage Thanks for the clear step-by-step instructions!

@BertrandBordage I would think it is better to let them know what is going on than truncating the tags. I would still option for the initial solution of displaying errors. A "non-developer" user might think it is a bug.

I agree with you, but at the same time... It's already how it happens on all the fields in the admin that have a limit, like Page.title and Page.slug. When you reach their limits, you cannot add more characters.

@BertrandBordage Yeah, you are right. And they display an error I think when you get past the limit.

gasman · 2023-11-08T13:07:07Z

wagtail/documents/forms.py

+        tags = self.cleaned_data["tags"]
+        max_tag_length = TagBase._meta.get_field("name").max_length
+        if any(len(tag) > max_tag_length for tag in tags):
+            raise forms.ValidationError("Tag(s) are over 100 characters")


This error message should reference max_tag_length rather than hard-coding the number 100.

Also, this needs tests.

And this needs to be translatable too

@gasman and @laymonage thanks for the comment. I will work on it

I have resolved this

Temidayo32 · 2023-11-17T05:24:29Z

@laymonage Please review.

laymonage

The server-side changes make sense to me, although I wonder if there's a way to enforce this validation automatically in custom forms that use tags.

And perhaps we could extract the clean_tags method into a mixin class, and make it so that the BaseDocumentForm and BaseImageForm extend from that, to reduce duplicity.

I'm not sure if it has to be in this PR's scope or if it's something we can do separately, but as mentioned in previous reviews it would be nice to have some handling of this on the client-side as well.

lb- · 2023-11-28T08:59:00Z

I think we can keep this PR scoped to server side and then raise another issue for future enhancements. Unless there is a major push back - I think this would be good to merge in @laymonage

lb-

This works great @Temidayo32

I will push up a change where we pull this validation logic up into wagtail/admin/forms/tags.py. This should solve for the concerns from @laymonage

As for client-side validation, I think that's a good idea but should be considered for tags as a whole. We do not do this for tag fields inside page editing for example, so I think it's best to be consistent and keep this server side. Maybe if we revisit the tags widget we can look at client-side validation. I have added a note here #11089 (comment)

Screenshots

I have included a smoke test of non-multiple upload validation also.

Fixes wagtail#11054

Temidayo32 mentioned this pull request Oct 24, 2023

Tags longer than 100 chars raise 500 error jazzband/django-taggit#872

Open

halitcelik reviewed Oct 24, 2023

View reviewed changes

wagtail/documents/forms.py Outdated Show resolved Hide resolved

BertrandBordage reviewed Oct 24, 2023

View reviewed changes

Temidayo32 force-pushed the document branch from 1fdcc26 to aac9127 Compare October 25, 2023 04:53

lb- added status:Needs Review component:Tagging labels Oct 31, 2023

gasman requested changes Nov 8, 2023

View reviewed changes

laymonage reviewed Nov 20, 2023

View reviewed changes

lb- self-requested a review November 28, 2023 08:59

lb- mentioned this pull request Nov 29, 2023

Tags/Filter tags - universal styling and behaviour #11089

Open

lb- approved these changes Nov 29, 2023

View reviewed changes

lb- force-pushed the document branch from 2275fce to 3ac8c5d Compare November 29, 2023 21:27

lb- removed the status:Needs Review label Nov 29, 2023

Add max tag length validation for multiple uploads (documents/images)

4851f4a

Fixes wagtail#11054

lb- force-pushed the document branch from 3ac8c5d to 4851f4a Compare November 29, 2023 21:44

lb- merged commit debc656 into wagtail:main Nov 29, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag Length Input Validation #11119

Tag Length Input Validation #11119

Temidayo32 commented Oct 23, 2023

squash-labs bot commented Oct 23, 2023

BertrandBordage left a comment

BertrandBordage Oct 24, 2023

BertrandBordage Oct 24, 2023

BertrandBordage Oct 24, 2023 •

edited

Loading

Temidayo32 Oct 25, 2023 •

edited

Loading

BertrandBordage Oct 24, 2023 •

edited

Loading

Temidayo32 Oct 25, 2023

BertrandBordage Oct 24, 2023

Temidayo32 Oct 25, 2023

Temidayo32 Oct 25, 2023

BertrandBordage Oct 25, 2023

Temidayo32 Oct 25, 2023 •

edited

Loading

gasman Nov 8, 2023

laymonage Nov 8, 2023

Temidayo32 Nov 9, 2023

Temidayo32 Nov 17, 2023

Temidayo32 commented Nov 17, 2023

laymonage left a comment •

edited

Loading

lb- commented Nov 28, 2023

lb- left a comment

	{% include 'django/forms/widgets/text.html' %}
	<p class="help">{{ widget.help_text }}</p>

	cleanTag(val: string) {
	return val && val[0] !== '"' && val.indexOf(' ') > -1 ? `"${val}"` : val;
	}

Tag Length Input Validation #11119

Tag Length Input Validation #11119

Conversation

Temidayo32 commented Oct 23, 2023

Footnotes

squash-labs bot commented Oct 23, 2023

Manage this branch in Squash

BertrandBordage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BertrandBordage Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Temidayo32 Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

BertrandBordage Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

1. Pass the max_length from TagBase.name in the context of AdminTagWidget, so that the JavaScript side knows what is the limit set in the backend

2. Truncate the tag in the frontend to never be more than the maximum length, in TagController.ts

3. Modify TagField.clean so that the end of it raises a validation error when one of the tags is too long

4. Modify BaseDocumentForm.Meta, BaseImageForm.Meta (and BaseMediaForm.Meta in wagtailmedia)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Temidayo32 Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Temidayo32 commented Nov 17, 2023

laymonage left a comment • edited Loading

Choose a reason for hiding this comment

lb- commented Nov 28, 2023

lb- left a comment

Choose a reason for hiding this comment

Screenshots

BertrandBordage Oct 24, 2023 •

edited

Loading

Temidayo32 Oct 25, 2023 •

edited

Loading

BertrandBordage Oct 24, 2023 •

edited

Loading

1. Pass the `max_length` from `TagBase.name` in the context of `AdminTagWidget`, so that the JavaScript side knows what is the limit set in the backend

2. Truncate the tag in the frontend to never be more than the maximum length, in `TagController.ts`

3. Modify `TagField.clean` so that the end of it raises a validation error when one of the tags is too long

4. Modify `BaseDocumentForm.Meta`, `BaseImageForm.Meta` (and `BaseMediaForm.Meta` in wagtailmedia)

Temidayo32 Oct 25, 2023 •

edited

Loading

laymonage left a comment •

edited

Loading