Use form validation errors for important UI feedback

[agjohnson]

I found myself rebuilding these messages in template code and figured it would be a good excuse to further consolidate the patterns we're using.

In short, these messages are emitted at view time and are not saved to the database. The view time logic is similar as notifications however.

A few questions stand for me:

• I like centering all of our notifications/errors around exceptions, as it retains an opportunity to emit exceptions from wherever we might want to move some of this logic. Arguably, this logic should not exist on a single view and could be moved on to modeling/queryset classes.
• I don't have a strong opinion on whether it's important for these messages to exist in the registry. I see arguments for both, but I chose to use the registry so that we can consolidate logic there -- format values and message lookup, etc.

I'm looking for input on these patterns.

On the template side, the display is now just a simple if and template logic similar to the current notification display, using the header/body render methods.

I don't know why GitHub is having such a hard time with the PR base, but the relevant commit here is: 1cf32db

• See base Some fixes for notifications #11094

---

Update: I'll leave the above, but the pattern here changed to using Form and validation errors

• Fixes Move UI for disabled importing for SSO users around ext-theme#26
• Fixes Projects: add project creation note about SSO ext-theme#248

[humitos]

Thanks for opening this PR. I think the UX will be improved with these notifications.

I have conflicting feelings with the proposed pattern here. Mainly, because it's used in a way that it wasn't thought for: not saving the notification in the database. For those notifications, I think that Django messages or form validation errors are a better fit.

The pattern for the notification is:

• from builders, raise an exception from anywhere in the build process. It will be catch at on_failure and the API will be called to be saved in the db
• from anywhere else with access to the db, just call Notifications.objects.add()

This PR is adding another bullet point to the original pattern and removing one of the bases: saving the notification in the db. So, it seems that you are re-using the rendering but dropping the storage, making this "a one-time notification, each time the user opens that view".

I think what you want here is a "state-based notification", which we already have a pattern for that:

• check if some conditions are met and create a notification
• show the notification
• check if those conditions have changed (via Django signals), and .cancel() the notification

(see

readthedocs.org/readthedocs/projects/views/private.py

Lines 115 to 133 in 83805a6

	def validate_primary_email(self, user):
	"""
	Sends a dismissable site notification to this user.
	Checks if the user has a primary email or if the primary email
	is verified or not. Sends a dismissable notification if
	either of the condition is False.
	"""
	email_qs = user.emailaddress_set.filter(primary=True)
	email = email_qs.first()
	if not email or not email.verified:
	Notification.objects.add(
	attached_to=user,
	message_id=MESSAGE_EMAIL_VALIDATION_PENDING,
	dismissable=True,
	format_values={
	"account_email_url": reverse("account_email"),
	},
	)

and

readthedocs.org/readthedocs/notifications/signals.py

Lines 51 to 68 in 83805a6

	@receiver(post_save, sender=EmailAddress)
	def user_email_verified(instance, *args, **kwargs):
	"""Check if the primary email is validated and cancel the notification."""
	if instance.primary:
	if instance.verified:
	Notification.objects.cancel(
	attached_to=instance.user,
	message_id=MESSAGE_EMAIL_VALIDATION_PENDING,
	)
	else:
	Notification.objects.add(
	attached_to=instance.user,
	message_id=MESSAGE_EMAIL_VALIDATION_PENDING,
	dismissable=True,
	format_values={
	"account_email_url": reverse("account_email"),
	},
	)

for an example of this "state-based notification")

The only difference that I found here is that you want the notification to be shown only on a particular Django view instead of being attached to a particular object, which makes me think we should add another field Notification.view_name (default=None) or similar and show them only on those specific views 👍🏼

I feel the pattern I'm describing here solves your problem, it's very clear and fits great in what we already have.

NOTE: I left some comments in the PR, but they are old now after writing this general review. I lean more towards the pattern I just describe here which is more explicit and clear.

[humitos]

You can differentiate this also by splitting it into a new variable making it explicit that they are different from the messages ones:

view_messages = [
 # ...
]

registry.add(view_messages)

I used this pattern with MkDocs and other build notifications in other files, as well.

[humitos]

The only difference that I found here is that you want the notification to be shown only on a particular Django view instead of being attached to a particular object, which makes me think we should add another field Notification.view_name (default=None) or similar and show them only on those specific views 👍🏼

Also, I just realize that using this pattern will give us the opportunity of using API-based notification for this as well when we need them.

[humitos]

Linking this PR readthedocs/ext-theme#274 since it's related to this backend changes.

[agjohnson]

I think that Django messages or form validation errors are a better fit.

Yeah, a bit. Though Django messages don't work as they are now temporary warnings via FUI ui toast messages. And validation errors happen after the form is submit. So neither was really a good fit.

