-
Notifications
You must be signed in to change notification settings - Fork 15
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
Ocelots - Eva and Lisa #16
base: main
Are you sure you want to change the base?
Changes from all commits
ac3dbc8
328da39
117d079
4518081
c98f2fe
1c939cc
fa58a4e
75587be
0d51e49
c70fe36
0fa01d9
32abd41
5de4710
c3ebae1
627beb2
69d642e
f836317
90935f2
8b42ff2
5dbd2a1
2289536
2b54534
17d1f93
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 |
---|---|---|
@@ -0,0 +1,153 @@ | ||
from app import db | ||
from app.models.customer import Customer | ||
from flask import Blueprint, jsonify, abort, make_response, request | ||
from app.routes_helper import validate_model | ||
from flask_sqlalchemy import Pagination | ||
|
||
|
||
customers_bp = Blueprint("customers_bp", __name__, url_prefix="/customers") | ||
|
||
# POST /customers | ||
@customers_bp.route("",methods=["POST"]) | ||
def create_customer(): | ||
customer_data = request.get_json() | ||
|
||
###### refactor ###### | ||
if "name" not in customer_data.keys(): | ||
abort(make_response({"details": f"Request body must include name."}, 400)) | ||
if "postal_code" not in customer_data.keys(): | ||
abort(make_response({"details": f"Request body must include postal_code."}, 400)) | ||
if "phone" not in customer_data.keys(): | ||
abort(make_response({"details": f"Request body must include phone."}, 400)) | ||
Comment on lines
+16
to
+21
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 like that you recognized this is a place that could be refactored, what kinds of changes were you thinking? |
||
|
||
new_customer = Customer( | ||
name = customer_data["name"], | ||
postal_code = customer_data["postal_code"], | ||
phone = customer_data["phone"] | ||
) | ||
|
||
db.session.add(new_customer) | ||
db.session.commit() | ||
|
||
return make_response(jsonify(new_customer.to_dict()), 201) | ||
|
||
|
||
# GET /customers | ||
@customers_bp.route("", methods=["GET"]) | ||
def get_customers_optional_query(): | ||
customer_query = Customer.query | ||
|
||
###### refactor ###### | ||
sort_query = request.args.get("sort") | ||
# check sort | ||
if sort_query == "name": | ||
customer_query = customer_query.order_by(Customer.name.asc()) | ||
elif sort_query == "registered_at": | ||
customer_query = customer_query.order_by(Customer.register_at.asc()) | ||
elif sort_query == "postal_code": | ||
customer_query = customer_query.order_by(Customer.postal_code.asc()) | ||
else: | ||
customer_query = customer_query.order_by(Customer.id.asc()) | ||
Comment on lines
+41
to
+50
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. There's a little redundancy in the calls to sort_param = request.args.get("sort")
sort_options = {
"name": Customer.name,
"registered_at": Customer.registered_at,
"postal_code": Customer.postal_code,
}
if sort_param in sort_options:
customer_query = customer_query.order_by(sort_options[sort_param])
else:
customer_query = customer_query.order_by(Customer.id) |
||
|
||
|
||
count_query = request.args.get("count", type=int) | ||
page_num_query = request.args.get("page_num", type=int) | ||
Comment on lines
+53
to
+54
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. Great way to check for an int when fetching the query parameters! |
||
|
||
# check count | ||
if count_query and not page_num_query: | ||
page = customer_query.paginate(page=1, per_page=count_query) | ||
customers = page.items | ||
elif count_query and page_num_query: | ||
page = customer_query.paginate(page=page_num_query, per_page=count_query) | ||
customers = page.items | ||
else: | ||
customers = customer_query.all() | ||
Comment on lines
+57
to
+64
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 believe we could reduce a little of the duplication in the first blocks based on what I'm seeing in the Flask docs about the default values used for
So if we have a page = customer_query.paginate(page=page_num_query, per_page=count_query) because |
||
|
||
|
||
customer_response = [] | ||
for customer in customers: | ||
customer_response.append(customer.to_dict()) | ||
|
||
return jsonify(customer_response) | ||
|
||
|
||
# GET /customers/<id> | ||
@customers_bp.route("/<customer_id>", methods=["GET"]) | ||
def get_customer_by_id(customer_id): | ||
customer_to_return = validate_model(Customer,customer_id) | ||
|
||
return customer_to_return.to_dict() | ||
|
||
|
||
# PUT /customers/<id> | ||
@customers_bp.route("/<customer_id>", methods=["PUT"]) | ||
def replace_customer_with_id(customer_id): | ||
customer = validate_model(Customer,customer_id) | ||
customer_data = request.get_json() | ||
|
||
###### refactor ###### | ||
if "name" not in customer_data.keys(): | ||
abort(make_response({"details": f"Request body must include name."}, 400)) | ||
if "postal_code" not in customer_data.keys(): | ||
abort(make_response({"details": f"Request body must include postal_code."}, 400)) | ||
if "phone" not in customer_data.keys(): | ||
abort(make_response({"details": f"Request body must include phone."}, 400)) | ||
Comment on lines
+89
to
+94
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 code would be a great candidate for a helper function since this customer data validation is duplicated across functions. |
||
|
||
customer.name = customer_data["name"] | ||
customer.postal_code = customer_data["postal_code"] | ||
customer.phone = customer_data["phone"] | ||
|
||
db.session.commit() | ||
|
||
return make_response(customer_data, 200) | ||
|
||
|
||
# DELETE /customers/<id> | ||
@customers_bp.route("/<customer_id>", methods=["DELETE"]) | ||
def delete_customer_by_id(customer_id): | ||
customer_to_delete = validate_model(Customer,customer_id) | ||
db.session.delete(customer_to_delete) | ||
db.session.commit() | ||
msg = f"Customer {customer_to_delete.id} successfully deleted" | ||
return make_response(jsonify({"id":customer_to_delete.id, "message":msg}), 200) | ||
|
||
|
||
# GET /<customer_id>/rentals | ||
@customers_bp.route("/<customer_id>/rentals", methods=["GET"]) | ||
def rentals_by_video(customer_id): | ||
customer = validate_model(Customer, customer_id) | ||
rentals = customer.videos | ||
|
||
customer_rental_response = [] | ||
for rental in rentals: | ||
customer_rental_response.append(rental.to_dict()) | ||
Comment on lines
+121
to
+123
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 would be a great candidate for a list comprehension, what could that look like? Are there other places you see across the project where a list comprehension would make sense? |
||
|
||
######## refactor ###### | ||
sort_query = request.args.get("sort") | ||
# check sort | ||
if sort_query == "title": | ||
customer_rental_response = sorted(customer_rental_response, key=lambda v: v['title']) | ||
elif sort_query == "release_date": | ||
customer_rental_response = sorted(customer_rental_response, key=lambda v: v['release_date']) | ||
else: | ||
customer_rental_response = sorted(customer_rental_response, key=lambda v: v['id']) | ||
|
||
count_query = request.args.get("count", type=int) | ||
page_num_query = request.args.get("page_num",1,type=int) | ||
# # check count | ||
if count_query and not page_num_query: | ||
# get the start and end index based on page number | ||
start_index = (page_num_query - 1) * count_query | ||
end_index = start_index + count_query | ||
items = customer_rental_response[start_index : end_index] | ||
page = Pagination(None, page_num_query, count_query, len(items), items) | ||
customer_rental_response = page.items | ||
elif count_query and page_num_query: | ||
start_index = (page_num_query - 1) * count_query | ||
end_index = start_index + count_query | ||
items = customer_rental_response[start_index : end_index] | ||
page = Pagination(None, page_num_query, count_query, len(items), items) | ||
customer_rental_response = page.items | ||
Comment on lines
+138
to
+150
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. Hmm, both side of this if/else are the same. What scenario were you trying to capture? |
||
|
||
|
||
return make_response(jsonify(customer_rental_response), 200) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,29 @@ | ||
from app import db | ||
import datetime | ||
|
||
class Customer(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
id = db.Column(db.Integer, primary_key=True, autoincrement = True) | ||
name = db.Column(db.String) | ||
postal_code = db.Column(db.String) | ||
phone = db.Column(db.String) | ||
Comment on lines
+7
to
+8
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 isn't necessary for this project, but out in the wild we'd likely look at what the max size these strings could possibly be is and give character limits in the definition. The top answers here give some explanation of possible performance impacts: https://stackoverflow.com/questions/1962310/importance-of-varchar-length-in-mysql-table |
||
register_at = db.Column(db.DateTime, default=datetime.date.today()) | ||
videos_checked_out_count = db.Column(db.Integer, default=0) | ||
videos = db.relationship("Video", secondary="rental", back_populates="customers") | ||
Comment on lines
+10
to
+11
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. In most situations, if we can derive the data we want from something else we are already storing, we should choose to do that over storing a new piece of data. If we store a piece of derivable data, we need to manually keep our related data in sync which introduces places for potential bugs, and we take up more memory for each record. In this case, we hold |
||
|
||
|
||
|
||
def to_dict(self): | ||
return { | ||
"id": self.id, | ||
"name": self.name, | ||
"postal_code": self.postal_code, | ||
"phone": self.phone, | ||
"register_at": self.register_at, | ||
"videos_checked_out_count": self.videos_checked_out_count | ||
} | ||
|
||
@classmethod | ||
def from_dict(cls, data): | ||
pass | ||
Comment on lines
+25
to
+27
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 is okay for personal projects as reminders, but when working in coding teams I would suggest removing function stubs like this before opening PRs. |
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,25 @@ | ||
from app import db | ||
import datetime | ||
|
||
class Rental(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
__tablename__ = "rental" | ||
video_id = db.Column(db.Integer, db.ForeignKey('video.id'), primary_key=True, nullable=False) | ||
customer_id = db.Column(db.Integer, db.ForeignKey('customer.id'), primary_key=True, nullable=False) | ||
Comment on lines
+6
to
+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. Really nice use of a composite primary key so we don't need to store a separate id attribute! |
||
due_date = db.Column(db.DateTime, default = (datetime.date.today() + datetime.timedelta(days=7))) | ||
|
||
|
||
|
||
def to_dict(self): | ||
return { | ||
"video_id": self.video_id, | ||
"customer_id": self.customer_id, | ||
"due_date": self.due_date.strftime("%Y-%m-%d") | ||
} | ||
|
||
|
||
@classmethod | ||
def from_dict(cls, data): | ||
pass | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,22 @@ | ||
from app import db | ||
import datetime | ||
|
||
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) | ||
release_date = db.Column(db.DateTime, default=datetime.date.today()) | ||
total_inventory = db.Column(db.Integer, default=0) | ||
available_inventory = 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. This is another field that we could derive, this time using |
||
customers = db.relationship("Customer", secondary="rental", back_populates="videos") | ||
|
||
def to_dict(self): | ||
return { | ||
"id": self.id, | ||
"title": self.title, | ||
"release_date": self.release_date.strftime("%Y-%m-%d"), | ||
"total_inventory": self.total_inventory | ||
} | ||
|
||
@classmethod | ||
def from_dict(cls, data): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
from app import db | ||
from app.models.video import Video | ||
from app.models.customer import Customer | ||
from app.models.rental import Rental | ||
from app.routes_helper import validate_model | ||
from flask import Blueprint, jsonify, abort, make_response, request | ||
import datetime | ||
|
||
rentals_bp = Blueprint("rentals_bp", __name__, url_prefix="/rentals") | ||
|
||
# POST | ||
@rentals_bp.route("/check-out", methods=["POST"]) | ||
def checkout_video(): | ||
checkout_data = request.get_json() | ||
|
||
# check vlaid customer and video | ||
try: | ||
customer = validate_model(Customer, checkout_data["customer_id"]) | ||
video = validate_model(Video, checkout_data["video_id"]) | ||
except KeyError as err: | ||
abort(make_response({"message":f"Missing {err.args[0]}."}, 400)) | ||
|
||
|
||
# check if the customer did rent the video | ||
rentals = Rental.query.all() | ||
rental_count = 0 | ||
for rental in rentals: | ||
if rental.video_id == video.id: | ||
rental_count += 1 | ||
if rental.video_id == video.id and rental.customer_id == customer.id: | ||
msg = f"Customer {customer.id} is already renting video {video.id}." | ||
abort(make_response({"message":msg}, 400)) | ||
|
||
|
||
# get rental count and determine if customer has already checked out the video | ||
# rental_count, rental_found = get_rental_count(customer.id, video.id) | ||
|
||
available_inventory = video.total_inventory - rental_count | ||
# error handling: if there are no videos left to be rented | ||
if available_inventory <= 0: | ||
abort(make_response({"message":"Could not perform checkout"}, 400)) | ||
video.available_inventory = available_inventory - 1 | ||
|
||
|
||
# -------if checkout is successful------ | ||
# updates the amount of videos the customer has checked out in the customer database | ||
customer.videos_checked_out_count += 1 | ||
|
||
|
||
new_rental = Rental(video_id = video.id, | ||
customer_id = customer.id, | ||
due_date = datetime.date.today() + datetime.timedelta(days=7) | ||
) | ||
|
||
db.session.add(new_rental) | ||
db.session.commit() | ||
|
||
check_out_response = {"customer_id": new_rental.customer_id, | ||
"video_id": new_rental.video_id, | ||
"due_date": new_rental.due_date.strftime("%Y-%m-%d"), | ||
"videos_checked_out_count": customer.videos_checked_out_count, | ||
"available_inventory": video.available_inventory | ||
} | ||
|
||
|
||
return make_response(jsonify(check_out_response), 200) | ||
|
||
|
||
# /POST | ||
@rentals_bp.route("/check-in", methods=["POST"]) | ||
def checkin_video(): | ||
check_in_data = request.get_json() | ||
|
||
# check vlaid customer and video | ||
try: | ||
customer = validate_model(Customer, check_in_data["customer_id"]) | ||
video = validate_model(Video, check_in_data["video_id"]) | ||
except KeyError as err: | ||
abort(make_response({"message":f"Missing {err.args[0]}."}, 400)) | ||
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.
|
||
|
||
# check if the customer did rent the video | ||
rentals = Rental.query.all() | ||
rental_found = False | ||
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 a boolean, I suggest creating a variable initialized to |
||
rental_count = 0 | ||
|
||
for rental in rentals: | ||
if rental.video_id == video.id: | ||
rental_count += 1 | ||
if (rental.customer_id == customer.id | ||
and rental.video_id == video.id): | ||
rental_found = True | ||
|
||
for rental in rentals: | ||
if rental.video_id == video.id and rental.customer_id == customer.id: | ||
rental_found = True | ||
Comment on lines
+82
to
+95
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 could handle this many ways. If we are using a query to grab the rentals, then I would recommend using SQLAlchemy's tools to manage the filtering. You could do something like: Rental.query.filter_by(customer_id=customer_id, video_id=video_id).first() Another option: Since we look up a reference to the customer on line 68, and it has an attribute
Comment on lines
+93
to
+95
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 loop duplicates what we're doing in the if-statement above, I think it could be removed. |
||
|
||
# error handling: if no record of rental exists | ||
if rental_found == False: | ||
msg = f"No outstanding rentals for customer {customer.id} and video {video.id}" | ||
abort(make_response({"message":msg}, 400)) | ||
|
||
# calculate available inventory and update status of video | ||
available_inventory = video.total_inventory - rental_count | ||
video.available_inventory = available_inventory + 1 | ||
|
||
# remove video from customer's checked out count | ||
customer.videos_checked_out_count -= 1 | ||
|
||
check_in_response = { | ||
"customer_id": customer.id, | ||
"video_id": video.id, | ||
"videos_checked_out_count": customer.videos_checked_out_count, | ||
"available_inventory": video.available_inventory | ||
} | ||
|
||
# delete record of rental from database | ||
Rental.query.filter_by(customer_id=customer.id, video_id = video.id).delete() | ||
db.session.commit() | ||
|
||
return make_response(jsonify(check_in_response), 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.
If there's a single route file it's okay to leave it in the app folder, but if we have more than one, for organization we should create a folder to hold all the route files.