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 - Mary Tian and Ivette Fernandez #57

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
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
8 changes: 7 additions & 1 deletion app/models/customer.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
from app import db

class Customer(db.Model):
id = db.Column(db.Integer, primary_key=True)
__tablename__ = "customers_table"

Choose a reason for hiding this comment

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

There's not a strong need to choose a different name for the model table here. The primary reason we might need to do this is if we were using our model to wrap an existing database table that happens not to follow the naming defaults of SqlAlchemy (matching the model name), or if we were an SQL purist and preferred plural table names to represent that tables do store multiple rows. However, in that case we would generally avoid appending _table to the end of the table name (it's repetitive to have table in name of the table).

In general, it's good to know that the __tablename__ value exists, but we don't have much need to use it.

id = db.Column(db.Integer, primary_key=True, autoincrement=True)

Choose a reason for hiding this comment

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

According to the SqlAlchemy docs about autoincrement:

The default value is the string "auto" which indicates that a single-column primary key that is of an INTEGER type with no stated client-side or python-side defaults should receive auto increment semantics automatically; all other varieties of primary key columns will not.

This means that here, since the primary is not a compound key, the type is integer, and there's no specified default value, the desired autoincrement behavior will apply even without listing it. It doesn't hurt anything to list it, but it's not required.

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. 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="rentals_table", backref="customers_table")

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_table.

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.

All this being said, having this secondary relationship is what allows the deletion of a video or customer which has particiapted in a rental (whether current or in the past). When a video or customer is deleted, SQLAlchemy is tracking that and will delete any rental that also refers to that video or customer. Without this relationship, we might need to look into cascading delete behavior for one-to-many relationships (or try to avoid actually deleting data at all, since data = money).

7 changes: 6 additions & 1 deletion app/models/rental.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
from app import db

class Rental(db.Model):
id = db.Column(db.Integer, primary_key=True)
__tablename__ = "rentals_table"
id = db.Column(db.Integer, primary_key=True, autoincrement=True)
customer_id = db.Column(db.Integer, db.ForeignKey('customers_table.id'),nullable=False)
video_id = db.Column(db.Integer, db.ForeignKey('videos_table.id'),nullable=False)
Comment on lines +6 to +7

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_in_status = db.Column(db.Boolean, default=False)
8 changes: 7 additions & 1 deletion app/models/video.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
from app import db

class Video(db.Model):
id = db.Column(db.Integer, primary_key=True)
__tablename__ = "videos_table"
id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String)
release_date = db.Column(db.DateTime)
total_inventory = db.Column(db.Integer)
# customers = db.relationship("Customer", secondary="rentals_table", backref="videos_table")

Choose a reason for hiding this comment

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

Using a backref on a relationship means that we don't need to set up the inverse relationship ourself. But as noted above, the use of a relationship with a secondary is less helpful than we might like in this situation. We might prefer to use a regular relationship between video and rental similar to my comment in customer.

    rentals = db.relationship("Rental", backref="video")


345 changes: 345 additions & 0 deletions app/routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,345 @@
from app import db
from app.models.customer import Customer
from app.models.video import Video
from app.models.rental import Rental
from flask import Blueprint, jsonify, request
from datetime import datetime, timezone, date, timedelta

customers_bp = Blueprint("customers_bp", __name__, url_prefix="/customers")
videos_bp = Blueprint("videos_bp", __name__, url_prefix=("/videos"))
rentals_bp = Blueprint("rentals_bp", __name__, url_prefix=("/rentals"))



@rentals_bp.route("/check-out", methods=["POST"])
def handle_rentals_out():
# if request.method == "POST":

Choose a reason for hiding this comment

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

Yes, since this is only registered for the POST method, no if check on the method is needed.

request_body = request.get_json()
customer_id = request_body.get("customer_id")
video_id = request_body.get("video_id")
due_date = date.today() + timedelta(days=7)

customer = Customer.query.get(customer_id)

Choose a reason for hiding this comment

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

We should check whether the customer_id is None before perfoming the lookup here. Likewise for video_id both here and in the check in endpoint. Currently, there are 4 tests that raise warnings (not visible by the regular pass/fail icons). To see them, we would need to run the tests from the terminal with pytest, or in VS Code take a look at the Output view labeled Python Test Log. The 4 tests with warnings omit either the customer id or video id from the request body in the check out and check in endpoints.

if customer_id is None:
return jsonify(None), 400
if customer is None:
return jsonify(None), 404

video = Video.query.get(video_id)
if video_id is None:
return jsonify(None), 400
if video is None:
return jsonify(None), 404


video_checked_out_count = Rental.query.filter_by(video_id=video_id, checked_in_status=False).count()
available_inventory = video.total_inventory - video_checked_out_count
Comment on lines +35 to +36

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" : "Could not perform checkout"
}), 400

