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 - Rebecca Z. and Shaina B. #53

Open
wants to merge 17 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
6 changes: 6 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,11 @@ def create_app(test_config=None):
migrate.init_app(app, db)

#Register Blueprints Here
from app.routes.customer_routes import customers_bp
from app.routes.video_routes import video_bp
from app.routes.rental_routes import rentals_bp
app.register_blueprint(customers_bp)
app.register_blueprint(video_bp)
app.register_blueprint(rentals_bp)
Comment on lines +35 to +40

Choose a reason for hiding this comment

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

👍

Nice job splitting your model routes into separate files. This can be very helpful when our route files starts getting long. Notice that since the files are in the routes folder, it's a little redundant to have route in the filename itself. Instead we might simply use customer.py, video.py, and rental.py. 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
17 changes: 16 additions & 1 deletion app/models/customer.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,19 @@
from flask import current_app

Choose a reason for hiding this comment

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

It looks like this might have been imported by a stray autocomplete?

from app import db

class Customer(db.Model):
id = db.Column(db.Integer, primary_key=True)
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)
postal_code = db.Column(db.String)
phone = db.Column(db.String)
register_at = db.Column(db.DateTime)
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", backref="checked_out")

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

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,
"postal_code": self.postal_code,
"phone": self.phone
}
Comment on lines +13 to +19

Choose a reason for hiding this comment

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

👍

Nice method to convert the model into a dict for returning. We have a variety of endpoint outputs that we need to handle in this project, but having a general-purpose dictionary converter is a great start!

53 changes: 53 additions & 0 deletions app/models/helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
from flask import request
from app.models.video import Video
from app.models.rental import Rental
from app.models.customer import Customer

def invalid_cust_data(request_body):

Choose a reason for hiding this comment

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

Nice validation helpers! With respect to the naming, try to use verbs for function names, so here we might name this validate_customer_data, is_invalid_customer_data. Functions are actions, so verbs are a good match for names. For the return values, try to avoid mixing types. Here, we're generally returning a dictionary to indicate there was an error. To indicate there was no error, rather than False, consider using None. None indicates the absence of a value, and here, the presence of a value indicates an error. Happily, it can still be used in boolean situations much as False can (since it's falsy), but returning None matches the the concept a little more closely.

if "name" not in request_body:
valid = {"details": "Request body must include name."}
elif "phone" not in request_body:
valid = {"details": "Request body must include phone."}
elif "postal_code" not in request_body:
valid = {"details": "Request body must include postal_code."}
else:
valid = False
return valid

def invalid_customer(customer_id):
invalid = False
if not customer_id.isnumeric():

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 none_value()

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.

invalid = {"message": f"Invalid customer id"}, 400
elif customer_id.isnumeric():
if Customer.query.get(customer_id) is None:
invalid = {"message": f"Customer {customer_id} was not found"}, 404
return invalid

def invalid_video_data(request_body):
if "title" not in request_body:
valid = {"details": "Request body must include title."}
elif "release_date" not in request_body:
valid = {"details": "Request body must include release_date."}
elif "total_inventory" not in request_body:
valid = {"details": "Request body must include total_inventory."}
else:
valid = False
return valid

def invalid_video(video_id):

Choose a reason for hiding this comment

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

This might be better called something like get_validated_video. It would be great if instead of only returning the boolean on success, it returns the actual video. In that case, we don't have such an easy way to distinguish between successful and unsuccessful results. In that case, we might return a tuple of values, where the first part might be a boolean representing valid or invalid, and the second part might hold the error dictionary on failure, or the actual video record on success.

So on success: return True, looked_up_video
and on failure: return False, error_message

