initial commit . All Tasks Done #16
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).
| password=password1, | ||
| ) | ||
| user.save() | ||
| except : |
There was a problem hiding this comment.
As a good coding practice, whenever you use try-except block, capture only the exceptions which you want to catch (IndexError, IntegrityError, etc.)
| l= BookRating.objects.filter(book_id=bookid).count() | ||
| for i in range(l): | ||
| rate=BookRating.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)| @@ -12,58 +15,71 @@ def index(request): | |||
|
|
|||
| def bookDetailView(request, bid): | |||
| template_name = 'store/book_detail.html' | |||
| book1=Book.objects.get(pk=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. You have imported get_object_or_404 but haven't used that.
| book_id = request.POST.get("bid") | ||
| book = BookCopy.objects.get(pk=book_id) | ||
| if(book): | ||
| message ="success" | ||
| book.status = True | ||
| book.borrower = None | ||
| book.borrow_date = None | ||
| book.save() | ||
| else : | ||
| message = "failure" |
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.
| def userBookRating(request): | ||
| response_data = { | ||
| 'message': "failure", | ||
| 'rating' : 0 | ||
| } | ||
| prating = '0' | ||
| userid=request.user.id | ||
| data = request.POST | ||
| rating =data.get('rating') | ||
| bookid = data.get('bid') | ||
| print(rating) | ||
| if BookRating.objects.filter(user_id=userid).filter(book_id=bookid).count() == 1: | ||
| prating = BookRating.objects.filter(user_id=userid).filter(book_id=bookid).first().rating | ||
| print('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.
| value= BookRating.objects.filter(book__exact=book1,user__exact=request.user) | ||
| user_rating=0 | ||
| if(value.count()>0): | ||
| user_rating=BookRating.objects.filter(book__exact=book1,user__exact=request.user).get().rating |
There was a problem hiding this comment.
This is not a good way to call ORM queries. This will run two queries on database. You could have directly done the following:
user_rating = value.get().rating
| class BookRating(models.Model): | ||
| book = models.ForeignKey(Book, on_delete=models.CASCADE) | ||
| user = models.ForeignKey(User, related_name='user',null=True,blank=True,on_delete=models.SET_NULL) | ||
| rating = models.FloatField(default=0.0) |
There was a problem hiding this comment.
The rating shall be given as an integer - please read proper instructions.
The user should not be null here, and a better option would be to use on_delete=models.CASCADE
You could have also used unique_together META option here.
|
Points have been updated! 🎉 |
CSoC Task 2 Submission
I have completed the following tasks