[WEB-4434]fix: user name validation #7617
Conversation
WalkthroughAdds client-side pattern validation for first_name and last_name in onboarding/profile forms, server-side regex validation in UserSerializer, and unit tests for serializer name validation. last_name remains optional on the client; new client error rendering appears for last_name. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant WebForm as Web Form (Onboarding/Profile)
participant ClientVal as Client Validation
participant API as API Endpoint
participant Serializer as UserSerializer
participant DB as Persistence
User->>WebForm: Enter first_name / last_name
User->>WebForm: Submit
WebForm->>ClientVal: Run required + pattern checks
alt Client validation fails
ClientVal-->>User: Show field error(s)
else Client validation passes
WebForm->>API: POST profile data
API->>Serializer: Deserialize + validate
alt Server regex validation fails
Serializer-->>API: ValidationError
API-->>WebForm: 400 with field errors
WebForm-->>User: Display server error(s)
else Valid
Serializer->>DB: Save/Update user
DB-->>Serializer: OK
API-->>WebForm: 200 Success
WebForm-->>User: Proceed
end
end
note over ClientVal,Serializer: Pattern allows letters, numbers, spaces, hyphens, underscores
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
… into fix-user-name-validation
…ne/plane into fix-user-name-validation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/plane/db/models/user.py (1)
58-61: Validators on names: OK; confirm server-side length policyAttaching validators here is fine and needs no migration. UI caps at 24 chars; model allows 255. If 24 is a product rule, enforce it server-side to avoid drift.
Option (no schema change):
from django.core.validators import MaxLengthValidator first_name = models.CharField(max_length=255, blank=True, validators=[validate_name, MaxLengthValidator(24)]) last_name = models.CharField(max_length=255, blank=True, validators=[validate_name, MaxLengthValidator(24)])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/api/plane/db/models/user.py(3 hunks)apps/web/core/components/onboarding/steps/profile/root.tsx(1 hunks)apps/web/core/components/profile/form.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and lint web apps
🔇 Additional comments (2)
apps/web/core/components/profile/form.tsx (2)
279-285: Removelast_nameglobally or reconsider scope
Thelast_namefield is still referenced in dozens of modules—store (workspace-member.store.ts), analytics providers (posthog-provider.tsx, intercom-provider.tsx), local-DB schemas, workspace/project member-columns, profile sidebar/form, onboarding steps, greetings components, etc. Removing it only from the form snippet is insufficient. Update the PR to remove alllast_nameusages and related plumbing across the codebase, or adjust the scope of this change.
250-254: Trim and tighten first_name validation- validate: (value) => { - if (!/^[a-zA-Z0-9 _-]+$/.test(value)) - return "First name can only contain letters, numbers, hyphens, and underscores."; - return true; - }, + validate: (value) => { + const v = value.trim(); + if (v.length === 0) return "First name can’t be empty or only spaces."; + if (!/^[A-Za-z0-9 _-]+$/.test(v)) + return "First name can only contain letters, numbers, spaces, hyphens (-), and underscores (_)."; + return true; + },Also confirm the intended max-length (UI caps at 24 vs server at 255) and align both ends.
| validate: (value) => { | ||
| if (!/^[a-zA-Z0-9 _-]+$/.test(value)) | ||
| return "First name can only contain letters, numbers, hyphens, and underscores."; | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align rule with copy and block whitespace-only names
Message says “First name” though label is “Name”, and you allow spaces but don’t mention them; also accepts only-spaces. Trim and update message.
- validate: (value) => {
- if (!/^[a-zA-Z0-9 _-]+$/.test(value))
- return "First name can only contain letters, numbers, hyphens, and underscores.";
- return true;
- },
+ validate: (value) => {
+ const v = value.trim();
+ if (v.length === 0) return "Name can't be empty or only spaces.";
+ if (!/^[A-Za-z0-9 _-]+$/.test(v))
+ return "Name can only contain letters, numbers, spaces, hyphens (-), and underscores (_).";
+ return true;
+ },If the PR removes last name in onboarding, also delete it from:
- TProfileSetupFormValues and defaultValues
- handleSubmitUserDetail payload (Line 84)
🏁 Script executed:
#!/bin/bash
# Find remaining onboarding references to last_name
rg -nP -C2 '\blast_name\b' apps/web/core/components/onboardingLength of output: 8338
Update name validation to trim whitespace and align message copy
apps/web/core/components/onboarding/steps/profile/root.tsx (lines 216–220):
- validate: (value) => {
- if (!/^[a-zA-Z0-9 _-]+$/.test(value))
- return "First name can only contain letters, numbers, hyphens, and underscores.";
- return true;
- },
+ validate: (value) => {
+ const trimmed = value.trim();
+ if (trimmed.length === 0) return "Name can't be empty or only spaces.";
+ if (!/^[A-Za-z0-9 _-]+$/.test(trimmed))
+ return "Name can only contain letters, numbers, spaces, hyphens (-), and underscores (_).";
+ return true;
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| validate: (value) => { | |
| if (!/^[a-zA-Z0-9 _-]+$/.test(value)) | |
| return "First name can only contain letters, numbers, hyphens, and underscores."; | |
| return true; | |
| }, | |
| validate: (value) => { | |
| const trimmed = value.trim(); | |
| if (trimmed.length === 0) | |
| return "Name can't be empty or only spaces."; | |
| if (!/^[A-Za-z0-9 _-]+$/.test(trimmed)) | |
| return "Name can only contain letters, numbers, spaces, hyphens (-), and underscores (_)."; | |
| return true; | |
| }, |
🤖 Prompt for AI Agents
In apps/web/core/components/onboarding/steps/profile/root.tsx around lines 216
to 220, update the validation to trim leading/trailing whitespace before testing
and adjust the error copy to mention spaces explicitly: compute const v = (value
|| "").trim(); then run the regex test against v and if it fails return "First
name can only contain letters, numbers, spaces, hyphens, and underscores."
(optionally also check if v === "" and return a required-name message if
needed).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/app/views/user/base.py (1)
76-84: Align errors and UX; consider serializer-level validation.
- The message omits “spaces” although they’re allowed by the regex. Field-specific messages help the UI pinpoint which input failed.
- Consider moving this validation into
UserSerializer(validate_first_name/validate_last_name) to cover both create and update uniformly and reduce view logic.- Optional: trim inputs before validation and enforce a sane max length to prevent all-space names and respect DB limits.
Example (serializer snippet, for reference outside this file):
# in UserSerializer NAME_PATTERN = re.compile(r"^[a-zA-Z0-9 _-]+$") def validate_first_name(self, v): v = v.strip() if not NAME_PATTERN.fullmatch(v): raise serializers.ValidationError( "First name can only contain letters, numbers, spaces, hyphens (-), and underscores (_)." ) return v def validate_last_name(self, v): if v is None: return v v = v.strip() if v != "" and not NAME_PATTERN.fullmatch(v): raise serializers.ValidationError( "Last name can only contain letters, numbers, spaces, hyphens (-), and underscores (_)." ) return v
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/api/plane/app/views/user/base.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/app/views/user/base.py (1)
8-8: LGTM: required import added.
reis needed for the validation below.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/api/plane/app/serializers/user.py (4)
10-12: Label stdlib import correctly
reis a standard library import, not "Django". Rename the section label for clarity. Optionally, group stdlib imports above third-party ones to follow common import ordering.-# Django import +# Standard library imports import re
19-23: Tighten copy: capitalize and punctuate error messageKeep user-facing strings consistent with nearby messages (“First name cannot contain a URL.”). Capitalize and add a period.
- raise serializers.ValidationError( - "first name can only contain letters, numbers, hyphens (-), and underscores (_)" - ) + raise serializers.ValidationError( + "First name can only contain letters, numbers, hyphens (-), and underscores (_)." + )
30-34: Mirror copy fix for last_name messageApply the same casing and punctuation for consistency.
- raise serializers.ValidationError( - "last name can only contain letters, numbers, hyphens (-), and underscores (_)" - ) + raise serializers.ValidationError( + "Last name can only contain letters, numbers, hyphens (-), and underscores (_)." + )
26-34: Guard against null last_name inputs (if API allows null)If clients can send
nullforlast_name,contains_url(value)will raise a TypeError. Consider short-circuiting empty/None before validations. Ifnullis impossible (model/serializer setsallow_null=False), ignore this.Example (outside the changed hunk, shown for clarity):
def validate_last_name(self, value): if value in (None, ""): return "" if value is None else value if contains_url(value): raise serializers.ValidationError("Last name cannot contain a URL.") if not re.match(r"^[a-zA-Z0-9_\- ]+$", value): raise serializers.ValidationError( "Last name can only contain letters, numbers, hyphens (-), and underscores (_)." ) return value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/api/plane/app/serializers/user.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/app/serializers/user.py (1)
apps/api/plane/utils/url.py (1)
contains_url(22-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
|
@dheeru0198 What happens in case of people use different language than english? Does this Validation fail? |
sriramveeraghanta
added
tests required
|
@sriramveeraghanta They cannot use other languages because we only accept English letters, and if they try, an error will be shown. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/api/plane/tests/unit/serializers/test_user.py (6)
11-27: Parametrize the “valid first names” test for clearer failures and isolated cases.Parametrization yields better reporting and avoids the manual loop.
Apply:
- def test_validate_first_name_valid(self): - """Test that valid first names are accepted""" - - serializer = UserSerializer() - valid_names = [ - "John", - "John Doe", - "John-Doe", - "John_Doe", - "John123", - ] - - for name in valid_names: - result = serializer.validate_first_name(name) - assert result == name + @pytest.mark.parametrize( + "name", ["John", "John Doe", "John-Doe", "John_Doe", "John123"] + ) + def test_validate_first_name_valid(self, name): + """Test that valid first names are accepted""" + serializer = UserSerializer() + assert serializer.validate_first_name(name) == name
28-43: Avoid brittle equality on error text; assert via regex match.Makes tests resilient to DRF ErrorDetail formatting.
Apply:
- for name in invalid_names: - with pytest.raises(serializers.ValidationError) as exc_info: - serializer.validate_first_name(name) - assert str(exc_info.value.detail[0]) == "First name cannot contain a URL." + for name in invalid_names: + with pytest.raises( + serializers.ValidationError, match=r"^First name cannot contain a URL\.$" + ): + serializer.validate_first_name(name)
83-98: Use regex-based assertion for the URL error message here too.Keep consistency with first_name URL tests.
Apply:
- for name in invalid_names: - with pytest.raises(serializers.ValidationError) as exc_info: - serializer.validate_last_name(name) - assert str(exc_info.value.detail[0]) == "Last name cannot contain a URL." + for name in invalid_names: + with pytest.raises( + serializers.ValidationError, match=r"^Last name cannot contain a URL\.$" + ): + serializer.validate_last_name(name)
99-118: Mirror the message fix for last_name; also consider real-world invalid chars.Include “spaces” in the message and (optionally) add cases like apostrophes and dots to document the decision explicitly.
Apply:
- assert str(exc_info.value.detail[0]) == ( - "last name can only contain letters, numbers, " - "hyphens (-), and underscores (_)" - ) + assert str(exc_info.value.detail[0]) == ( + "last name can only contain letters, numbers, spaces, " + "hyphens (-), and underscores (_)" + )Optional extra negatives to add near this test:
+ "O'Connor", + "Jr.",
11-118: Prefer parametrization for all looped cases.Applies to both valid/invalid lists across first and last name tests for consistent style and clearer test IDs.
If helpful, I can push a patch converting all list+loops to @pytest.mark.parametrize.
1-118: Add one serializer-level integration test.Call
UserSerializer(data={...})and assertis_valid()/errorsso we also catch wiring issues beyond direct method calls.Example (append at end):
def test_user_serializer_integration_field_errors(): s = UserSerializer(data={"first_name": "http://x", "last_name": "Smith"}) assert not s.is_valid() assert "first_name" in s.errors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/api/plane/tests/unit/serializers/test_user.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/tests/unit/serializers/test_user.py (1)
apps/api/plane/app/serializers/user.py (3)
UserSerializer(14-71)validate_first_name(15-24)validate_last_name(26-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/api/plane/tests/unit/serializers/test_user.py (2)
1-4: Good test module setup.Imports are minimal and correct for pytest + DRF ValidationError usage.
119-119: Expand negative coverage: non-ASCII and bare domains.Given the product decision to allow only English letters, add explicit tests for non-ASCII names and bare domains (“example.com”) to ensure they’re rejected (and that URL detection meets expectations).
Suggested additions:
+ @pytest.mark.parametrize("name", ["Álvaro", "Müller", "Renée", "Иван", "张三"]) + def test_validate_first_name_non_ascii_rejected(self, name): + s = UserSerializer() + with pytest.raises(serializers.ValidationError, match=r"letters, numbers"): + s.validate_first_name(name) + + @pytest.mark.parametrize("name", ["example.com", "foo.bar"]) + def test_validate_first_name_bare_domain_rejected(self, name): + s = UserSerializer() + with pytest.raises(serializers.ValidationError): + s.validate_first_name(name)If
contains_urlcurrently misses bare domains, either adjust it or change this test expectation accordingly.
|
Let's wait until we figure out if non english languages should be allowed or not |
dheeru0198
dismissed
their stale review
Need to rethink about supporting non english languages
Description
This PR will add validation for the user's first name and last name.
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Tests