library management system created #1
Conversation
There was a problem hiding this comment.
Your code needs a lot of improvement and you have derailed from the task completely. You were suppose to write an API with django rest framework. Please refer to this documentation and refactor your code:
https://www.django-rest-framework.org/
Moreover, always remember to include environment specific files/folders, like __pychache__, in your .gitignore file.
| return_date = models.DateField() | ||
| returned = models.BooleanField(default=False) |
There was a problem hiding this comment.
Why do we have a return_date here when we have a separate table for returned books.
library_management/models.py
Outdated
| class BookRequest(models.Model): | ||
| REQUEST_STATUS_CHOICES = ( | ||
| ('Pending', 'Pending'), | ||
| ('Approved', 'Approved'), | ||
| ('Rejected', 'Rejected'), | ||
| ) | ||
| user = models.ForeignKey(User, on_delete=models.CASCADE) | ||
| book = models.ForeignKey(Book, on_delete=models.CASCADE) | ||
| request_date = models.DateTimeField(auto_now_add=True) | ||
| status = models.CharField( | ||
| max_length=20, choices=REQUEST_STATUS_CHOICES, default='Pending') | ||
| rejection_reason = models.TextField(blank=True, null=True) | ||
|
|
||
| def __str__(self): | ||
| return f'{self.user.username} requests {self.book.name}' |
There was a problem hiding this comment.
We can modify this table to store information about issued and returned books as well. This way we can get rid of BookReturn and BookIssue.
There was a problem hiding this comment.
Thanks for the feedback. I have updated the code.
library_management/permissions.py
Outdated
| if user.has_perm("library_management.delete_book"): | ||
| return False | ||
| if user.has_perm("library_management.change_book"): | ||
| return False |
There was a problem hiding this comment.
I don't understand If the user has permission why do we return false?
library_management/permissions.py
Outdated
| if user.has_perm("library_management.change_book"): | ||
| return True | ||
|
|
||
| if user.is_librarian == False: |
There was a problem hiding this comment.
If the above condition fails this condition would always be true. We can replace this redundant check with an else.
library_management/serializers.py
Outdated
| class BookSerializer(serializers.ModelSerializer): | ||
| class Meta: | ||
| model = Book | ||
| fields = '__all__' |
There was a problem hiding this comment.
Returning all fields is not a good practice because down the line if you need to add sensitive fields every related serializer would serialize that as well. You should list every field instead.
library_management/views.py
Outdated
|
|
||
| class BookRequestView(View): | ||
| template_name = 'book_request.html' | ||
|
|
||
| def get(self, request): | ||
| form = BookRequestForm() | ||
| return render(request, self.template_name, {'form': form}) | ||
|
|
There was a problem hiding this comment.
I don't see permissions being applied here. Shouldn't users and librarian have different permissions, even serializers for this modal?
There was a problem hiding this comment.
Yes, user and librarian have separate permissions. I have updated the code.Kindly provide feedback
rohan-saeed
force-pushed
the
library-management
branch
from
ee569ec to
b1f74c5
Compare
library/settings.py
Outdated
There was a problem hiding this comment.
Good work on using environment variables for email settings.
library_management/admin.py
Outdated
There was a problem hiding this comment.
# Register your models here
You should try to remove pre existing comments like this one
There was a problem hiding this comment.
Good use of choices but we can utilize classes for choices as well. For example like this:
class MyModel(models.Model): class Month(models.IntegerChoices): JAN = 1, "JANUARY" FEB = 2, "FEBRUARY" MAR = 3, "MAR" # (...) month = models.PositiveSmallIntegerField( choices=Month.choices, default=Month.JAN )
You can refer to the 2nd answer in this [link](https://stackoverflow.com/questions/18676156/how-to-properly-use-the-choices-field-option-in-django) for more info
library_management/cron.py
Outdated
There was a problem hiding this comment.
If we have a return_date field in models, why do we need to calculate it again?
There was a problem hiding this comment.
Good work on using inbuilt permission checks.
There was a problem hiding this comment.
I think you mixed up the read_only fields on LIbrarianIssueSerializer and UserIssueSerializer as the User should have all read_only fields and the Librarian should have access to all of them.
Same issue in LibrarianTicketSerializer and UserTicketSerializer.
There was a problem hiding this comment.
We only need to filter the queryset for the user when the user has a USER role. The librarian role should be able to see all the elements in the table.
Why are we using the BookIssue model in the BookTicketDetail view?
users/models.py
Outdated
There was a problem hiding this comment.
Good use of is_librarian field.
We can once again utilize a class for the choices.
users/forms.py
Outdated
There was a problem hiding this comment.
I think we need to shift to DRF for this one.
There was a problem hiding this comment.
Good work on the is created validation check.
No description provided.