Skip to content

Commit 5245ebb

Browse files
authored
Use form validation errors for important UI feedback (#11095)
* Add base classes for prevalidation Form and rich validation errors - Prevalidaition on forms allows throwing errors on the initial form display, before the user submits the form. - Rich validation errors gives non-field error message types (info, error, warning, etc) and header/title text * Refactor project create form to use form validation errors Instead of using notifications without a database, use form validation errors to raise error states on specific forms, or outright disable them. * Add logic for team, owner, and SSO validations * Invert conditional to avoid `not any()` * Clarify some of the prevalidation form internals * Fix is_valid call * Fix not implemented throw * Add tests for form prevalidation * Pytest tests -> unittest tests
1 parent ea43371 commit 5245ebb

File tree

4 files changed

+322
-3
lines changed

4 files changed

+322
-3
lines changed

readthedocs/core/forms.py

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from crispy_forms.layout import Fieldset, Layout, Submit
66
from django import forms
77
from django.contrib.auth.models import User
8+
from django.core.exceptions import NON_FIELD_ERRORS
89
from django.forms.fields import CharField
910
from django.utils.translation import gettext_lazy as _
1011

@@ -98,6 +99,96 @@ class Meta:
9899
fields = ["allow_ads"]
99100

100101

102+
class PrevalidatedForm(forms.Form):
103+
104+
"""
105+
Form class that allows raising form errors before form submission.
106+
107+
The base ``Form`` does not support validation errors while the form is
108+
unbound (does not have ``data`` defined). There are cases in our UI where we
109+
want to show errors and/or disabled the form before the user has a chance to
110+
interact with the form -- for example, when a feature is unavailable or
111+
disabled for the user or organization.
112+
113+
This provides the ``clean_prevalidation`` method, which acts much like the
114+
``clean`` method. Any validation errors raised in this method surface as non
115+
field errors in the UI.
116+
"""
117+
118+
def __init__(self, *args, **kwargs):
119+
self._prevalidation_errors = None
120+
super().__init__(*args, **kwargs)
121+
122+
def is_valid(self):
123+
# This differs from ``Form`` in that we don't care if the form is bound
124+
return not self.errors
125+
126+
@property
127+
def is_disabled(self):
128+
return self._prevalidation_errors is not None
129+
130+
def full_clean(self):
131+
"""
132+
Extend full clean method with prevalidation cleaning.
133+
134+
Where :py:method:`forms.Form.full_clean` bails out if there is no bound
135+
data on the form, this method always checks prevalidation no matter
136+
what. This gives errors before submission and after submission.
137+
"""
138+
# Always call prevalidation, ``full_clean`` bails if the form is unbound
139+
self._clean_prevalidation()
140+
141+
super().full_clean()
142+
143+
# ``full_clean`` sets ``self._errors``, so we prepend prevalidation
144+
# errors after calling the parent ``full_clean``
145+
if self._prevalidation_errors is not None:
146+
non_field_errors = []
147+
non_field_errors.extend(self._prevalidation_errors)
148+
non_field_errors.extend(self._errors.get(NON_FIELD_ERRORS, []))
149+
self._errors[NON_FIELD_ERRORS] = non_field_errors
150+
151+
def _clean_prevalidation(self):
152+
"""
153+
Catch validation errors raised by the subclassed ``clean_validation()``.
154+
155+
This wraps ``clean_prevalidation()`` using the same pattern that
156+
:py:method:`form.Form._clean_form` wraps :py:method:`clean`. Validation
157+
errors raised in the subclass method will be eventually added to the
158+
form error list but :py:method:`full_clean`.
159+
"""
160+
try:
161+
self.clean_prevalidation()
162+
except forms.ValidationError as validation_error:
163+
self._prevalidation_errors = [validation_error]
164+
165+
def clean_prevalidation(self):
166+
raise NotImplementedError()
167+
168+
169+
class RichValidationError(forms.ValidationError):
170+
171+
"""
172+
Show non-field form errors as titled messages.
173+
174+
This uses more of the FUI message specification to give a really clear,
175+
concise error message to the user. Without this class, non-field validation
176+
errors show at the top of the form with a title "Error", which isn't as
177+
helpful to users as something like "Connected service required".
178+
179+
:param header str: Message header/title text
180+
:param message_class str: FUI CSS class to use on the message -- "info",
181+
etc. Default: "error".
182+
"""
183+
184+
def __init__(
185+
self, message, code=None, params=None, header=None, message_class=None
186+
):
187+
super().__init__(message, code, params)
188+
self.header = header
189+
self.message_class = message_class
190+
191+
101192
class FacetField(forms.MultipleChoiceField):
102193

103194
"""

readthedocs/projects/forms.py

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
from re import fullmatch
55
from urllib.parse import urlparse
66

7+
from allauth.socialaccount.models import SocialAccount
78
from django import forms
89
from django.conf import settings
910
from django.contrib.auth.models import User
@@ -12,12 +13,15 @@
1213
from django.utils.translation import gettext_lazy as _
1314

1415
from readthedocs.builds.constants import INTERNAL
16+
from readthedocs.core.forms import PrevalidatedForm, RichValidationError
1517
from readthedocs.core.history import SimpleHistoryModelForm
18+
from readthedocs.core.permissions import AdminPermission
1619
from readthedocs.core.utils import slugify, trigger_build
1720
from readthedocs.core.utils.extend import SettingsOverrideObject
1821
from readthedocs.integrations.models import Integration
1922
from readthedocs.invitations.models import Invitation
2023
from readthedocs.oauth.models import RemoteRepository
24+
from readthedocs.organizations.models import Team
2125
from readthedocs.projects.models import (
2226
AddonsConfig,
2327
Domain,
@@ -78,6 +82,112 @@ class ProjectBackendForm(forms.Form):
7882
backend = forms.CharField()
7983

8084

85+
class ProjectFormPrevalidateMixin:
86+
87+
"""Provides shared logic between the automatic and manual create forms."""
88+
89+
def __init__(self, *args, **kwargs):
90+
self.user = kwargs.pop("user", None)
91+
super().__init__(*args, **kwargs)
92+
93+
def clean_prevalidation(self):
94+
# Shared conditionals between automatic and manual forms
95+
self.user_has_connected_account = SocialAccount.objects.filter(
96+
user=self.user,
97+
).exists()
98+
self.user_is_nonowner_with_sso = None
99+
self.user_missing_admin_permission = None
100+
if settings.RTD_ALLOW_ORGANIZATIONS:
101+
# TODO there should be some way to initially select the organization
102+
# and maybe the team too. It's mostly safe to automatically select
103+
# the first organization, but explicit would be better. Reusing the
104+
# organization selection UI works, we only really need a query param
105+
# here.
106+
self.user_is_nonowner_with_sso = all(
107+
[
108+
AdminPermission.has_sso_enabled(self.user),
109+
AdminPermission.organizations(
110+
user=self.user,
111+
owner=False,
112+
).exists(),
113+
]
114+
)
115+
116+
# TODO this logic should be possible from AdminPermission
117+
# AdminPermssion.is_admin only inspects organization owners, so the
118+
# additional team check is necessary
119+
self.user_has_admin_permission = any(
120+
[
121+
AdminPermission.organizations(
122+
user=self.user,
123+
owner=True,
124+
).exists(),
125+
Team.objects.admin(self.user).exists(),
126+
]
127+
)
128+
129+
130+
class ProjectAutomaticForm(ProjectFormPrevalidateMixin, PrevalidatedForm):
131+
def clean_prevalidation(self):
132+
"""
133+
Block user from using this form for important blocking states.
134+
135+
We know before the user gets a chance to use this form that the user
136+
might not have the ability to add a project into their organization.
137+
These errors are raised before the user submits the form.
138+
"""
139+
super().clean_prevalidation()
140+
if not self.user_has_connected_account:
141+
url = reverse("socialaccount_connections")
142+
raise RichValidationError(
143+
_(
144+
f"You must first <a href='{url}'>add a connected service "
145+
f"to your account</a> to enable automatic configuration of "
146+
f"repositories."
147+
),
148+
header=_("No connected services found"),
149+
)
150+
if settings.RTD_ALLOW_ORGANIZATIONS:
151+
if self.user_is_nonowner_with_sso:
152+
raise RichValidationError(
153+
_(
154+
"Only organization owners may create new projects "
155+
"when single sign-on is enabled."
156+
),
157+
header=_("Organization single sign-on enabled"),
158+
)
159+
if not self.user_has_admin_permission:
160+
raise RichValidationError(
161+
_(
162+
"You must be on a team with admin permissions "
163+
"in order to add a new project to your organization."
164+
),
165+
header=_("Admin permission required"),
166+
)
167+
168+
169+
class ProjectManualForm(ProjectFormPrevalidateMixin, PrevalidatedForm):
170+
def clean_prevalidation(self):
171+
super().clean_prevalidation()
172+
if settings.RTD_ALLOW_ORGANIZATIONS:
173+
if self.user_is_nonowner_with_sso:
174+
raise RichValidationError(
175+
_(
176+
"Projects cannot be manually configured when "
177+
"single sign-on is enabled for your organization."
178+
),
179+
header=_("Organization single sign-on enabled"),
180+
)
181+
if not self.user_has_admin_permission:
182+
raise RichValidationError(
183+
_(
184+
"You must be on a team with admin permissions "
185+
"in order to add a new project to your organization."
186+
),
187+
header=_("Admin permission required"),
188+
)
189+
190+
81191
class ProjectBasicsForm(ProjectForm):
82192

83193
"""Form for basic project fields."""

readthedocs/projects/views/private.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,10 @@
5555
IntegrationForm,
5656
ProjectAdvancedForm,
5757
ProjectAdvertisingForm,
58+
ProjectAutomaticForm,
5859
ProjectBasicsForm,
5960
ProjectConfigForm,
61+
ProjectManualForm,
6062
ProjectRelationshipForm,
6163
RedirectForm,
6264
TranslationForm,
@@ -397,9 +399,11 @@ def post(self, request, *args, **kwargs):
397399
def get_context_data(self, **kwargs):
398400
context = super().get_context_data(**kwargs)
399401
context['view_csrf_token'] = get_token(self.request)
400-
context['has_connected_accounts'] = SocialAccount.objects.filter(
401-
user=self.request.user,
402-
).exists()
402+
403+
if settings.RTD_EXT_THEME_ENABLED:
404+
context["form_automatic"] = ProjectAutomaticForm(user=self.request.user)
405+
context["form_manual"] = ProjectManualForm(user=self.request.user)
406+
403407
return context
404408

405409

0 commit comments

Comments
 (0)