Scissors - Araceli #60
Conversation
|
|
||
| if "name" not in request_body.keys() or "postal_code" not in request_body.keys() or "phone" not in request_body.keys(): | ||
|
|
||
| return jsonify({"Error": "Enter info for all fields"}), 400 |
| if customers is None: | ||
| return 404 |
There was a problem hiding this comment.
If there are no customers, query.all() will return an empty list, unlike query.get() which does return None. This check will always fail, if you want to return an error message in the case of no customers, the conditional needs to check for an empty list or falsey.
| "id": self.id, | ||
| "title": self.title, | ||
| "release_date": self.release_date, | ||
| "total_inventory": self.total_inventory |
There was a problem hiding this comment.
Small bug here, "available_inventory" should be part of the json output for a video.
| return jsonify({"id": customer.id}), 200 | ||
|
|
||
| else: | ||
| return jsonify({"Error": f"Customer {customer_id} does not exist"}), 404 |
There was a problem hiding this comment.
Line 49 and this line are doing the same work, but the error messages are different. Consider pulling this out into a helper function to make it easier to create consistent error messages. The Flask abort function might be useful in this case, it makes it possible to return a response directly from a helper function.
| results = db.session.query(Customer, Video, Rental)\ | ||
| .join(Customer, Customer.id==Rental.customer_id)\ | ||
| .join(Video, Video.id==Rental.video_id)\ | ||
| .filter(Customer.id==id).all() |
| video = Video.query.get(rental.video_id) | ||
|
|
||
| # if customer and video exist | ||
| if customer: |
There was a problem hiding this comment.
This only checks to see if the customer exists. If the video does not exist, line 41 will raise an exception.
| video = Video.query.get(rental.video_id) | ||
|
|
||
| # if customer exists and is valid | ||
| if customer and customer.videos_checked_out >= 0: |
There was a problem hiding this comment.
Checking for customer.videos_checked_out >= 0 will allow customer.videos_checked_out to be set to -1.
|
|
||
| # add it to database and commit | ||
| db.session.commit() | ||
|
|
There was a problem hiding this comment.
How are you recording that the rental has ended? Consider this sequence (customer has 0 videos, video1 has inventory 1, video 2 has inventory 1:
- A customer checks out 2 videos (customer.videos_checked_out = 2, video1.total_inventory = 0, video2.total_inventory = 0)
- Check in with customer & video1 (customer.videos_checked_out = 1, video1.total_inventory = 1)
- Check in with customer & video1: This should return 400 as this customer does not have this video checked out anymore. However, it does not - customer.videos_checked_out will be set to 0 and video1.total_inventory will be set to 2, and the API will return a status of 200. This will set up a situation in the DB where the total inventory has gone up, and the customer has 0 videos checked out, both of which are incorrect.
| customer.videos_checked_out += 1 | ||
|
|
||
| # decrease videos.total_inventory by 1 | ||
| if video.total_inventory > 0: |
There was a problem hiding this comment.
The column total_inventory not change when doing rentals, as this is how many copies the video store owns. There are two ways to check to see if a rental is possible - either use the available_inventory column from models/video.py line 11, or calculate it based on how many rentals have this video id.
| __tablename__ = "customer" | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| name = db.Column(db.String) | ||
| postal_code = db.Column(db.Integer) |
There was a problem hiding this comment.
This solution is fine but it causes the code to fail 2 smoke tests (there was inconsistency around the postal_code in the tests).
|
Great work on this project! The code is very clean and readable. There are some bugs with rentals, but the rest of the functionality is solid. This project is green, you hit all of the learning goals. |
No description provided.