Skip to content
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

Initial models creation #28

Open
wants to merge 6 commits into
base: nightly
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 88 additions & 1 deletion FriendsWave/social/models.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,90 @@
from functools import _Descriptor
from django.db import models
from django.db.models.base import Model
from django.db.models.fields.related import OneToOneField

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need 2 line between 2 first level code in python (ref PEP 8) https://pep8.org/

Copy link

@ttgedeon ttgedeon Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General
1- A light comment(specifying purpose) should be added on each field for each model, and a doc-string(purpose/utility) added to each class of the code
2- _Descriptor class is imported form functools module without a clear use
3- Always provide a related name for each Fk and M2M relation
4- Always specify the on_delete behavior on each FK relation
5- use same name to refer to the same reality troughout the code. status vs etat, title vs subject, start vs start_date, end vs end_date
6- Use a Mixins to handle creation, last update time, and soft delete of every

class Profil(models.Models):
pseudo = models.CharField(max_length=100)
image = models.ImageField()
Copy link

@ttgedeon ttgedeon Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1- Make sure static file settings if well configure and Specify specific storage of each static data with functionnality
2- Consider storing an email on Profile

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user profile_image intead of image: it's more meaningfull

user = models.ForeignKey('auth.User', on_delete=models.CASCADE)
friends = models.ManyToManyField('self', through='Friend')
topics = models.ManyToManyField('Topic', through='UserTopic')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

topics relates to what? topics created by the user, read by the users, deleted by the user.... Use topics_of_interest: it more meaningfull

etat = models.BooleanField(default=False)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

etat field is meaning less. active, is_active, profile_is_active are increasingly meaningful


class Friend(models.Model):
follower = models.ForeignKey(Profil, OneToOneField = models.CASCADE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify a related_name on this field to avoid duplication on foreignt key definition by the ORM

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1- Wrong setup of the ForeignKey relations using OneToOneField

followed = models.ForeignKey(Profil, OneToOneField = models.CASCADE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify a related_name on this field to avoid duplication on foreignt key definition by the ORM

status = models.BooleanField()

class Topic(models.Model):
subject = models.TextField(max_length=100)
description = models.TextField(max_length=100)
creator = models.ForeignKey(Profil, on_delete=models.SET_NULL)

class UserTopic(models.Model):
profil = models.ForeignKey(Profil, on_delete=models.CASCADE)
topic = models.ForeignKey(Topic, on_delete=models.CASCADE)
choice_date = models.DateField()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

choice_date vs creation date use a timestamp model mixins to thandle it automatically in created field

raison = models.TextField(max_length=100)
Copy link

@ttgedeon ttgedeon Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"raison": always use English name to refer to field, variables, ... in code

start_date = models.DateField()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add field like last_opened for practical reasons

end_date = models.DateField(null=True)


class Group(models.Model):
designation = models.TextField(max_length=100)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider replacing designation by name

_description = models.TextField(max_length=100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A field name should not start with _

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the underscore on "_description"

type = models.BooleanField()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is a python's reserved word. You can propose aomething alse.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"type" field is meaningless

created_at = models.DateField()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The created_at field is intended to be used in what case ? Are you planning to write it value yourself everytime?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created_at: should be created, and automatically generated by a Timestamp mixin

members = models.ManyToManyField(Profil, through='Member')
private_key = models.TextField(max_length=100, null=True)

class Invitation(models.Model):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible foreign key conflit in this class to handle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is two Foreign key to the same class.

profil = models.ForeignKey(Profil, on_delete=models.CASCADE)
group = models.ForeignKey(Group, on_delete=models.CASCADE)
sujbject = models.ForeignKey(Profil, on_delete=models.CASCADE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ubdate "sujbject"

created_at = models.DateField()
etat = models.BooleanField(default=False)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider storing etat which might refer to state in a charfield with predefined choice list


class Member(models.Model):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider adding a state boolean on this class to track multiple times sbdy becomes or leaves his group privileges

group = models.ForeignKey(Group, on_delete=models.CASCADE)
profil = models.ForeignKey(Profil, on_delete=models.CASCADE)
is_admin = models.BooleanField(default=False)
is_owner = models.BooleanField(default=False)
start_date = models.DateField()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start_date should be datetime and handled by a model mixin

end_date = models.DateField(null=True)

class Content(models.Model):
Profil = models.ForeignKey(Profil, on_delete=models.CASCADE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class attributes don't start like this, refer to the PEP8 coding style.

Copy link

@ttgedeon ttgedeon Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1- you are using the "Profil" class name to define a field in an other class
2- No need to link content to Topic class
3- this class should store obvious common attributes between Post and comment leaving any specialized attribute to be defined in corresponding classes

topic = models.ForeignKey(Topic, on_delete=models.CASCADE)
title = models.TextField()
content = models.TextField()

class Meta:
abstract = True

class Post(Content):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1-a post should be able to store a static file

pass

class Comment(Content):
post = models.ForeignKey(Post, on_delete=models.CASCADE)
Copy link

@ttgedeon ttgedeon Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why linking comment to post? what about comment on a comment. review it


class Like(models.Model):
profil = models.ForeignKey(Profil, on_delete=models.CASCADE)
Content = models.ForeignKey(Profil, on_delete=models.CASCADE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are using "Content" class name to name a field on an other class

created_at = models.DateField()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created_at should be created, handled with a mixin automatically and must be a datetime


class share(models.Model):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Model name don't start like this. Refer to the PEP8 documentation for coding style guideline

sender = models.ForeignKey(Profil, on_delete=models.CASCADE)
recipient = models.ForeignKey(Profil, on_delete=models.CASCADE)
post = models.ForeignKey(Post, on_delete=models.CASCADE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two things can be shared: post & comment. handle this


class Notification(models.Model):
sender = models.ForeignKey(Profil, on_delete=models.CASCADE)
recipient = models.ForeignKey(Profil, on_delete=models.CASCADE)
created_at = models.DateField()
title = models.TextField(max_length=50)
message = models.TextField(max_length=100)




# Create your models here.
1 change: 0 additions & 1 deletion FriendsWave/social/views.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
from django.shortcuts import render

# Create your views here.