I think what you want here is a "state-based notification"

Yes, this would work, but you are correct that we'd need more on the modeling. I didn't want to go this path as I'm not convinced this would be reusable or that it warrants complicating all of the rest of notifications.

For this view, there need to be two individually addressable blocks too, one for automatic, and one manual.

[agjohnson]

Thinking about all this more, I would say that complicating our modeling/notifications for just this view doesn't feel worth it. I would say what you're describing is closer to technically correct, but it does involve a lot of complication for such a minor use case.

We are creating a new pattern in our notification system mostly just for this view in either plan, so I wouldn't avoid what I have here just on the basis of it being a new pattern.

So, without taking on a lot of work here, the paths that I have here are:

• Reuse notifications. Messages do what I want already, but this doesn't involve the database.
• Create a mix of template logic and view logic that does not use notifications, but replicates most of what notifications are doing.

The reason I came to use Messages here was that I felt it was better to re-use code, even if in a slightly new way, than it was to replicate work/patterns we've just consolidated. That is, I think it's a big win to move all our permanent/stateful messages to singular display logic, and I'm happy to extend our patterns to fit that.

If you don't agree that re-using this pattern is okay, I'm just going to go back to approximating notifications one-off in template/view logic. I agree on the overall stateful notification pattern, but I don't think stateful per-view/per-block notifications on this view are that high on our needs/priorities.

Also, I just realize that using this pattern will give us the opportunity of using API-based notification for this as well when we need them.

What other views are you thinking of here?

[humitos]

The reason I came to use Messages here was that I felt it was better to re-use code, even if in a slightly new way, than it was to replicate work/patterns we've just consolidated. That is, I think it's a big win to move all our permanent/stateful messages to singular display logic, and I'm happy to extend our patterns to fit that.

Thinking about re-using code is always good. I agree with that.

If you don't agree that re-using this pattern is okay, I'm just going to go back to approximating notifications one-off in template/view logic.

I don't want to go back to the template/view logic. Mainly, because removing the logic from there was one of the goals of the new notification system 😉

I agree on the overall stateful notification pattern, but I don't think stateful per-view/per-block notifications on this view are that high on our needs/priorities.

Honestly, I don't think that adding a new field to the Notification object + a Django signal makes the pattern more complex ¹. I think the use case your are describing here fits perfectly with this idea of statefule notifications that we already have in place --the only we will be changing would be that "instead of being attached only to a model (User, in this case), it will be attached to a view as well (the new field)". I can give it a quick try if you want and open a small PR to show what I have in mind. Let me know.

What other views are you thinking of here?

I haven't thought too much about this yet, but the UX you are describing here (blocking a UI because "something needs to be done first") seems pretty obvious to me. Thinking quickly about this, I image we can use this UX on Read the Docs for Business more for features that are based on plans. For example:

• you cannot enable Google SSO before upgrading to Pro
• you cannot create a Domain with the Basic plan
• same for search and pageviews analytics, and audit logs
• ... any other view we want to block for users without "permissions"

I think we will want to re-use this pattern sooner that what we are thinking now 😄 . This pattern will move away all this logic from the templates and backed it up in the backend re-using the normal notifications pattern we have 💯 . The more I write about this the more convinced I am 😅

Footnotes

1. as a general rule, I usually think that having exceptions to the normal flow/pattern is harder to read/parse and maintain over time, since it adds another thing to keep in mind while working with it. ↩

[agjohnson]

I think there are some good thoughts there! I would need to think through this pattern a bit more first before committing to it though. This would be great to brainstorm more on I feel.

I would probably agree the technical change wouldn't be huge initially, but I do see where this pattern would be high friction in development, at least compared to a simple template conditionals. I do want to get away from maintaining error messages and conditional logic in template code though.

Some notifications make a lot of sense as stateful notifications -- notifications like subscription errors for example. This pattern feels low friction to me because we are going to show the subscription error notification on many different views and these notifications won't be altering these views besides display of the notification message.

The use cases we're talking about here would be altering parts of individual views, and these notifications would be emit from signals that are unrelated to the view -- for example, on organization team change signal we emit/retract the notification for required admin permissions on project creation.

That is, the answer to "How do I add/change a conditional to disable the project creation form?" seems like it would be:

• Add the message/notification and signal handler with the conditional logic
• And after any change to the conditional, run a migration or manual shell operation to iterate over users/projects/organization/etc to re-evaluate the conditional logic and emit/retract new notifications.

Compared to {% if user_can_create_automatically %}, this seems rather high friction to our development.

Anyways, that's to say that I think we should continue playing with this pattern for notifications on high level messages on projects/organizations. For very acute template logic like what I am doing on the project creation page, the pattern isn't as clear of a win yet.

