Use form validation errors for important UI feedback

readthedocs/core/forms.py

@@ -115,7 +115,10 @@ class PrevalidatedForm(forms.Form):
     field errors in the UI.
     """
 
-    @property
+    def __init__(self, *args, **kwargs):
+        self._prevalidation_errors = None
+        super().__init__(*args, **kwargs)
+
     def is_valid(self):
         # This differs from ``Form`` in that we don't care if the form is bound
         return not self.errors
@@ -133,7 +136,6 @@ def full_clean(self):
         what. This gives errors before submission and after submission.
         """
         # Always call prevalidation, ``full_clean`` bails if the form is unbound
-        self._prevalidation_errors = None
         self._clean_prevalidation()
 
         super().full_clean()
@@ -151,7 +153,7 @@ def _clean_prevalidation(self):
         Catch validation errors raised by the subclassed ``clean_validation()``.
 
         This wraps ``clean_prevalidation()`` using the same pattern that
-        :py:method:`form.Form._clean_form` wraps :py:method:`clean`. An validation
+        :py:method:`form.Form._clean_form` wraps :py:method:`clean`. Validation
         errors raised in the subclass method will be eventually added to the
         form error list but :py:method:`full_clean`.
         """
@@ -160,6 +162,9 @@ def _clean_prevalidation(self):
         except forms.ValidationError as validation_error:
             self._prevalidation_errors = [validation_error]
 
+    def clean_prevalidation(self):
+        raise NotImplementedError()
+
 
 class RichValidationError(forms.ValidationError):
 

readthedocs/projects/forms.py

@@ -82,7 +82,10 @@ class ProjectBackendForm(forms.Form):
     backend = forms.CharField()
 
 
-class ProjectFormParamMixin:
+class ProjectFormPrevalidateMixin:
+
+    """Provides shared logic between the automatic and manual create forms."""
+
     def __init__(self, *args, **kwargs):
         self.user = kwargs.pop("user", None)
         super().__init__(*args, **kwargs)
@@ -113,7 +116,7 @@ def clean_prevalidation(self):
             # TODO this logic should be possible from AdminPermission
             # AdminPermssion.is_admin only inspects organization owners, so the
             # additional team check is necessary
-            self.user_missing_admin_permission = not any(
+            self.user_has_admin_permission = any(
                 [
                     AdminPermission.organizations(
                         user=self.user,
@@ -124,7 +127,7 @@ def clean_prevalidation(self):
             )
 
 
-class ProjectAutomaticForm(ProjectFormParamMixin, PrevalidatedForm):
+class ProjectAutomaticForm(ProjectFormPrevalidateMixin, PrevalidatedForm):
     def clean_prevalidation(self):
         """
         Block user from using this form for important blocking states.
@@ -153,7 +156,7 @@ def clean_prevalidation(self):
                     ),
                     header=_("Organization single sign-on enabled"),
                 )
-            if self.user_missing_admin_permission:
+            if not self.user_has_admin_permission:
                 raise RichValidationError(
                     _(
                         "You must be on a team with admin permissions "
@@ -163,7 +166,7 @@ def clean_prevalidation(self):
                 )
 
 
-class ProjectManualForm(ProjectFormParamMixin, PrevalidatedForm):
+class ProjectManualForm(ProjectFormPrevalidateMixin, PrevalidatedForm):
     def clean_prevalidation(self):
         super().clean_prevalidation()
         if settings.RTD_ALLOW_ORGANIZATIONS:
@@ -175,7 +178,7 @@ def clean_prevalidation(self):
                     ),
                     header=_("Organization single sign-on enabled"),
                 )
