-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate profile form fields #4910
Validate profile form fields #4910
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4910 +/- ##
==========================================
+ Coverage 76.65% 76.66% +<.01%
==========================================
Files 158 158
Lines 10057 10057
Branches 1269 1269
==========================================
+ Hits 7709 7710 +1
Misses 2007 2007
+ Partials 341 340 -1
|
@@ -35,23 +35,6 @@ def test_edit_profile(self): | |||
self.assertEqual(self.user.last_name, 'Docs') | |||
self.assertEqual(self.user.profile.homepage, 'readthedocs.org') | |||
|
|||
def test_edit_profile_with_invalid_values(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the tests.
Just one doubt, max_length
is already well-tested feature of django, then what was the need for the these tests ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure we don't break this in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay...
Thanks..!!
|
||
self.assertTrue(len(long_first_name) > 30) | ||
self.assertTrue(len(long_last_name) > 30) | ||
self.assertTrue(len(long_homepage) > 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could just use string multiplication to make the test shorter and more clear, 'a' * 31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for guiding me on this.
I have made the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for your contribution!
Fixes #4887