Asya & Nourhan - Sprucies #49
Conversation
There was a problem hiding this comment.
Great work Asya and Nourhan! What a great team! Y'all met all the learning goals for this project and passed all the tests. Good work in organizing this project by having the routes in separate files.
I provided feedback on how y'all can refactor by omitting redundant logic, using helper methods, and list comprehension.
Y'all earned a 🟢 Green 🟢 !
|
|
||
|
|
There was a problem hiding this comment.
I don't think adding changes to this file was intentional. Remember that git add . will add files with changes to be staged ( the . representing 'all' ). Whenever a file is accidentally added into staging, we can remove it from the current batch of changes using the command git rm path/of/file. Always use git status to check which files you're actually staging to be committed!
| customer_id = db.Column(db.Integer, primary_key=True, autoincrement = True) |
There was a problem hiding this comment.
Integer primary key columns will auto-increment by default so it's not required to provide that setting.
We can read more about the autoincrement attribute in the SQL_Alchemy documentation
| postal_code = db.Column(db.String) | ||
| phone = db.Column(db.String) | ||
| registered_at = db.Column(db.DateTime, nullable = True) |
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.
| customer_id = db.Column(db.Integer, db.ForeignKey('customer.customer_id'), primary_key=True,nullable=True) | ||
| video_id = db.Column(db.Integer, db.ForeignKey('video.video_id'), primary_key=True,nullable=True) | ||
| due_date = db.Column(db.DateTime) | ||
| videos_checked_out_count = db.Column(db.Integer) | ||
| available_inventory = db.Column(db.Integer) | ||
| videos_checked_in = db.Column(db.Boolean, default=False) |
There was a problem hiding this comment.
Comments from Customer class can be applied here.
| phone = db.Column(db.String) | ||
| registered_at = db.Column(db.DateTime, nullable = True) | ||
| videos = db.relationship("Video", secondary="rental", backref="customers") |
There was a problem hiding this comment.
To decrease repetition in the customer_routes file, we can include a helper function to return the data about an instance in a dictionary.
Example:
def to_dict(self):
return {
"id" : self.customer_id,
"name": self.name,
"phone": self.phone,
"postal_code": self.postal_code
}
| @videos_bp.route("", methods=["GET"]) | ||
| def get_videos(): | ||
| videos = Video.query.all() | ||
| video_response = [] | ||
|
|
||
| for video in videos: | ||
| video_response.append({ | ||
| "id": video.video_id, | ||
| "title": video.title, | ||
| "release_date": video.release_date, | ||
| "total_inventory": video.total_inventory | ||
| }) | ||
| return jsonify(video_response) |
There was a problem hiding this comment.
If there was a helper function to return video data as a dictionary, we could use list comprehension to build the video_response
| @videos_bp.route("", methods=["GET"]) | |
| def get_videos(): | |
| videos = Video.query.all() | |
| video_response = [] | |
| for video in videos: | |
| video_response.append({ | |
| "id": video.video_id, | |
| "title": video.title, | |
| "release_date": video.release_date, | |
| "total_inventory": video.total_inventory | |
| }) | |
| return jsonify(video_response) | |
| @videos_bp.route("", methods=["GET"]) | |
| def get_videos(): | |
| videos = Video.query.all() | |
| video_response = [ video.to_dict() for video in videos ] | |
| return jsonify(video_response) |
|
|
||
|
|
||
| @videos_bp.route("/<video_id>", methods=["GET"]) | ||
| def get_one_video(video_id): |
| return jsonify(response_body), 200 | ||
|
|
||
|
|
||
| @videos_bp.route("/<video_id>", methods=["PUT"]) |
| return jsonify(response_body), 200 | ||
|
|
||
|
|
||
| @videos_bp.route("/<video_id>", methods=["DELETE"]) |
| return {"id": video.video_id} | ||
|
|
||
|
|
||
| @videos_bp.route("/<video_id>/rentals", methods=["GET"]) |
There was a problem hiding this comment.
👍 Good work! This route was tricky.
No description provided.