new release #507
new release #507
Conversation
![code-review-doctor[bot]](https://avatars.githubusercontent.com/in/83430?s=60&v=4){kind=small}
| from common.utils import COUNTRIES | ||
| from teams.models import Teams | ||
|
|
||
|
|
||
| class Contact(models.Model): | ||
| class Contact(BaseModel): |
There was a problem hiding this comment.
This model has lots of fields. Consider splitting into multiple models. More info.
| Profile, related_name="contact_created_by", on_delete=models.SET_NULL, null=True | ||
| ) | ||
| created_on = models.DateTimeField(_("Created on"), auto_now_add=True) | ||
| twitter_username = models.CharField(max_length=255, blank=True,null=True) |
There was a problem hiding this comment.
| twitter_username = models.CharField(max_length=255, blank=True,null=True) | |
| twitter_username = models.CharField(max_length=255, blank=True, default="") |
null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". More details.
| if user is None or user.is_anonymous: | ||
| self.created_by = None | ||
| self.updated_by = None | ||
| super(BaseModel, self).save(*args, **kwargs) |
There was a problem hiding this comment.
| super(BaseModel, self).save(*args, **kwargs) | |
| super().save(*args, **kwargs) |
It's unnecessary to use arguments when calling super for the parent class. Explained here.
| self.updated_by = None | ||
| # If updated only set updated_by value don't touch created_by | ||
| self.updated_by = user | ||
| super(BaseModel, self).save(*args, **kwargs) |
There was a problem hiding this comment.
| super(BaseModel, self).save(*args, **kwargs) | |
| super().save(*args, **kwargs) |
As above, These super arguments are unnecessary.
| def save(self, *args, **kwargs): | ||
| self.slug = slugify(self.name) | ||
| super().save(*args, **kwargs) | ||
|
|
||
|
|
||
| class Account(models.Model): | ||
| class Account(BaseModel): |
There was a problem hiding this comment.
This model has lots of fields. Consider splitting into multiple models. More.
| return { | ||
| 'email' : self.user.email, | ||
| 'id' : self.user.id, | ||
| 'is_active' : self.user.is_active, | ||
| 'profile_pic' : self.user.profile_pic | ||
| } |
There was a problem hiding this comment.
| return { | |
| 'email' : self.user.email, | |
| 'id' : self.user.id, | |
| 'is_active' : self.user.is_active, | |
| 'profile_pic' : self.user.profile_pic | |
| } | |
| return { | |
| "email": self.user.email, | |
| "id": self.user_id, | |
| "is_active": self.user.is_active, | |
| "profile_pic": self.user.profile_pic, | |
| } |
| def user_details(self): | ||
| return { | ||
| 'email' : self.user.email, | ||
| 'id' : self.user.id, |
There was a problem hiding this comment.
self.user.id performs a database read when id is evaluated. You could take advantage of Django's caching of related fields by using self.user_id, which does not do a database read. More.
| from common.utils import CURRENCY_CODES | ||
| from teams.models import Teams | ||
|
|
||
|
|
||
| class Invoice(models.Model): | ||
| class Invoice(BaseModel): |
There was a problem hiding this comment.
This model has lots of fields. Consider splitting into multiple models. Read more.
|
|
||
| class Lead(models.Model): | ||
| class Lead(BaseModel): |
There was a problem hiding this comment.
This model has lots of fields. Consider splitting into multiple models. More info.
|
|
||
|
|
||
| class Event(models.Model): | ||
| class PlannerEvent(BaseModel): |
There was a problem hiding this comment.
This model has lots of fields. Consider splitting into multiple models. Read more.
No description provided.