-
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
AC2 - gp2 (Nad, Dalia) #18
base: main
Are you sure you want to change the base?
Changes from all commits
6b0ae4e
7066edc
9ccf6b9
24dc9bb
4a606fa
4d27307
c63609b
2aaef4c
4ecc307
3ab2512
45e1414
c2363b5
5c5ee9f
ddc388b
a7f8099
fc4a2ab
28f07e5
4191ce4
cc5f7b8
0ac5d5b
dd641d5
8529910
3f9a52e
1e2d665
8d701be
7d71887
779e559
55e2681
ac67ffe
a9345bc
756c406
4390699
8eea1f7
e411bd5
f67dce1
b4ab9a4
2ff8adc
7104446
7f35bab
cd25f63
945e9e6
e26ab26
46dac1c
e8bbe82
cf2b426
3c21e3d
3b13760
f752f44
26e509e
f08512f
975391f
dd1f614
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,22 @@ | ||
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, nullable=False, autoincrement=True) | ||
name = db.Column(db.String, nullable=False) | ||
postal_code = db.Column(db.String, nullable=False) | ||
phone = db.Column(db.String, nullable=False) | ||
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 |
||
registered_at = db.Column(db.DateTime, default=datetime.datetime.now().strftime("%a, %d %b %Y %X %z")) | ||
videos_checked_out_count = db.Column(db.Integer, nullable=False, 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. 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 |
||
videos = db.relationship("Video", secondary="rental", back_populates="customers") | ||
|
||
def to_dict(self): | ||
customer_dict = { | ||
"id": self.id, | ||
"name": self.name, | ||
"postal_code": self.postal_code, | ||
"phone": self.phone, | ||
"registered_at": self.registered_at, | ||
"videos_checked_out_count": self.videos_checked_out_count | ||
} | ||
return customer_dict |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,8 @@ | ||
from app import db | ||
# from datetime import datetime, timedelta | ||
|
||
class Rental(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
id = db.Column(db.Integer, primary_key=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. 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) |
||
customer_id = db.Column(db.Integer, db.ForeignKey("customer.id"), nullable=False) | ||
video_id = db.Column(db.Integer, db.ForeignKey("video.id"), nullable=False) | ||
due_date = db.Column(db.DateTime) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,21 @@ | ||
from app import db | ||
|
||
|
||
def available_inventory_default(context): | ||
return context.get_current_parameters()['total_inventory'] | ||
class Video(db.Model): | ||
id = db.Column(db.Integer, primary_key=True) | ||
id = db.Column(db.Integer, primary_key=True, autoincrement=True, nullable=False) | ||
title = db.Column(db.String, nullable=False) | ||
total_inventory = db.Column(db.Integer, nullable=False) | ||
available_inventory = db.Column(db.Integer, default=available_inventory_default) | ||
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 |
||
release_date = db.Column(db.DateTime, nullable=False) | ||
customers = db.relationship("Customer",secondary="rental",back_populates="videos") | ||
|
||
def to_dict(self): | ||
return { | ||
"id": self.id, | ||
"title": self.title, | ||
"total_inventory": self.total_inventory, | ||
"release_date": self.release_date | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
from app import db | ||
from app.models.customer import Customer | ||
from app.models.rental import Rental | ||
from app.models.video import Video | ||
from app.routes.helpers import validate_model, validate_request_body | ||
from flask import Blueprint, jsonify, request, make_response, abort | ||
import datetime | ||
|
||
|
||
customers_bp = Blueprint("customers_bp", __name__, url_prefix="/customers") | ||
|
||
|
||
@customers_bp.route("", methods=["GET"]) | ||
def get_all_customers(): | ||
customer_query = Customer.query | ||
|
||
sort_param = request.args.get("sort") | ||
if sort_param: | ||
if sort_param == "name": | ||
customer_query = customer_query.order_by(Customer.name) | ||
elif sort_param == "registered_at": | ||
customer_query = customer_query.order_by(Customer.registered_at) | ||
elif sort_param == "postal_code": | ||
customer_query = customer_query.order_by(Customer.postal_code) | ||
else: | ||
customer_query = customer_query.order_by(Customer.id) | ||
else: | ||
customer_query = customer_query.order_by(Customer.id) | ||
Comment on lines
+17
to
+28
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,
"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_param = request.args.get("count") | ||
page_num_param = request.args.get("page_num") | ||
Comment on lines
+30
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. Something neat I just learned: in count_query = request.args.get("count", type=int)
page_num_param = request.args.get("page_num", type=int) |
||
|
||
pagination = False | ||
count = None | ||
page_num = None | ||
if count_param and count_param.isdigit(): | ||
count = int(count_param) | ||
pagination = True | ||
|
||
if page_num_param and page_num_param.isdigit(): | ||
page_num = int(page_num_param) | ||
pagination = True | ||
|
||
customers = [] | ||
if pagination: | ||
if page_num is None: | ||
page_num = 1 | ||
Comment on lines
+46
to
+47
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 seeing a little inconsistency in how
Since this code doesn't change the default from Flask's default value, to be consistent I recommend explicitly setting defaults for both |
||
|
||
page = customer_query.paginate(per_page=count,page=page_num) | ||
customers = page.items | ||
else: | ||
customers = customer_query.all() | ||
|
||
customers_response = [customer.to_dict() for customer in customers] | ||
|
||
return make_response(jsonify(customers_response), 200) | ||
|
||
|
||
@customers_bp.route("/<customer_id>", methods=["GET"]) | ||
def get_one_customer(customer_id): | ||
customer = validate_model(Customer, customer_id) | ||
customer_data = customer.to_dict() | ||
|
||
return make_response(jsonify(customer_data), 200) | ||
|
||
|
||
@customers_bp.route("", methods=["POST"]) | ||
def add_one_customer(): | ||
request_body = request.get_json() | ||
required_attributes = ["name", "postal_code", "phone"] | ||
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 these attributes are used in more than one function to validate request bodies, it might be worth pulling this list out and making it a constant at the top of the file to avoid duplication. |
||
|
||
validate_request_body(request_body, required_attributes) | ||
Comment on lines
+69
to
+72
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. Tiny style nitpick: since |
||
|
||
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() | ||
db.session.refresh(new_customer, ["id"]) | ||
|
||
return make_response(jsonify({"id":new_customer.id}), 201) | ||
|
||
|
||
@customers_bp.route("/<customer_id>", methods=["DELETE"]) | ||
def delete_one_customer(customer_id): | ||
customer = validate_model(Customer, customer_id) | ||
db.session.delete(customer) | ||
db.session.commit() | ||
|
||
return make_response(jsonify({"id":int(customer_id)}), 200) | ||
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. To confirm to the user what action was taken, I recommend adding some text that describes what was done successfully to all of the routes that aren't returning dictionaries of data. 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 agree. It is just that the specifications in the readme asked for a that response. Perhaps we should have added and extra key: value pair to the response json with a message. |
||
|
||
|
||
@customers_bp.route("/<customer_id>", methods=["PUT"]) | ||
def update_one_customer(customer_id): | ||
customer = validate_model(Customer, customer_id) | ||
request_body = request.get_json() | ||
required_attributes = ["name", "phone", "postal_code"] | ||
|
||
validate_request_body(request_body, required_attributes) | ||
|
||
customer.name = request_body["name"] | ||
customer.phone = request_body["phone"] | ||
customer.postal_code = request_body["postal_code"] | ||
db.session.commit() | ||
response_body = request_body | ||
response_body['registered_at'] = customer.registered_at | ||
|
||
return make_response(jsonify(response_body), 200) | ||
|
||
|
||
@customers_bp.route("/<customer_id>/rentals", methods=["GET"]) | ||
def get_customer_checked_out_videos(customer_id): | ||
validate_model(Customer, customer_id) | ||
|
||
possible_query_params = {"sort" : "", | ||
"count": 0, | ||
"page_num": 0} | ||
|
||
for query_param in possible_query_params: | ||
if query_param in request.args: | ||
possible_query_params[query_param] = request.args.get(query_param) | ||
|
||
join_query = db.session.query(Rental, Video)\ | ||
.join(Video, Rental.video_id==Video.id)\ | ||
.filter(Rental.customer_id == customer_id) | ||
|
||
sort_params = ["title", "release_date"] | ||
for param in sort_params: | ||
if possible_query_params["sort"] == param: | ||
join_query = join_query.order_by(param) | ||
Comment on lines
+128
to
+131
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 think we could simplify this just a little so we don't need to manually write the for-loop: sort_params = ["title", "release_date"]
if possible_query_params["sort"] in sort_params:
join_query = join_query.order_by(possible_query_params["sort"]) |
||
|
||
if possible_query_params["count"] and \ | ||
possible_query_params["count"].isdigit(): | ||
possible_query_params["count"] = int(possible_query_params["count"]) | ||
if possible_query_params["page_num"] and \ | ||
possible_query_params["page_num"].isdigit(): | ||
possible_query_params["page_num"] = \ | ||
int(possible_query_params["page_num"]) | ||
Comment on lines
+133
to
+139
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 chunk of code is a little dense. I suggest using variables to make it a little easier to read at a glance: count_param = possible_query_params["count"]
if count_param and count_param.isdigit():
possible_query_params["count"] = int(count_param)
page_num_param = possible_query_params["page_num"]
if page_num_param and page_num_param.isdigit():
possible_query_params["page_num"] = int(page_num_param) |
||
else: | ||
possible_query_params["page_num"] = 1 | ||
join_query = join_query.paginate( | ||
page=possible_query_params["page_num"], | ||
per_page=possible_query_params["count"]).items | ||
else: | ||
join_query = join_query.all() | ||
|
||
response_body = [] | ||
for row in join_query: | ||
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 consider adding a new variable to this function that holds the results of the query for clarity. At this point |
||
response_body.append({ | ||
"id": row.Video.id, | ||
"title": row.Video.title, | ||
"total_inventory": row.Video.total_inventory, | ||
"release_date": row.Video.release_date | ||
}) | ||
Comment on lines
+150
to
+155
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. Is there something about how paginate returns items such that we need to access the video attributes in this way? I'm wondering if this could be rewritten to take advantage of 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. this was the only way I could access the results of a join query. I'm not sure how to do it in a different way to make use of the to_dict function. 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 was the only way I could access the result of the join query. I don't know how to make it in another way to make use of the to_dict function. 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 agree with Dalia! I tried to access them in the regular way and the attributes of the model object weren't recognized. 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. Ahh, makes sense, because of the join the rows returned by the query are not just Video models. If we only need the video information for the response, we could see if SQLAlchemy has tools for getting the Videos associated with each row, but I think that's far more work than necessary for what we're trying to do here. 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 for future projects: when I cloned down the code to test things out there was a migrations folder, but the versions folder had been deleted. In a larger project, we would want to keep the versions in source control so that everyone is sharing the same, most updated version of the database schema. 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 don't understand why the versions folder isn't there. I can see it in my local version ... and that's the version that we pushed to github. Do you have an idea what we might have done wrong? 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 your local migrations folders might not have been in sync? I see a couple version files that it looks like were deleted in the last PR on the repo: https://github.com/dnabilali/retro-video-store/pull/11/files, check for files marked as deleted |
||
|
||
return make_response(jsonify(response_body),200) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
from flask import abort, make_response,jsonify | ||
|
||
def validate_model(model, id): | ||
try: | ||
int(id) | ||
except: | ||
abort(make_response({"message": f"{id} is an invalid {model.__name__} id"}, 400)) | ||
|
||
model_instance = model.query.get(id) | ||
|
||
if not model_instance: | ||
abort(make_response({"message": f"{model.__name__} {id} was not found"}, 404)) | ||
|
||
return model_instance | ||
|
||
def validate_request_body(request_body,required_data): | ||
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 flexibility of this for checking data to create & update models! |
||
if not request_body: | ||
msg = "An empty or invalid json object was sent." | ||
abort(make_response(jsonify({"details":msg}),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. Tiny style nitpick: for best practices we'd want a space after the comma as a visual separation between the |
||
|
||
for data in required_data: | ||
if data not in request_body: | ||
msg = f"Request body must include {data}. Request failed" | ||
abort(make_response(jsonify({"details":msg}),400)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
from flask import Blueprint, request, make_response, jsonify, abort | ||
from app.routes.customer_routes import validate_model, validate_request_body | ||
from app.models.customer import Customer | ||
from app.models.video import Video | ||
from app.models.rental import Rental | ||
from app import db | ||
from datetime import datetime, timedelta | ||
|
||
MAX_DAYS_RENTALS = 7 | ||
|
||
rentals_bp = Blueprint("rentals_bp", __name__, url_prefix="/rentals") | ||
|
||
@rentals_bp.route("/check-out", methods=["POST"]) | ||
def checkout_video_to_customer(): | ||
request_body = request.get_json() | ||
required_data = ["customer_id", "video_id"] | ||
|
||
validate_request_body(request_body, required_data) | ||
|
||
valid_customer = validate_model(Customer, request_body["customer_id"]) | ||
valid_video = validate_model(Video, request_body["video_id"]) | ||
|
||
if valid_video.available_inventory < 1: | ||
abort(make_response(jsonify({"message":"Could not perform checkout"}), 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. In a real life scenario, we might want to add more specific info like "Could not perform checkout: no copies available" |
||
|
||
if valid_video in valid_customer.videos: | ||
abort(make_response(jsonify({"message":"This customer already has this video checked out"}), 400)) | ||
|
||
new_rental = Rental( | ||
customer_id=valid_customer.id, | ||
video_id=valid_video.id, | ||
due_date=datetime.now() + timedelta(days=MAX_DAYS_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 wondering if this is something that could be set as a default on the column like the |
||
valid_video.available_inventory -= 1 | ||
valid_customer.videos_checked_out_count += 1 | ||
db.session.add(new_rental) | ||
db.session.commit() | ||
|
||
response_body = { | ||
"customer_id" : valid_customer.id, | ||
"video_id" : valid_video.id, | ||
"due_date" : new_rental.due_date, | ||
"videos_checked_out_count" : valid_customer.videos_checked_out_count, | ||
"available_inventory" : valid_video.available_inventory | ||
} | ||
|
||
return make_response(jsonify(response_body), 200) | ||
|
||
@rentals_bp.route("/check-in", methods=["POST"]) | ||
def check_in_video(): | ||
request_body = request.get_json(silent=True) | ||
required_data = ["customer_id", "video_id"] | ||
|
||
validate_request_body(request_body,required_data) | ||
|
||
customer = validate_model(Customer,request_body["customer_id"]) | ||
video = validate_model(Video,request_body["video_id"]) | ||
rental = Rental.query.filter_by(video_id=video.id, customer_id=customer.id).first() | ||
|
||
if not rental: | ||
msg = f"No outstanding rentals for customer {customer.id} and video {video.id}" | ||
abort(make_response(jsonify({"message":msg}),400)) | ||
|
||
video.available_inventory += 1 | ||
customer.videos_checked_out_count -= 1 | ||
|
||
db.session.delete(rental) | ||
db.session.commit() | ||
|
||
response_data = {} | ||
response_data["video_id"] = video.id | ||
response_data["customer_id"] = customer.id | ||
response_data["videos_checked_out_count"] = customer.videos_checked_out_count | ||
response_data["available_inventory"] = video.available_inventory | ||
|
||
return make_response(jsonify(response_data),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.
Just curious, was this addition to manage issues you had updating models?
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.
we found out that if we add this optional parameter changes made to the data types of attributes for our models will be reflected in the migration. here the documentation for that: https://alembic.sqlalchemy.org/en/latest/autogenerate.html