-
Notifications
You must be signed in to change notification settings - Fork 27
Newsletters models and migration files added. #233
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
base: newsletters
Are you sure you want to change the base?
Conversation
…sletters-models
newsletters/models.py
Outdated
return "%s" % (self.department) | ||
|
||
|
||
class papers(models.Model): |
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.
Please change it to Newsletter.
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 sir.
newsletters/models.py
Outdated
''' | ||
Stores the pdfs of newsletters | ||
''' | ||
name = models.CharField(null=False, |
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.
Change name to title
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 sir. will change this.
newsletters/models.py
Outdated
''' | ||
name = models.CharField(null=False, | ||
max_length=25) | ||
uploaded_by = models.ForeignKey(User) |
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 think this field must be linked to FacultyDetail
model.
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.
Yes sir, I did not notice this existing model. Will update this.
newsletters/models.py
Outdated
from django.contrib.auth.models import User | ||
|
||
|
||
class department(models.Model): |
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.
@dhruvhacks Is there a need of creating this model ? I think only faculties can upload the newsletters and you can find their department in the FacultyDetail table. Please let me know your thoughts over it.
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.
@RishabhJain2018
Sir, while developing this feature, I created this model since every department will have multiple newsletters. Having department
as an attribute within newsletters
model will make redundant entries within it. A separate model will help more organised structure where all newsletters will be stored within one model linked to their respective departments.
Please tell if there is an alternative. 🙂
newsletters/models.py
Outdated
year = models.DateField() | ||
paper = models.FileField(upload_to="newsletters", blank=False, null=False) | ||
department = models.ForeignKey(department) | ||
|
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.
Please add created_by
and modified_by
fields.
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.
@RishabhJain2018 uploaded_by
attribute already exists in the model which will be linked to FacultyDetail
model. So should I still add created_by
attribute ??
Moreover, we may add created_at
and modified_at
attributes which may help in tracking the changes.
e6a3a42
to
ff36cb7
Compare
''' | ||
department = models.CharField(max_length=50, | ||
blank=False, | ||
null=False) |
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 don't think you need this explicitly.
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 sir. I'll merge this into newsletters model itself.
Stores the detail of department linked with its newsletters | ||
''' | ||
department = models.CharField(max_length=50, | ||
blank=False, |
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 don't think you need this explicitly.
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 sir. I'll shift this into newsletters model itself.
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.
@deshraj
Sir, I revised the model and felt we must keep it separate for easy management of newsletters. While designing this model I categorized all the newsletters into their respective departments. By having a separate model for department, we can easily link all the newsletters to a common department. Making it as an attribute in one model will dissolve the categories.
This categorization can be viewed in django admin-
Below screenshot displays all the departments which lists all the newsletters of that department.
Under any department, all it's newsletters can be managed this way-
Please sir let me know your thoughts over it. I'll make the changes accordingly. 🙂
Stores the pdfs of newsletters | ||
''' | ||
title = models.CharField(null=False, | ||
max_length=25) |
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.
Shift this to the previous line.
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 sir.
''' | ||
title = models.CharField(null=False, | ||
max_length=25) | ||
uploaded_by = models.ForeignKey(User) |
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.
Add related_model
attribute here.
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 sir. Will add this.
uploaded_by = models.ForeignKey(User) | ||
year = models.DateField() | ||
paper = models.FileField(upload_to="newsletters", | ||
blank=False, |
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.
Remove this
year = models.DateField() | ||
paper = models.FileField(upload_to="newsletters", | ||
blank=False, | ||
null=False) |
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.
Remove this.
created_at = models.DateTimeField() | ||
modified_at = models.DateTimeField() | ||
|
||
def create_newsletter(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.
Can you please tell me the use case of this method?
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.
Sir, I had in mind of writing some test cases for this model but did not frame them. Will remove it for now and add this in a separate PR with test cases.
This PR contains the models and migration files for the infoconnect-newsletters section as developed in #232 .