-            if self.user_missing_admin_permission:
+            if not self.user_has_admin_permission:
                 raise RichValidationError(
                     _(
                         "You must be on a team with admin permissions "

readthedocs/rtd_tests/tests/test_project_forms.py

@@ -1,12 +1,17 @@
 from unittest import mock
 
+import pytest
+from allauth.socialaccount.models import SocialAccount
 from django.contrib.auth.models import User
+from django.core.exceptions import NON_FIELD_ERRORS, ValidationError
 from django.test import TestCase
 from django.test.utils import override_settings
 from django_dynamic_fixture import get
 
 from readthedocs.builds.constants import EXTERNAL, LATEST, STABLE
 from readthedocs.builds.models import Version
+from readthedocs.core.forms import RichValidationError
+from readthedocs.organizations.models import Organization, Team
 from readthedocs.projects.constants import (
     MULTIPLE_VERSIONS_WITH_TRANSLATIONS,
     MULTIPLE_VERSIONS_WITHOUT_TRANSLATIONS,
@@ -21,8 +26,10 @@
     EmailHookForm,
     EnvironmentVariableForm,
     ProjectAdvancedForm,
+    ProjectAutomaticForm,
     ProjectBasicsForm,
     ProjectExtraForm,
+    ProjectManualForm,
     TranslationForm,
     UpdateProjectForm,
     WebHookForm,
@@ -424,6 +431,124 @@ def test_external_version_not_in_default_branch_choices(self):
         )
 
 
+class TestProjectPrevalidationForms(TestCase):
+    def setUp(self):
+        # User with connection
+        # User without connection
+        self.user_github = get(User)
+        self.social_github = get(SocialAccount, user=self.user_github)
+        self.user_email = get(User)
+
+    def test_form_prevalidation_email_user(self):
+        form_auto = ProjectAutomaticForm(user=self.user_email)
+        form_manual = ProjectManualForm(user=self.user_email)
+
+        # Test validation errors directly
+        with pytest.raises(ValidationError) as validation_error:
+            form_auto.clean_prevalidation()
+        assert validation_error.type is RichValidationError
+        form_manual.clean_prevalidation()
+
+        # Test downstream
+        assert not form_auto.is_valid()
+        assert form_auto.errors == {NON_FIELD_ERRORS: mock.ANY}
+
+        assert form_manual.is_valid()
+        assert form_manual.errors == {}
+
+    def test_form_prevalidation_github_user(self):
+        form_auto = ProjectAutomaticForm(user=self.user_github)
+        form_manual = ProjectManualForm(user=self.user_github)
+
+        # Test validation errors directly
+        form_auto.clean_prevalidation()
+        form_manual.clean_prevalidation()
+
+        # Test downstream
+        assert form_auto.is_valid()
+        assert form_auto.errors == {}
+
+        assert form_manual.is_valid()
+        assert form_manual.errors == {}
+
+
+@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
+class TestProjectPrevalidationFormsWithOrganizations(TestCase):
+    def setUp(self):
+        self.user_owner = get(User)
+        self.social_owner = get(SocialAccount, user=self.user_owner)
+        self.user_admin = get(User)
+        self.social_admin = get(SocialAccount, user=self.user_admin)
+        self.user_readonly = get(User)
+        self.social_readonly = get(SocialAccount, user=self.user_readonly)
+
+        self.organization = get(
+            Organization,
+            owners=[self.user_owner],
+            projects=[],
+        )
+        self.team_admin = get(
+            Team,
+            access="admin",
+            organization=self.organization,
+            members=[self.user_admin],
+        )
+        self.team_readonly = get(
+            Team,
+            access="readonly",
+            organization=self.organization,
+            members=[self.user_readonly],
+        )
+
+    def test_form_prevalidation_readonly_user(self):
+        form_auto = ProjectAutomaticForm(user=self.user_readonly)
+        form_manual = ProjectManualForm(user=self.user_readonly)
+
+        # Test validation errors directly
+        with pytest.raises(
+            RichValidationError, match=r"admin permissions"
+        ) as validation_error:
+            form_auto.clean_prevalidation()
+        with pytest.raises(
+            RichValidationError, match=r"admin permissions"
+        ) as validation_error:
+            form_manual.clean_prevalidation()
+
+        # Test downstream
+        assert not form_auto.is_valid()
+        assert form_auto.errors == {NON_FIELD_ERRORS: mock.ANY}
+        assert not form_manual.is_valid()
+        assert form_manual.errors == {NON_FIELD_ERRORS: mock.ANY}
+
+    def test_form_prevalidation_admin_user(self):
+        form_auto = ProjectAutomaticForm(user=self.user_admin)
+        form_manual = ProjectManualForm(user=self.user_admin)
+
+        # Test validation errors directly
+        form_auto.clean_prevalidation()
+        form_manual.clean_prevalidation()
+
+        # Test downstream
+        assert form_auto.is_valid()
+        assert form_auto.errors == {}
+        assert form_manual.is_valid()
+        assert form_manual.errors == {}
+
+    def test_form_prevalidation_owner_user(self):
+        form_auto = ProjectAutomaticForm(user=self.user_owner)
+        form_manual = ProjectManualForm(user=self.user_owner)
+
+        # Test validation errors directly
+        form_auto.clean_prevalidation()
+        form_manual.clean_prevalidation()
+
+        # Test downstream
+        assert form_auto.is_valid()
+        assert form_auto.errors == {}
+        assert form_manual.is_valid()
+        assert form_manual.errors == {}
+
+
 class TestTranslationForms(TestCase):
     def setUp(self):
         self.user_a = get(User)