Emily C - Spruce #52
Emily C - Spruce #52
Conversation
… for customers plus merge Cas's model updates
There was a problem hiding this comment.
Great work Cassie and Emily! Y'all met all the learning goals for this project and passed all the tests. Y'all did a great job in organizing your code and utilizing helper functions.
I provided feedback on how y'all can refactor by omitting redundant logic, using list comprehensions, and improving the quality of our classes.
Project grade: Green 🟢 !
| from .routes import videos_bp | ||
| from .routes_for_customers import customers_bp | ||
| from .routes_for_rentals import rentals_bp | ||
| app.register_blueprint(videos_bp) | ||
| app.register_blueprint(customers_bp) | ||
| app.register_blueprint(rentals_bp) |
| registered_at = db.Column(db.DateTime) | ||
| postal_code = db.Column(db.String) | ||
| phone = db.Column(db.String) |
There was a problem hiding this comment.
At the moment, any of these columns can contain null values. Consider revisiting each column to determine which column should truly have null values versus not. For example, is it useful to store a Customer without a name or phone number? Those two columns can be considered required information to store about a customer, despite our requirements not strictly describing that behavior.
| release_date = db.Column(db.DateTime) | ||
| total_inventory = db.Column(db.Integer) |
There was a problem hiding this comment.
Same comment about required information from the Customer class can also be applied to Video and Rental.
| from app.models.rental import Rental | ||
| out_count = Rental.query.filter_by(video_id = self.id).count() | ||
| available_inventory = self.total_inventory - out_count | ||
| return available_inventory |
There was a problem hiding this comment.
Rather than querying the database every time we need to update available_inventory, we can take advantage of the rental relationship attribute defined earlier in this class. self.rental will generate a list of rental instances containing a specific video (aka list of rental objects with a video_id of 2). Every time we check-out or check-in a video to/from a customer, the self.rental also updates to reflect the newest count of rentals for a video.
| def available_inventory(self): | |
| from app.models.rental import Rental | |
| out_count = Rental.query.filter_by(video_id = self.id).count() | |
| available_inventory = self.total_inventory - out_count | |
| return available_inventory | |
| out_count = len(self.rental) | |
| available_inventory = self.total_inventory - out_count | |
| return available_inventory |
There was a problem hiding this comment.
If y'all want to see the output of self.rental we can utilize print statements and the vars function to display the rental instance objects in dictionary form.
| def available_inventory(self): | |
| from app.models.rental import Rental | |
| out_count = Rental.query.filter_by(video_id = self.id).count() | |
| available_inventory = self.total_inventory - out_count | |
| return available_inventory | |
| def available_inventory(self): | |
| print(self.rental) | |
| for video in self.rental: | |
| print(vars(video)) | |
| out_count = Rental.query.filter_by(video_id = self.id).count() | |
| available_inventory = self.total_inventory - out_count | |
| return available_inventory |
|
|
||
| videos_bp = Blueprint("videos", __name__, url_prefix="/videos") | ||
|
|
||
| @videos_bp.route("", methods=["GET", "POST"]) |
There was a problem hiding this comment.
To follow the single responsibility principle, consider separating routes into their own functions.
| customers_response = [] | ||
| for customer in customers: | ||
| customers_response.append(customer.create_customer_dict()) |
There was a problem hiding this comment.
List comprehension would be handy here
|
|
||
|
|
||
|
|
||
| @customers_bp.route("/<customer_id>", methods=["GET", "DELETE", "PUT"]) |
| @customers_bp.route("/<customer_id>/rentals", methods=["GET"]) | ||
| def customers_current_rentals(customer_id): | ||
| customer = Customer.query.get(customer_id) | ||
| if not customer: | ||
| return_message = {"message": f"Customer {customer_id} was not found"} | ||
| return make_response(return_message, 404) | ||
|
|
||
| videos_checked_out =[] | ||
| rentals =Rental.query.filter_by(customer_id = customer.id).all() | ||
| for rental in rentals: | ||
| videos_checked_out.append(rental.create_dict()) | ||
| return jsonify(videos_checked_out), 200 |
There was a problem hiding this comment.
The Customer model has the relationship attribute rental which is a list of rental instances specific to a customer via customer_id. This data is actually retrieved when we query the Customer table on line 89, making the Rental query on line 95 redundant.
Instead, we can iterate through customer.rental and receive the same outcome with less database queries.
| @customers_bp.route("/<customer_id>/rentals", methods=["GET"]) | |
| def customers_current_rentals(customer_id): | |
| customer = Customer.query.get(customer_id) | |
| if not customer: | |
| return_message = {"message": f"Customer {customer_id} was not found"} | |
| return make_response(return_message, 404) | |
| videos_checked_out =[] | |
| rentals =Rental.query.filter_by(customer_id = customer.id).all() | |
| for rental in rentals: | |
| videos_checked_out.append(rental.create_dict()) | |
| return jsonify(videos_checked_out), 200 | |
| @customers_bp.route("/<customer_id>/rentals", methods=["GET"]) | |
| def customers_current_rentals(customer_id): | |
| customer = Customer.query.get(customer_id) | |
| if not customer: | |
| return_message = {"message": f"Customer {customer_id} was not found"} | |
| return make_response(return_message, 404) | |
| videos_checked_out =[] | |
| for rental in customer.rental: | |
| videos_checked_out.append(rental.create_dict()) | |
| return jsonify(videos_checked_out), 200 |
| if not customer: | ||
| return make_response({"message":"Could not perform checkout"}, 404) | ||
| elif not video: | ||
| return make_response({"message":"Could not perform checkout"}, 404) |
There was a problem hiding this comment.
We can combine these conditionals:
| if not customer: | |
| return make_response({"message":"Could not perform checkout"}, 404) | |
| elif not video: | |
| return make_response({"message":"Could not perform checkout"}, 404) | |
| if not customer or not video: | |
| return make_response({"message":"Could not perform checkout"}, 404) |
| return make_response(new_rental.create_dict(), 200) | ||
|
|
||
|
|
||
| @rentals_bp.route("/check-in", methods=["POST"]) |
No description provided.