-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spruce - Gabe and Asha #40
base: master
Are you sure you want to change the base?
Changes from all commits
fc7092b
87dad42
1240245
96da8ef
85b6d8e
1f60559
25d46ff
c6a12f2
d866713
0e135b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,26 @@ | ||
from app import db | ||
|
||
|
||
class Customer(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
id = db.Column(db.Integer, primary_key=True) | ||
name = db.Column(db.String) | ||
registered_at = db.Column(db.DateTime) | ||
postal_code = db.Column(db.String) | ||
phone = db.Column(db.String) | ||
Comment on lines
+6
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep in mind that the default value for |
||
videos = db.relationship("Video", secondary="rental", backref="customer") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice that your code doesn't use this relationship in the logic anywhere. Using the However, in this application, our rental relationship between videos and customers isn't a pure join table. It has its own information that also needs to be tracked, and which SQLAlchemy can't automatically manage (such as the checked in status, and due date). In this case, using reciprocal one-to-many relationships can bring us some of the benefits of moving around the relationship links, while still leaving it to us to explicitly set up the rental model ourselves. Consider a configuration like the following: rentals = db.relationship("Rental", backref="customer") If we had a customer model in the variable |
||
|
||
def to_json(self): | ||
return { | ||
"id": self.id, | ||
"name": self.name, | ||
"registered_at": self.registered_at, | ||
"postal_code": self.postal_code, | ||
"phone": self.phone | ||
} | ||
|
||
def rental_dict(self, rental): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love seeing multiple rendering methods. Getting as much of this kind of logic out of the routes themselves is great! And here, the majority of the data we need to render comes from the customer, so homing the method in the Customer model is a reasonable choice. For systems that need to render a variety of outputs (and this project actually has a surprisingly large number of outputs), there can be a large number of custom renderers, and we might want to avoid cluttering up our models with a lot of rendering functions, and sometimes we might need values from several disparate locations. In that case, we might make extra classes whose entire responsibility is knowing how to render a certain output from the supplied data. |
||
return { | ||
"due_date": rental.due_date, | ||
"name": self.name, | ||
"phone": self.phone, | ||
"postal_code": self.postal_code} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,10 @@ | ||
from app import db | ||
|
||
""" | ||
A Customer can have many Videos through a secondary table (i.e. Rentals), and a Video can be checked out/in by many Customers through the same Rentals table. | ||
""" | ||
class Rental(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
customer_id = db.Column(db.Integer, db.ForeignKey('customer.id'), primary_key=True, nullable=False) | ||
video_id = db.Column(db.Integer, db.ForeignKey('video.id'), primary_key=True, nullable=False) | ||
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I generally agree with your decision here to make the foreign keys not nullable. A rental without a customer or video is of questionable value. However, when a row referenced by a foreign key is deleted, postgres attempts to resolve the lost data by setting those foreign keys to NULL, which this constraint prevents, and would cause the final tests (deleting videos and customers in a rental) to fail. In this case, the secondary relationship is what is allowing those tests to pass, as it causes SQLAlchemy to delete the rows in the secondary table that depend on a deleted foreign key. This may be what we want, but on the other hand, data is very valuable to a company. Just be sure you have considered the alternatives and are intentionally choosing an approach after weighing your options. |
||
due_date = db.Column(db.DateTime) | ||
checked_out = db.Column(db.Boolean, default=False) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,23 @@ | ||
from app import db | ||
|
||
|
||
class Video(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
id = db.Column(db.Integer, primary_key=True) | ||
title = db.Column(db.String) | ||
release_date = db.Column(db.DateTime) | ||
total_inventory = db.Column(db.Integer) | ||
|
||
def to_json(self): | ||
return { | ||
"id": self.id, | ||
"title": self.title, | ||
"release_date": self.release_date, | ||
"total_inventory": self.total_inventory | ||
} | ||
|
||
def rental_dict(self, rental): | ||
return { | ||
"release_date": self.release_date, | ||
"title": self.title, | ||
"due_date": rental.due_date | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,269 @@ | ||
from app import db | ||
from flask import Blueprint | ||
from app.models.customer import Customer | ||
from app.models.video import Video | ||
from app.models.rental import Rental | ||
from flask import Blueprint, jsonify, request, make_response | ||
from datetime import datetime, timedelta | ||
|
||
|
||
customers_bp = Blueprint("customers", __name__, url_prefix="/customers") | ||
videos_bp = Blueprint("videos", __name__, url_prefix="/videos") | ||
rentals_bp = Blueprint("rentals", __name__, url_prefix="/rentals") | ||
# -------------------------------- | ||
# ----------- CUSTOMERS ---------- | ||
# -------------------------------- | ||
|
||
|
||
# GET ALL CUSTOMERS | ||
@customers_bp.route("", methods=["GET"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice job splitting your endpoints into separate functions. There are still shared pieces of functionality in some of the endpoints that we can think about trying to reduce duplication (especially looking up a record by its id), but on the plus side, in each function, we only need to think about the logic related to that specific endpoint. |
||
def get_customers(): | ||
customers = Customer.query.all() | ||
customers_response = [] | ||
for customer in customers: | ||
customers_response.append(customer.to_json()) | ||
Comment on lines
+22
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches our pattern of being a good candidate for a list comprehension. When we have result = []
for value in values:
result.append(value.some_method()) We can write this as result = [value.some_method() for value in values] So here we could write customers_response = [customer.to_json() for customer in customers] |
||
return jsonify(customers_response), 200 | ||
|
||
|
||
# CREATE CUSTOMER | ||
@customers_bp.route("", methods=["POST"]) | ||
def create_customer(): | ||
request_body = request.get_json() | ||
if "name" not in request_body: | ||
return jsonify({"details": "Request body must include name."}), 400 | ||
elif "postal_code" not in request_body: | ||
return jsonify({"details": "Request body must include postal_code."}), 400 | ||
elif "phone" not in request_body: | ||
return jsonify({"details": "Request body must include phone."}), 400 | ||
Comment on lines
+32
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the outcome of any of these conditions being true is for the function to exit, these could all be |
||
new_customer = Customer(name=request_body["name"], | ||
postal_code=request_body["postal_code"], | ||
phone=request_body["phone"]) | ||
db.session.add(new_customer) | ||
db.session.commit() | ||
return jsonify(new_customer.to_json()), 201 | ||
|
||
|
||
# GET CUSTOMER BY ID | ||
@customers_bp.route("/<id>", methods=["GET"]) | ||
def get_customer(id): | ||
if id.isnumeric() != True: | ||
return {"message": "Customer id provided is not a number."}, 400 | ||
Comment on lines
+49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The most dependable way to check whether a python string represents an integral value is to try to convert it to an int in a try block, then if it raises a ValueError, we know it wasn't an int, so here, that could look like: try:
int(id)
except ValueError:
return {"message": "Customer id provided is not a number."}, 400 The string methods like |
||
customer = Customer.query.get(id) | ||
if customer is None: | ||
return {"message": f"Customer {id} was not found"}, 404 | ||
else: | ||
return jsonify(customer.to_json()), 200 | ||
|
||
|
||
# UPDATE CUSTOMER BY ID | ||
@customers_bp.route("/<id>", methods=["PUT"]) | ||
def update_customer(id): | ||
customer = Customer.query.get(id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice that we should be checking the validity of the id when looking up a model in all of our endpoints. If the client passed in an id to this endpoint like "hello", this code would crash. |
||
if customer is None: | ||
return {"message": f"Customer {id} was not found"}, 404 | ||
if request.method == "PUT": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this endpoint is only registered for the PUT verb, there's no need to check the method of the request. If it weren't this verb, this method wouldn't have been called. |
||
request_body = request.get_json() | ||
if "name" not in request_body: | ||
return {"details": "Request body must include name."}, 400 | ||
elif "postal_code" not in request_body: | ||
return {"details": "Request body must include postal_code."}, 400 | ||
elif "phone" not in request_body: | ||
return {"details": "Request body must include phone."}, 400 | ||
Comment on lines
+66
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice how similar this checking is to the POST version. We could think about making a helper method to share this logic in both cases. |
||
customer.name = request_body["name"] | ||
customer.postal_code = request_body["postal_code"] | ||
customer.phone = request_body["phone"] | ||
db.session.commit() | ||
return jsonify(customer.to_json()), 200 | ||
|
||
|
||
# DELETE CUSTOMER BY ID | ||
@customers_bp.route("/<id>", methods=["DELETE"]) | ||
def delete_customer(id): | ||
customer = Customer.query.get(id) | ||
if customer is None: | ||
return jsonify({"message": (f"Customer {id} was not found")}), 404 | ||
else: | ||
db.session.delete(customer) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to reiterate, the deletion of a customer that is being used in a rental is also going to delete any rental record that references this customer due to the secondary relationship that was configured between video and customer using the rental table. Without that relationship, once a customer had been involved in a rental (currently checked out or not), it would become impossible to delete the customer from the database since the foreign key relationships in rental were set to be non-nullable. |
||
db.session.commit() | ||
return jsonify({"id": customer.id}), 200 | ||
|
||
# -------------------------------- | ||
# ----------- VIDEOS ---------- | ||
# -------------------------------- | ||
Comment on lines
+90
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The basic CRUD operations for videos are largely the same as for customers, so look for places where the feedback on customer endpoints could apply to video endpoints as well. |
||
|
||
|
||
# GET ALL VIDEOS | ||
@videos_bp.route("", methods=["GET"]) | ||
def get_videos(): | ||
videos = Video.query.all() | ||
videos_response = [] | ||
for video in videos: | ||
videos_response.append(video.to_json()) | ||
return jsonify(videos_response), 200 | ||
|
||
|
||
# CREATE VIDEO | ||
@videos_bp.route("", methods=["POST"]) | ||
def create_video(): | ||
if request.method == "POST": | ||
request_body = request.get_json() | ||
if "title" not in request_body: | ||
return {"details": "Request body must include title."}, 400 | ||
elif "release_date" not in request_body: | ||
return {"details": "Request body must include release_date."}, 400 | ||
elif "total_inventory" not in request_body: | ||
return {"details": "Request body must include total_inventory."}, 400 | ||
new_video = Video(title=request_body["title"], | ||
release_date=request_body["release_date"], | ||
total_inventory=request_body["total_inventory"]) | ||
db.session.add(new_video) | ||
db.session.commit() | ||
return jsonify(new_video.to_json()), 201 | ||
|
||
|
||
# GET VIDEO BY ID | ||
@videos_bp.route("/<id>", methods=["GET"]) | ||
def get_video(id): | ||
if id.isnumeric() != True: | ||
return {"message": "Video id provided is not a number."}, 400 | ||
video = Video.query.get(id) | ||
if video is None: | ||
return {"message": f"Video {id} was not found"}, 404 | ||
if request.method == "GET": | ||
return jsonify(video.to_json()), 200 | ||
|
||
|
||
# UPDATE VIDEO BY ID | ||
@videos_bp.route("/<id>", methods=["PUT"]) | ||
def update_video(id): | ||
video = Video.query.get(id) | ||
if video is None: | ||
return {"message": f"Video {id} was not found"}, 404 | ||
if request.method == "PUT": | ||
request_body = request.get_json() | ||
if "title" not in request_body: | ||
return {"details": "Request body must include title."}, 400 | ||
elif "release_date" not in request_body: | ||
return {"details": "Request body must include release_date."}, 400 | ||
elif "total_inventory" not in request_body: | ||
return {"details": "Request body must include total_inventory."}, 400 | ||
video.title = request_body["title"] | ||
video.request_body = request_body["release_date"] | ||
video.total_inventory = request_body["total_inventory"] | ||
db.session.commit() | ||
return jsonify(video.to_json()), 200 | ||
|
||
|
||
# DELETE VIDEO BY ID | ||
@videos_bp.route("/<id>", methods=["DELETE"]) | ||
def delete_video(id): | ||
video = Video.query.get(id) | ||
if video is None: | ||
return {"message": f"Video {id} was not found"}, 404 | ||
db.session.delete(video) | ||
db.session.commit() | ||
return {"id": video.id}, 200 | ||
|
||
|
||
# -------------------------------- | ||
# ----------- RENTALS ---------- | ||
# -------------------------------- | ||
|
||
|
||
# CHECK OUT A VIDEO TO A CUSTOMER | ||
@rentals_bp.route("/check-out", methods=["POST"]) | ||
def checkout_video(): | ||
request_body = request.get_json() | ||
if "customer_id" not in request_body: | ||
return {"details": "Request body must include customer_id."}, 400 | ||
elif "video_id" not in request_body: | ||
return {"details": "Request body must include video_id."}, 400 | ||
customer_id = request_body["customer_id"] | ||
video_id = request_body["video_id"] | ||
customer = Customer.query.get(customer_id) | ||
video = Video.query.get(video_id) | ||
Comment on lines
+183
to
+184
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even here, we should ensure that the ids are of the proper format. With the number of places this is required, a helper class method in the model types would be helpful. |
||
# check if customer and video exist | ||
if customer is None or video is None: | ||
return make_response("", 404) | ||
num_of_videos_rented = Rental.query.filter_by( | ||
video_id=video.id, checked_out=True).count() | ||
available_inventory = video.total_inventory - num_of_videos_rented | ||
Comment on lines
+188
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving the logic for calculating the checked out count and available inventory to helper methods in the Rental model. |
||
if available_inventory == 0: | ||
return jsonify({"message": f"Could not perform checkout"}), 400 | ||
new_rental = Rental(customer_id=customer.id, | ||
video_id=video.id, | ||
due_date=(datetime.now() + timedelta(days=7)), | ||
checked_out=True) | ||
db.session.add(new_rental) | ||
db.session.commit() | ||
videos_checkout_by_customer = Rental.query.filter_by( | ||
customer_id=customer.id, checked_out=True).count() | ||
available_inventory -= 1 | ||
return jsonify({ | ||
"customer_id": new_rental.customer_id, | ||
"video_id": new_rental.video_id, | ||
"due_date": new_rental.due_date, | ||
"videos_checked_out_count": videos_checkout_by_customer, "available_inventory": available_inventory}), 200 | ||
|
||
|
||
# CHECK IN A VIDEO TO A CUSTOMER | ||
@rentals_bp.route("/check-in", methods=["POST"]) | ||
def checkin_video(): | ||
request_body = request.get_json() | ||
if "customer_id" not in request_body: | ||
return {"details": "Request body must include customer_id."}, 400 | ||
elif "video_id" not in request_body: | ||
return {"details": "Request body must include video_id."}, 400 | ||
customer_id = request_body["customer_id"] | ||
video_id = request_body["video_id"] | ||
customer = Customer.query.get(customer_id) | ||
video = Video.query.get(video_id) | ||
# check if customer and video exist | ||
if customer is None or video is None: | ||
return make_response("", 404) | ||
rental = Rental.query.filter_by( | ||
video_id=video.id, customer_id=customer.id, checked_out=True).first() | ||
Comment on lines
+224
to
+225
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider ordering the rentals by the due date. A customer could have multiple rentals for the same video outstanding, and it would be nice to guarantee the oldest rental is considered to be returned first. |
||
if rental is None: | ||
return jsonify({"message": f"No outstanding rentals for customer {customer.id} and video {video.id}"}), 400 | ||
rental.checked_out = False | ||
db.session.commit() | ||
num_of_videos_rented = Rental.query.filter_by( | ||
video_id=video.id, checked_out=True).count() | ||
Comment on lines
+230
to
+231
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do like your use of count() when you just need the number of records. This saves the database from having to send back the full data of the rows, and the database itself is very good at counting rows. |
||
available_inventory = video.total_inventory - num_of_videos_rented | ||
videos_checkout_by_customer = Rental.query.filter_by( | ||
customer_id=customer.id, checked_out=True).count() | ||
return jsonify({ | ||
"customer_id": rental.customer_id, | ||
"video_id": rental.video_id, | ||
"videos_checked_out_count": videos_checkout_by_customer, "available_inventory": available_inventory}), 200 | ||
|
||
|
||
# GET RENTALS BY CUSTOMER | ||
@customers_bp.route("/<id>/rentals", methods=["GET"]) | ||
def rentals_by_video(id): | ||
customer = Customer.query.get(id) | ||
if customer is None: | ||
return jsonify({"message": f"Customer {id} was not found"}), 404 | ||
rentals = Rental.query.filter_by( | ||
customer_id=customer.id, checked_out=True).all() | ||
Comment on lines
+247
to
+248
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've done a good job of remembering to add the |
||
rentals_response = [] | ||
for rental in rentals: | ||
video_id = rental.video_id | ||
video = Video.query.get(video_id) | ||
rentals_response.append(video.rental_dict(rental)) | ||
return jsonify(rentals_response), 200 | ||
|
||
|
||
# GET RENTALS BY VIDEO | ||
@videos_bp.route("/<id>/rentals", methods=["GET"]) | ||
def rentals_by_video(id): | ||
video = Video.query.get(id) | ||
if video is None: | ||
return jsonify({"message": f"Video {id} was not found"}), 404 | ||
rentals = Rental.query.filter_by(video_id=video.id, checked_out=True).all() | ||
rentals_response = [] | ||
for rental in rentals: | ||
customer_id = rental.customer_id | ||
customer = Customer.query.get(customer_id) | ||
rentals_response.append(customer.rental_dict(rental)) | ||
return jsonify(rentals_response), 200 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Generic single-database configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
One thing we can do to help our routes file from getting too large is to consider making multiple files containing routes separated by the resource type. We might have a
routes
folder instead of aroutes.py
file, and inside that folder (along with a__init__.py
) we could have a file per resource, socustomer.py
,video.py
, andrental.py
. Where each would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simplybp
rather than including the resource name as part of it.Then here, we could import and register the blueprints like