Completed all tasks #15
Conversation
| username = request.POST['username'] | ||
| password = request.POST['password'] |
There was a problem hiding this comment.
You are directly accessing POST data without checking if it even exists. This may lead to server crash if a user access this endpoint with invalid request data. The good behavior would have been to throw a client error (400), rather than server error (500).
| class Rating(models.Model): | ||
| book_id = models.IntegerField() | ||
| user_id = models.IntegerField() | ||
| rating = models.CharField(max_length=10,default='0') |
There was a problem hiding this comment.
It is better to use ForeignKey relation, rather than directly storing the ids. Also, store the rating as an integer and add validators to that, so as to ensure that it lies between 0 and 10.
| } | ||
| template_name = 'store/book_detail.html' | ||
| context['book'] = Book.objects.get(id=bid) |
There was a problem hiding this comment.
This may fail with invalid book ID given in POST request, and would lead to server error. Expected behavior is to inform user with Not found (404) error.
| } | ||
| template_name = 'store/book_detail.html' | ||
| context['book'] = Book.objects.get(id=bid) | ||
| context['num_available']=BookCopy.objects.filter(book_id=bid).filter(status=False).count() |
There was a problem hiding this comment.
You should have read the comment in the models.py file.
status=False denotes that the book isn't available, whereas status=True denotes that it is available.
| if prating == '-1': | ||
| rate = Rating.objects.create(book_id=bookid,user_id=userid,rating=rating) | ||
| response_data['message'] = "success" | ||
| else : | ||
| rate=Rating.objects.filter(user_id=userid).filter(book_id=bookid).first() | ||
| rate.rating = rating | ||
| rate.save() | ||
| response_data['message'] = "success" | ||
| book=Book.objects.get(id=bookid) |
There was a problem hiding this comment.
Check how you could have used get_or_create() here. However, this is fine!
| l=Rating.objects.filter(book_id=bookid).count() | ||
| for i in range(l): | ||
| rate=Rating.objects.filter(book_id=bookid)[i] | ||
| sum = sum + int(rate.rating) |
There was a problem hiding this comment.
This is not a good way to call ORM queries. This will run two queries on the database.
You could have done it this way:
for rate in Rating.objects.filter(book_id=bookid):
sum = sum + int(rate.rating)| rating =data.get('rating') | ||
| bookid = data.get('bid') | ||
| print(rating) | ||
| if Rating.objects.filter(user_id=userid).filter(book_id=bookid).count() == 1: | ||
| prating = Rating.objects.filter(user_id=userid).filter(book_id=bookid).first().rating | ||
| print('rating') | ||
| if prating == '-1': | ||
| rate = Rating.objects.create(book_id=bookid,user_id=userid,rating=rating) | ||
| response_data['message'] = "success" | ||
| else : | ||
| rate=Rating.objects.filter(user_id=userid).filter(book_id=bookid).first() | ||
| rate.rating = rating |
There was a problem hiding this comment.
You've not put a backend validation on the rating, so the user can simply edit the JS code you've written in the template and easily put invalid values of rating.
| data = request.POST | ||
| copybook_id = data.get('bid') | ||
| if copybook_id is not None: | ||
| copybook = BookCopy.objects.get(id=copybook_id) | ||
| copybook.borrow_date = None | ||
| copybook.borrower_id = None | ||
| copybook.status = False | ||
| copybook.save() |
There was a problem hiding this comment.
There must be a validation in the backend when a user is returning the book, to make sure that he has only borrowed the book. Otherwise, a simple POST request will make the BookCopy to be returned, and would set its status as True.
|
Points have been updated! 🎉 |
CSoC Task 2 Submission
I have completed the following tasks