Jkmarx/group name duplicate error #1573
Conversation
| def save(self, *args, **kwargs): | ||
| if len(self.name) == 0: | ||
| logger.error("Group name cannot be empty.") | ||
| return | ||
| raise ValidationError('Group name cannot be empty') | ||
| elif self.duplicate_group_name_exists(): |
There was a problem hiding this comment.
Need to re-evaluate this logic because in Group model: name = models.CharField(_('name'), max_length=80, unique=True)
There was a problem hiding this comment.
Removed the elif because the DB returns an integrityError, but the if return does not return an exception. Please see latest.
| :returns: Boolean based on if a duplicate name exists | ||
| """ | ||
| # Catches empty strings & removes trail/lead white spaces | ||
| if self.name: |
There was a problem hiding this comment.
I'm not sure if we need this extra check here. This may have only been applicable to duplicate slug checking
There was a problem hiding this comment.
No longer require the method.
Current coverage is 34.92% (diff: 16.66%)@@ develop #1573 diff @@
==========================================
Files 340 340
Lines 23456 23576 +120
Methods 0 0
Messages 0 0
Branches 1251 1251
==========================================
+ Hits 8187 8233 +46
- Misses 15269 15343 +74
Partials 0 0
|
| @@ -1875,7 +1875,7 @@ def get_managed_group(self): | |||
| def save(self, *args, **kwargs): | |||
There was a problem hiding this comment.
It looks like the preferred way to validate model fields is via full_clean() and clean() model methods instead of overriding save(): https://docs.djangoproject.com/en/1.7/ref/models/instances/#django.db.models.Model.clean
There was a problem hiding this comment.
Also, the group name cannot be blank already: name = models.CharField(_('name'), max_length=80, unique=True), so no additional checking for that is necessary. It might be a good idea to strip whitespace characters though.
…into jkmarx/group-name-duplicate-error
| @@ -1845,6 +1845,13 @@ class ExtendedGroup(Group): | |||
| can_edit = False | |||
| manager_group_uuid = None | |||
|
|
|||
| def clean(self): | |||
| # Don't allow only white spaces. | |||
| if len(self.name) == 0: | |||
There was a problem hiding this comment.
As I mentioned earlier, it looks like the group name cannot be blank already: name = models.CharField(_('name'), max_length=80, unique=True), so no additional checking for that is necessary. It might be a good idea to strip whitespace characters though.
| if len(self.name) == 0: | ||
| logger.error("Group name cannot be empty.") | ||
| raise ValidationError("Group name cannot be empty") | ||
| return self.name.strip() |
There was a problem hiding this comment.
The clean() method should not return any values: https://docs.djangoproject.com/en/1.7/ref/models/instances/#django.db.models.Model.clean
| return | ||
| else: | ||
| super(ExtendedGroup, self).save(*args, **kwargs) | ||
| self.name = self.clean() |
There was a problem hiding this comment.
The clean() method does not return a value (https://docs.djangoproject.com/en/1.7/_modules/django/db/models/base/#Model.clean). Also, validation should be done using the full_clean() method (https://docs.djangoproject.com/en/1.7/ref/models/instances/#django.db.models.Model.full_clean). Finally, overriding the save() method might cause the validation to be run twice in some cases and not run at all in others (http://stackoverflow.com/a/23684110).
There was a problem hiding this comment.
https://docs.djangoproject.com/en/1.7/ref/models/instances/#django.db.models.Model.full_clean - "Note that full_clean() will not be called automatically when you call your model’s save() method. " That's only an issue with ModelForm. With ModelForms, the clean() methods do return cleaned_data according to Two Scoops 11.7.
|
Please note, this will not be merged until the next milestone (post-code review of course). |
|
|
||
| new_ext_group.name = new_ext_group.name.strip() | ||
| try: | ||
| new_ext_group.full_clean() |
There was a problem hiding this comment.
There is no need to call full_clean() if there is no custom clean() method. The save() method will raise an exception if the group name is blank (a DB constraint) but it will not check for group names that contain whitespace chars. Is that the desired behavior?
There was a problem hiding this comment.
The save() method does not raise an exception if the group name is blank.
There was a problem hiding this comment.
OK, that's correct. Now, should we allow group names that contain whitespace chars only? What about leading and/or trailing whitespace chars?
| except ValidationError as e: | ||
| raise ImmediateHttpResponse(HttpBadRequest( | ||
| 'Invalid group creation request: %s.' % e | ||
| )) | ||
| new_ext_group.save() |
There was a problem hiding this comment.
While save() does not perform validation checks, it can still raise IntegrityError for duplicate names, so that exception should be handled here.
Resolves #1477