new_rental = Rental(
customer_id=customer.id,
video_id=video.id,
due_date=due_date
)

db.session.add(new_rental)
db.session.commit()

video_checked_out_count = Rental.query.filter_by(video_id=video_id).count()

Choose a reason for hiding this comment

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

This should have the same check for being checked out as earlier in the function (or use the same helper methods if the query logic gets moved into the model class).

available_inventory = video.total_inventory - video_checked_out_count

return {
"customer_id": new_rental.customer_id,
"video_id": new_rental.video_id,
"due_date": new_rental.due_date,
"videos_checked_out_count": video_checked_out_count,
"available_inventory": available_inventory
}, 200

@rentals_bp.route("/check-in", methods=["POST"])
def handle_rentals_in():
request_body = request.get_json()
customer_id = request_body.get("customer_id")
video_id = request_body.get("video_id")

customer = Customer.query.get(customer_id)

Choose a reason for hiding this comment

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

Here, too, watch the order of the error handling.

if customer_id is None:
return jsonify(None), 400
if customer is None:
return jsonify(None), 404

video = Video.query.get(video_id)
if video_id is None:
return jsonify(None), 400
if video is None:
return jsonify(None), 404

rental = Rental.query.filter_by(customer_id=customer_id, video_id=video_id, checked_in_status=False).order_by(Rental.due_date.asc()).first()

Choose a reason for hiding this comment

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

👍

Nice consideration to sort the found rentals. If the customer had multiple copies of a video checked out, we might like to ensure that the oldest is checked in first.

if rental is None:
return jsonify({"message" : f"No outstanding rentals for customer {customer_id} and video {video_id}"
}), 400

rental.checked_in_status = True
video_checked_out_count = Rental.query.filter_by(video_id=video_id, checked_in_status=False).count()
available_inventory = video.total_inventory - video_checked_out_count

db.session.commit()

return {
"customer_id": rental.customer_id,
"video_id": rental.video_id,
"videos_checked_out_count": video_checked_out_count,
"available_inventory": available_inventory
}, 200

@customers_bp.route("/<customer_id>/rentals", methods=["GET"])
def list_customer_rentals(customer_id):
customer = Customer.query.get(customer_id)

Choose a reason for hiding this comment

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

We should have similar validation for the customer_id as we have for the CRUD customer endpoints (no non-numeric values allowed).

if customer is None:
return jsonify({
"message" : f"Customer {customer_id} was not found"
}), 404
customer_rentals = Rental.query.filter_by(customer_id=customer_id, checked_in_status=False).all()
customer_rentals_response = []
for customer_rental in customer_rentals:
video = Video.query.get(customer_rental.video_id)

Choose a reason for hiding this comment

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

With relationships configured as suggested above, we could avoid this query here and get the associated video simply as customer_rental.video.

customer_rentals_response.append({
"release_date" : video.release_date,
"title" : video.title,
"due_date" : customer_rental.due_date
})
return jsonify(customer_rentals_response)


@videos_bp.route("/<video_id>/rentals", methods=["GET"])
def list_video_rentals(video_id):
video = Video.query.get(video_id)

Choose a reason for hiding this comment

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

Similar considerations for the customer rentals endpoint apply here as well.

if video is None:
return jsonify({
"message" : f"Video {video_id} was not found"
}), 404
video_rentals = Rental.query.filter_by(video_id=video_id, checked_in_status=False).all()
video_rentals_response = []
for video_rental in video_rentals:
customer = Customer.query.get(video_rental.customer_id)
video_rentals_response.append({
"due_date" : video_rental.due_date,
"name" : customer.name,
"phone" : customer.phone,
"postal_code" : customer.postal_code
})
return jsonify(video_rentals_response)


@videos_bp.route("", methods=["GET", "POST"])
def handle_videos():
if request.method == "GET":
videos = Video.query.all()
videos_response = []
for video in videos:
videos_response.append({
"id": video.id,
"title": video.title,
"release_date": video.release_date,
"total_inventory": video.total_inventory
Comment on lines +145 to +148

Choose a reason for hiding this comment

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

There are many places in our routes where we need to build a dictionary like this (or very similar). Consider making a helper method as an instance method of the video class (e.g. def to_dict(self)).

})

return jsonify(videos_response), 200

elif request.method == "POST":
request_body = request.get_json()
title = request_body.get("title")
release_date = request_body.get("release_date")
total_inventory = request_body.get("total_inventory")

if not title:
return jsonify({
"details" : "Request body must include title."
}), 400

if not release_date:
return jsonify({
"details" : "Request body must include release_date."
}), 400

if not total_inventory:
return jsonify({
"details" : "Request body must include total_inventory."
}), 400
Comment on lines +159 to +172

Choose a reason for hiding this comment

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

