-
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 - Rebecca Z. and Shaina B. #53
base: master
Are you sure you want to change the base?
Changes from all commits
6dfab80
81b1fa2
3eb1e65
ecdbdc0
669e8f7
aab6e88
63b7e50
aaa0d4a
0a5b25a
719f7c6
9cca5ca
f6a5d69
3c7dba3
06f0770
abf932b
7f97924
b3891ee
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,19 @@ | ||
from flask import current_app | ||
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. 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) | ||
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. According to the SqlAlchemy docs about autoincrement:
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
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="rentals", backref="checked_out") | ||
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, | ||
"postal_code": self.postal_code, | ||
"phone": self.phone | ||
} | ||
Comment on lines
+13
to
+19
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 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! |
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): | ||
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 validation helpers! With respect to the naming, try to use verbs for function names, so here we might name this |
||
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(): | ||
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(video_id)
except ValueError:
return none_value() The string methods like |
||
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): | ||
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 might be better called something like So on success: 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,24 @@ | ||
from sqlalchemy.orm import backref | ||
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. Another stray import? In general, we can get to the parts of sqlalchemy that we need through our |
||
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" | ||
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'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
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. 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
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. 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 |
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 | ||
} |
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"]) | ||
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. 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
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 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] | ||
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 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: | ||
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. 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 | ||
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. 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) | ||
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. 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) | ||
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(one_customer.to_json()), 200 |
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
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 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)), | ||
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 using |
||
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() | ||
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 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() | ||
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. 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() | ||
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. 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
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. 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 | ||
|
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") | ||
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. |
||
|
||
|
||
@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 |
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.
👍
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 usecustomer.py
,video.py
, andrental.py
. 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