-
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 - Diana Arreola, Supriya Chavan, Yuan Du #17
base: main
Are you sure you want to change the base?
Changes from all commits
f5ad6aa
9ab6d68
4ac3520
b869af5
412da7c
5065de9
20f0611
b1c5168
91d3867
0180dd7
53be405
aa0f372
b62dffc
0feee78
64f6b57
72f798a
22e69e7
ad37ad8
17d5e51
5d57ffb
32c9c16
9a1d79a
a093e4a
8fe85e4
128eb2c
eb6f049
d80ea25
60275c6
f68ef15
bdfdb48
2fcc2b0
621875f
55659a2
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,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, nullable=False) | ||
postal_code = db.Column(db.String, nullable=False) | ||
phone = db.Column(db.String, nullable=False) | ||
registered_at = db.Column(db.DateTime, default=(datetime.date.today())) | ||
videos_checked_out_count = db.Column(db.Integer, default=0) | ||
rentals = db.relationship("Rental", back_populates="customer") | ||
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 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 added the check_out_status at the end and didn't realize or think that we could refactor customer model and routes . But it's a good learning to check all the models and endpoints if we can derive some attributes based on the change. |
||
|
||
def to_dict(self): | ||
return { | ||
"id": self.id, | ||
"name": self.name, | ||
"postal_code": self.postal_code, | ||
"phone": self.phone, | ||
"registered_at": self.registered_at | ||
} | ||
|
||
@classmethod | ||
def from_dict(cls, customer_data): | ||
new_customer = Customer(name = customer_data["name"], | ||
postal_code = customer_data["postal_code"], | ||
phone = customer_data["phone"], | ||
registered_at = customer_data["registered_at"] | ||
) | ||
return new_customer |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,30 @@ | ||
from app import db | ||
import datetime | ||
|
||
class Rental(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
id = db.Column(db.Integer, primary_key=True,nullable=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. Another option could be to create a composite primary key from the # One possible set up
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) 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 had the idea that one customer can rent multiple copies of the same video so but we will definitely look into the composite primary key for any other applications we might use. |
||
due_date = db.Column(db.DateTime, default=(datetime.date.today()+datetime.timedelta(days=7))) | ||
check_out_status = db.Column(db.Boolean,default = 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. Nice use of default! |
||
customer_id = db.Column(db.Integer, db.ForeignKey('customer.id'),nullable=False) | ||
video_id = db.Column(db.Integer, db.ForeignKey('video.id'),nullable=False) | ||
customer = db.relationship("Customer", back_populates="rentals") | ||
video = db.relationship("Video", back_populates="rentals") | ||
|
||
def to_dict(self): | ||
rental_dict = {} | ||
rental_dict["id"] = self.id | ||
rental_dict["customer_id"] = self.customer_id | ||
rental_dict["video_id"] = self.video_id | ||
rental_dict["due_date"] = self.due_date | ||
rental_dict["check_out_status"] = self.check_out_status | ||
return rental_dict | ||
|
||
@classmethod | ||
def from_dict(cls, rental_data): | ||
new_rental = Rental( | ||
customer_id = rental_data["customer_id"], | ||
video_id = rental_data["video_id"] | ||
) | ||
return new_rental | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,26 @@ | ||
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.String, nullable=False) | ||
total_inventory = db.Column(db.Integer, nullable=False) | ||
videos_checked_out_count = db.Column(db.Integer) | ||
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 by querying for currently checked out rentals with this video id and finding the length. |
||
rentals = db.relationship("Rental", back_populates="video") | ||
|
||
def to_dict(self): | ||
return { | ||
"id": self.id, | ||
"title": self.title, | ||
"release_date": self.release_date, | ||
"total_inventory": self.total_inventory | ||
|
||
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 suggest removing this blank line since it isn't meaningfully separating code that does different things. |
||
} | ||
|
||
@classmethod | ||
def from_dict(cls, video_data): | ||
new_video = Video(title=video_data["title"], | ||
release_date=video_data["release_date"], | ||
total_inventory=video_data["total_inventory"] | ||
) | ||
Comment on lines
+22
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. The indentation is a little weird here, I would suggest one of the following 2 formats: new_video = Video(title=video_data["title"],
release_date=video_data["release_date"],
total_inventory=video_data["total_inventory"]
)
new_video = Video(
title=video_data["title"],
release_date=video_data["release_date"],
total_inventory=video_data["total_inventory"]
) |
||
return new_video |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,172 @@ | ||
from flask import Blueprint, jsonify, abort, make_response, request | ||
from app import db | ||
from datetime import datetime | ||
from app.models.customer import Customer | ||
from app.models.rental import Rental | ||
from app.models.video import Video | ||
from app.routes.routes_helper import validate_model, validate_num_queries | ||
|
||
customers_bp = Blueprint("customers_bp", __name__, url_prefix="/customers") | ||
|
||
# CREATE New Customer -- POST | ||
@customers_bp.route("", methods=["POST"]) | ||
def create_customer(): | ||
request_body = request.get_json() | ||
|
||
try: | ||
new_customer = Customer(name=request_body["name"], | ||
postal_code=request_body["postal_code"], | ||
phone=request_body["phone"] | ||
# registered_at=datetime.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. Great use of default on the model for this field! Gentle reminder that we want to clean up commented code before opening pull requests. |
||
) | ||
except KeyError as key_error: | ||
abort(make_response({"details":f"Request body must include {key_error.args[0]}."}, 400)) | ||
|
||
db.session.add(new_customer) | ||
db.session.commit() | ||
|
||
customer_response = new_customer.to_dict() | ||
return jsonify(customer_response), 201 | ||
|
||
@customers_bp.route("", methods=["GET"]) | ||
def read_all_customers(): | ||
customer_query = Customer.query | ||
# customers = customer_query.all() | ||
|
||
sort_query = request.args.get("sort") | ||
if sort_query: | ||
if sort_query == "name": | ||
customer_query = customer_query.order_by(Customer.name.asc()) | ||
elif sort_query == "postal_code": | ||
customer_query = customer_query.order_by(Customer.postal_code.asc()) | ||
elif sort_query == "registered_at": | ||
customer_query = customer_query.order_by(Customer.registered_at.asc()) | ||
else: | ||
customer_query = customer_query.order_by(Customer.id.asc()) | ||
Comment on lines
+36
to
+45
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 here in our calls to sort_param = request.args.get("sort")
sort_options = {
"name": Customer.name.asc(),
"registered_at": Customer.registered_at.asc(),
"postal_code": Customer.postal_code.asc(),
}
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.asc()) |
||
|
||
count_query = request.args.get("count") # check if count and page/ if only count, display all pages | ||
page_num_query = request.args.get("page_num") | ||
Comment on lines
+47
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. Something neat I just learned that could reduce a little code: in count_query = request.args.get("count", type=int)
page_num_query = request.args.get("page_num", type=int) 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. very cool! will definitely look into it |
||
|
||
if validate_num_queries(count_query) and validate_num_queries(page_num_query): | ||
# need to check if count_query and page_num wuery are valid nums | ||
|
||
page = customer_query.paginate(page=int(page_num_query), per_page=int(count_query), error_out=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. We should use suggestions from PEP8 to drop this over a couple lines to stay under the best practice of 79 characters max per line. https://peps.python.org/pep-0008/#maximum-line-length I see several lines across the project this applies to, so just something to keep an eye out for in the future. 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. Thank you! will definitely look into linting so that our code utilizes best linting practices. |
||
customers = customer_query.all() | ||
|
||
customers_response = [] | ||
|
||
for items in page.items: | ||
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 it represents a single Model instance, I'd suggest a singular name 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. Thank you for catching this! 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. Yes, we missed it. Will remember |
||
customers_response.append(items.to_dict()) | ||
Comment on lines
+56
to
+59
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 a great candidate for a list comprehension: customers_response = [item.to_dict() for item in page.items] What other places do you see across the project where a list comprehension would make sense? |
||
return jsonify(customers_response), 200 | ||
|
||
if validate_num_queries(count_query) and not validate_num_queries(page_num_query): | ||
page = customer_query.paginate(per_page=int(count_query), error_out=False) | ||
customers = customer_query.all() | ||
customers_response = [] | ||
|
||
for items in page.items: | ||
customers_response.append(items.to_dict()) | ||
return jsonify(customers_response), 200 | ||
Comment on lines
+62
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 have a lot of similar code between this if and the one above it. I believe we could reduce the duplication 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 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. Thanks for going over that! |
||
|
||
|
||
customers = customer_query.all() | ||
customers_response = [] | ||
for customer in customers: | ||
customers_response.append(customer.to_dict()) | ||
return jsonify(customers_response), 200 | ||
|
||
@customers_bp.route("/<customer_id>", methods=["GET"]) | ||
def read_one_customer(customer_id): | ||
customer = validate_model(Customer, customer_id) | ||
return customer.to_dict() | ||
|
||
@customers_bp.route("/<customer_id>", methods=["PUT"]) | ||
def update_customer(customer_id): | ||
customer = validate_model(Customer, customer_id) | ||
|
||
request_body = request.get_json() | ||
# customer = Customer.from_dict(request_body) | ||
try: | ||
customer.name = request_body["name"] | ||
customer.postal_code = request_body["postal_code"] | ||
customer.phone = request_body["phone"] | ||
except KeyError as key_error: | ||
abort(make_response({"details":f"Request body must include {key_error.args[0]}."}, 400)) | ||
|
||
db.session.commit() | ||
customer_response = customer.to_dict() | ||
|
||
return jsonify(customer_response),200 | ||
|
||
@customers_bp.route("/<customer_id>", methods=["DELETE"]) | ||
def delete_customer(customer_id): | ||
customer = validate_model(Customer, customer_id) | ||
|
||
customer_rentals = db.session.query(Rental).filter_by(customer_id=customer.id).all() | ||
|
||
if customer_rentals: | ||
for rental in customer_rentals: | ||
db.session.delete(rental) | ||
db.session.commit() | ||
Comment on lines
+107
to
+110
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 the choice to delete the rentals since we are deleting the customer model, so these rentals would no longer have an associated account to attach to. |
||
|
||
db.session.delete(customer) | ||
db.session.commit() | ||
|
||
customer_response = customer.to_dict() | ||
return jsonify(customer_response),200 | ||
|
||
## `GET /customers/<id>/rentals` GET /customers/<id>/rentals | ||
@customers_bp.route("/<id>/rentals", methods=["GET"]) | ||
def read_all_customer_rentals(id): | ||
customer = validate_model(Customer, id) | ||
video_query = Rental.query.filter_by(customer_id=customer.id).join(Video) | ||
|
||
sort_query = request.args.get("sort") | ||
if sort_query: | ||
if sort_query == "title": | ||
video_query = video_query.order_by(Video.title.asc()) # Rental.video.title.asc() | ||
else: | ||
video_query = video_query.order_by(Video.id.asc()) | ||
|
||
count_query = request.args.get("count") # check if count and page/ if only count, display all pages | ||
page_num_query = request.args.get("page_num") | ||
|
||
if validate_num_queries(count_query) and validate_num_queries(page_num_query): | ||
# need to check if count_query and page_num wuery are valid nums | ||
|
||
page = video_query.paginate(page=int(page_num_query), per_page=int(count_query), error_out=False) | ||
video_query = video_query.all() | ||
|
||
video_result = [] | ||
|
||
for items in page.items: | ||
video_result.append(items.video.to_dict()) | ||
return jsonify(video_result), 200 | ||
|
||
if validate_num_queries(count_query) and not validate_num_queries(page_num_query): | ||
page = video_query.paginate(per_page=int(count_query), error_out=False) | ||
video_query = video_query.all() | ||
video_result = [] | ||
|
||
for items in page.items: | ||
video_result.append(items.video.to_dict()) | ||
return jsonify(video_result), 200 | ||
Comment on lines
+124
to
+153
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 code to handle our sorting params has a lot of similarities, with more time, I would highly suggest looking at how we could extract this code and generalize it to a helper function that works for multiple models. 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 something we thought about too, glad to see our suspicions were correct :D |
||
|
||
video_result = [] | ||
video_query = video_query.all() | ||
for video in video_query: | ||
video_result.append(video.video.to_dict()) | ||
|
||
return jsonify(video_result), 200 | ||
|
||
@customers_bp.route("/<id>/history", methods=["GET"]) | ||
def read_a_customers_all_rental_history(id): | ||
customer = validate_model(Customer, id) | ||
customer_rental = Rental.query.filter_by(customer_id=customer.id,check_out_status = False).join(Video).all() | ||
|
||
customer_history_res =[] | ||
for video in customer_rental: | ||
customer_history_res.append(video.video.to_dict()) | ||
return jsonify(customer_history_res), 200 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
from flask import Blueprint, jsonify, abort, make_response, request | ||
from app import db | ||
from app.models.rental import Rental | ||
from app.models.customer import Customer | ||
from app.models.video import Video | ||
from app.routes.routes_helper import validate_model | ||
|
||
rentals_bp = Blueprint("rentals_bp", __name__, url_prefix="/rentals") | ||
|
||
## `POST /rentals/check-out` | ||
@rentals_bp.route("/check-out", methods=["POST"]) | ||
def create_rental(): | ||
request_body = request.get_json() | ||
|
||
try: | ||
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 a lot of code to surround in a try/catch and is causing the main flow of our code to be indented when most of it doesn't need to be. What are the relevant parts that could throw the error? I would surround just those lines so it's clear what the try/catch is targeting. Another possibility could be to do away with the try/catch entirely and use the customer_id = dict.get("customer_id", default=None)
customer = validate_model(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. This is smart way of using our existing abort call code block. Thank You. |
||
customer = validate_model(Customer, request_body["customer_id"]) | ||
video = validate_model(Video, request_body["video_id"]) | ||
|
||
videos_checked_out = db.session.query(Rental).filter_by(video_id=video.id).all() # look into count method | ||
available_inventory = 0 | ||
|
||
if not videos_checked_out: | ||
Comment on lines
+19
to
+22
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 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. Yes, it makes sense. Thanks for catching that. 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. ty! Do not know why that didn't click earlier :) |
||
available_inventory = video.total_inventory | ||
else: | ||
available_inventory = video.total_inventory - len(videos_checked_out) | ||
|
||
if available_inventory == 0: | ||
abort(make_response({"message":f"Could not perform checkout"}, 400)) | ||
|
||
new_rental = Rental(customer_id=request_body["customer_id"], | ||
video_id=request_body["video_id"] | ||
) | ||
|
||
customer.videos_checked_out_count += 1 | ||
except KeyError as key_error: | ||
abort(make_response({"message":f"Could not perform checkout bc {key_error.args[0]}"}, 400)) | ||
|
||
db.session.add(new_rental) | ||
db.session.add(customer) | ||
db.session.commit() | ||
|
||
rental_response = new_rental.to_dict() | ||
rental_response["videos_checked_out_count"] = customer.videos_checked_out_count | ||
available_inventory -= 1 | ||
rental_response["available_inventory"] = available_inventory | ||
|
||
return jsonify(rental_response), 200 | ||
|
||
|
||
#`POST /rentals/check-in` | ||
@rentals_bp.route("/check-in", methods=["POST"]) | ||
def delete_and_update_one_rental(): | ||
request_body = request.get_json() | ||
|
||
try: | ||
customer = validate_model(Customer, request_body["customer_id"]) | ||
video = validate_model(Video, request_body["video_id"]) | ||
|
||
rental = db.session.query(Rental).filter_by(video_id=video.id, customer_id=customer.id).first() # if customer has more than two rentals | ||
# checked_in_rental = validate_model(Rental, {"id": rental.id, "customer_id": rental.customer_id, "video_id": rental.video_id}) | ||
Comment on lines
+59
to
+60
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 we have a reference to the customer on line 56, we could also look at filtering its matching_rentals = [rental for rental in customer.rentals
if rental.video_id == 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. Thank you Kelsey! This makes a ton of sense, I don't think initially I understood the relationships we created in our models, and now it's beginning to make a lot more sense |
||
|
||
if not rental: | ||
abort(make_response({"message": f"No outstanding rentals for customer {customer.id} and video {video.id}"}, 400)) | ||
|
||
if rental.check_out_status == False: | ||
abort(make_response({"message":f"Could not perform checkout bc already checked out"}, 400)) | ||
|
||
videos_checked_out = db.session.query(Rental).filter_by(video_id=video.id).all() # look into count method, refactor duplicate code | ||
available_inventory = video.total_inventory - len(videos_checked_out) | ||
|
||
customer.videos_checked_out_count -= 1 | ||
available_inventory += 1 | ||
rental.check_out_status = False | ||
|
||
except KeyError as key_error: | ||
abort(make_response({"message":f"Could not perform checkout bc {key_error.args[0]}"}, 400)) | ||
|
||
db.session.add(rental) | ||
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 we are not adding a new rental, just updating an existing one, we should be able to remove this 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. makes sense ty! 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. Thank you! Yes it makes sense. |
||
db.session.commit() | ||
|
||
rental_response = rental.to_dict() | ||
rental_response["videos_checked_out_count"] = customer.videos_checked_out_count | ||
rental_response["available_inventory"] = available_inventory | ||
|
||
return jsonify(rental_response), 200 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
from flask import abort, make_response | ||
|
||
def validate_model(cls, model_id): | ||
try: | ||
model_id = int(model_id) | ||
except: | ||
abort(make_response({"message":f"{cls.__name__} {model_id} invalid"}, 400)) | ||
|
||
model = cls.query.get(model_id) | ||
|
||
if not model: | ||
abort(make_response({"message":f"{cls.__name__} {model_id} was not found"}, 404)) | ||
|
||
return model | ||
|
||
# return True if its a valid number, return False it's not a number | ||
def validate_num_queries(query_param): | ||
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 function is not shared between multiple route files, I would consider placing it in the specific route file that uses it. |
||
try: | ||
query_int = int(query_param) | ||
except: | ||
return False | ||
return True |
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.
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
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.
ty Kelsey! great to know :)