When things repeat three times, that's a great chance to look at how to dry up our code. Notice that for all three of these cases, we check for a value, and then build a dictionary with an error if it's empty. What if we built a a list of pairs of values: the value to check, and the string to use in the error, and looped over the list?


new_video = Video(
title=title,
release_date=release_date,
total_inventory=total_inventory
)
db.session.add(new_video)
db.session.commit()

return {
"id": new_video.id,
"title" : new_video.title,
"total_inventory" : new_video.total_inventory
}, 201


@videos_bp.route("/<video_id>", methods= ["GET", "PUT", "DELETE"])
def handle_video(video_id):
if not video_id.isnumeric():
return jsonify(None), 400
Comment on lines +191 to +192

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(video_id)
    except ValueError:
        return jsonify(None), 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.


video = Video.query.get(video_id)

if video is None:
return jsonify({
"message" : f"Video {video_id} was not found"
}), 404

if request.method == "GET":
return jsonify({
"id" : video.id,
"title" : video.title,
"release_date" : video.release_date,
"total_inventory" : video.total_inventory
Comment on lines +202 to +206

Choose a reason for hiding this comment

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

This is another spot that could benefit from moving the dictionary generation into a helper in the video model.

}), 200

elif request.method == "DELETE":
db.session.delete(video)
db.session.commit()

return jsonify({
"id" : video.id
}), 200

elif request.method == "PUT":
request_body = request.get_json()
title = request_body.get("title")
release_date = request_body.get("release_date")
total_inventory = request_body.get("total_inventory")

if not (title and release_date and total_inventory):
return jsonify({
"details" : "Invalid data"
}), 400
Comment on lines +223 to +226

Choose a reason for hiding this comment

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

👍

Good job performing similar validations as for creating a video record through POST.


video.title = title
video.release_date = release_date
video.total_inventory = total_inventory

db.session.commit()

return jsonify({
"id" : video.id,
"title" : video.title,
"release_date" : video.release_date,
"total_inventory" : video.total_inventory,
Comment on lines +235 to +238

Choose a reason for hiding this comment

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

Here's a third repetition. As I said, if we write the same code three times, that's a great time to start thinking about drying it up.

}), 200

@customers_bp.route("", methods=["GET", "POST"])

Choose a reason for hiding this comment

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

Similar feedback as for the video routes apply here for the customer routes.

def handle_customers():
if request.method == "GET":
customers = Customer.query.all()
customers_response = []
for customer in customers:
customers_response.append({
"id" : customer.id,
"name" : customer.name,
"registered_at" : customer.registered_at,
"postal_code" : customer.postal_code,
"phone" : customer.phone
})

return jsonify(customers_response), 200

elif request.method == "POST":
request_body = request.get_json()
name = request_body.get("name")
postal_code = request_body.get("postal_code")
phone = request_body.get("phone")

if not postal_code:
return jsonify({
"details" : "Request body must include postal_code."
}), 400

if not name:
return jsonify({
"details" : "Request body must include name."
}), 400

if not phone:
return jsonify({
"details" : "Request body must include phone."
}), 400

new_customer = Customer(
name=name,
postal_code=postal_code,
phone=phone,
registered_at=datetime.now(timezone.utc).strftime("%a, %d %b %Y %X %z")

Choose a reason for hiding this comment

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

Consider using datetime.utcnow() as a slightly simpler way to get the UTC current time (without needing timezone). Also, since the column for registered_at is a DateTime, it shouldn't be necessary to convert the datetime value to a string.

)

db.session.add(new_customer)
db.session.commit()

return jsonify({
"id" : new_customer.id
}), 201

@customers_bp.route("/<customer_id>", methods=["GET", "PUT", "DELETE"])
def handle_customer(customer_id):
if not customer_id.isnumeric():
return jsonify(None), 400

customer = Customer.query.get(customer_id)

if customer is None:
return jsonify({
"message" : f"Customer {customer_id} was not found"
}), 404

if request.method == "GET":
return jsonify({
"id" : customer.id,
"name" : customer.name,
"registered_at" : customer.registered_at,
"postal_code" : customer.postal_code,
"phone" : customer.phone
}), 200

elif request.method == "PUT":
request_body = request.get_json()
name = request_body.get("name")
postal_code = request_body.get("postal_code")
phone = request_body.get("phone")

if not (name and postal_code and phone):
return jsonify({
"details" : "Invalid data"
}), 400

customer.name = name
customer.postal_code = postal_code
customer.phone = phone

db.session.commit()

return jsonify({
"id" : customer.id,
"name" : customer.name,
"registered_at" : customer.registered_at,
"postal_code" : customer.postal_code,
"phone" : customer.phone
}), 200

elif request.method == "DELETE":
db.session.delete(customer)
db.session.commit()

return jsonify({
"id" : customer.id
}), 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