[agjohnson]

For example:

• you cannot enable Google SSO before upgrading to Pro
• you cannot create a Domain with the Basic plan
• same for search and pageviews analytics, and audit logs
• ... any other view we want to block for users without "permissions"

What I found interesting in your list is that it seems like all of these examples, and my example here, probably do indeed fit best as form validation errors.

So, maybe your original suggestion of using form validation errors is one to digest a bit more first.

The examples you list a pretty straight forward. These views all use form instances in a very basic way, so it would only take emitting the validation error from a Form method.

The project creation UI might need some convincing here, as the Form instance is hidden rather deeply in the HTML structure, controlled mostly through FUI elements. I will try this quickly to see.

The big unknown here is if Form instances will display validation errors in the same way while the Form is still unbound.

A minor point here is that validation errors only have a body, there is no header. I am liking how the headers on notifications can be concise and clear, with body to back up the header. Not a huge point though.

What I think I will really appreciate with this pattern is that all of the control of the form function is now on the Form instance, and not split between form, view, and template logic.

---

Edit: And as I expected, Form instances don't like having validation errors thrown while the form is still unbound. But this was pretty easy:

class ProjectAutomaticForm(forms.Form):

    def precheck(self):
        if True:
            return forms.ValidationError("You can't do that.")

    @property
    def errors(self):
        if not self.is_bound:
            if (precheck_error := self.precheck()) is not None:
                errors = {}
                errors[NON_FIELD_ERRORS] = self.error_class(
                    [precheck_error],
                    error_class="nonfield",
                    renderer=self.renderer,
                )
                return errors
        else:
            return super().errors

[humitos]

And after any change to the conditional, run a migration or manual shell operation to iterate over users/projects/organization/etc to re-evaluate the conditional logic and emit/retract new notifications.

The need of changing the conditional is a good point that I hadn't considered.

So, maybe your original suggestion of using form validation errors is one to digest a bit more first.

If it's possible, I'd follow this path, yeah, that looks more Django-starndard.

A minor point here is that validation errors only have a body, there is no header. I am liking how the headers on notifications can be concise and clear, with body to back up the header. Not a huge point though.

I agree here. I don't think we require a header on these messages.

Edit: And as I expected, Form instances don't like having validation errors thrown while the form is still unbound. But this was pretty easy:

It seems you were able to figure this out in a simple way. I looks great to me 💯 . Do they render as you expected in the frontend?

[agjohnson]

Sounds good! I might put together a basic implementation that I can work off of today and tomorrow, but might hand off some of this work afterwards.

What I wrote above wasn't super clear, so to clarify: I am specifically talking about implementing Form classes that throw unbound form validation errors (forms that show errors on GET, before the user submits the form).

It seems you were able to figure this out in a simple way. I looks great to me 💯 . Do they render as you expected in the frontend?

They did render, I just did a quick test outputting the form.as_p though. I think they should render okay-ish with crispy forms though.

I agree here. I don't think we require a header on these messages.

The Form class does have a configurable error_class too. A custom class that includes a header seems possible, but doesn't need to be our first focus either.

[agjohnson]

I was quite easily able to add both prevalidation on form instances and titled validation error messages. I spent some time refining the pattern and matching it to the Form implementation a bit closer. I feel this pattern is much cleaner than doing this in the view and definitely cleaner than in template code.

What I really like is that all of the reasons why a form can't be valid are all in one place now. The implementation over Form doesn't feel very deep at all either, and in fact should still work on the legacy dashboard without changes even.

It looks quite close to how the initial pass did, nothing much changed:

And the supporting template changes are minimal and really straight forward:
readthedocs/ext-theme@d6aa0e8

@humitos How does this approach feel now? Does this seem more reusable?

[humitos]

This pattern is perfect! ¹ Pretty clean and all the logic is self-contained with the form 💯

Footnotes

1. thanks for taking the time to discuss and iterate on it 🙏🏼 ↩

[agjohnson]

I am going to add a test or two to start and will move this from a draft PR. It seems this is close, I'd like to get some feedback on the permission checks before merging.

[agjohnson]

I added some basic tests that the validation is working as expected for some of the important cases. Changes are the last few commits, starting with f38e7a8

[humitos]

I'm happy where this pattern ended up. I think it's great and follow Django standards 👍🏼 . It's ready to merge IMO.

I pinged Santos about the test cases because I think we are mixing two patterns that shouldn't be mixed, tho.

[stsewd]

I think this is rendered as text, not HTML, or at least the "normal" errors are. If we are rendering errors as HTML, we should be careful, there are other places where we include user input in them.