Now for the case of type consistency (as mentioned above, we might pack this all into a single dictionary with well-named keys (e.g., status, record, error) or make a small class to hold this information. There are lots of options, but doing so would simplify retrieving our values.

We might even consider making this method a class method of the Video class, since it's responsible for locating Video instances.

invalid = False
if not video_id.isnumeric():
invalid = {"message": f"Invalid video id"}, 400
elif video_id.isnumeric():
if Video.query.get(video_id) is None:
invalid = {"message": f"Video {video_id} was not found"}, 404
return invalid

def invalid_rental_data(request_body):
if "customer_id" not in request_body:
invalid = {"details": "Request body must include customer_id."}
elif "video_id" not in request_body:
invalid = {"details": "Request body must include video_id."}
else:
invalid = False
return invalid
22 changes: 21 additions & 1 deletion app/models/rental.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,24 @@
from sqlalchemy.orm import backref

Choose a reason for hiding this comment

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

Another stray import? In general, we can get to the parts of sqlalchemy that we need through our db instance.

from app import db
from app.models.video import Video
from app.models.customer import Customer

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

Choose a reason for hiding this comment

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

I'm curious why you chose to rename the table used to hold the rental information, while using the default names for customer and video.

id = db.Column(db.Integer, primary_key=True, autoincrement = True, nullable=False)
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 +9 to +10

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.

Also, it's more usual to include the foreign keys in a compound primary key when working with a pure join table, where the compound key also signifies that there can be only a single record in the table between the pair due to primary key uniqueness constraints. In our situation, we wouldn't want that since that would mean that a customer could only ever check out a particular video once and only once! The inclusion of the autoincrementing id key prevents this effect (since the compound key will always be unique) but it also means that having the foreign keys as part of the primary key has no effect. So we can leave the primary key designation off the foreign keys.

due_date = db.Column(db.DateTime)
checked_out = db.Column(db.Boolean, default = False)

def rental_by_title(self):
video = Video.query.get(self.video_id)
return{
"title": video.title
}

def cust_by_name(self):
customer = Customer.query.get(self.customer_id)
return {
"name": customer.name
}
Comment on lines +14 to +24

Choose a reason for hiding this comment

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

These are useful helpers for the rental output. We can make use of SQLAlchemy relationships to add properties to the rental model performing this lookup for us!

We can add relationship like

    video = db.relationship("Video", backref="rentals")
    customer = db.relationship("Customer", backref="rentals")

Then if we have a rental instance, we could get the video title as rental.video.title, or the customer name as rental.customer.name.

15 changes: 14 additions & 1 deletion app/models/video.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,17 @@
from app import db

class Video(db.Model):
id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String, nullable=False)
release_date = db.Column(db.DateTime, nullable=False)
total_inventory = db.Column(db.Integer, nullable=False)

# customers = db.relationship("Customer", secondary="rentals")

def video_dict(self):
return {
"id": self.id,
"title": self.title,
"release_date": self.release_date,
"total_inventory": self.total_inventory
}
Empty file removed app/routes.py
Empty file.
74 changes: 74 additions & 0 deletions app/routes/customer_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
from app import db
from flask import Blueprint, jsonify, request
from app.models.customer import Customer
from app.models.rental import Rental
from app.models.video import Video
from app.models.helper import invalid_cust_data, invalid_customer

customers_bp = Blueprint("customers", __name__, url_prefix="/customers")

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

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. And having the additional helper functions for validating ids especially useful. Consider extending the helpers that validate existing ids to actually retrieve the record as well.

def post_customer():
request_body = request.get_json()
invalid_response = invalid_cust_data(request_body)
if not invalid_response:
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
return jsonify(invalid_response), 400
Comment on lines +14 to +23

Choose a reason for hiding this comment

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

Consider reordering this check into a guard clause as follows, rather than having the error behavior isolated at the end.

    if invalid_response:
        return jsonify(invalid_response), 400

    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


@customers_bp.route("", methods=["GET"])
def get_customers():
customers = Customer.query.all()
customer_list = [customer.to_json() for customer in customers]

Choose a reason for hiding this comment

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

👍

Nice list comprehension

return jsonify(customer_list), 200


@customers_bp.route("/<customer_id>", methods=["GET"])
def get_one_customer(customer_id):
invalid_response = invalid_customer(customer_id)
if invalid_response:
return invalid_response
one_customer = Customer.query.get(customer_id)
return jsonify(one_customer.to_json()), 200

