-
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: Sarah and Kaitlyn #45
base: master
Are you sure you want to change the base?
Changes from all commits
6f5693a
8cda78f
67ada14
a59e4e6
baef22f
ae0898c
aec2fcc
6aa5a85
ba08a8b
39b7123
ba94d12
f1622a3
2874b8c
74495f0
b0944e1
c4ff6b4
332590b
f2397e3
1fba000
cc4c3a5
b159db1
fdb4689
e9c9c3b
7d67dc3
cf5d7c4
f9d14cb
f781561
507322a
4121b70
a1d11ac
65f8a5c
69db09e
54c4f78
a244082
1fecfbe
18ca179
86d14dd
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,2 +1 @@ | ||
web: gunicorn 'app:create_app()' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
from flask import Blueprint, jsonify, request | ||
from app import db | ||
from app.models.video import Video | ||
from app.models.customer import Customer | ||
from app.models.rental import Rental | ||
import datetime | ||
|
||
customers_bp = Blueprint("customers_bp", __name__, url_prefix="/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. Since this is the only blueprint in this file now, we can just call it |
||
|
||
|
||
@customers_bp.route("", methods=["GET"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice job splitting your endpoints into separate functions. There are still shared pieces of functionality in some of the endpoints that we can think about trying to reduce duplication (especially looking up a record by its id), but on the plus side, in each function, we only need to think about the logic related to that specific endpoint. |
||
def get_customers(): | ||
customers = Customer.query.all() | ||
for customer in customers: | ||
if customer.deleted_at: | ||
customers.remove(customer) | ||
customer_response = [customer.customer_dict() for customer in customers] | ||
Comment on lines
+14
to
+17
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.
Instead, we can simply skip over the records we don't want during the list comprehension, like: customer_response = [customer.customer_dict() for customer in customers if not customer.deleted_at] |
||
return jsonify(customer_response), 200 | ||
|
||
|
||
@customers_bp.route("/<customer_id>", methods=["GET"]) | ||
def get_customers_by_id(customer_id): | ||
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 |
||
return jsonify(None), 400 | ||
|
||
customer = Customer.query.get(customer_id) | ||
if not customer: | ||
response_body = {"message": f"Customer {customer_id} was not found"} | ||
return jsonify(response_body), 404 | ||
if customer.deleted_at: | ||
return jsonify(None), 404 | ||
Comment on lines
+26
to
+31
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. Instead of needing to have this extra logic here, consider a helper in the Customer class that looks up the id (with query.get) and would still return None if the record was deleted. Just like we don't want the tests to need to care about how things are being stored in the db, we also want to reduce the amount that the routes themselves need to know about how the data is being managed. |
||
response_body = customer.customer_dict() | ||
return jsonify(response_body), 200 | ||
|
||
|
||
@customers_bp.route("", methods=["POST"]) | ||
def post_customer(): | ||
request_body = request.get_json() | ||
if "name" not in request_body: | ||
response_body = {"details": "Request body must include name."} | ||
return jsonify(response_body), 400 | ||
elif "postal_code" not in request_body: | ||
response_body = {"details": "Request body must include postal_code."} | ||
return jsonify(response_body), 400 | ||
elif "phone" not in request_body: | ||
response_body = { | ||
"details": "Request body must include phone."} | ||
return jsonify(response_body), 400 | ||
Comment on lines
+39
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the outcome of any of these conditions being true is for the function to exit, these could all be |
||
|
||
new_customer = Customer( | ||
name=request_body["name"], | ||
postal_code=request_body["postal_code"], | ||
phone=request_body["phone"] | ||
) | ||
db.session.add(new_customer) | ||
db.session.commit() | ||
response_body = new_customer.customer_dict() | ||
return jsonify(response_body), 201 | ||
|
||
|
||
@customers_bp.route("/<customer_id>", methods=["PUT"]) | ||
def put_customer_by_id(customer_id): | ||
|
||
customer = Customer.query.get(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. 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 not customer: | ||
return jsonify({"message": f"Customer {customer_id} was not found"}), 404 | ||
if customer.deleted_at: | ||
return jsonify(None), 404 | ||
|
||
request_body = request.get_json() | ||
if ( | ||
"name" not in request_body or | ||
"postal_code" not in request_body or | ||
"phone" not in request_body | ||
): | ||
return jsonify({"details": "Invalid data"}), 400 | ||
|
||
customer.name = request_body["name"] | ||
customer.postal_code = request_body["postal_code"] | ||
customer.phone = request_body["phone"] | ||
|
||
db.session.commit() | ||
|
||
response_body = customer.customer_dict() | ||
return jsonify(response_body), 200 | ||
|
||
|
||
@customers_bp.route("/<customer_id>", methods=["DELETE"]) | ||
def delete_customer(customer_id): | ||
customer = Customer.query.get(customer_id) | ||
if not customer: | ||
return jsonify({"message": f"Customer {customer_id} was not found"}), 404 | ||
Comment on lines
+90
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also check whether the customer is already deleted in this route. Basically, we need to do it everywhere we try to work with a customer, which is why moving that into a helper method can be useful! |
||
|
||
customer.deleted_at = datetime.datetime.now() | ||
|
||
db.session.commit() | ||
|
||
response_body = customer.customer_dict() | ||
return jsonify(response_body), 200 | ||
|
||
|
||
@customers_bp.route("/<customer_id>/rentals", methods=["GET"]) | ||
def rentals_by_id(customer_id): | ||
customer = Customer.query.get(customer_id) | ||
if not customer: | ||
response_body = {"message": f"Customer {customer_id} was not found"} | ||
return jsonify(response_body), 404 | ||
if customer.deleted_at: | ||
return jsonify(None), 404 | ||
rental_response = [rental.id for rental in customer.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. Notice this is getting the rental id, not the id of the video in the rental. The tests happen to pass because of the relatively low number of records involved (since there's basically one of each kind, each record ends up with id 1). We need to be really careful to make sure that the tests don't just pass, but also are logically consistent. Also, due to the relationships defined on your models, a separate video lookup isn't really required. You could go directly from the customer to the video dictionaries like: response_body = [rental.video.create_dict() for rental in customer.rentals] Note that this has the potential to return rentals about videos that have been deleted. Maybe that's ok, maybe not (sort of a business rule decision), but just be aware of that. When we leave data in the database, we have to be especially careful to filter those "phantom" records everywhere. |
||
if not rental_response: | ||
return jsonify([]), 200 | ||
|
||
response_body = [ | ||
Video.query.get(video).create_dict() for video in rental_response | ||
] | ||
return jsonify(response_body), 200 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,24 @@ | ||
from sqlalchemy.sql.functions import func | ||
from app import db | ||
|
||
|
||
class Customer(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
customer_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(timezone=True), server_default=func.now() | ||
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 use of a server default value. This actually affects the schema, and ensures that the database will always populate a default value when a customer row is created, regardless of whether or not it's through our API. However, the functions we can specify for the server default are a bit of a toss-up. Ideally, we'd just like to store things in UTC and not worry about timezones, but that might not be straightforward with the server default. For a little more flexibility, we could mark the column as non-nullable, and use a client default, which lets us run any Python code, and would still prevent the addition of rows lacking the data. You would need to play around with this a bit more to see which approach might work best. |
||
) | ||
videos_checked_out = db.Column(db.Integer, default=0) | ||
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. Rather than storing this as a column, we can calculate it as needed. It's the number of outstanding rentals related to this customer. Generally, we prefer to avoid storing data that can be calculated from other, more authoritative data, since if the column exists, there are effectively two sources of truth, which could come to disagree. |
||
deleted_at = db.Column(db.DateTime, nullable=True) | ||
Comment on lines
+7
to
+14
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 |
||
rentals = db.relationship("Rental", backref="customer") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice relationship declaration. |
||
|
||
def customer_dict(self): | ||
return { | ||
"id": self.customer_id, | ||
"name": self.name, | ||
"postal_code": self.postal_code, | ||
"phone": self.phone, | ||
"register_at": self.register_at.strftime("%a, %d %b %Y %X %z") | ||
} | ||
Comment on lines
+17
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. 👍 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! Notice that it's a little redundant to have customer in the method name itself. Consider the case where we have a customer instance in a variable called customer. Then to convert to a dictionary, we end up writing |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,36 @@ | ||
from app import db | ||
from sqlalchemy.sql.functions import func | ||
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.
|
||
import datetime | ||
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" | ||
id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
video_id = db.Column(db.Integer, db.ForeignKey('video.id'), primary_key=True, nullable=False) | ||
customer_id = db.Column(db.Integer, db.ForeignKey('customer.customer_id'), primary_key=True, nullable=False) | ||
Comment on lines
+11
to
+12
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, nullable=False, default=datetime.datetime.utcnow() + datetime.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. 👍 Something like this would be the alternative to using a server_default as in customer. |
||
|
||
def rental_dict(self): | ||
video = Video.query.get(self.video_id) | ||
available_inventory = video.total_inventory - len(video.rentals) | ||
customer = Customer.query.get(self.customer_id) | ||
|
||
return { | ||
"video_id": self.video_id, | ||
"customer_id": self.customer_id, | ||
"videos_checked_out_count": len(customer.rentals), | ||
"available_inventory": available_inventory | ||
} | ||
|
||
# Using static method decorator. The method references Rental but doesn't | ||
# depend on any instance or behavior of the Rental model, but adds functionality to this model. | ||
@staticmethod | ||
def checkin_dict(customer, video): | ||
Comment on lines
+29
to
+30
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 adding an additional helper. It can be tough to decide where to put it when multiple types are involved. Sometimes making it an instance method of one or the other class might feel more appropriate, or we might make it a class or static method, or we might make an entirely separate class that just "knows" about building the desired output structure from several inputs. It will vary from case to case. |
||
return { | ||
"video_id": video.id, | ||
"customer_id": customer.customer_id, | ||
"videos_checked_out_count": len(customer.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. It looks like you made the decision to represent checking in a video by deleting a prior rental record. Notice that if you had instead chosen to mark the rental as "returned" somehow, then the count of rentals wouldn't necessarily be the count of "checked out" rentals. I mention this only because since you went to the effort of avoiding the deletion of videos and customers, it feels a bit incongruous to delete rental rows. If we had a column in the rental to track the returned data (like the deleted date for videos and customers) then we would need to use that to filter out rentals from this count. |
||
"available_inventory": video.total_inventory - len(video.rentals) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,40 @@ | ||
from app import db | ||
from flask import request | ||
|
||
|
||
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.Date, nullable=False) | ||
total_inventory = db.Column(db.Integer, nullable=False) | ||
deleted_at = db.Column(db.DateTime, nullable=True) | ||
Comment on lines
+7
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. 👍 Nice job thinking about which columns should be nullable. |
||
rentals = db.relationship("Rental", backref="video") | ||
|
||
def create_dict(self): | ||
return { | ||
"id": self.id, | ||
"title": self.title, | ||
"release_date": self.release_date, | ||
"total_inventory": self.total_inventory | ||
} | ||
|
||
def update(self, form_data): | ||
self.title = form_data["title"] | ||
self.release_date = form_data["release_date"] | ||
self.total_inventory = form_data["total_inventory"] | ||
|
||
db.session.commit() | ||
Comment on lines
+21
to
+26
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 really like adding methods that update the record (and database) to the model class. It already has a dependency on the db instance (it needed it to derived from Model) and its responsibility really centers around moving data in and out of the database. So anything that does that is an excellent match for becoming a helper here. |
||
|
||
@classmethod | ||
def from_json(cls): | ||
Comment on lines
+28
to
+29
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. Having a method to create an instance from a dict is a great idea, but instead of becoming dependent on the |
||
request_body = request.get_json() | ||
|
||
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 new_video |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
from flask import Blueprint, jsonify, request | ||
from app import db | ||
from app.models.video import Video | ||
from app.models.customer import Customer | ||
from app.models.rental import Rental | ||
|
||
rentals_bp = Blueprint("rentals_bp", __name__, url_prefix="/rentals") | ||
|
||
|
||
@rentals_bp.route("/check-out", methods=["POST"]) | ||
def check_out(): | ||
request_body = request.get_json() | ||
if "customer_id" not in request_body: | ||
response_body = {"details": "Request body must include customer_id."} | ||
return jsonify(response_body), 400 | ||
elif "video_id" not in request_body: | ||
response_body = {"details": "Request body must include video_id."} | ||
return jsonify(response_body), 400 | ||
|
||
video_id = request_body["video_id"] | ||
customer_id = request_body["customer_id"] | ||
video = Video.query.get(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. Even here we should be doing id format checks, and whether the record was deleted. Leaving the data in the database has far-reaching effects! |
||
if not video: | ||
return jsonify(None), 404 | ||
video_inventory = video.total_inventory - len(video.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. Consider adding a helper method to the video model so that we don't have to put the logic for this calculation here in the route. |
||
if video_inventory == 0: | ||
response_body = {"message": "Could not perform checkout"} | ||
return jsonify(response_body), 400 | ||
|
||
customer = Customer.query.get(customer_id) | ||
if not customer: | ||
return jsonify(None), 404 | ||
|
||
new_rental = Rental( | ||
video_id=request_body["video_id"], | ||
customer_id=request_body["customer_id"] | ||
) | ||
|
||
db.session.add(new_rental) | ||
customer.videos_checked_out = len(customer.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. Your rental_dict helper already calculates the videos checked out count dynamically. Think about removing this column entirely. |
||
db.session.commit() | ||
response_body = new_rental.rental_dict() | ||
return jsonify(response_body), 200 | ||
|
||
|
||
@rentals_bp.route("/check-in", methods=["POST"]) | ||
def check_in(): | ||
# TODO: make a helper function(s) to make this check and query. Repeating logic from checkout endpoint. | ||
request_body = request.get_json() | ||
if "customer_id" not in request_body: | ||
response_body = {"details": "Request body must include customer_id."} | ||
return jsonify(response_body), 400 | ||
elif "video_id" not in request_body: | ||
response_body = {"details": "Request body must include video_id."} | ||
return jsonify(response_body), 400 | ||
|
||
video_id = request_body["video_id"] | ||
video = Video.query.get(video_id) | ||
customer_id = request_body["customer_id"] | ||
customer = Customer.query.get(customer_id) | ||
|
||
# Separate checks beacuse they return different status codes. | ||
if not (video and customer): | ||
response_body = {"message": f"No outstanding rentals for customer {customer_id} and video {video_id}"} | ||
return jsonify(response_body), 404 | ||
Comment on lines
+63
to
+65
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 the test for this doesn't check the message. The 404 (not found) message could indicate which of the customer or video wasn't found, not that there are no outstanding rentals. |
||
|
||
if customer.videos_checked_out == 0: | ||
response_body = {"message": f"No outstanding rentals for customer {customer_id} and video {video_id}"} | ||
return jsonify(response_body), 400 | ||
Comment on lines
+67
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. I would be more comfortable doing the actual Rental lookup you perform a few lines down first, and if no rental is found, then this message would be appropriate. The current check is a proxy for that lookup, but could drift out of agreement if direct modifications are ever made to the database. |
||
|
||
# finding rental associated with specfic the custoemr_id and video_id and | ||
# deleting their rental record of the sepcified video. | ||
Rental.query.filter_by(customer_id=customer_id, video_id=video_id).delete() | ||
db.session.commit() | ||
# creating checkin dictionary for the response body of a successful request. | ||
# Updates the videos_checked_out_count and available inventory. | ||
customer.videos_checked_out = len(customer.rentals) | ||
response_body = Rental.checkin_dict(customer, video) | ||
return jsonify(response_body), 200 |
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 making multiple files containing routes separated by the resource type. For organization, we might put them in a
routes
folder instead of leaving them in the root. Inside that folder (along with a__init__.py
) we could have a file per resource, socustomer.py
,video.py
, andrental.py
. If they're all in a routes folder, we wouldn't need to include route in the name. Each resource route file would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simplybp
rather than including the resource name as part of it.Then here, we could import and register the blueprints like