Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,9 @@ def create_app(test_config=None):
migrate.init_app(app, db)

#Register Blueprints Here
from .routes import customers_bp, videos_bp, rentals_bp
app.register_blueprint(customers_bp)
app.register_blueprint(videos_bp)
app.register_blueprint(rentals_bp)
Comment on lines +35 to +38

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 a routes.py file, and inside that folder (along with a __init__.py) we could have a file per resource, so customer.py, video.py, and rental.py. Where each would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simply bp rather than including the resource name as part of it.

Then here, we could import and register the blueprints like

    from .routes import customer, video, rental
    app.register_blueprint(customer.bp)
    app.register_blueprint(video.bp)
    app.register_blueprint(rental.bp)


return app
24 changes: 23 additions & 1 deletion app/models/customer.py
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that the default value for nullable is True, so if we don't set it to false, the columns will be allowed to be NULL. When adding column definitions to our models, we should consider whether it makes sense to allow a record to exist without that data, such as a Customer without a name, a Video without a title, or a Rental without a due date. It's true that we can add checks for this in our logic that adds entries, but we should try to leverage as much validation help from the database as possible. If we tell the database that certain columns are required (not nullable), then we'll encounter an error if we accidentally try to create a row without a value for those columns (which is a good thing)!

videos = db.relationship("Video", secondary="rental", backref="customer")

Choose a reason for hiding this comment

The 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 secondary attribute on a relationship is more appropriate when we have a simple many-to-many relationship, such as with a pure join table. In that case, a relationship like this would allow SQLAlchemy to automatically manage the join table between the two models being linked and we could establish those links by using the relationship like a collection, by appending, removing, etc. In that case, the backref attribute supplies the equivalent name to attach to the model on the other end of the relationship. Since the local name here is videos, a closer match in semantics for the other side might be customers rather than customer, since there are potentially many customers who have rented a video (though again, this isn't used in your code).

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 customer, then we could access the rentals associated with the customer as customer.rentals. If there were a similar relationship on the video side, then to get the video title of a particular customer rental, we could write code like customer.rentals[0].video.title. SQLAlchemy takes care of using the foreign key information to fill in the relationships.


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):

Choose a reason for hiding this comment

The 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}
10 changes: 8 additions & 2 deletions app/models/rental.py
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

Choose a reason for hiding this comment

The 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)
21 changes: 20 additions & 1 deletion app/models/video.py
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
}
269 changes: 269 additions & 0 deletions app/routes.py
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"])

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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 if rather than elif. More interestingly, notice how each of these conditions is more or less the same. They differ only in the key being checked for in the request, and the string being used in the error message. If we put the keys into a list and iterated over it, how could we rewrite this to avoid duplicating the logic three times (or potentially more if there were other keys we needed to check!)?

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

Choose a reason for hiding this comment

The 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 isnumeric and isdigit are more about detecting classes of characters (think Unicode) than they are for determining what numeric types we might be able to convert the string value into.

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)

Choose a reason for hiding this comment

The 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":

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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)

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've done a good job of remembering to add the checked_out=True in your queries, but notice how easy it would be to forget, or if there was a new dev on your team, it might not occur to them they need it. Moving even a single complex line into a well-named helper function can be very useful for avoiding errors.

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
1 change: 1 addition & 0 deletions migrations/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Generic single-database configuration.
Loading