@customers_bp.route("/<customer_id>/rentals", methods=["GET"])
def get_cust_rental(customer_id):
if Customer.query.get(customer_id) is None:

Choose a reason for hiding this comment

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

Note that we should validate the id for any of these other endpoints as well, not just the one that had a test written.

return jsonify({"message": f"Customer {customer_id} was not found"}), 404
all_rentals = Rental.query.filter_by(customer_id = customer_id) # get rental by cust_id

Choose a reason for hiding this comment

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

We should restrict the retrieved rentals to those that are currently checked out (the rentals that are checked in would be more for the optional history endpoint).

response = []
for rental in all_rentals: # to access formatted rental information
response.append(rental.rental_by_title())
return jsonify(response), 200

@customers_bp.route("/<customer_id>", methods=["PUT"])
def update_customer(customer_id):
one_customer = Customer.query.get(customer_id)
invalid_cust = invalid_customer(customer_id)

Choose a reason for hiding this comment

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

We should perform this check before trying to retrieve the customer (in case the id isn't valid).

if invalid_cust:
return invalid_cust
request_body = request.get_json()
invalid_response = invalid_cust_data(request_body)
if invalid_response:
return jsonify(invalid_response), 400
one_customer.name = request_body["name"]
one_customer.phone = request_body["phone"]
one_customer.postal_code = request_body["postal_code"]
db.session.commit()
return jsonify(one_customer.to_json()), 200

@customers_bp.route("/<customer_id>", methods=["DELETE"])
def delete_customer(customer_id):
one_customer = Customer.query.get(customer_id)
invalid_cust = invalid_customer(customer_id)
if invalid_cust:
return invalid_cust
db.session.delete(one_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(one_customer.to_json()), 200
79 changes: 79 additions & 0 deletions app/routes/rental_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
from app import db
from flask import Blueprint, jsonify, request
from app.models.rental import Rental
from app.models.customer import Customer
from app.models.video import Video
from datetime import timedelta, datetime
from app.models.helper import invalid_rental_data

rentals_bp = Blueprint("rentals", __name__, url_prefix="/rentals")

@rentals_bp.route("/check-out", methods=["POST"])
def post_rental():
request_body = request.get_json()
invalid_request = invalid_rental_data(request_body)
if invalid_request:
return jsonify(invalid_request), 400

one_video = Video.query.get(request_body["video_id"])
one_customer = Customer.query.get(request_body["customer_id"])
if one_video is None:
return jsonify({"Bad Request": f"Video not found."}), 404
elif one_customer is None:
return jsonify({"Bad Request": f"Customer not found."}), 404
video_current_checked_out = Rental.query.filter_by(checked_out=True, video_id=one_video.id).count()
available_inventory = one_video.total_inventory - video_current_checked_out
Comment on lines +24 to +25

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 Video or Rental model.

if available_inventory == 0:
return jsonify({"message": "Could not perform checkout"}), 400
new_rental = Rental(
customer_id=one_customer.id,
video_id = one_video.id,
due_date = (datetime.now(tz=None) + timedelta(days=7)),

Choose a reason for hiding this comment

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

Consider using utcnow() rather than now(), especially if there's no timezone information on the datetime in the database. When working with dates and time, it's usually a good idea to pass them around and store them in UTC, and only convert them to local time when needed for display (like in a UI). Working with dates and times is tricky!

checked_out = True
)
db.session.add(new_rental)
db.session.commit()
cust_current_checked_out = Rental.query.filter_by(customer_id=one_customer.id, checked_out=True).count()

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 to a helper method in the Customer or Rental model.

available_inventory -=1
response_body = {
"customer_id": one_customer.id,
"video_id": one_video.id,
"due_date": new_rental.due_date,
"videos_checked_out_count": cust_current_checked_out,
"available_inventory": available_inventory
}
return jsonify(response_body), 200

@rentals_bp.route("/check-in", methods=["POST"])
def post_rental_return():
request_body = request.get_json()
invalid_request = invalid_rental_data(request_body)
if invalid_request:
return jsonify(invalid_request), 400

one_video = Video.query.get(request_body["video_id"])
one_customer = Customer.query.get(request_body["customer_id"])
if one_video is None:
return jsonify({"Bad Request": f"Video not found."}), 404
elif one_customer is None:
return jsonify({"Bad Request": f"Customer not found."}), 404
video_current_checked_in = Rental.query.filter_by(checked_out=True).count()

Choose a reason for hiding this comment

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

If we want to find a checked out record for the customer and video, we also need to include the customer and video in the filter_by query.

video_checked_out = Rental.query.filter_by(checked_out=False).count()

Choose a reason for hiding this comment

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

Here, we should include the video id in the query for getting the count of checked out rentals for the video and those that are checked out. We should also look this up after we have checked in the video so the counts line up. I think the tests work for this since they expect to check in a video (so the checked out count should be 0 and everything should be available). Since this code is looking for the count of checked in rentals before we checked any in, it still ends up getting a count of 0, but it's getting the correct result (in this situation only) for an erroneous reason.

if video_current_checked_in == 0:
return jsonify({"message": f"No outstanding rentals for customer {one_customer.id} and video {one_video.id}"}), 400
returned_rental = Rental(
customer_id = one_customer.id,
video_id = one_video.id,
checked_out = False
)
db.session.add(returned_rental)
Comment on lines +64 to +69

Choose a reason for hiding this comment

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

We don't want to add a new rental that marked false. Instead, we should modify the checked out state of an existing rental.

db.session.commit()
available_inventory = one_video.total_inventory - video_checked_out
response_body = {
"customer_id": one_customer.id,
"video_id": one_video.id,
"videos_checked_out_count": video_checked_out,
"available_inventory": available_inventory
}
return jsonify(response_body), 200

86 changes: 86 additions & 0 deletions app/routes/video_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
from app import db
from flask import Blueprint, jsonify, request
from app.models.helper import invalid_video, invalid_video_data
from app.models.video import Video
from app.models.rental import Rental

video_bp = Blueprint("video_bp", __name__, url_prefix="/videos")

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.



@video_bp.route("", methods=["GET"])
def get_videos():
videos = Video.query.all()
video_list = [video.video_dict() for video in videos]
return jsonify(video_list), 200


@video_bp.route("/<video_id>", methods=["GET"])
def get_one_video(video_id):
invalid_response = invalid_video(video_id)
if invalid_response:
return invalid_response

one_video = Video.query.get(video_id)
return jsonify(one_video.video_dict()), 200


@video_bp.route("/<video_id>/rentals", methods=["GET"])
def get_vid_rental(video_id):
if Video.query.get(video_id) is None:
return jsonify({"message": f"Video {video_id} was not found"}), 404
all_videos = Rental.query.filter_by(id=video_id) # get rental by cust_id
response = []
for video in all_videos: # to access formatted rental information
response.append(video.cust_by_name())
return jsonify(response), 200


@video_bp.route("", methods=["POST"])
def post_video():
request_body = request.get_json()
invalid_response = invalid_video_data(request_body)
if not invalid_response:
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.video_dict()), 201

return jsonify(invalid_response), 400


@video_bp.route("/<video_id>", methods=["PUT"])
def update_video(video_id):
one_video = Video.query.get(video_id)
invalid_vid = invalid_video(video_id)
if invalid_vid:
return invalid_vid

request_body = request.get_json()
invalid_response = invalid_video_data(request_body)

if invalid_response:
return jsonify(invalid_response), 400

one_video.title = request_body["title"]
one_video.release_date = request_body["release_date"]
one_video.total_inventory = request_body["total_inventory"]

db.session.commit()
return jsonify(one_video.video_dict()), 200


@video_bp.route("/<video_id>", methods=["DELETE"])
def delete_video(video_id):
one_video = Video.query.get(video_id)
invalid_vid = invalid_video(video_id)
if invalid_vid:
return invalid_vid

db.session.delete(one_video)
db.session.commit()
return jsonify(one_video.video_dict()), 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