feat: Add social media username validation#1043
Closed
nourzakhama2003 wants to merge 6 commits into
Closed
Conversation
Contributor
👀 Peer Review RequiredHi @nourzakhama2003! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds new Discord/Slack/GitHub username validators, applies them to Profile fields (reducing max_length), calls model validation on save, extends ProfileUpdateForm with a shared social-field cleaner and per-field cleaners, and adds comprehensive tests for validators, model, and form validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as "ProfileUpdateForm"
participant Validator as "validate_*_username"
participant Model as "Profile (full_clean)"
participant DB
User->>Form: submit form data (discord/slack/github)
Form->>Validator: call validator per field (via _clean_social_media_username)
Validator-->>Form: raise/return ValidationError or pass
Form->>Model: save instance (instance bound)
Model->>Model: full_clean() -> run field validators
Model-->>Form: raise ValidationError back to form (if any)
Model->>DB: save (if validation passed)
DB-->>User: persist confirmed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/forms.py`:
- Around line 750-775: Replace the duplicated logic in clean_discord_username,
clean_slack_username, and clean_github_username with a single helper (e.g.,
_clean_username(field_key, validator)) that fetches username =
self.cleaned_data.get(field_key, ""), runs the passed validator
(validate_discord_username / validate_slack_username / validate_github_username)
inside a try/except, and on ValidationError re-raises forms.ValidationError
using the original error message; then have each clean_* method simply call this
helper with its field name and validator. Ensure the helper returns the username
and that the signatures of clean_discord_username, clean_slack_username, and
clean_github_username remain present but delegate to _clean_username to preserve
Django form behavior.
In `@web/models.py`:
- Around line 66-83: The CharField max_length values for discord_username,
slack_username, and github_username must be reduced to match their validators to
prevent DB-level persistence of overlong values: change discord_username
max_length from 50 to 32, slack_username max_length from 50 to 21, and
github_username max_length from 50 to 39 (update the fields discord_username,
slack_username, github_username accordingly) and create/apply a migration so the
schema and validators remain consistent.
In `@web/tests/test_social_media_validators.py`:
- Around line 41-50: The tests for validate_discord_username currently assert
translated message substrings; instead update them to assert the
ValidationError.code (a stable error code) equals a specific code (e.g.,
"invalid_discord_username") after raising the error from
validate_discord_username; replace self.assertIn("2 and 32", str(cm.exception))
with an assertion like self.assertEqual(cm.exception.code,
"invalid_discord_username") and apply the same change to the other similar
assertions referenced (the other validate_discord_username tests around the
noted ranges).
- Around line 189-229: The tests (e.g., test_profile_with_valid_usernames,
test_profile_with_invalid_discord_username,
test_profile_with_invalid_slack_username,
test_profile_with_invalid_github_username) currently persist invalid Profile
instances using Profile.objects.create(...) before calling full_clean(); change
each test to instantiate unsaved profiles with Profile(... ) (no
.objects.create), then call profile.full_clean() (assert no exception for the
valid case, and use with self.assertRaises(ValidationError) for the invalid
cases) so invalid data is not written to the DB and the tests remain isolated
and intention-revealing.
In `@web/validators.py`:
- Around line 26-33: The current logic in validators.py trims everything after
the first "#" into username_part which allows invalid discriminators like
"User#", "User#abcd", or "User#1234#x"; update the validation to first ensure
the value contains at most one "#" (or none), and if a "#" is present validate
the suffix is exactly 4 numeric digits (the Discord discriminator) before
accepting the username_part; keep raising ValidationError (use the same message
or add a second message for invalid discriminator) when the username length or
discriminator format is invalid and reference the existing variables/value and
function that raises ValidationError to locate where to apply the checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: af54cc89-7101-40ab-93a5-786e0c792f79
📒 Files selected for processing (4)
web/forms.pyweb/models.pyweb/tests/test_social_media_validators.pyweb/validators.py
Author
|
Hi team, This PR is ready for review. Could someone please take a look? @ayesha1145 @omsherikar (or any other active contributor you see) Thank you! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/forms.py`:
- Around line 750-765: The _clean_social_media_username helper is catching a
ValidationError and converting it to a plain string which drops error codes and
metadata; instead, re-raise a forms.ValidationError preserving the original
ValidationError details (e.g., use the caught exception's error_list /
message_dict / error_list structure or rethrow the original ValidationError) so
error codes from your validators in web/validators.py (like
invalid_discord_length, invalid_slack_chars) remain intact; update
_clean_social_media_username to catch ValidationError as e and raise
forms.ValidationError(e.error_list or e) (or otherwise pass along e.messages and
e.code metadata) rather than raising a plain string.
In `@web/models.py`:
- Around line 66-83: The Profile.save method currently doesn't run model
validation so field validators like validate_discord_username,
validate_slack_username, and validate_github_username can be bypassed; update
Profile.save to call self.full_clean() before super().save(...) (while
preserving the existing referral_code generation logic that checks/sets
self.referral_code via generate_referral_code()) so all validators run on every
save.
In `@web/tests/test_social_media_validators.py`:
- Around line 198-248: The tests create new Profile(user=self.user, ...) which
conflicts with the existing Profile created by User.objects.create_user via the
post_save signal; instead reuse the existing profile via self.user.profile: in
test_profile_with_valid_social_usernames,
test_profile_with_invalid_discord_username,
test_profile_with_invalid_slack_username and
test_profile_with_invalid_github_username get the profile object with profile =
self.user.profile, set the discord_username/slack_username/github_username
fields on that profile, then call profile.full_clean() (and profile.save() for
the valid case) so you don't hit the OneToOne uniqueness error or unrelated user
validation errors.
In `@web/validators.py`:
- Around line 107-113: The current re.match allows consecutive hyphens; update
the pattern used in the validation (the re.match call that checks the variable
value) to reject consecutive hyphens by adding a negative lookahead (e.g. start
the regex with (?!.*--) ) so usernames like "octo--cat" fail; keep raising the
same ValidationError with code "invalid_github_chars" and the existing message
so behavior and error surface stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac9c5cfc-20e6-4325-b124-ee77f111e380
📒 Files selected for processing (4)
web/forms.pyweb/models.pyweb/tests/test_social_media_validators.pyweb/validators.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/forms.py (1)
709-723:⚠️ Potential issue | 🟡 MinorAlign the form
max_lengthvalues with the backend limits.These fields still advertise 50 characters even though
Profileand the validators now enforce 37/21/39. Matching the form limits will give users the right browser-side constraint and avoid generic 50-character errors masking the platform-specific feedback this PR adds.🔧 Suggested adjustment
discord_username = forms.CharField( - max_length=50, + max_length=37, required=False, widget=TailwindInput(), help_text="Discord username (visible if profile is public)", ) slack_username = forms.CharField( - max_length=50, required=False, widget=TailwindInput(), help_text="Slack username (visible if profile is public)" + max_length=21, + required=False, + widget=TailwindInput(), + help_text="Slack username (visible if profile is public)", ) github_username = forms.CharField( - max_length=50, + max_length=39, required=False, widget=TailwindInput(), help_text="GitHub username (visible if profile is public)", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/forms.py` around lines 709 - 723, The form fields discord_username, slack_username, and github_username in web/forms.py still use max_length=50 but the backend/Profile validators enforce 37, 21, and 39 respectively; update those CharField max_length values to 37 for discord_username, 21 for slack_username, and 39 for github_username so the browser-side constraints match the backend validation and surface platform-specific errors correctly.
♻️ Duplicate comments (1)
web/tests/test_social_media_validators.py (1)
203-205: 🧹 Nitpick | 🔵 TrivialReuse
self.user.profileand assertsave()in these invalid model tests.
create_user()already creates aProfile, so constructing a secondProfile(user=self.user, ...)pulls in an unrelateduseruniqueness error. Reusingself.user.profilekeeps the assertions isolated and lets these tests cover the newProfile.save()guarantee instead of onlyfull_clean().🔧 Suggested pattern
- profile = Profile( - user=self.user, - discord_username="A", # Too short - slack_username="valid", - github_username="valid", - ) - with self.assertRaises(ValidationError) as cm: - profile.full_clean() + profile = self.user.profile + profile.discord_username = "A" # Too short + profile.slack_username = "valid" + profile.github_username = "valid" + with self.assertRaises(ValidationError) as cm: + profile.save() self.assertIn("discord_username", cm.exception.message_dict)As per coding guidelines
**/tests/**: Ensure tests are descriptive, cover edge cases, and follow the project's existing test patterns. Verify test isolation and meaningful assertions.Also applies to: 217-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/tests/test_social_media_validators.py` around lines 203 - 205, Tests create a second Profile manually causing unique-user conflicts; update the tests that currently construct Profile(user=self.user, ...) to reuse the existing profile from setUp via self.user.profile, mutate the fields to the invalid values being tested, then call and assert on profile.save() (e.g., expect a ValidationError) so the tests exercise Profile.save() behavior rather than only full_clean(); update the test cases mentioned (around the setUp block and the tests covering lines ~217-251) to follow this pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/models.py`:
- Around line 66-83: Existing rows may violate the new max_length/validators and
Profile.save() now calls full_clean(), so create and run a data migration before
deploying the model change that (1) scans profiles for invalid/over-limit values
for discord_username, slack_username, github_username (and any other
social-handle fields being shrunk), (2) either trims values to the new
max_length or moves originals to a temp backup column/log and sets them
blank/null depending on business rules, and (3) logs/report affected user IDs so
you can verify; deploy that migration first (or temporarily keep the old
max_length/remove full_clean() from Profile.save() until backfill completes)
then release the model/validator changes.
In `@web/validators.py`:
- Around line 107-113: Update the ValidationError message raised where the regex
check on value occurs so the user-facing text matches the rule enforced by the
regex: state that GitHub usernames may contain alphanumerics and hyphens, cannot
start or end with a hyphen, and may not contain consecutive hyphens (e.g.,
"octo--cat" is invalid). Modify the string inside the existing _("...") passed
to ValidationError (keeping the same code="invalid_github_chars" and use of _())
to reflect the consecutive-hyphen restriction.
---
Outside diff comments:
In `@web/forms.py`:
- Around line 709-723: The form fields discord_username, slack_username, and
github_username in web/forms.py still use max_length=50 but the backend/Profile
validators enforce 37, 21, and 39 respectively; update those CharField
max_length values to 37 for discord_username, 21 for slack_username, and 39 for
github_username so the browser-side constraints match the backend validation and
surface platform-specific errors correctly.
---
Duplicate comments:
In `@web/tests/test_social_media_validators.py`:
- Around line 203-205: Tests create a second Profile manually causing
unique-user conflicts; update the tests that currently construct
Profile(user=self.user, ...) to reuse the existing profile from setUp via
self.user.profile, mutate the fields to the invalid values being tested, then
call and assert on profile.save() (e.g., expect a ValidationError) so the tests
exercise Profile.save() behavior rather than only full_clean(); update the test
cases mentioned (around the setUp block and the tests covering lines ~217-251)
to follow this pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12ea4fb2-ebc9-4d14-8df7-3df3f07253c2
📒 Files selected for processing (4)
web/forms.pyweb/models.pyweb/tests/test_social_media_validators.pyweb/validators.py
github-actions
Bot
added
files-changed: 6
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/validators.py`:
- Line 13: Add explicit type hints to the public validator function signatures:
change validate_discord_username, validate_* at the symbol names found at lines
~61 and ~89 to accept value: str | None (or Optional[str]) and return -> None;
update each function signature (e.g., def validate_discord_username(value: str |
None) -> None:) so static type checkers and linters understand the input/output
types while leaving the internal logic unchanged.
- Line 73: Remove the redundant lower-bound length checks that follow earlier
falsy-value returns: replace the condition "if len(value) < 1 or len(value) >
21:" (and the analogous check at the second occurrence) with a single
upper-bound check "if len(value) > 21:" so the validation only enforces the max
length; keep the existing early returns for falsy values intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 857ceb01-8559-46a4-a33a-1fd46677ea5e
⛔ Files ignored due to path filters (2)
web/migrations/0064_cleanup_social_media_usernames.pyis excluded by!**/migrations/**web/migrations/0065_alter_profile_social_username_fields.pyis excluded by!**/migrations/**
📒 Files selected for processing (1)
web/validators.py
- Add explicit type hints to all validator function signatures: Optional[str] -> None - Remove redundant len(value) < 1 checks after early returns for empty values - Improves code clarity and enables better static analysis
github-actions
Bot
dismissed
their stale review
All review conversations have been resolved.
Contributor
🚚 This Repository Is MovingHi @nourzakhama2003, thank you for your contribution! We are in the process of migrating most of the logic from this repository to our new repository: alphaonelabs/learn. What this means for your PRPlease do not merge or continue work here. Instead:
This PR has been automatically closed. Once you have opened your PR in the new repository, feel free to reference it here. Thank you for your understanding and continued support! 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add Social Media Username Validation
Problem
The Alpha One Labs platform currently allows users to enter any text in social media username fields (
discord_username,slack_username,github_username) without validation. This leads to:Solution
Implemented platform-specific validators for each social media platform following their official username requirements:
New Features
web/validators.py- Three custom validators:validate_discord_username(): 2-32 characters, alphanumeric + special chars (-, _, .)validate_slack_username(): 1-21 characters, lowercase only, must start with lettervalidate_github_username(): 1-39 characters, alphanumeric + hyphens (no leading/trailing hyphens)Model-Level Validation - Added validators to
Profilemodel fields to ensure data integrity at database layerForm-Level Validation - Enhanced
ProfileUpdateFormwithclean_*_username()methods for user-friendly error messagesChanges
Testing
Test Coverage:
Benefits
Type
Checklist
Purpose
Add platform-specific validation for social media username fields (discord_username, slack_username, github_username) on the Profile model and surface user-facing validation errors in forms to prevent invalid or platform-incompatible values from being saved.
Key Modifications
web/validators.py
web/models.py
web/forms.py
web/tests/test_social_media_validators.py
Commits
Impact / Notes