CSoC Task 2 Submission#7
Conversation
krashish8
left a comment
krashish8
left a comment
There was a problem hiding this comment.
Great work on the assignment @shubhanshu02! I loved the originality of your submission. Will update the points later!
| </div> | ||
| <div class="col-sm-6"> | ||
| <label class="control-label" for="first_name">First Name*:</label> | ||
| <input type="first_name" name="first_name" id="first_name" placeholder="Password"> |
There was a problem hiding this comment.
placeholder shall not be "Password"
| <div class="col-sm-9"> | ||
| <label class="control-label" for="password">Password*:</label> | ||
| <input type="password" name="password" id="password" placeholder="Password"> | ||
| Password should be atleast 8 characters long | ||
| </div> |
There was a problem hiding this comment.
It is preferable to let the user enter the password twice, for obvious reasons. However, this is fine for this assignment.
| 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).
| # Create a new user | ||
| new = User(username= query[0]) | ||
| # Set password | ||
| new.set_password(query[1]) | ||
| # Set the name of the user | ||
| new.first_name = query[2] | ||
| new.last_name = query[3] | ||
| # Save the User model object | ||
| new.save() |
There was a problem hiding this comment.
You could have directly used create_user(), however, this is fine!
| book = models.ForeignKey(Book, null=True, on_delete=models.SET_NULL) | ||
| user = models.ForeignKey(User, null=True, on_delete=models.SET_NULL) |
There was a problem hiding this comment.
The user and book 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.
| totalRatings = UserRating.objects.filter(book = book) | ||
| # check if user is logged in | ||
| # and rating objects are present for the book | ||
| if request.user.is_authenticated and totalRatings.count() > 0: | ||
| myRating = totalRatings.filter(user_id = request.user.id).first() | ||
| # If user rating is found | ||
| if myRating != None: | ||
| givenRating = myRating.rating | ||
| # Else the rating for the book by the user is None | ||
| else: | ||
| givenRating = None | ||
|
|
||
|
|
||
| context = { | ||
| 'book': book, | ||
| 'num_available': available_copies, | ||
| 'usrating': givenRating, |
There was a problem hiding this comment.
Consider the case when another user has rated the book, and the current user hasn't done so. Then the local variable givenRating won't be assigned any value, which will raise an error.
| for i in Book.objects.all(): | ||
| avail.append(i.id) | ||
|
|
||
| # If the id in the request is in the list, | ||
| if bid in avail: | ||
| book = Book.objects.filter(id = bid).first() |
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, one to append and the other to filter.
| book_id = request.POST['bid'] | ||
| value = request.POST['value'] |
There was a problem hiding this comment.
You've not put a backend validation on the rating, so the user can easily put invalid values of rating.
| if book != None: | ||
| # Set status to available | ||
| book.status = True | ||
| # Set borrower to none | ||
| book.borrower = None | ||
| # Set borrow date to none | ||
| book.borrow_date = None | ||
| # Save the object | ||
| book.save() | ||
| msg = 'success' |
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.
|
Also, make sure to run |
|
Points updated! 🎉 |
|
Thanks, @krashish8 for having a look at my PR. I have noted your suggestions and update them in my fork of the submission. |
Bumps [django](https://github.com/django/django) from 2.2.1 to 2.2.13. - [Release notes](https://github.com/django/django/releases) - [Commits](django/django@2.2.1...2.2.13) Signed-off-by: dependabot[bot] <support@github.com>
Bump django from 2.2.1 to 2.2.13
Updated Packages
2 participants
CSoC Task 2 Submission
I have completed the following tasks.